-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
feat: Migrate eth_accounts and permittedChains to CAIP-25 endowment #27847
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, updated, and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@json-schema-spec/[email protected], npm/@json-schema-tools/[email protected] |
👍 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. |
@metamask-bot update-policies |
.yarn/patches/@json-schema-tools-reference-resolver-npm-1.2.6-4e1497c16d.patch
Show resolved
Hide resolved
@metamaskbot update-policies |
Policies updated. 🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff |
app/scripts/lib/rpc-method-middleware/handlers/ethereum-chain-utils.js
Outdated
Show resolved
Hide resolved
app/scripts/lib/rpc-method-middleware/handlers/wallet-requestPermissions.ts
Outdated
Show resolved
Hide resolved
app/scripts/lib/rpc-method-middleware/handlers/wallet-revokePermissions.ts
Outdated
Show resolved
Hide resolved
@metamaskbot update-policies |
Policies updated. 🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff |
@SocketSecurity ignore npm/@metamask/[email protected] i know that mcmire guy |
@SocketSecurity ignore npm/@metamask/[email protected] i still know that mcmire fellow |
@SocketSecurity ignore npm/@metamask/[email protected] the fetch isn't new, but even then it's fine because it fetches caller supplied url |
@metamaskbot update-policies |
Policies updated. 🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff |
@metamaskbot update-policies |
Policies updated. 🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff |
app/scripts/lib/rpc-method-middleware/handlers/wallet-revokePermissions.ts
Outdated
Show resolved
Hide resolved
|
@metamaskbot update-policies |
Policies updated. 🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff |
…cenario to wallet_revokePermissions e2e test (#29761) <!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** * Fix incorrect snap-account-signature e2e test fixtures / starting state (accounts permissioned before they exist in the wallet) * Add `endowment:permitted-chains` scenario to `wallet_revokePermissions` e2e test [](https://codespaces.new/MetaMask/metamask-extension/pull/29761?quickstart=1) ## **Related issues** See: #27847 ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: Alex Donesky <[email protected]> Co-authored-by: Mark Stacey <[email protected]>
ui/components/multichain/edit-accounts-modal/edit-accounts-modal.tsx
Outdated
Show resolved
Hide resolved
test/e2e/fixture-builder.js
Outdated
@@ -479,10 +479,7 @@ class FixtureBuilder { | |||
caveats: [ | |||
{ | |||
type: 'restrictReturnedAccounts', | |||
value: [ |
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 have attempted to extract this here: #29783
…al.tsx Co-authored-by: Mark Stacey <[email protected]>
It would be helpful to have manual testing instructions in the PR description for the migration, e.g. how to test that everything works correctly when updating. I've provided similar instructions in the past on how to do "update testing", e.g. in this PR: #26485 |
## **Description** Two of the permission fixture builders were adding permissions for accounts that do not exist. They have been updated to only grant permissions for the selected account, which is the only account guaranteed to exist in the default fixture. [](https://codespaces.new/MetaMask/metamask-extension/pull/29783?quickstart=1) ## **Related issues** This was extracted from #27847 ## **Manual testing steps** See that E2E tests still pass ## **Screenshots/Recordings** N/A ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.
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!
Builds ready [18440d6]
Page Load Metrics (1692 ± 55 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [15fa319]
Page Load Metrics (1581 ± 45 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Description
This PR replaces the replaces the internal
eth_accounts
andendowment:permittedChains
permission structure with a CAIP-25 endowment. It adds adapter logic to translate to and from the new internal CAIP-25 permissions. This change should be transparent to wallet users and to dapps except foronetwo cases, see below. This change is required in order to support CAIP-25 and CAIP-27 requests in a follow-up PR that enables the Multichain API.Related issues
Related: MetaMask/core#4784
Manual testing steps
There should be no user or dapp facing difference in behavior except:
wallet_revokePermissions
and specifying eithereth_accounts
orendowment:permitted-chains
, the entire CAIP-25 permission will be revoked. It will appear to the dapp as if botheth_accounts
andendowment:permitted-chains
were revoked.wallet_getPermissions
for a permitted dapp when the wallet is locked,eth_accounts
should be returned in addition toendowment:permitted-chains
. Currently there is a regression onmain
where onlyendowment:permitted-chains
gets returned when the wallet is locked.Locked Wallet Behavior with dapp connected
Other than the two noted items below, this behavior matches that in
main
eth_accounts
returns []wallet_getPermissions
returns permissions incl eth_accountswallet_revokePermissions
works as usual and revokes eth_accounts and revoke permitted-chains togethermain
where eth_accounts and permitted-chains aren't revoked as a pair if either is revokedeth_requestAccounts
prompts for unlock, after unlock returns accounts if any are permitted, otherwise shows connection promptwallet_requestPermissions
prompts for unlockaccountsChanged
empty array on lock. no event after revokePermissions which makes sense since the dapp was told empty array on lock and now it's actually empty array so no changes have occurred as far as the dapp should be concerned.wallet_addEthereum
orwallet_switchEthereumChain
flows without account permissions, these permissions will be removed with this migration. We think this ok because:Testing the migration
main
dist/chrome
directory and proceed through onboardingmain
and create a dev buildRepeat the above steps but with the line above replaced with the following for example:
state.data.NetworkController = {};
state.data.NetworkController = 'foobar';
state.data.NetworkController.selectedNetworkClientId = null;
state.data.NetworkController.networkConfigurationsByChainId = 'foobar';
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist