-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Jl/caip multichain/fix connection flow for permitted chains #27471
Jl/caip multichain/fix connection flow for permitted chains #27471
Conversation
…-flow-for-permittedChains
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. |
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.
might be worth making this change against develop
?
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.
Yeah makes sense. Even just a bit less code in the feature branch too is good! 😅
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.
…equest as default since component itself now figures out the default
…-flow-for-permittedChains
}, | ||
], | ||
}, | ||
[PermissionNames.permittedChains]: {}, |
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 believe this change may be breaking something about the UI in the wallet_switchEthereumChain
permissions flow on the legacy API?
Screen.Recording.2024-09-30.at.3.07.51.PM.mov
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 don' think I understand what is broken here. If I am correct, you start without Ethereum mainnet permissioned, and then switch to it. After approving the switch, Ethereum mainnet is checked inside the EditNetworkModal. I assume this dapp has EIP-1193 granted permissions, otherwise the switch chain call would have failed (rather than prompting the user to approve or not)
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.
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.
ahh gotcha. missed that
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.
Fixed here 60f7518
will pull into PR against dev as well
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.
@@ -649,7 +595,7 @@ describe('permission background API methods', () => { | |||
); | |||
}); | |||
|
|||
it('calls updateCaveat with the chain added', () => { | |||
it('calls updateCaveat with the chain added and all eip155 accounts synced', () => { |
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 does "all eip155 accounts synced" mean?
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.
From the eip155 scopeObjects, gather the unique list of eip155 accounts, and then assign that list back to each eip155 scopeObject (overwriting any accounts array value that was previous there)
…-flow-for-permittedChains
…-flow-for-permittedChains
…-flow-for-permittedChains
…-flow-for-permittedChains
Quality Gate passedIssues Measures |
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!
…-flow-for-permittedChains
Description
Connects the new AmonHenV2 Connection Flow to CAIP Multichain:
ConnectPage
Approval now uses the caveat values from eth_accounts and endowment:permitted-chains as the default selected account and chainswallet_createSession
passes a list of supported eth accounts and eth chainIds based on the supported scopes to be used as the preselected/default values in the ConnectPage Approvalwallet_createSession
removes supported eip155 scopes that were not approved and adds ones that were not in the original request but were approvedRelated issues
Fixes:
Manual testing steps
Replace the account addresses below with your own addresses to test the preselected accounts. This request should preselect mainnet and sepolia (which is different from the default which preselects all non testnet networks)
replace this with your own address to test preselected accounts.
This one works for preselecting chains
You can also combine the params of these two wallet_requestPermissions examples
One for eth_requestAccounts
And of course you can connect via the wallet UI directly.
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist