test: automatically install anvil during yarn install via foundryup.ts
#28393
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@esbuild/[email protected], npm/[email protected], npm/[email protected] |
Builds ready [388136a]
Page Load Metrics (2080 ± 113 ms)
Bundle size diffs
|
davidmurdoch
force-pushed
the
foundryup
branch
from
09725a7 to
d9c2f36
Compare
davidmurdoch
force-pushed
the
foundryup
branch
from
ef26f51 to
4a41223
Compare
davidmurdoch
changed the title
anvil during yarn install via foundryup.ts
Builds ready [b52e36d]
Page Load Metrics (1991 ± 254 ms)
Bundle size diffs
|
Builds ready [47e7693]
Page Load Metrics (1749 ± 54 ms)
Bundle size diffs
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
davidmurdoch
force-pushed
the
foundryup
branch
3 times, most recently
from
ffe4413 to
49b0a78
Compare
.circleci/config.yml
Outdated
| @@ -449,12 +449,15 @@ jobs: | |||
| - run: | |||
| name: Save Yarn version | |||
| command: yarn --version > /tmp/YARN_VERSION | |||
| - run: | |||
| name: Save Foundry version | |||
| command: node -e "process.stdout.write(require('./package.json').foundry)" > /tmp/FOUNDRY_VERSION | |||
There was a problem hiding this comment.
So I've added a new field to our package.json: foundry. It contains the release we use in the installation process, as well as in this cache process.
I don't like it.
Please please please recommend a better way <3
davidmurdoch
force-pushed
the
foundryup
branch
from
49b0a78 to
197466c
Compare
| paths: | ||
| - .yarn/cache | ||
| - .metamask/cache # should match yarn's relative location |
There was a problem hiding this comment.
Adds the anvil binary to the CI cache.
| - persist_to_workspace: | ||
| root: . | ||
| paths: | ||
| - .metamask/cache # ensures anvil is installed |
There was a problem hiding this comment.
Now that I'm doing a self review, I'm thinking this probably isn't required, since the anvil binary we need is symlinked to .metamask/cache from within in node_modules/.bin, and I think persist_to_workspace will copy the symlinks automatically when it copies node_modules.
Can't hurt to keep it though?
| @@ -64,6 +64,9 @@ test-results/ | |||
| !.yarn/versions | |||
| development/generate-attributions/.yarn/* | |||
|
|
|||
| # MetaMask | |||
| .metamask/* | |||
There was a problem hiding this comment.
I cache the foundry foundries in a new .metamask root folder. we want to ignore it.
Maybe this is where I could put the foundry version? instead of the package.json? hmmmm. not sure I like that either
package.json
Outdated
| @@ -755,5 +760,6 @@ | |||
| "@metamask/solana-wallet-snap>@solana/web3.js>bigint-buffer": false | |||
| } | |||
| }, | |||
| "packageManager": "[email protected]" | |||
| "packageManager": "[email protected]", | |||
| "foundry": "nightly-31dd1f77fd9156d09836486d97963cec7f555343" | |||
There was a problem hiding this comment.
😢 there's not much we can do about it... it doesn't seem they have semantic versioning? 🤔
package.json
Outdated
| @@ -9,7 +9,8 @@ | |||
| "scripts": { | |||
| "webpack": "tsx ./development/webpack/launch.ts", | |||
| "webpack:clearcache": "./development/clear-webpack-cache.js", | |||
| "postinstall": "yarn webpack:clearcache", | |||
| "foundryup": "tsx --tsconfig ./test/helpers/foundry/tsconfig.json ./test/helpers/foundry/foundryup.mts", | |||
| "postinstall": "yarn webpack:clearcache && yarn foundryup --binaries anvil --version \"$(node -e \"process.stdout.write(require('./package.json').foundry)\")\"", | |||
There was a problem hiding this comment.
This is another place I use the package.json's foundry property (the other was in the circleci yaml). Have you thought of a better way to do this yet?
There was a problem hiding this comment.
Should we hardcode checksums of the binaries we expect somewhere in our repo, and then verify that the binaries are correct. foundryup (both this script or the original bash script), doesn't have the version immutability guarantee that npm/yarn do 🤔, so it makes our developers (and thus users, by a stretch) susceptible to supply chain attacks.
cc @kumavis @naugtur: a summary: this PR adds foundry's anvil to the extension; it is a binary downloaded from Foundry's Github Releases page, so it can be swapped out at any time.
There was a problem hiding this comment.
How important is keping it up to date? Could we install it once and keep the binary in the repo? It's easier and less likely to be ignored. If we do checksums, people will just update checksums and be annoyed ;)
There was a problem hiding this comment.
How important is keping it up to date?
This is to replace Ganache, a package that hasn't been updated in over a year now. So apparently we don't need it to be updated all too often. :-)
| const { enableGlobalCache } = parseYaml(readFileSync('.yarnrc.yml', 'utf-8')); | ||
| const CACHE_DIR = enableGlobalCache | ||
| ? join(homedir(), '.cache', 'metamask') | ||
| : join(cwd(), '.metamask', 'cache'); |
There was a problem hiding this comment.
Probably not necessary, but this ensures that we keep our own cache alongside the .yarn cache. Why? I had to put it somewhere, and copying whatever we do for yarn just felt right, I guess.
| : join(cwd(), '.metamask', 'cache'); | ||
|
|
||
| if (parsedArgs.command === 'cache clean') { | ||
| await rm(CACHE_DIR, { recursive: true, force: true }); |
There was a problem hiding this comment.
Have a cache clean was mostly just useful when writing this code and manually testing it. Feature could be removed if we don't want to keep code we hopefully won't need.
|
|
||
| if (parsedArgs.command === 'cache clean') { | ||
| await rm(CACHE_DIR, { recursive: true, force: true }); | ||
| say('done!'); |
There was a problem hiding this comment.
The say function name is copied from the original foundryup bash script. I liked it, so I kept it.
test/helpers/foundry/helpers.mts
Outdated
| Forge = 'forge', | ||
| Cast = 'cast', | ||
| Chisel = 'chisel', | ||
| } |
There was a problem hiding this comment.
we could use all these just by adding to --binaries <binary you want> to the yarn foundryup command. neat!
| .strict() | ||
| // disable yargs's version, as it doesn't make sense here | ||
| .version(false) | ||
| // use the scriptName in `--help` output | ||
| .scriptName('yarn foundryup') | ||
| // wrap output at a maximum of 120 characters or `stdout.columns` | ||
| .wrap(Math.min(120, stdout.columns)) | ||
| .parserConfiguration({ | ||
| 'strip-aliased': true, | ||
| 'strip-dashed': true, | ||
| }) | ||
| // enable ENV parsing, which allows the user to specify foundryup options | ||
| // via environment variables prefixed with `FOUNDRYUP_` | ||
| .env('FOUNDRYUP') |
There was a problem hiding this comment.
This was jsut copied and tweaked from the webpack yargs setup script.
| description: 'Specify the binaries to install', | ||
| default: [Binary.Anvil], | ||
| choices: Object.values(Binary) as Binary[], | ||
| coerce: (values: Binary[]): Binary[] => [...new Set(values)], // Remove duplicates |
There was a problem hiding this comment.
Deduping here because yargs allows duplicates in its array type, but we don't want duplicates.
| if (/^nightly/u.test(rawVersion)) { | ||
| return { version: 'nightly', tag: rawVersion }; | ||
| // we don't validate the version much, we just trust the user | ||
| } else if (/^\d/u.test(rawVersion)) { | ||
| return { version: `v${rawVersion}`, tag: rawVersion }; | ||
| } |
There was a problem hiding this comment.
This chunk was ported from the bash script. It's weird though, because foundry doesn't publish tagged versions... only nightly-<commit hash> and nightly tags. Kept it here just to maintain feature parity.
davidmurdoch
force-pushed
the
foundryup
branch
from
197466c to
ea969d8
Compare
| .xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx | ||
|
|
||
| ╔═╗ ╔═╗ ╦ ╦ ╔╗╔ ╔╦╗ ╦═╗ ╦ ╦ Portable and modular toolkit | ||
| ╠╣ ║ ║ ║ ║ ║║║ ║║ ╠╦╝ ╚╦╝ for Ethereum Application Development | ||
| ╚ ╚═╝ ╚═╝ ╝╚╝ ═╩╝ ╩╚═ ╩ written in Rust. | ||
|
|
||
| .xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx | ||
|
|
||
| Repo : https://github.com/foundry-rs/ | ||
| Book : https://book.getfoundry.sh/ | ||
| Chat : https://t.me/foundry_rs/ | ||
| Support : https://t.me/foundry_support/ | ||
| Contribute : https://github.com/orgs/foundry-rs/projects/2/ | ||
|
|
||
| .xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx.xOx |
There was a problem hiding this comment.
This is a bit obnoxious. I wouldn't hate it if you asked me to remove it. :-)
| const agent = new (url.protocol === 'http:' ? HttpAgent : HttpsAgent)({ | ||
| keepAlive: true, | ||
| }); |
There was a problem hiding this comment.
We end up querying the same connection multiple times, so keeping the connection alive in between queries improves performance.
| @@ -0,0 +1,17 @@ | |||
| import 'unzipper'; | |||
|
|
|||
| declare module 'unzipper' { | |||
There was a problem hiding this comment.
added some missing types to the unzipper package
davidmurdoch
force-pushed
the
foundryup
branch
from
2ed3990 to
6f329a5
Compare
Builds ready [6f329a5]
Page Load Metrics (1789 ± 62 ms)
Bundle size diffs
|
| @@ -82,6 +82,8 @@ ignores: | |||
| - '@testing-library/dom' | |||
| - 'mini-css-extract-plugin' | |||
| - 'webpack-cli' | |||
| # foundryup | |||
| - 'tar' | |||
There was a problem hiding this comment.
just a nit: maybe we can add this under the # used in testing + ci section, like this:
- 'tar' # foundryup
what do you think?
| alias: 'v', | ||
| description: | ||
| 'Specify the version (see: https://github.com/foundry-rs/foundry/tags)', | ||
| default: 'nightly', |
There was a problem hiding this comment.
should we default to nightly, or should we set a fixed version and update it in separate PRs periodically, to make sure nothing is broken with a version update?
There was a problem hiding this comment.
The install script I added in package.json uses a pinned version. This script defaults to nightly; eventually I think we'll to release this code to npm as a stand-alone package for other developers to use in the future, so I think nightly would make sense when we do that, so we should keep it as nightly here. Just my opinion.
There was a problem hiding this comment.
oh I see, it make sense, thank you for clarifying 🙏
| export function parseArgs(args: string[] = argv.slice(2)) { | ||
| const { $0, _, ...parsed } = yargs() | ||
| // Ensure unrecognized commands/options are reported as errors. | ||
| .strict() |
There was a problem hiding this comment.
woha didn't know about this!
| } from './helpers.mts'; | ||
|
|
||
| const parsedArgs = parseArgs(); | ||
|
|
There was a problem hiding this comment.
maybe we could split the different logic into smaller functions? From chatgpt could be something like:
- getCacheDirectory
- getBinaryArchiveUrl
- cleanCache
- checkAndDownloadBinaries
- installBinaries
- main
so it's easier to read and test?
There was a problem hiding this comment.
Sure, most of those make sense.
I do think over-abstraction is a real concern, and can lead to reduce readability in an effort to improve it. Like cleanCache, for example, is already a short bit of code that is straight forward to read, moving the logic to its own file and function would increase the lines of code from 1 to about 5 (including the import and calling the function, and maybe more if adding JSDoc comments).
Feel free to add functions where you see fit, just don't blindly listen to all of ChatGPTs recommendations, I think it goes a bit too far sometimes.
There was a problem hiding this comment.
I see, your point make sense to me! I've added a proposal for split in this PR, which also add unit tests. Let me know if this makes sense, or if you prefer to have it encapsulated with less functions 🙏
| "terser": "^5.7.0", | ||
| "terser-webpack-plugin": "^5.3.10", | ||
| "through2": "^4.0.2", | ||
| "ts-node": "^10.9.2", | ||
| "tsx": "^4.7.1", | ||
| "tsx": "^4.19.2", |
There was a problem hiding this comment.
This has been updated to fix stack traces on Node 20+
| "darwin-amd64": "6ad1c31bb8bc8c78551792ad538028385eb5a44d105f9af40ddeb1b75c1327b1", | ||
| "darwin-arm64": "cbb36860b2e08a7d5a35bec430904a1dc0319602ac0923017b81d9a8ffdcc5b0", | ||
| "linux-amd64": "56054e7d59547beb6adacbb9060a58595a8c5cca185f946317334ece509e0433", | ||
| "linux-arm64": "16494ae631d8ef73944abaaaa756661c1d43bbfc7642e7fff0a0d807d6d143c4", | ||
| "win32-amd64": "2034bdf35023e59c5dc06d6717a8eb2cede1204dbc60fd2507564422f03dceb3" |
There was a problem hiding this comment.
Yes, these are slightly annoying to have to generate :-)
| } | ||
| } | ||
| }, | ||
| "version": "nightly-31dd1f77fd9156d09836486d97963cec7f555343" |
There was a problem hiding this comment.
foundry doesn't publish versions, just tags like this. it's weird, but it's all we got.
| ? JSON.parse(rawChecksums) | ||
| : rawChecksums; | ||
| } catch { | ||
| throw new Error('Invalid checksums'); |
There was a problem hiding this comment.
we could do a lot more validation of the checksum object... but I think it's bit premature, since maybe the way I implemented it is horrible.
| transform: (entry) => { | ||
| if (checksumAlgorithm) { | ||
| const hash = createHash(checksumAlgorithm); | ||
| const passThrough = new Minipass({ async: true }); |
There was a problem hiding this comment.
Minipass is almost like Node's Stream.PassThrough, but different enough that if we use PassThrough the data becomes corrupted (the hash is correct, but the binary is out of order -- I couldn't figure out why it happens). So it is lame that we need a whole new lib for this, but it is what it is.
| const hashStream = async function* (source: Entry) { | ||
| for await (const chunk of source) { | ||
| hash.update(chunk); | ||
| yield chunk; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Instead of teeing the stream I'm just reading the data as it passes through the pipeline and then letting it continue on untouched. This could be done with a Stream.PassThrough, but that would requires a creating a new class that extends Stream.PassThrough.
Builds ready [ba04942]
Page Load Metrics (2045 ± 54 ms)
Bundle size diffs
|
I've ported Foundry's foundryup bash script to TypeScript. It downloads a pinned version of the
anvilbinary onyarn install.While you don't need to do anything, other than run
yarn(oryarn install), you can customize the installation options by runningyarn foundryup <options>. Try runningyarn foundryup --helpto see all that it can do.An explanation of how it works:
The
yarn foundryupcommand caches (to.metamask/cache/*) the downloaded binaries (defaults to onlyanvil, but it can install all the others) and symlinks them from the cache intonode_modules/.bin/<binary name>. This makes successive installs nearly instant.In CI the cache itself is cached (same process as for the
.yarn/cachecache), so that CI doesn't have to redownload binaries on every workflow.The
foundryupcode is intended to be generic, and could eventually be a partial replacement for theganachepackaghe for most users (for cli users only, not for programmatic use cases). For this reason, the script uses defaults that are not tailored to our needs; instead we use a newpackage.jsonpropertyfoundryupto describe what defaults we do want, and the script uses those values.In addition to the original
foundryupbash script features, this script supports checksumming the downloaded binaries; a feature requested by the MM security team. See thefoundryupproperty in ourpackage.jsonfor more details.A feature that is missing from this script, but is in the original
foundryupbash script,is support for downloading themanpages.This script should support the following platforms, but i've only tested Linux+AMD:
Linux on ARM and AMD
Mac on ARM and AMD
Windows on AMD (likely works on arm-based Windows machines via OS's emulation)
--
This also adds a new yarn command:
anvil. You can use it just like you did withyarn ganache, i.e.,yarn anvil; note: the defaults and CLI options are not going to be exactly the same asganache.