feat: add @metamask/foundryup package
#5810
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
davidmurdoch
force-pushed
the
add-foundryup
branch
from
c638121 to
30fb702
Compare
… read, default to local cache.
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
|
Update on the changes I made to this PR:
The challenge i am facing now is this linting error I am unsure on how to handle it. I will post a message in the wallet framework channel to get some guidance. |
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
| @@ -24,6 +24,11 @@ const config = createConfig([ | |||
| // e.g. `txreceipt_status`, `signTypedData_v4`, `token_id` | |||
| camelcase: 'off', | |||
| 'id-length': 'off', | |||
| 'import-x/no-nodejs-modules': 'off', | |||
There was a problem hiding this comment.
It would be nice if we didn't have to add these rules globally. It's one of our (soft) goals to simplify this file eventually.
Can you explain why these are needed? I can probably guess why, but just want to make sure.
There was a problem hiding this comment.
ah so the rule is disabled because this is a Node.js package that needs to use Node.js built-in modules.
There was a problem hiding this comment.
not sure why these needed to be added. cc @cortisiko ?
| @@ -0,0 +1 @@ | |||
| enableGlobalCache: false | |||
There was a problem hiding this comment.
This is the default yarn setting on Extension, so it best replicates how it is used in practice right now.
| @@ -0,0 +1 @@ | |||
| .metamask | |||
There was a problem hiding this comment.
.metamask is the name of the root directory for the cache for the binaries we download
| ``` | ||
|
|
||
| Kind of weird, but it seems to work okay. You can probably use `npx anvil` in place of `node_modules/.bin/anvil`, but | ||
| getting it to work in all scenarios (cross platform and in CI) wasn't straightforward. `yarn bin anvil` doesn't work |
There was a problem hiding this comment.
Does yarn dlx anvil work instead? That's the equivalent to npx in Yarn v2+.
There was a problem hiding this comment.
IIRC, dlx isn't exactly equivalent. I think it was that npx executes anything in the .bin directory, dlx only executes executables declared by the package.jsons's bin. I can't check right now.
| // An object that configures minimum threshold enforcement for coverage results | ||
| coverageThreshold: { | ||
| global: { | ||
| branches: 50, |
There was a problem hiding this comment.
Is there a way we can start with 100% test coverage instead of 50%? If the threshold is below 100%, it ends up being extremely annoying in the future, as any new gaps in coverage that are introduced in the future are very difficult to find.
There was a problem hiding this comment.
Hey @mcmire, the reason for adjusting the threshold is that we need a bit more flexibility to reach 100% coverage over time. For now, the team's goal was to hit at least 50%, and we're currently at around 55%, so we're above that baseline.
Since I have another PR that depends on this package, would you be open to us following up in a separate PR focused solely on bringing coverage to 100%?
There was a problem hiding this comment.
I'd love to, and normally wouldn't put a PR with such low coverage. But I didn't want to keep blocking Mobile. I won't be able to do any more work for at least a month. Not sure if @cortisiko has the time to increase coverage.
| @@ -0,0 +1,6 @@ | |||
| declare module 'fs' { | |||
There was a problem hiding this comment.
Hmm, I'd like to avoid ambient types if we can. I wonder if we're using an older version of @types/node?
There was a problem hiding this comment.
I'd like to as well, but the correct types were missing :-)
| @@ -0,0 +1,17 @@ | |||
| import 'unzipper'; | |||
There was a problem hiding this comment.
Is there no @types/unzipper?
There was a problem hiding this comment.
they are incomplete types.
| @@ -63,7 +63,7 @@ main().catch((error) => { | |||
| * The entrypoint to this script. | |||
| */ | |||
| async function main() { | |||
| const { cache, fix, quiet } = parseCommandLineArguments(); | |||
| const { cache, fix, quiet } = await parseCommandLineArguments(); | |||
There was a problem hiding this comment.
Why were these changes added?
There was a problem hiding this comment.
oh good observation. I was going down a rabbit hole of addressing linting issue. This particular change was to address: 'TSError: ⨯ Unable to compile TypeScript: .
| "extends": "../../tsconfig.packages.build.json", | ||
| "compilerOptions": { | ||
| "baseUrl": "./", | ||
| "lib": ["ES2021", "DOM"], |
There was a problem hiding this comment.
- Why is
DOMneeded here if this script is intended to run on Node? - Why is
ES2021needed here? (I can probably guess why, but am curious)
There was a problem hiding this comment.
I think ES2021 was the minimum version that worked with the existing version of this package that we are using in Metamask Extension. Not sure why DOM is added 🤔
Co-authored-by: Elliot Winkler <[email protected]>
Explanation
This PR introduces a JavaScript-based foundryup installer that simplifies the installation of Foundry toolchain binaries—Anvil, Cast, Forge, and Chisel. The installer is designed to be cross-platform and work across macOS, Linux, and Windows, supporting both ARM and AMD architectures.
It's been used in the MetaMask Extension for a few months now: MetaMask/metamask-extension#28393
References
Changelog
Checklist