-
-
Notifications
You must be signed in to change notification settings - Fork 192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
eth-json-rpc-provider
migration - Integration into packages/
#1738
Conversation
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring: Next stepsTake a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with |
8175a45
to
4984cb6
Compare
@SocketSecurity ignore-all All packages versions are current with main branch. |
046290c
to
fcd3b95
Compare
eth-json-rpc-provider
into core monorepo (2/2)eth-json-rpc-provider
migration - C: Integration into packages/
b6c95cc
to
43fae9b
Compare
a014d40
to
ebcfc39
Compare
615fb8e
to
4412ff7
Compare
03c8d0d
to
69a276d
Compare
69a276d
to
e42c619
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking this as "request changes" because of the change to network-controller
(see comment there).
@@ -4,6 +4,7 @@ describe('Package exports', () => { | |||
it('has expected exports', () => { | |||
expect(Object.keys(allExports)).toMatchInlineSnapshot(` | |||
Array [ | |||
"SafeEventEmitterProvider", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one follows from the export statement change for SafeEventEmitterProvider
https://github.com/MetaMask/core/pull/1738/files#r1357548245. The test is checking what non-types are being exported from index.ts
.
@@ -1,3 +1,3 @@ | |||
export * from './provider-from-engine'; | |||
export * from './provider-from-middleware'; | |||
export type { SafeEventEmitterProvider } from './safe-event-emitter-provider'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes what is being exported (the whole class instead of just the type). What's the reason for this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See: first line of network-controller/tests/fake-provider
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, no problem. I guess this wouldn't be a breaking change, so it's okay.
@@ -33,7 +33,7 @@ | |||
"@metamask/controller-utils": "^5.0.2", | |||
"@metamask/eth-json-rpc-infura": "^9.0.0", | |||
"@metamask/eth-json-rpc-middleware": "^12.0.0", | |||
"@metamask/eth-json-rpc-provider": "^2.1.0", | |||
"@metamask/eth-json-rpc-provider": "^2.2.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Given that we are affecting other packages in this PR, should we bump everything to ^2.2.0 first? I think it'd be better to keep this PR just focused to eth-json-rpc-provider
if we can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is necessary because of the reference path shadowing thing we discussed. Once eth-json-rpc-provider
is moved into packages/
, downstream packages can no longer use previous versions by pointing to node_modules/
, because all imports from @metamask/eth-json-rpc-provider
will always point to the path in packages/
, and therefore to the latest version. This seems to happen even if the migrated package isn't added to the reference paths in the downstream pkgs' typescript config files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, but isn't this happening because network-controller
is currently using version 2.1.0? If we bumped that first then the latest version of the package being used and current version of the package as it exists after being migrated would be the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could bump eth-json-rpc-provider to 2.2.0 everywhere first in a separate PR, but that PR wouldn't include the changes being made to network-controller/
here (the last two commits).
Because fixing the ../dist/..
import statements requires fixing that export statement in eth-json-rpc-provider
, we'd still end up with changes being made to both packages simultaneously before the migration can be merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we could change the SafeEventEmitterProvider
export statement in the original repo and release 2.2.1 first, but that would mean redoing the migration process from the start because of the new git history.
@@ -1,4 +1,4 @@ | |||
import { SafeEventEmitterProvider } from '@metamask/eth-json-rpc-provider/dist/safe-event-emitter-provider'; | |||
import { SafeEventEmitterProvider } from '@metamask/eth-json-rpc-provider'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See line 85:
export class FakeProvider extends SafeEventEmitterProvider {
Importing SafeEventEmitterProvider
as a type breaks that line, because it's being used as a class.
This is why the previous import statement pointed directly to node_modules/.../dist/...
instead of using the SafeEventEmitterProvider
type exported by the package.
That import statement can no longer be used (it won't point to node_modules
with eth-json-rpc-provider
in packages/
) which is why the export from eth-json-rpc-provider
needs to be fixed from type to class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. Thank you for explaining, somehow that was not connecting.
@mcmire Are there still any changes to the PR you'd like to see, or maybe a different approach we could try for the |
…downstream packages
0b71faa
to
a32e68a
Compare
Removing this as I've determined that the network controller changes are non-breaking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good with this PR!
eth-json-rpc-provider
migration - C: Integration into packages/
eth-json-rpc-provider
migration - Integration into packages/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
…changes from recent releases (#1807) ## Explanation - Updating root README to include @metamask/eth-json-rpc-provider. - Updating dependency graph to reflect changes made between releases v78.0.0 through v81.0.0. ## References - See #1738 ## Changelog ### `core` - **CHANGED**: Updated README dependency graph. ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate
Explanation
This PR implements the following incremental steps in the process for migrating
eth-json-rpc-provider
into the core monorepo:Phase B: Staging from
merged-packages/
5. Port tags
Push ported tags to core repo
Verify that the tag diff links in CHANGELOG are working
Phase C: Integration into
packages/
1. The big leap
merged-packages/
topackages/
.yarn install
in the root directory.yarn workspace @metamask/<package-name> test
.2. Update downstream repos
3. Linter fixes
yarn constraints --fix
(run twice).yarn workspace @metamask/<package-name> changelog:validate
and apply the diffs.4. Resolve downstream errors
@metamask/{eth-json-rpc-provider,rpc-errors}
#1653@ts-expect-error TODO:
annotations.5. Finalize merge
packages/<package-name>
directory into core main branch.See #1551 (comment) for an outline of the entire process.
Next Steps
Blocked by
@metamask/utils
bump: deps: @metamask/utils@^6.2.0->^8.1.0 #1639@metamask/{eth-json-rpc-provider,rpc-errors}
#1653References
@metamask/eth-json-rpc-provider
to core monorepo #1685eth-json-rpc-provider
into core monorepo #1551Changelog
@metamask/eth-json-rpc-provider
SafeEventEmitterProvider
as a value (class), not just a type.@metamask/network-controller
@metamask/eth-json-rpc-provider
to ^2.2.0Checklist