-
Notifications
You must be signed in to change notification settings - Fork 5k
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/caip 25 permission origin #26296
Jl/caip multichain/caip 25 permission origin #26296
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. |
@metamaskbot update-policies |
app/scripts/lib/multichain-api/caip-permission-adapter-middleware.test.js
Outdated
Show resolved
Hide resolved
app/scripts/lib/multichain-api/caip-permission-adapter-middleware.test.js
Outdated
Show resolved
Hide resolved
Policies updated |
app/scripts/lib/multichain-api/caip-permission-adapter-middleware.test.js
Outdated
Show resolved
Hide resolved
expect(next).toHaveBeenCalled(); | ||
}); | ||
|
||
it('allows the request if the requested scope method is authorized in the wallet scope and the current scope does not exist in the authorization', async () => { |
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.
oh ok I see. this is an interesting interpretation that we should probably discuss. wallet_watchAsset
is authorized in the wallet scope and we're saying it can be used against any other scope in the session. Not saying its wrong. Hadn't thougth about this case tho
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 this is a weird one. I remember we talked about this and decided something like either
wallet
scoped methods should actually contain a list of CAIP-2 scopes that they're authorized to be called with
or- they should be authorized within each scope they will be used with
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.
Something to discuss with casa. but what are your thoughts on these options?
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.
both are a little awkward, but the latter is easier to implement. With this particular method, wallet_watchAsset
, it makes more sense to associate it with a specific chain/scope, but that isn't necessarily the case with other wallet_
methods. If we take a look at wallet_swapAssets
, it's no longer as obvious what we should do. Does specifying wallet_swapAssets
for a scope mean that we can send from, send to, or both? CAIP-25 authorizations do not let us be this specific
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.
Yea agreed. This needs further definition. I agree the latter approach is simpler and probably what we should do for now
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.
addressed 0751af2
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.
Asked @bumblefudge about this in CASA discord
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.
Just a few nits and question
…ssion-origin' into jl/caip-multichain/caip-25-permission-origin
LGTM! |
@@ -189,6 +189,7 @@ export async function providerAuthorizeHandler(req, res, _next, end, hooks) { | |||
value: { | |||
requiredScopes: grantedRequiredScopes, | |||
optionalScopes: grantedOptionalScopes, | |||
isMultichainOrigin: true, |
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.
should we maybe put this in sessionProperties
to store it?
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.
🤔 maybe? So we'd add it to all permissions even if not present in the request? I suppose thats okay. Happy either way
Description
isMultichainOrigin
flag to the CAIP-25 permissionisMultichainOrigin
is false for the existing authorizationisMultichainOrigin
as falseisMultichainOrigin
trueisMultichainOrigin
true when CAIP-25 permission is granted as part of the multichain flow viaprovider_authorize
isMultichainOrigin
false when a CAIP-25 permission is granted (not updated) as part ofeth_requestAccounts
orwallet_requestPermissions
in the EIP-1193 flowRelated issues
See: https://github.com/MetaMask/MetaMask-planning/issues/2922
See: https://github.com/MetaMask/MetaMask-planning/issues/2862
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist