-
-
Notifications
You must be signed in to change notification settings - Fork 185
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: Add CAIP-25 permission and adapters to @metamask/multichain
#4784
base: main
Are you sure you want to change the base?
Conversation
## Explanation This PR fixes a lot of the linting and typescript errors. still some left but this covers a lot of it. <!-- Thanks for your contribution! Take a moment to answer these questions so that reviewers have the information they need to properly understand your changes: * What is the current state of things and why does it need to change? * What is the solution your changes offer and how does it work? * Are there any changes whose purpose might not obvious to those unfamiliar with the domain? * If your primary goal was to update one package but you found you had to update another one along the way, why did you do so? * If you had to upgrade a dependency, why did you do so? --> ## References <!-- Are there any issues that this pull request is tied to? Are there other links that reviewers should consult to understand these changes better? Are there client or consumer pull requests to adopt any breaking changes? For example: * Fixes #12345 * Related to #67890 --> ## Changelog <!-- If you're making any consumer-facing changes, list those changes here as if you were updating a changelog, using the template below as a guide. (CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or FIXED. For security-related issues, follow the Security Advisory process.) Please take care to name the exact pieces of the API you've added or changed (e.g. types, interfaces, functions, or methods). If there are any breaking changes, make sure to offer a solution for consumers to follow once they upgrade to the changes. Finally, if you're only making changes to development scripts or tests, you may replace the template below with "None". --> ### `@metamask/package-a` - **<CATEGORY>**: Your change here - **<CATEGORY>**: Your change here ### `@metamask/package-b` - **<CATEGORY>**: Your change here - **<CATEGORY>**: Your change here ## 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 - [ ] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes --------- Co-authored-by: Jiexi Luan <jiexiluan@gmail.com>
## Explanation <!-- Thanks for your contribution! Take a moment to answer these questions so that reviewers have the information they need to properly understand your changes: * What is the current state of things and why does it need to change? * What is the solution your changes offer and how does it work? * Are there any changes whose purpose might not obvious to those unfamiliar with the domain? * If your primary goal was to update one package but you found you had to update another one along the way, why did you do so? * If you had to upgrade a dependency, why did you do so? --> Added ESM exports for multichain package ## References <!-- Are there any issues that this pull request is tied to? Are there other links that reviewers should consult to understand these changes better? Are there client or consumer pull requests to adopt any breaking changes? For example: * Fixes #12345 * Related to #67890 --> ## Changelog <!-- If you're making any consumer-facing changes, list those changes here as if you were updating a changelog, using the template below as a guide. (CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or FIXED. For security-related issues, follow the Security Advisory process.) Please take care to name the exact pieces of the API you've added or changed (e.g. types, interfaces, functions, or methods). If there are any breaking changes, make sure to offer a solution for consumers to follow once they upgrade to the changes. Finally, if you're only making changes to development scripts or tests, you may replace the template below with "None". --> ### `@metamask/package-a` - **<CATEGORY>**: Your change here - **<CATEGORY>**: Your change here ### `@metamask/package-b` - **<CATEGORY>**: Your change here - **<CATEGORY>**: Your change here ## 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 - [ ] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes
@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.
|
@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.
|
@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.
|
@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.
|
@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.
|
@metamaskbot publish-preview |
## Explanation Creates the initial (empty) `@metamask/multichain` package to make downstream additions to this package easier to review. ## References Upstream: #4811 Downstream: #4784 ## Changelog ### `@metamask/multichain` - **ADDED**: Initial skeleton package for `@metamask/multichain` ## Checklist - [X] I've updated the test suite for new or updated code as appropriate - [X] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [X] I've highlighted breaking changes using the "BREAKING" category above as appropriate - [X] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes --------- Co-authored-by: Mark Stacey <markjstacey@gmail.com> Co-authored-by: Alex <adonesky@gmail.com>
## Explanation Loosen `getEthAccounts` and `getPermittedEthChainIds` param type ## References <!-- Are there any issues that this pull request is tied to? Are there other links that reviewers should consult to understand these changes better? Are there client or consumer pull requests to adopt any breaking changes? For example: * Fixes #12345 * Related to #67890 --> ## Changelog <!-- If you're making any consumer-facing changes, list those changes here as if you were updating a changelog, using the template below as a guide. (CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or FIXED. For security-related issues, follow the Security Advisory process.) Please take care to name the exact pieces of the API you've added or changed (e.g. types, interfaces, functions, or methods). If there are any breaking changes, make sure to offer a solution for consumers to follow once they upgrade to the changes. Finally, if you're only making changes to development scripts or tests, you may replace the template below with "None". --> ### `@metamask/package-a` - **<CATEGORY>**: Your change here - **<CATEGORY>**: Your change here ### `@metamask/package-b` - **<CATEGORY>**: Your change here - **<CATEGORY>**: Your change here ## 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 - [ ] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes
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.
Made a super shallow pass over this. I will give this a closer look tomorrow.
export const KnownRpcMethods: Record<NonWalletKnownCaipNamespace, string[]> = { | ||
eip155: Eip155Methods, | ||
}; | ||
|
||
export const KnownWalletNamespaceRpcMethods: Record< | ||
NonWalletKnownCaipNamespace, | ||
string[] | ||
> = { | ||
eip155: WalletEip155Methods, | ||
}; | ||
|
||
export const KnownNotifications: Record<NonWalletKnownCaipNamespace, string[]> = | ||
{ | ||
eip155: ['accountsChanged', 'chainChanged', 'eth_subscription'], | ||
}; |
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.
Instead of forcing the types for these data structures, is it possible to use satisfies
to enforce the shapes?
export const KnownRpcMethods: Record<NonWalletKnownCaipNamespace, string[]> = { | |
eip155: Eip155Methods, | |
}; | |
export const KnownWalletNamespaceRpcMethods: Record< | |
NonWalletKnownCaipNamespace, | |
string[] | |
> = { | |
eip155: WalletEip155Methods, | |
}; | |
export const KnownNotifications: Record<NonWalletKnownCaipNamespace, string[]> = | |
{ | |
eip155: ['accountsChanged', 'chainChanged', 'eth_subscription'], | |
}; | |
export const KnownRpcMethods = { | |
eip155: Eip155Methods, | |
} as const satisfies Record<NonWalletKnownCaipNamespace, string[]>; | |
export const KnownWalletNamespaceRpcMethods = { | |
eip155: WalletEip155Methods, | |
} as const satisfies Record<NonWalletKnownCaipNamespace, string[]>; | |
export const KnownNotifications = { | |
eip155: ['accountsChanged', 'chainChanged', 'eth_subscription'], | |
} as const satisfies Record<NonWalletKnownCaipNamespace, string[]>; |
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.
doing this seems to break things where i use .include()
or .filter()
on these constants. An example error is Argument of type 'string' is not assignable to parameter of type '"wallet_addEthereumChain"
, which forces me to do change this:
.filter(
(method: string) => !KnownWalletNamespaceRpcMethods.eip155.includes(method),
)
to
.filter(
(method: (typeof KnownWalletNamespaceRpcMethods.eip155)[number]) =>
!KnownWalletNamespaceRpcMethods.eip155.includes(method),
)
return parseCaipChainId(scopeString); | ||
} | ||
|
||
return {}; |
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.
Usually using an empty object to represent an unsuccessful result is a smell. Does it make sense to return something else such as null
instead?
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.
The idea was to be able to destructure without checking for null/undefined first.
Usage currently looks something like
const { namespace, reference } = parseScopeString(scopeString);
if (!namespace && !reference) {
}
vs
const parsedScopeString = parseScopeString(scopeString);
if (!parsedScopeString) {
}
const { namespace, reference } = parsedScopeString;
export const KnownWalletRpcMethods: string[] = [ | ||
'wallet_registerOnboarding', | ||
'wallet_scanQRCode', | ||
]; | ||
const WalletEip155Methods = ['wallet_addEthereumChain']; |
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.
It seems that by using string[]
we are discarding type information that could be valuable later. For instance, if we wanted a type from this array we couldn't do that easily. Thoughts on removing and using as const
?
export const KnownWalletRpcMethods: string[] = [ | |
'wallet_registerOnboarding', | |
'wallet_scanQRCode', | |
]; | |
const WalletEip155Methods = ['wallet_addEthereumChain']; | |
export const KnownWalletRpcMethods = [ | |
'wallet_registerOnboarding', | |
'wallet_scanQRCode', | |
] as const; | |
const WalletEip155Methods = ['wallet_addEthereumChain'] as const; |
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.
makes the types too narrow and difficult to consume downstream. Same problem as #4784 (comment)
]; | ||
const WalletEip155Methods = ['wallet_addEthereumChain']; | ||
|
||
const Eip155Methods = MetaMaskOpenRPCDocument.methods |
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.
The type on this would be string[]
. Similar comment as above, do we want to keep the type in case it's useful?
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.
makes the types too narrow and difficult to consume downstream. Same problem as #4784 (comment)
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
@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.
|
…e they are implicitly granted 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.
Did another pass. Wasn't able to get to the meat of the code today, so a lot of comments are relatively surface-level still. I still need to read through the CAIPs to understand more context, so I will try doing that tomorrow.
|
||
export const Caip25CaveatType = 'authorizedScopes'; | ||
|
||
export const Caip25CaveatFactoryFn = (value: Caip25CaveatValue) => { |
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.
Is the Fn
at the end of the name here necessary, since Factory
is already in the name?
Alternatively, is there anything really special about this function as opposed to other functions? What about calling this createCaip25Caveat
?
export const Caip25CaveatFactoryFn = (value: Caip25CaveatValue) => { | |
export const createCaip25Caveat = (value: Caip25CaveatValue) => { |
* @param caip25CaveatValue - The CAIP-25 permission caveat value to remove the scope from. | ||
* @returns The updated CAIP-25 permission caveat value. | ||
*/ | ||
export function removeScope( |
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.
If this function is intended to operate on a caveat, what are your thoughts on putting the caveat as the first argument? I feel like this would be a more intuitive interface than putting it second. It would also allow for adding more arguments in the future without needing to rearrange the order of the arguments.
}); | ||
}); | ||
|
||
it('builds the expected permission specification', () => { |
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 is "it" here? Is it the endowment, or the thing that builds the endowment?
expect(specification.endowmentGetter()).toBeNull(); | ||
}); | ||
|
||
it('builds the caveat', () => { |
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.
The "it" here seems to be different from the previous test.
It seems that in this test file, we are testing different parts of this module. If that is true, what are your thoughts on using describe
s to represent those parts so we know what exactly is being tested? For instance:
describe('caip25EndowmentBuilder', () => {
describe('specificationBuilder', () => {
it('builds the expected permission specification', () => {
// ...
});
});
});
describe('Caip25CaveatFactoryFn', () => {
it('builds the caveat', () => {
// ...
});
});
describe('Caip25CaveatMutatorFactories', () => {
describe('.authorizedScopes', () => {
describe('removeScope', () => {
// ...
});
describe('removeAccount', () => {
// ...
});
});
});
})); | ||
const MockScopeAssert = jest.mocked(ScopeAssert); | ||
|
||
const { removeAccount } = Caip25CaveatMutatorFactories[Caip25CaveatType]; |
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 seem to test removeAccount
as it comes from this Caip25CaveatMutatorFactories
object, but we are testing removeScope
as it is exported from the whole module, even though this object also has a removeScope
method. Is that intentional?
return mergedScopeObject; | ||
}; | ||
|
||
export const mergeScopes = ( |
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, it looks like we have two similarly sounding functions. I can see why: one operates on a pair of ScopeObject
s, whereas this one operates on a pair of ScopesObject
s. That seems somewhat confusing.
I know naming is hard, but since ScopesObject
organizes ScopeObject
s by scopeString
, maybe it's worth encoding that into the name somehow? Perhaps literally scopeObjectsByScopeString
. Then this function would become mergeScopeObjectsByScopeString
(which is an accidental play on words, I know, but I think still makes sense?).
|
||
describe('Scope Validation', () => { | ||
describe('isValidScope', () => { | ||
it.each([ |
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.
Similar comment as elsewhere. I am finding this a bit difficult to read in its current form. I end up having to go back and forth between the data that's used to construct the test and the actual body of the test to understand what each of these values means and how they're used. What are your thoughts on writing these tests out?
it('does not throw an error if required scopes are defined but none are valid', () => { | ||
expect( | ||
validateScopes( | ||
{ 'eip155:1': {} as unknown as ExternalScopeObject }, |
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.
If we know that this is not an ExternalScopeObject
and so we expect this to fail type checking, should we indicate that instead?
{ 'eip155:1': {} as unknown as ExternalScopeObject }, | |
// @ts-expect-error Intentionally invalid input | |
{ 'eip155:1': {} }, |
(similar for below cases)
@@ -3,8 +3,16 @@ | |||
"compilerOptions": { | |||
"baseUrl": "./", | |||
"outDir": "./dist", | |||
"rootDir": "./src" | |||
"rootDir": "./src", | |||
"resolveJsonModule": 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.
What are we using this for? I didn't see a JSON file being imported, but I might have missed 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.
api-specs imports json under the hood
type Hex, | ||
type NonEmptyArray, | ||
} from '@metamask/utils'; | ||
import { strict as assert } from 'assert'; |
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.
Note that this makes this module dependent on Node (or at least, it means that whatever bundler the consumer is using needs to fill in this module). Is that intentional? Is there a way to use plain JavaScript instead?
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
Explanation
This PR updates
@metamask/multichain
to provide types, CAIP-25 permission, and helpers/adapters for the new permission, which can be shared across the extension & mobile clients.These tools and utilities will be used in both clients (mobile + extension)'s multichain API implementations.
File Overview
packages/multichain/src/adapters/
: Helpers that get and set legacy permission values from and to the new CAIP-25 permissionpackages/multichain/src/caip25Permission.ts
: Constants, types, mutators, and a specification builder for a CAIP-25 permissionpackages/multichain/src/index.ts
: Barrel exportpackages/multichain/src/scope/
: Types for CAIP-217 and our internal normalized/flattened version of them. Additionally contains helpers for validating shape, normalizing/merging, and checking support (i.e. if the wallet is able to serve the chain with it's requested methods and notifications)References
Upstream: #4812
Downstream: #4813
Key Multichain API Standards implemented here:
scope
defintionOpen PR that uses this new package for migrating the legacy permissions to CAIP-25 permission in the extension: MetaMask/metamask-extension#27847
Changelog
@metamask/multichain
Checklist