feat: create merger and factory for the Caip25Permission #5283
Conversation
| }); | ||
| }, | ||
| // TODO: Update Caip25EndowmentSpecification type with proper merger type | ||
| merger: (leftValue: Caip25CaveatValue, rightValue: Caip25CaveatValue) => { |
There was a problem hiding this comment.
the merger should actually be defined on the caveat of the permission, not the actual permission itself
| 'optionalScopes', | ||
| ); | ||
|
|
||
| return [mergedValue, diff]; |
There was a problem hiding this comment.
I like your approach here! The mergedValue and diff MUST be a valid CAIP-25 caveat value, i.e. it must be of the shape
{
requiredScopes: {...},
optionalScopes: {...},
isMultichainOrigin,
...
}
so keep requiredScopes and optionalScopes separate
There was a problem hiding this comment.
This brings up an interesting point though, how do we want to handle diffs outside of requiredScopes and optionalScopes?
There was a problem hiding this comment.
If I understood what you mean, they are actually all kept separate.
Which is why if you notice mergeScopesForCaip25CaveatValue implementation, it will either take optionalScopes or requiredScopes to which it is merging (while keeping the other one untouched), so we call the function twice, one for optionalScopes and one for requiredScopes, merging each to their respective counterparts, but being separate from each other. Does this make sense ?
Regarding second question, yeah not sure how we want to handle diff outside optionalScopes or requiredScopes, so left it for now (by default it retains the value from leftValue aka original state
There was a problem hiding this comment.
after this refactor here 145cc29
I chose to go with keeping isMultichainOrigin from rightValue, since it's incoming request which should take precedence I believe ? Session properties are omitted though, but not sure which should be maintained or how that should behave.
| ): Caip25CaveatValue { | ||
| const newValue = cloneDeep(leftValue); | ||
|
|
||
| Object.entries(rightValue[scopeToMerge]).forEach( |
There was a problem hiding this comment.
hmm... We have a mergeScopes helper for NormalizedScopesObject already, but I guess that's technically not usable as is here
|
@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. |
| * @param scopeToMerge - The required or optional scopes from the [CAIP-25](https://github.com/ChainAgnostic/CAIPs/blob/main/CAIPs/caip-25.md) request. | ||
| * @returns The combined CAIP-25 permission caveat value. | ||
| */ | ||
| function mergeScopesForCaip25CaveatValue( |
There was a problem hiding this comment.
What are your thoughts on making the existing mergeScopes more generic so that it can handle both NormalizedScopesObjects and InternalScopesObjects OR renaming mergeScopes to mergeNormalizedScopes and adding a new mergeInternalScopes?
There was a problem hiding this comment.
I think renaming to mergeInternalScopes should be good for now (right ? We are actually merging InternalScopesObjects, not NormalizedScopesObjects).
Making a more generic approach in my mind should be good on a future refactor when we need to actually merge NormalizedScopesObjects, so basically abstracting the behaviour when it's actually necessary to be used. What do you think ?
| @@ -49,7 +49,7 @@ export { parseScopeString } from './scope/types'; | |||
| export { | |||
| normalizeScope, | |||
| mergeScopeObject, | |||
| mergeScopes, | |||
| mergeNormalizedScopes as mergeScopes, | |||
There was a problem hiding this comment.
This was done automatically via refactor shortcut, but raises a good question
Do we want to export this as mergeScopes so consumer doesn't have to change his implementation ?
Or regular name export mergeNormalizedScopes and refactor whoever is using this ?
Raising this question mainly as I'm unaware if for now we are the only consumers of this API or if external sources are using it
Explanation
Addressing https://github.com/MetaMask/MetaMask-planning/issues/4017
References
Changelog
@metamask/package-a@metamask/package-bChecklist