From d2409511c7d2f3ff7b19fadb6495d84230ea8be8 Mon Sep 17 00:00:00 2001 From: ffmcgee Date: Tue, 4 Feb 2025 14:29:55 +0100 Subject: [PATCH 01/17] feat: add merger function to caip25 specification builder --- packages/multichain/src/caip25Permission.ts | 74 +++++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/packages/multichain/src/caip25Permission.ts b/packages/multichain/src/caip25Permission.ts index 9a417f3c707..e2e3b4cfc3d 100644 --- a/packages/multichain/src/caip25Permission.ts +++ b/packages/multichain/src/caip25Permission.ts @@ -24,8 +24,10 @@ import { cloneDeep, isEqual } from 'lodash'; import { getEthAccounts } from './adapters/caip-permission-adapter-eth-accounts'; import { assertIsInternalScopesObject } from './scope/assert'; import { isSupportedScopeString } from './scope/supported'; +import { getUniqueArrayItems } from './scope/transform'; import { parseScopeString, + type InternalScopeString, type ExternalScopeString, type InternalScopeObject, type InternalScopesObject, @@ -187,6 +189,78 @@ const specificationBuilder: PermissionSpecificationBuilder< ); } }, + merger: (leftValue: Caip25CaveatValue, rightValue: Caip25CaveatValue) => { + const newValue = cloneDeep(leftValue); + + // TODO: util function + // TODO: address type assertion + Object.entries(rightValue.requiredScopes).forEach( + ([scopeString, rightScopeObject]) => { + const leftRequiredScopeObject = + newValue.requiredScopes[scopeString as InternalScopeString]; + if (!leftRequiredScopeObject) { + newValue.requiredScopes[scopeString as InternalScopeString] = + rightScopeObject; + } else { + newValue.requiredScopes[scopeString as InternalScopeString] = { + accounts: getUniqueArrayItems([ + ...leftRequiredScopeObject.accounts, + ...rightScopeObject.accounts, + ]), + }; + } + }, + ); + + Object.entries(rightValue.optionalScopes).forEach( + ([scopeString, rightScopeObject]) => { + const leftRequiredScopeObject = + newValue.optionalScopes[scopeString as InternalScopeString]; + if (!leftRequiredScopeObject) { + newValue.optionalScopes[scopeString as InternalScopeString] = + rightScopeObject; + } else { + newValue.optionalScopes[scopeString as InternalScopeString] = { + accounts: getUniqueArrayItems([ + ...leftRequiredScopeObject.accounts, + ...rightScopeObject.accounts, + ]), + }; + } + }, + ); + + const diff = cloneDeep(leftValue); + + // TODO: same for requiredScopes, util function, type assertion address + for (const [scopeString, mergedScopeObject] of Object.entries( + newValue.optionalScopes, + )) { + const originalScopeObject = + diff.optionalScopes[scopeString as InternalScopeString]; + console.log(scopeString); + if (originalScopeObject) { + const newAccounts = mergedScopeObject.accounts.filter( + (account) => + !diff.optionalScopes[ + scopeString as InternalScopeString + ]?.accounts.includes(account), + ); + if (newAccounts.length > 0) { + diff.optionalScopes[scopeString as InternalScopeString] = { + accounts: newAccounts, + }; + continue; + } + delete diff.optionalScopes[scopeString as InternalScopeString]; + } else { + diff.optionalScopes[scopeString as InternalScopeString] = + mergedScopeObject; + } + } + + return [newValue, diff]; + }, }; }; From 6fc8f084a2ff56376975511475052cf004e7aea8 Mon Sep 17 00:00:00 2001 From: ffmcgee Date: Tue, 4 Feb 2025 16:19:25 +0100 Subject: [PATCH 02/17] refactor: code clean up --- packages/multichain/src/caip25Permission.ts | 166 ++++++++++++-------- 1 file changed, 101 insertions(+), 65 deletions(-) diff --git a/packages/multichain/src/caip25Permission.ts b/packages/multichain/src/caip25Permission.ts index e2e3b4cfc3d..dbd673b02bd 100644 --- a/packages/multichain/src/caip25Permission.ts +++ b/packages/multichain/src/caip25Permission.ts @@ -67,6 +67,85 @@ export const createCaip25Caveat = (value: Caip25CaveatValue) => { }; }; +/** + * + * @param leftValue - The existing CAIP-25 permission caveat value. + * @param rightValue - The incoming CAIP-25 permission caveat value. + * @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. + */ +const mergeScopesForCaip25CaveatValue = ( + leftValue: Caip25CaveatValue, + rightValue: Caip25CaveatValue, + scopeToMerge: keyof Pick< + Caip25CaveatValue, + 'optionalScopes' | 'requiredScopes' + >, +): Caip25CaveatValue => { + const newValue = cloneDeep(leftValue); + Object.entries(rightValue[scopeToMerge]).forEach( + ([scopeString, rightScopeObject]) => { + const internalScopeString = scopeString as InternalScopeString; + const leftRequiredScopeObject = + newValue[scopeToMerge][internalScopeString]; + if (!leftRequiredScopeObject) { + newValue[scopeToMerge][internalScopeString] = rightScopeObject; + } else { + newValue[scopeToMerge][internalScopeString] = { + accounts: getUniqueArrayItems([ + ...leftRequiredScopeObject.accounts, + ...rightScopeObject.accounts, + ]), + }; + } + }, + ); + return newValue; +}; + +/** + * + * @param originalValue - The existing CAIP-25 permission caveat value. + * @param mergedValue - The result from merging existing and incoming CAIP-25 permission caveat values. + * @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 differential between original and merged CAIP-25 permission caveat values. + */ +const diffScopesForCaip25CaveatValue = ( + originalValue: Caip25CaveatValue, + mergedValue: Caip25CaveatValue, + scopeToMerge: keyof Pick< + Caip25CaveatValue, + 'optionalScopes' | 'requiredScopes' + >, +): Caip25CaveatValue => { + const diff = cloneDeep(originalValue); + for (const [scopeString, mergedScopeObject] of Object.entries( + mergedValue[scopeToMerge], + )) { + const internalScopeString = scopeString as InternalScopeString; + const originalScopeObject = diff[scopeToMerge][internalScopeString]; + + if (originalScopeObject) { + const newAccounts = mergedScopeObject.accounts.filter( + (account) => + !diff[scopeToMerge][ + scopeString as InternalScopeString + ]?.accounts.includes(account), + ); + if (newAccounts.length > 0) { + diff[scopeToMerge][internalScopeString] = { + accounts: newAccounts, + }; + continue; + } + delete diff[scopeToMerge][internalScopeString]; + } else { + diff[scopeToMerge][internalScopeString] = mergedScopeObject; + } + } + return diff; +}; + type Caip25EndowmentCaveatSpecificationBuilderOptions = { findNetworkClientIdByChainId: (chainId: Hex) => NetworkClientId; listAccounts: () => { address: Hex }[]; @@ -190,76 +269,33 @@ const specificationBuilder: PermissionSpecificationBuilder< } }, merger: (leftValue: Caip25CaveatValue, rightValue: Caip25CaveatValue) => { - const newValue = cloneDeep(leftValue); - - // TODO: util function - // TODO: address type assertion - Object.entries(rightValue.requiredScopes).forEach( - ([scopeString, rightScopeObject]) => { - const leftRequiredScopeObject = - newValue.requiredScopes[scopeString as InternalScopeString]; - if (!leftRequiredScopeObject) { - newValue.requiredScopes[scopeString as InternalScopeString] = - rightScopeObject; - } else { - newValue.requiredScopes[scopeString as InternalScopeString] = { - accounts: getUniqueArrayItems([ - ...leftRequiredScopeObject.accounts, - ...rightScopeObject.accounts, - ]), - }; - } - }, + // TODO: proper name for this guy + const partiallyMergedValue = mergeScopesForCaip25CaveatValue( + leftValue, + rightValue, + 'requiredScopes', ); - Object.entries(rightValue.optionalScopes).forEach( - ([scopeString, rightScopeObject]) => { - const leftRequiredScopeObject = - newValue.optionalScopes[scopeString as InternalScopeString]; - if (!leftRequiredScopeObject) { - newValue.optionalScopes[scopeString as InternalScopeString] = - rightScopeObject; - } else { - newValue.optionalScopes[scopeString as InternalScopeString] = { - accounts: getUniqueArrayItems([ - ...leftRequiredScopeObject.accounts, - ...rightScopeObject.accounts, - ]), - }; - } - }, + const mergedValue = mergeScopesForCaip25CaveatValue( + partiallyMergedValue, + rightValue, + 'optionalScopes', ); - const diff = cloneDeep(leftValue); - - // TODO: same for requiredScopes, util function, type assertion address - for (const [scopeString, mergedScopeObject] of Object.entries( - newValue.optionalScopes, - )) { - const originalScopeObject = - diff.optionalScopes[scopeString as InternalScopeString]; - console.log(scopeString); - if (originalScopeObject) { - const newAccounts = mergedScopeObject.accounts.filter( - (account) => - !diff.optionalScopes[ - scopeString as InternalScopeString - ]?.accounts.includes(account), - ); - if (newAccounts.length > 0) { - diff.optionalScopes[scopeString as InternalScopeString] = { - accounts: newAccounts, - }; - continue; - } - delete diff.optionalScopes[scopeString as InternalScopeString]; - } else { - diff.optionalScopes[scopeString as InternalScopeString] = - mergedScopeObject; - } - } + // TODO: proper name for this guy + const partialDiff = diffScopesForCaip25CaveatValue( + leftValue, + mergedValue, + 'requiredScopes', + ); + + const diff = diffScopesForCaip25CaveatValue( + partialDiff, + mergedValue, + 'optionalScopes', + ); - return [newValue, diff]; + return [mergedValue, diff]; }, }; }; From a057674864137468a52599e5e41269c7ba758ee8 Mon Sep 17 00:00:00 2001 From: ffmcgee Date: Tue, 4 Feb 2025 16:21:12 +0100 Subject: [PATCH 03/17] refactor: minor change --- packages/multichain/src/caip25Permission.ts | 164 ++++++++++---------- 1 file changed, 83 insertions(+), 81 deletions(-) diff --git a/packages/multichain/src/caip25Permission.ts b/packages/multichain/src/caip25Permission.ts index dbd673b02bd..e1d51fab2cf 100644 --- a/packages/multichain/src/caip25Permission.ts +++ b/packages/multichain/src/caip25Permission.ts @@ -67,85 +67,6 @@ export const createCaip25Caveat = (value: Caip25CaveatValue) => { }; }; -/** - * - * @param leftValue - The existing CAIP-25 permission caveat value. - * @param rightValue - The incoming CAIP-25 permission caveat value. - * @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. - */ -const mergeScopesForCaip25CaveatValue = ( - leftValue: Caip25CaveatValue, - rightValue: Caip25CaveatValue, - scopeToMerge: keyof Pick< - Caip25CaveatValue, - 'optionalScopes' | 'requiredScopes' - >, -): Caip25CaveatValue => { - const newValue = cloneDeep(leftValue); - Object.entries(rightValue[scopeToMerge]).forEach( - ([scopeString, rightScopeObject]) => { - const internalScopeString = scopeString as InternalScopeString; - const leftRequiredScopeObject = - newValue[scopeToMerge][internalScopeString]; - if (!leftRequiredScopeObject) { - newValue[scopeToMerge][internalScopeString] = rightScopeObject; - } else { - newValue[scopeToMerge][internalScopeString] = { - accounts: getUniqueArrayItems([ - ...leftRequiredScopeObject.accounts, - ...rightScopeObject.accounts, - ]), - }; - } - }, - ); - return newValue; -}; - -/** - * - * @param originalValue - The existing CAIP-25 permission caveat value. - * @param mergedValue - The result from merging existing and incoming CAIP-25 permission caveat values. - * @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 differential between original and merged CAIP-25 permission caveat values. - */ -const diffScopesForCaip25CaveatValue = ( - originalValue: Caip25CaveatValue, - mergedValue: Caip25CaveatValue, - scopeToMerge: keyof Pick< - Caip25CaveatValue, - 'optionalScopes' | 'requiredScopes' - >, -): Caip25CaveatValue => { - const diff = cloneDeep(originalValue); - for (const [scopeString, mergedScopeObject] of Object.entries( - mergedValue[scopeToMerge], - )) { - const internalScopeString = scopeString as InternalScopeString; - const originalScopeObject = diff[scopeToMerge][internalScopeString]; - - if (originalScopeObject) { - const newAccounts = mergedScopeObject.accounts.filter( - (account) => - !diff[scopeToMerge][ - scopeString as InternalScopeString - ]?.accounts.includes(account), - ); - if (newAccounts.length > 0) { - diff[scopeToMerge][internalScopeString] = { - accounts: newAccounts, - }; - continue; - } - delete diff[scopeToMerge][internalScopeString]; - } else { - diff[scopeToMerge][internalScopeString] = mergedScopeObject; - } - } - return diff; -}; - type Caip25EndowmentCaveatSpecificationBuilderOptions = { findNetworkClientIdByChainId: (chainId: Hex) => NetworkClientId; listAccounts: () => { address: Hex }[]; @@ -269,7 +190,6 @@ const specificationBuilder: PermissionSpecificationBuilder< } }, merger: (leftValue: Caip25CaveatValue, rightValue: Caip25CaveatValue) => { - // TODO: proper name for this guy const partiallyMergedValue = mergeScopesForCaip25CaveatValue( leftValue, rightValue, @@ -282,7 +202,6 @@ const specificationBuilder: PermissionSpecificationBuilder< 'optionalScopes', ); - // TODO: proper name for this guy const partialDiff = diffScopesForCaip25CaveatValue( leftValue, mergedValue, @@ -457,3 +376,86 @@ function removeScope( operation: CaveatMutatorOperation.RevokePermission, }; } + +/** + * + * @param leftValue - The existing CAIP-25 permission caveat value. + * @param rightValue - The incoming CAIP-25 permission caveat value. + * @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( + leftValue: Caip25CaveatValue, + rightValue: Caip25CaveatValue, + scopeToMerge: keyof Pick< + Caip25CaveatValue, + 'optionalScopes' | 'requiredScopes' + >, +): Caip25CaveatValue { + const newValue = cloneDeep(leftValue); + + Object.entries(rightValue[scopeToMerge]).forEach( + ([scopeString, rightScopeObject]) => { + const internalScopeString = scopeString as InternalScopeString; + const leftRequiredScopeObject = + newValue[scopeToMerge][internalScopeString]; + if (!leftRequiredScopeObject) { + newValue[scopeToMerge][internalScopeString] = rightScopeObject; + } else { + newValue[scopeToMerge][internalScopeString] = { + accounts: getUniqueArrayItems([ + ...leftRequiredScopeObject.accounts, + ...rightScopeObject.accounts, + ]), + }; + } + }, + ); + + return newValue; +} + +/** + * + * @param originalValue - The existing CAIP-25 permission caveat value. + * @param mergedValue - The result from merging existing and incoming CAIP-25 permission caveat values. + * @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 differential between original and merged CAIP-25 permission caveat values. + */ +function diffScopesForCaip25CaveatValue( + originalValue: Caip25CaveatValue, + mergedValue: Caip25CaveatValue, + scopeToMerge: keyof Pick< + Caip25CaveatValue, + 'optionalScopes' | 'requiredScopes' + >, +): Caip25CaveatValue { + const diff = cloneDeep(originalValue); + + for (const [scopeString, mergedScopeObject] of Object.entries( + mergedValue[scopeToMerge], + )) { + const internalScopeString = scopeString as InternalScopeString; + const originalScopeObject = diff[scopeToMerge][internalScopeString]; + + if (originalScopeObject) { + const newAccounts = mergedScopeObject.accounts.filter( + (account) => + !diff[scopeToMerge][ + scopeString as InternalScopeString + ]?.accounts.includes(account), + ); + if (newAccounts.length > 0) { + diff[scopeToMerge][internalScopeString] = { + accounts: newAccounts, + }; + continue; + } + delete diff[scopeToMerge][internalScopeString]; + } else { + diff[scopeToMerge][internalScopeString] = mergedScopeObject; + } + } + + return diff; +} From 19d87253d6b3b53048a56f0e4afe48a03757770d Mon Sep 17 00:00:00 2001 From: ffmcgee Date: Wed, 5 Feb 2025 15:58:32 +0100 Subject: [PATCH 04/17] test: unit tests for merger --- .../multichain/src/caip25Permission.test.ts | 208 ++++++++++++++++++ packages/multichain/src/caip25Permission.ts | 1 + 2 files changed, 209 insertions(+) diff --git a/packages/multichain/src/caip25Permission.test.ts b/packages/multichain/src/caip25Permission.test.ts index 4ae70e7c018..746b458cbf7 100644 --- a/packages/multichain/src/caip25Permission.test.ts +++ b/packages/multichain/src/caip25Permission.test.ts @@ -37,6 +37,7 @@ describe('caip25EndowmentBuilder', () => { endowmentGetter: expect.any(Function), allowedCaveats: [Caip25CaveatType], validator: expect.any(Function), + merger: expect.any(Function), }); expect(specification.endowmentGetter()).toBeNull(); @@ -470,6 +471,213 @@ describe('caip25EndowmentBuilder', () => { ); }); }); + + describe('permission merger', () => { + const initLeftValue: Caip25CaveatValue = { + optionalScopes: { + 'eip155:1': { + accounts: ['eip155:1:0xdead'], + }, + }, + requiredScopes: {}, + isMultichainOrigin: false, + }; + + // @ts-expect-error TODO: update type in caip25Permission + const { merger } = caip25EndowmentBuilder.specificationBuilder({ + methodHooks: { + findNetworkClientIdByChainId: jest.fn(), + listAccounts: jest.fn(), + }, + }); + + it.each<{ + rightValue: Caip25CaveatValue; + expectedMergedValue: Caip25CaveatValue; + expectedDiff: Caip25CaveatValue; + }>([ + { + rightValue: { + optionalScopes: { + 'eip155:1': { + accounts: ['eip155:1:0xbeef'], + }, + }, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedMergedValue: { + optionalScopes: { + 'eip155:1': { accounts: ['eip155:1:0xdead', 'eip155:1:0xbeef'] }, + }, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedDiff: { + optionalScopes: { + 'eip155:1': { + accounts: ['eip155:1:0xbeef'], + }, + }, + isMultichainOrigin: false, + requiredScopes: {}, + }, + }, + { + rightValue: { + optionalScopes: { + 'eip155:10': { + accounts: [], + }, + }, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedMergedValue: { + optionalScopes: { + 'eip155:1': { accounts: ['eip155:1:0xdead'] }, + 'eip155:10': { + accounts: [], + }, + }, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedDiff: { + optionalScopes: { + 'eip155:10': { + accounts: [], + }, + }, + isMultichainOrigin: false, + requiredScopes: {}, + }, + }, + { + rightValue: { + optionalScopes: { + 'eip155:10': { + accounts: ['eip155:10:0xbeef'], + }, + }, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedMergedValue: { + optionalScopes: { + 'eip155:1': { accounts: ['eip155:1:0xdead'] }, + 'eip155:10': { accounts: ['eip155:10:0xbeef'] }, + }, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedDiff: { + optionalScopes: { + 'eip155:10': { + accounts: ['eip155:10:0xbeef'], + }, + }, + isMultichainOrigin: false, + requiredScopes: {}, + }, + }, + { + rightValue: { + optionalScopes: { + 'eip155:1': { + accounts: ['eip155:1:0xdead', 'eip155:1:0xbeef'], + }, + 'eip155:10': { + accounts: ['eip155:10:0xdead', 'eip155:10:0xbeef'], + }, + }, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedMergedValue: { + optionalScopes: { + 'eip155:1': { accounts: ['eip155:1:0xdead', 'eip155:1:0xbeef'] }, + 'eip155:10': { accounts: ['eip155:10:0xdead', 'eip155:10:0xbeef'] }, + }, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedDiff: { + optionalScopes: { + 'eip155:1': { + accounts: ['eip155:1:0xbeef'], + }, + 'eip155:10': { + accounts: ['eip155:10:0xdead', 'eip155:10:0xbeef'], + }, + }, + isMultichainOrigin: false, + requiredScopes: {}, + }, + }, + { + rightValue: { + optionalScopes: { + 'eip155:1': { + accounts: ['eip155:1:0xdead', 'eip155:1:0xbadd'], + }, + 'eip155:10': { + accounts: ['eip155:10:0xbeef', 'eip155:10:0xbadd'], + }, + 'eip155:426161': { + accounts: [ + 'eip155:426161:0xdead', + 'eip155:426161:0xbeef', + 'eip155:426161:0xbadd', + ], + }, + }, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedMergedValue: { + optionalScopes: { + 'eip155:1': { accounts: ['eip155:1:0xdead', 'eip155:1:0xbadd'] }, + 'eip155:10': { accounts: ['eip155:10:0xbeef', 'eip155:10:0xbadd'] }, + 'eip155:426161': { + accounts: [ + 'eip155:426161:0xdead', + 'eip155:426161:0xbeef', + 'eip155:426161:0xbadd', + ], + }, + }, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedDiff: { + optionalScopes: { + 'eip155:1': { accounts: ['eip155:1:0xbadd'] }, + 'eip155:10': { accounts: ['eip155:10:0xbeef', 'eip155:10:0xbadd'] }, + 'eip155:426161': { + accounts: [ + 'eip155:426161:0xdead', + 'eip155:426161:0xbeef', + 'eip155:426161:0xbadd', + ], + }, + }, + isMultichainOrigin: false, + requiredScopes: {}, + }, + }, + ])( + 'should return the expected merged value $expectedMergedValue and expected diff $expectedDiff', + async ({ rightValue, expectedMergedValue, expectedDiff }) => { + const [newValue, diff] = merger(initLeftValue, rightValue); + + expect(newValue).toStrictEqual( + expect.objectContaining(expectedMergedValue), + ); + expect(diff).toStrictEqual(expect.objectContaining(expectedDiff)); + }, + ); + }); }); describe('caip25CaveatBuilder', () => { diff --git a/packages/multichain/src/caip25Permission.ts b/packages/multichain/src/caip25Permission.ts index e1d51fab2cf..3a3a2dfb9d9 100644 --- a/packages/multichain/src/caip25Permission.ts +++ b/packages/multichain/src/caip25Permission.ts @@ -189,6 +189,7 @@ const specificationBuilder: PermissionSpecificationBuilder< ); } }, + // TODO: Update Caip25EndowmentSpecification type with proper merger type merger: (leftValue: Caip25CaveatValue, rightValue: Caip25CaveatValue) => { const partiallyMergedValue = mergeScopesForCaip25CaveatValue( leftValue, From de396ba5e41602446cc5e7b055a5c18d262ef7e6 Mon Sep 17 00:00:00 2001 From: ffmcgee Date: Wed, 5 Feb 2025 16:07:14 +0100 Subject: [PATCH 05/17] feat: add factory method to caip25 specification builder --- packages/multichain/src/caip25Permission.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/multichain/src/caip25Permission.ts b/packages/multichain/src/caip25Permission.ts index 3a3a2dfb9d9..ba3902ac679 100644 --- a/packages/multichain/src/caip25Permission.ts +++ b/packages/multichain/src/caip25Permission.ts @@ -4,12 +4,14 @@ import type { EndowmentGetterParams, ValidPermissionSpecification, PermissionValidatorConstraint, + PermissionFactory, PermissionConstraint, EndowmentCaveatSpecificationConstraint, } from '@metamask/permission-controller'; import { CaveatMutatorOperation, PermissionType, + constructPermission, } from '@metamask/permission-controller'; import type { CaipAccountId, Json } from '@metamask/utils'; import { @@ -160,6 +162,7 @@ type Caip25EndowmentSpecification = ValidPermissionSpecification<{ targetName: typeof Caip25EndowmentPermissionName; endowmentGetter: (_options?: EndowmentGetterParams) => null; validator: PermissionValidatorConstraint; + factory: PermissionFactory>; allowedCaveats: Readonly> | null; }>; @@ -189,6 +192,11 @@ const specificationBuilder: PermissionSpecificationBuilder< ); } }, + factory: (permissionOptions, _requestData) => { + return constructPermission({ + ...permissionOptions, + }); + }, // TODO: Update Caip25EndowmentSpecification type with proper merger type merger: (leftValue: Caip25CaveatValue, rightValue: Caip25CaveatValue) => { const partiallyMergedValue = mergeScopesForCaip25CaveatValue( From 058a08cd09f4f32d9a97379d5f98e5599f4aab51 Mon Sep 17 00:00:00 2001 From: ffmcgee Date: Wed, 5 Feb 2025 18:18:27 +0100 Subject: [PATCH 06/17] address code review --- .../multichain/src/caip25Permission.test.ts | 415 +++++++++--------- packages/multichain/src/caip25Permission.ts | 62 +-- 2 files changed, 239 insertions(+), 238 deletions(-) diff --git a/packages/multichain/src/caip25Permission.test.ts b/packages/multichain/src/caip25Permission.test.ts index 746b458cbf7..e4f42339641 100644 --- a/packages/multichain/src/caip25Permission.test.ts +++ b/packages/multichain/src/caip25Permission.test.ts @@ -37,7 +37,6 @@ describe('caip25EndowmentBuilder', () => { endowmentGetter: expect.any(Function), allowedCaveats: [Caip25CaveatType], validator: expect.any(Function), - merger: expect.any(Function), }); expect(specification.endowmentGetter()).toBeNull(); @@ -471,219 +470,12 @@ describe('caip25EndowmentBuilder', () => { ); }); }); - - describe('permission merger', () => { - const initLeftValue: Caip25CaveatValue = { - optionalScopes: { - 'eip155:1': { - accounts: ['eip155:1:0xdead'], - }, - }, - requiredScopes: {}, - isMultichainOrigin: false, - }; - - // @ts-expect-error TODO: update type in caip25Permission - const { merger } = caip25EndowmentBuilder.specificationBuilder({ - methodHooks: { - findNetworkClientIdByChainId: jest.fn(), - listAccounts: jest.fn(), - }, - }); - - it.each<{ - rightValue: Caip25CaveatValue; - expectedMergedValue: Caip25CaveatValue; - expectedDiff: Caip25CaveatValue; - }>([ - { - rightValue: { - optionalScopes: { - 'eip155:1': { - accounts: ['eip155:1:0xbeef'], - }, - }, - requiredScopes: {}, - isMultichainOrigin: false, - }, - expectedMergedValue: { - optionalScopes: { - 'eip155:1': { accounts: ['eip155:1:0xdead', 'eip155:1:0xbeef'] }, - }, - requiredScopes: {}, - isMultichainOrigin: false, - }, - expectedDiff: { - optionalScopes: { - 'eip155:1': { - accounts: ['eip155:1:0xbeef'], - }, - }, - isMultichainOrigin: false, - requiredScopes: {}, - }, - }, - { - rightValue: { - optionalScopes: { - 'eip155:10': { - accounts: [], - }, - }, - requiredScopes: {}, - isMultichainOrigin: false, - }, - expectedMergedValue: { - optionalScopes: { - 'eip155:1': { accounts: ['eip155:1:0xdead'] }, - 'eip155:10': { - accounts: [], - }, - }, - requiredScopes: {}, - isMultichainOrigin: false, - }, - expectedDiff: { - optionalScopes: { - 'eip155:10': { - accounts: [], - }, - }, - isMultichainOrigin: false, - requiredScopes: {}, - }, - }, - { - rightValue: { - optionalScopes: { - 'eip155:10': { - accounts: ['eip155:10:0xbeef'], - }, - }, - requiredScopes: {}, - isMultichainOrigin: false, - }, - expectedMergedValue: { - optionalScopes: { - 'eip155:1': { accounts: ['eip155:1:0xdead'] }, - 'eip155:10': { accounts: ['eip155:10:0xbeef'] }, - }, - requiredScopes: {}, - isMultichainOrigin: false, - }, - expectedDiff: { - optionalScopes: { - 'eip155:10': { - accounts: ['eip155:10:0xbeef'], - }, - }, - isMultichainOrigin: false, - requiredScopes: {}, - }, - }, - { - rightValue: { - optionalScopes: { - 'eip155:1': { - accounts: ['eip155:1:0xdead', 'eip155:1:0xbeef'], - }, - 'eip155:10': { - accounts: ['eip155:10:0xdead', 'eip155:10:0xbeef'], - }, - }, - requiredScopes: {}, - isMultichainOrigin: false, - }, - expectedMergedValue: { - optionalScopes: { - 'eip155:1': { accounts: ['eip155:1:0xdead', 'eip155:1:0xbeef'] }, - 'eip155:10': { accounts: ['eip155:10:0xdead', 'eip155:10:0xbeef'] }, - }, - requiredScopes: {}, - isMultichainOrigin: false, - }, - expectedDiff: { - optionalScopes: { - 'eip155:1': { - accounts: ['eip155:1:0xbeef'], - }, - 'eip155:10': { - accounts: ['eip155:10:0xdead', 'eip155:10:0xbeef'], - }, - }, - isMultichainOrigin: false, - requiredScopes: {}, - }, - }, - { - rightValue: { - optionalScopes: { - 'eip155:1': { - accounts: ['eip155:1:0xdead', 'eip155:1:0xbadd'], - }, - 'eip155:10': { - accounts: ['eip155:10:0xbeef', 'eip155:10:0xbadd'], - }, - 'eip155:426161': { - accounts: [ - 'eip155:426161:0xdead', - 'eip155:426161:0xbeef', - 'eip155:426161:0xbadd', - ], - }, - }, - requiredScopes: {}, - isMultichainOrigin: false, - }, - expectedMergedValue: { - optionalScopes: { - 'eip155:1': { accounts: ['eip155:1:0xdead', 'eip155:1:0xbadd'] }, - 'eip155:10': { accounts: ['eip155:10:0xbeef', 'eip155:10:0xbadd'] }, - 'eip155:426161': { - accounts: [ - 'eip155:426161:0xdead', - 'eip155:426161:0xbeef', - 'eip155:426161:0xbadd', - ], - }, - }, - requiredScopes: {}, - isMultichainOrigin: false, - }, - expectedDiff: { - optionalScopes: { - 'eip155:1': { accounts: ['eip155:1:0xbadd'] }, - 'eip155:10': { accounts: ['eip155:10:0xbeef', 'eip155:10:0xbadd'] }, - 'eip155:426161': { - accounts: [ - 'eip155:426161:0xdead', - 'eip155:426161:0xbeef', - 'eip155:426161:0xbadd', - ], - }, - }, - isMultichainOrigin: false, - requiredScopes: {}, - }, - }, - ])( - 'should return the expected merged value $expectedMergedValue and expected diff $expectedDiff', - async ({ rightValue, expectedMergedValue, expectedDiff }) => { - const [newValue, diff] = merger(initLeftValue, rightValue); - - expect(newValue).toStrictEqual( - expect.objectContaining(expectedMergedValue), - ); - expect(diff).toStrictEqual(expect.objectContaining(expectedDiff)); - }, - ); - }); }); describe('caip25CaveatBuilder', () => { const findNetworkClientIdByChainId = jest.fn(); const listAccounts = jest.fn(); - const { validator } = caip25CaveatBuilder({ + const { validator, merger } = caip25CaveatBuilder({ findNetworkClientIdByChainId, listAccounts, }); @@ -905,5 +697,210 @@ describe('caip25CaveatBuilder', () => { }, }), ).toBeUndefined(); + + describe('permission merger', () => { + const initLeftValue: Caip25CaveatValue = { + optionalScopes: { + 'eip155:1': { + accounts: ['eip155:1:0xdead'], + }, + }, + requiredScopes: {}, + isMultichainOrigin: false, + }; + + it.each<{ + rightValue: Caip25CaveatValue; + expectedMergedValue: Caip25CaveatValue; + expectedDiff: Caip25CaveatValue; + }>([ + { + rightValue: { + optionalScopes: { + 'eip155:1': { + accounts: ['eip155:1:0xbeef'], + }, + }, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedMergedValue: { + optionalScopes: { + 'eip155:1': { accounts: ['eip155:1:0xdead', 'eip155:1:0xbeef'] }, + }, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedDiff: { + optionalScopes: { + 'eip155:1': { + accounts: ['eip155:1:0xbeef'], + }, + }, + isMultichainOrigin: false, + requiredScopes: {}, + }, + }, + { + rightValue: { + optionalScopes: { + 'eip155:10': { + accounts: [], + }, + }, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedMergedValue: { + optionalScopes: { + 'eip155:1': { accounts: ['eip155:1:0xdead'] }, + 'eip155:10': { + accounts: [], + }, + }, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedDiff: { + optionalScopes: { + 'eip155:10': { + accounts: [], + }, + }, + isMultichainOrigin: false, + requiredScopes: {}, + }, + }, + { + rightValue: { + optionalScopes: { + 'eip155:10': { + accounts: ['eip155:10:0xbeef'], + }, + }, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedMergedValue: { + optionalScopes: { + 'eip155:1': { accounts: ['eip155:1:0xdead'] }, + 'eip155:10': { accounts: ['eip155:10:0xbeef'] }, + }, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedDiff: { + optionalScopes: { + 'eip155:10': { + accounts: ['eip155:10:0xbeef'], + }, + }, + isMultichainOrigin: false, + requiredScopes: {}, + }, + }, + { + rightValue: { + optionalScopes: { + 'eip155:1': { + accounts: ['eip155:1:0xdead', 'eip155:1:0xbeef'], + }, + 'eip155:10': { + accounts: ['eip155:10:0xdead', 'eip155:10:0xbeef'], + }, + }, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedMergedValue: { + optionalScopes: { + 'eip155:1': { accounts: ['eip155:1:0xdead', 'eip155:1:0xbeef'] }, + 'eip155:10': { + accounts: ['eip155:10:0xdead', 'eip155:10:0xbeef'], + }, + }, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedDiff: { + optionalScopes: { + 'eip155:1': { + accounts: ['eip155:1:0xbeef'], + }, + 'eip155:10': { + accounts: ['eip155:10:0xdead', 'eip155:10:0xbeef'], + }, + }, + isMultichainOrigin: false, + requiredScopes: {}, + }, + }, + { + rightValue: { + optionalScopes: { + 'eip155:1': { + accounts: ['eip155:1:0xdead', 'eip155:1:0xbadd'], + }, + 'eip155:10': { + accounts: ['eip155:10:0xbeef', 'eip155:10:0xbadd'], + }, + 'eip155:426161': { + accounts: [ + 'eip155:426161:0xdead', + 'eip155:426161:0xbeef', + 'eip155:426161:0xbadd', + ], + }, + }, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedMergedValue: { + optionalScopes: { + 'eip155:1': { accounts: ['eip155:1:0xdead', 'eip155:1:0xbadd'] }, + 'eip155:10': { + accounts: ['eip155:10:0xbeef', 'eip155:10:0xbadd'], + }, + 'eip155:426161': { + accounts: [ + 'eip155:426161:0xdead', + 'eip155:426161:0xbeef', + 'eip155:426161:0xbadd', + ], + }, + }, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedDiff: { + optionalScopes: { + 'eip155:1': { accounts: ['eip155:1:0xbadd'] }, + 'eip155:10': { + accounts: ['eip155:10:0xbeef', 'eip155:10:0xbadd'], + }, + 'eip155:426161': { + accounts: [ + 'eip155:426161:0xdead', + 'eip155:426161:0xbeef', + 'eip155:426161:0xbadd', + ], + }, + }, + isMultichainOrigin: false, + requiredScopes: {}, + }, + }, + ])( + 'should return the expected merged value $expectedMergedValue and expected diff $expectedDiff', + async ({ rightValue, expectedMergedValue, expectedDiff }) => { + const [newValue, diff] = merger(initLeftValue, rightValue); + + expect(newValue).toStrictEqual( + expect.objectContaining(expectedMergedValue), + ); + expect(diff).toStrictEqual(expect.objectContaining(expectedDiff)); + }, + ); + }); }); }); diff --git a/packages/multichain/src/caip25Permission.ts b/packages/multichain/src/caip25Permission.ts index ba3902ac679..a90402e56cd 100644 --- a/packages/multichain/src/caip25Permission.ts +++ b/packages/multichain/src/caip25Permission.ts @@ -87,7 +87,9 @@ export const caip25CaveatBuilder = ({ findNetworkClientIdByChainId, listAccounts, }: Caip25EndowmentCaveatSpecificationBuilderOptions): EndowmentCaveatSpecificationConstraint & - Required> => { + Required< + Pick + > => { return { type: Caip25CaveatType, validator: ( @@ -154,6 +156,36 @@ export const caip25CaveatBuilder = ({ ); } }, + merger: ( + leftValue: Caip25CaveatValue, + rightValue: Caip25CaveatValue, + ): [Caip25CaveatValue, Caip25CaveatValue] => { + const partiallyMergedValue = mergeScopesForCaip25CaveatValue( + leftValue, + rightValue, + 'requiredScopes', + ); + + const mergedValue = mergeScopesForCaip25CaveatValue( + partiallyMergedValue, + rightValue, + 'optionalScopes', + ); + + const partialDiff = diffScopesForCaip25CaveatValue( + leftValue, + mergedValue, + 'requiredScopes', + ); + + const diff = diffScopesForCaip25CaveatValue( + partialDiff, + mergedValue, + 'optionalScopes', + ); + + return [mergedValue, diff]; + }, }; }; @@ -197,34 +229,6 @@ const specificationBuilder: PermissionSpecificationBuilder< ...permissionOptions, }); }, - // TODO: Update Caip25EndowmentSpecification type with proper merger type - merger: (leftValue: Caip25CaveatValue, rightValue: Caip25CaveatValue) => { - const partiallyMergedValue = mergeScopesForCaip25CaveatValue( - leftValue, - rightValue, - 'requiredScopes', - ); - - const mergedValue = mergeScopesForCaip25CaveatValue( - partiallyMergedValue, - rightValue, - 'optionalScopes', - ); - - const partialDiff = diffScopesForCaip25CaveatValue( - leftValue, - mergedValue, - 'requiredScopes', - ); - - const diff = diffScopesForCaip25CaveatValue( - partialDiff, - mergedValue, - 'optionalScopes', - ); - - return [mergedValue, diff]; - }, }; }; From 29965df229d7ecb2601c57386a63b8dd7de8ab5c Mon Sep 17 00:00:00 2001 From: ffmcgee Date: Wed, 5 Feb 2025 20:07:32 +0100 Subject: [PATCH 07/17] test: fix nested describe block --- .../multichain/src/caip25Permission.test.ts | 340 +++++++++--------- 1 file changed, 170 insertions(+), 170 deletions(-) diff --git a/packages/multichain/src/caip25Permission.test.ts b/packages/multichain/src/caip25Permission.test.ts index e4f42339641..8b72ac21860 100644 --- a/packages/multichain/src/caip25Permission.test.ts +++ b/packages/multichain/src/caip25Permission.test.ts @@ -697,210 +697,210 @@ describe('caip25CaveatBuilder', () => { }, }), ).toBeUndefined(); + }); - describe('permission merger', () => { - const initLeftValue: Caip25CaveatValue = { - optionalScopes: { - 'eip155:1': { - accounts: ['eip155:1:0xdead'], - }, + describe('permission merger', () => { + const initLeftValue: Caip25CaveatValue = { + optionalScopes: { + 'eip155:1': { + accounts: ['eip155:1:0xdead'], }, - requiredScopes: {}, - isMultichainOrigin: false, - }; - - it.each<{ - rightValue: Caip25CaveatValue; - expectedMergedValue: Caip25CaveatValue; - expectedDiff: Caip25CaveatValue; - }>([ - { - rightValue: { - optionalScopes: { - 'eip155:1': { - accounts: ['eip155:1:0xbeef'], - }, + }, + requiredScopes: {}, + isMultichainOrigin: false, + }; + + it.each<{ + rightValue: Caip25CaveatValue; + expectedMergedValue: Caip25CaveatValue; + expectedDiff: Caip25CaveatValue; + }>([ + { + rightValue: { + optionalScopes: { + 'eip155:1': { + accounts: ['eip155:1:0xbeef'], }, - requiredScopes: {}, - isMultichainOrigin: false, }, - expectedMergedValue: { - optionalScopes: { - 'eip155:1': { accounts: ['eip155:1:0xdead', 'eip155:1:0xbeef'] }, - }, - requiredScopes: {}, - isMultichainOrigin: false, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedMergedValue: { + optionalScopes: { + 'eip155:1': { accounts: ['eip155:1:0xdead', 'eip155:1:0xbeef'] }, }, - expectedDiff: { - optionalScopes: { - 'eip155:1': { - accounts: ['eip155:1:0xbeef'], - }, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedDiff: { + optionalScopes: { + 'eip155:1': { + accounts: ['eip155:1:0xbeef'], }, - isMultichainOrigin: false, - requiredScopes: {}, }, + isMultichainOrigin: false, + requiredScopes: {}, }, - { - rightValue: { - optionalScopes: { - 'eip155:10': { - accounts: [], - }, + }, + { + rightValue: { + optionalScopes: { + 'eip155:10': { + accounts: [], }, - requiredScopes: {}, - isMultichainOrigin: false, }, - expectedMergedValue: { - optionalScopes: { - 'eip155:1': { accounts: ['eip155:1:0xdead'] }, - 'eip155:10': { - accounts: [], - }, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedMergedValue: { + optionalScopes: { + 'eip155:1': { accounts: ['eip155:1:0xdead'] }, + 'eip155:10': { + accounts: [], }, - requiredScopes: {}, - isMultichainOrigin: false, }, - expectedDiff: { - optionalScopes: { - 'eip155:10': { - accounts: [], - }, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedDiff: { + optionalScopes: { + 'eip155:10': { + accounts: [], }, - isMultichainOrigin: false, - requiredScopes: {}, }, + isMultichainOrigin: false, + requiredScopes: {}, }, - { - rightValue: { - optionalScopes: { - 'eip155:10': { - accounts: ['eip155:10:0xbeef'], - }, + }, + { + rightValue: { + optionalScopes: { + 'eip155:10': { + accounts: ['eip155:10:0xbeef'], }, - requiredScopes: {}, - isMultichainOrigin: false, }, - expectedMergedValue: { - optionalScopes: { - 'eip155:1': { accounts: ['eip155:1:0xdead'] }, - 'eip155:10': { accounts: ['eip155:10:0xbeef'] }, - }, - requiredScopes: {}, - isMultichainOrigin: false, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedMergedValue: { + optionalScopes: { + 'eip155:1': { accounts: ['eip155:1:0xdead'] }, + 'eip155:10': { accounts: ['eip155:10:0xbeef'] }, }, - expectedDiff: { - optionalScopes: { - 'eip155:10': { - accounts: ['eip155:10:0xbeef'], - }, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedDiff: { + optionalScopes: { + 'eip155:10': { + accounts: ['eip155:10:0xbeef'], }, - isMultichainOrigin: false, - requiredScopes: {}, }, + isMultichainOrigin: false, + requiredScopes: {}, }, - { - rightValue: { - optionalScopes: { - 'eip155:1': { - accounts: ['eip155:1:0xdead', 'eip155:1:0xbeef'], - }, - 'eip155:10': { - accounts: ['eip155:10:0xdead', 'eip155:10:0xbeef'], - }, + }, + { + rightValue: { + optionalScopes: { + 'eip155:1': { + accounts: ['eip155:1:0xdead', 'eip155:1:0xbeef'], }, - requiredScopes: {}, - isMultichainOrigin: false, - }, - expectedMergedValue: { - optionalScopes: { - 'eip155:1': { accounts: ['eip155:1:0xdead', 'eip155:1:0xbeef'] }, - 'eip155:10': { - accounts: ['eip155:10:0xdead', 'eip155:10:0xbeef'], - }, + 'eip155:10': { + accounts: ['eip155:10:0xdead', 'eip155:10:0xbeef'], }, - requiredScopes: {}, - isMultichainOrigin: false, }, - expectedDiff: { - optionalScopes: { - 'eip155:1': { - accounts: ['eip155:1:0xbeef'], - }, - 'eip155:10': { - accounts: ['eip155:10:0xdead', 'eip155:10:0xbeef'], - }, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedMergedValue: { + optionalScopes: { + 'eip155:1': { accounts: ['eip155:1:0xdead', 'eip155:1:0xbeef'] }, + 'eip155:10': { + accounts: ['eip155:10:0xdead', 'eip155:10:0xbeef'], }, - isMultichainOrigin: false, - requiredScopes: {}, }, + requiredScopes: {}, + isMultichainOrigin: false, }, - { - rightValue: { - optionalScopes: { - 'eip155:1': { - accounts: ['eip155:1:0xdead', 'eip155:1:0xbadd'], - }, - 'eip155:10': { - accounts: ['eip155:10:0xbeef', 'eip155:10:0xbadd'], - }, - 'eip155:426161': { - accounts: [ - 'eip155:426161:0xdead', - 'eip155:426161:0xbeef', - 'eip155:426161:0xbadd', - ], - }, + expectedDiff: { + optionalScopes: { + 'eip155:1': { + accounts: ['eip155:1:0xbeef'], + }, + 'eip155:10': { + accounts: ['eip155:10:0xdead', 'eip155:10:0xbeef'], }, - requiredScopes: {}, - isMultichainOrigin: false, }, - expectedMergedValue: { - optionalScopes: { - 'eip155:1': { accounts: ['eip155:1:0xdead', 'eip155:1:0xbadd'] }, - 'eip155:10': { - accounts: ['eip155:10:0xbeef', 'eip155:10:0xbadd'], - }, - 'eip155:426161': { - accounts: [ - 'eip155:426161:0xdead', - 'eip155:426161:0xbeef', - 'eip155:426161:0xbadd', - ], - }, + isMultichainOrigin: false, + requiredScopes: {}, + }, + }, + { + rightValue: { + optionalScopes: { + 'eip155:1': { + accounts: ['eip155:1:0xdead', 'eip155:1:0xbadd'], + }, + 'eip155:10': { + accounts: ['eip155:10:0xbeef', 'eip155:10:0xbadd'], + }, + 'eip155:426161': { + accounts: [ + 'eip155:426161:0xdead', + 'eip155:426161:0xbeef', + 'eip155:426161:0xbadd', + ], }, - requiredScopes: {}, - isMultichainOrigin: false, }, - expectedDiff: { - optionalScopes: { - 'eip155:1': { accounts: ['eip155:1:0xbadd'] }, - 'eip155:10': { - accounts: ['eip155:10:0xbeef', 'eip155:10:0xbadd'], - }, - 'eip155:426161': { - accounts: [ - 'eip155:426161:0xdead', - 'eip155:426161:0xbeef', - 'eip155:426161:0xbadd', - ], - }, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedMergedValue: { + optionalScopes: { + 'eip155:1': { accounts: ['eip155:1:0xdead', 'eip155:1:0xbadd'] }, + 'eip155:10': { + accounts: ['eip155:10:0xbeef', 'eip155:10:0xbadd'], + }, + 'eip155:426161': { + accounts: [ + 'eip155:426161:0xdead', + 'eip155:426161:0xbeef', + 'eip155:426161:0xbadd', + ], }, - isMultichainOrigin: false, - requiredScopes: {}, }, + requiredScopes: {}, + isMultichainOrigin: false, }, - ])( - 'should return the expected merged value $expectedMergedValue and expected diff $expectedDiff', - async ({ rightValue, expectedMergedValue, expectedDiff }) => { - const [newValue, diff] = merger(initLeftValue, rightValue); - - expect(newValue).toStrictEqual( - expect.objectContaining(expectedMergedValue), - ); - expect(diff).toStrictEqual(expect.objectContaining(expectedDiff)); + expectedDiff: { + optionalScopes: { + 'eip155:1': { accounts: ['eip155:1:0xbadd'] }, + 'eip155:10': { + accounts: ['eip155:10:0xbeef', 'eip155:10:0xbadd'], + }, + 'eip155:426161': { + accounts: [ + 'eip155:426161:0xdead', + 'eip155:426161:0xbeef', + 'eip155:426161:0xbadd', + ], + }, + }, + isMultichainOrigin: false, + requiredScopes: {}, }, - ); - }); + }, + ])( + 'should return the expected merged value $expectedMergedValue and expected diff $expectedDiff', + async ({ rightValue, expectedMergedValue, expectedDiff }) => { + const [newValue, diff] = merger(initLeftValue, rightValue); + + expect(newValue).toStrictEqual( + expect.objectContaining(expectedMergedValue), + ); + expect(diff).toStrictEqual(expect.objectContaining(expectedDiff)); + }, + ); }); }); From beda24f4fd9ba68d88cfc41bdea35935e3977bd6 Mon Sep 17 00:00:00 2001 From: ffmcgee Date: Wed, 5 Feb 2025 20:41:42 +0100 Subject: [PATCH 08/17] test: fix builds the expected permission specification after factory func --- packages/multichain/src/caip25Permission.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/multichain/src/caip25Permission.test.ts b/packages/multichain/src/caip25Permission.test.ts index 8b72ac21860..48d7972db77 100644 --- a/packages/multichain/src/caip25Permission.test.ts +++ b/packages/multichain/src/caip25Permission.test.ts @@ -37,6 +37,7 @@ describe('caip25EndowmentBuilder', () => { endowmentGetter: expect.any(Function), allowedCaveats: [Caip25CaveatType], validator: expect.any(Function), + factory: expect.any(Function), }); expect(specification.endowmentGetter()).toBeNull(); From 99939da3e1ec7c090212e61670874c2c097b3c78 Mon Sep 17 00:00:00 2001 From: ffmcgee Date: Thu, 6 Feb 2025 12:04:12 +0100 Subject: [PATCH 09/17] test: add test for factory method --- .../multichain/src/caip25Permission.test.ts | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/packages/multichain/src/caip25Permission.test.ts b/packages/multichain/src/caip25Permission.test.ts index 48d7972db77..0839c37f2b2 100644 --- a/packages/multichain/src/caip25Permission.test.ts +++ b/packages/multichain/src/caip25Permission.test.ts @@ -1,6 +1,9 @@ import { CaveatMutatorOperation, + type PermissionConstraint, + type PermissionOptions, PermissionType, + constructPermission, } from '@metamask/permission-controller'; import type { Caip25CaveatValue } from './caip25Permission'; @@ -20,6 +23,11 @@ jest.mock('./scope/supported', () => ({ })); const MockScopeSupported = jest.mocked(ScopeSupported); +jest.mock('@metamask/permission-controller', () => ({ + ...jest.requireActual('@metamask/permission-controller'), + constructPermission: jest.fn(), +})); + const { removeAccount, removeScope } = Caip25CaveatMutators[Caip25CaveatType]; describe('caip25EndowmentBuilder', () => { @@ -42,6 +50,35 @@ describe('caip25EndowmentBuilder', () => { expect(specification.endowmentGetter()).toBeNull(); }); + + describe('factory', () => { + const { factory } = caip25EndowmentBuilder.specificationBuilder({ + methodHooks: { + findNetworkClientIdByChainId: jest.fn(), + listAccounts: jest.fn(), + }, + }); + + it('should call `constructPermission` with provided options', () => { + const mockRequestData = { origin: 'test.com' }; + const mockOptions: PermissionOptions = { + caveats: [ + { + type: Caip25CaveatType, + value: { + requiredScopes: {}, + optionalScopes: {}, + isMultichainOrigin: true, + }, + }, + ], + target: Caip25EndowmentPermissionName, + invoker: 'test.com', + }; + factory(mockOptions, mockRequestData); + expect(constructPermission).toHaveBeenCalledWith({ ...mockOptions }); + }); + }); }); describe('createCaip25Caveat', () => { From 145cc29e6b83ef513f60d76c31e08d63a8ec9590 Mon Sep 17 00:00:00 2001 From: ffmcgee Date: Wed, 12 Feb 2025 00:20:53 +0100 Subject: [PATCH 10/17] refactor: code review changes --- .../caip-permission-adapter-session-scopes.ts | 4 +- packages/multichain/src/caip25Permission.ts | 61 ++++--------------- packages/multichain/src/index.ts | 2 +- .../multichain/src/scope/transform.test.ts | 10 +-- packages/multichain/src/scope/transform.ts | 44 +++++++++++-- 5 files changed, 60 insertions(+), 61 deletions(-) diff --git a/packages/multichain/src/adapters/caip-permission-adapter-session-scopes.ts b/packages/multichain/src/adapters/caip-permission-adapter-session-scopes.ts index 7e05eb01ad3..cfabd575ce7 100644 --- a/packages/multichain/src/adapters/caip-permission-adapter-session-scopes.ts +++ b/packages/multichain/src/adapters/caip-permission-adapter-session-scopes.ts @@ -7,7 +7,7 @@ import { KnownWalletNamespaceRpcMethods, KnownWalletRpcMethods, } from '../scope/constants'; -import { mergeScopes } from '../scope/transform'; +import { mergeNormalizedScopes } from '../scope/transform'; import type { InternalScopesObject, NonWalletKnownCaipNamespace, @@ -94,7 +94,7 @@ export const getSessionScopes = ( 'requiredScopes' | 'optionalScopes' >, ) => { - return mergeScopes( + return mergeNormalizedScopes( getNormalizedScopesObject(caip25CaveatValue.requiredScopes), getNormalizedScopesObject(caip25CaveatValue.optionalScopes), ); diff --git a/packages/multichain/src/caip25Permission.ts b/packages/multichain/src/caip25Permission.ts index a90402e56cd..d203fad04d4 100644 --- a/packages/multichain/src/caip25Permission.ts +++ b/packages/multichain/src/caip25Permission.ts @@ -26,7 +26,7 @@ import { cloneDeep, isEqual } from 'lodash'; import { getEthAccounts } from './adapters/caip-permission-adapter-eth-accounts'; import { assertIsInternalScopesObject } from './scope/assert'; import { isSupportedScopeString } from './scope/supported'; -import { getUniqueArrayItems } from './scope/transform'; +import { mergeInternalScopes } from './scope/transform'; import { parseScopeString, type InternalScopeString, @@ -160,18 +160,21 @@ export const caip25CaveatBuilder = ({ leftValue: Caip25CaveatValue, rightValue: Caip25CaveatValue, ): [Caip25CaveatValue, Caip25CaveatValue] => { - const partiallyMergedValue = mergeScopesForCaip25CaveatValue( - leftValue, - rightValue, - 'requiredScopes', + const mergedRequiredScopes = mergeInternalScopes( + leftValue.requiredScopes, + rightValue.requiredScopes, ); - - const mergedValue = mergeScopesForCaip25CaveatValue( - partiallyMergedValue, - rightValue, - 'optionalScopes', + const mergedOptionalScopes = mergeInternalScopes( + leftValue.optionalScopes, + rightValue.optionalScopes, ); + const mergedValue: Caip25CaveatValue = { + requiredScopes: mergedRequiredScopes, + optionalScopes: mergedOptionalScopes, + isMultichainOrigin: rightValue.isMultichainOrigin, + }; + const partialDiff = diffScopesForCaip25CaveatValue( leftValue, mergedValue, @@ -390,44 +393,6 @@ function removeScope( }; } -/** - * - * @param leftValue - The existing CAIP-25 permission caveat value. - * @param rightValue - The incoming CAIP-25 permission caveat value. - * @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( - leftValue: Caip25CaveatValue, - rightValue: Caip25CaveatValue, - scopeToMerge: keyof Pick< - Caip25CaveatValue, - 'optionalScopes' | 'requiredScopes' - >, -): Caip25CaveatValue { - const newValue = cloneDeep(leftValue); - - Object.entries(rightValue[scopeToMerge]).forEach( - ([scopeString, rightScopeObject]) => { - const internalScopeString = scopeString as InternalScopeString; - const leftRequiredScopeObject = - newValue[scopeToMerge][internalScopeString]; - if (!leftRequiredScopeObject) { - newValue[scopeToMerge][internalScopeString] = rightScopeObject; - } else { - newValue[scopeToMerge][internalScopeString] = { - accounts: getUniqueArrayItems([ - ...leftRequiredScopeObject.accounts, - ...rightScopeObject.accounts, - ]), - }; - } - }, - ); - - return newValue; -} - /** * * @param originalValue - The existing CAIP-25 permission caveat value. diff --git a/packages/multichain/src/index.ts b/packages/multichain/src/index.ts index 60732796b47..267bc895bc4 100644 --- a/packages/multichain/src/index.ts +++ b/packages/multichain/src/index.ts @@ -49,7 +49,7 @@ export { parseScopeString } from './scope/types'; export { normalizeScope, mergeScopeObject, - mergeScopes, + mergeNormalizedScopes as mergeScopes, normalizeAndMergeScopes, } from './scope/transform'; diff --git a/packages/multichain/src/scope/transform.test.ts b/packages/multichain/src/scope/transform.test.ts index b5e01b5cce9..4efc3d4b10e 100644 --- a/packages/multichain/src/scope/transform.test.ts +++ b/packages/multichain/src/scope/transform.test.ts @@ -1,6 +1,6 @@ import { normalizeScope, - mergeScopes, + mergeNormalizedScopes, mergeScopeObject, normalizeAndMergeScopes, } from './transform'; @@ -255,7 +255,7 @@ describe('Scope Transform', () => { describe('mergeScopes', () => { it('merges the scopeObjects with matching scopeString', () => { expect( - mergeScopes( + mergeNormalizedScopes( { 'eip155:1': { methods: ['a', 'b', 'c'], @@ -282,7 +282,7 @@ describe('Scope Transform', () => { it('preserves the scopeObjects with no matching scopeString', () => { expect( - mergeScopes( + mergeNormalizedScopes( { 'eip155:1': { methods: ['a', 'b', 'c'], @@ -322,12 +322,12 @@ describe('Scope Transform', () => { }); }); it('returns an empty object when no scopes are provided', () => { - expect(mergeScopes({}, {})).toStrictEqual({}); + expect(mergeNormalizedScopes({}, {})).toStrictEqual({}); }); it('returns an unchanged scope when two identical scopeObjects are provided', () => { expect( - mergeScopes( + mergeNormalizedScopes( { 'eip155:1': validScopeObject }, { 'eip155:1': validScopeObject }, ), diff --git a/packages/multichain/src/scope/transform.ts b/packages/multichain/src/scope/transform.ts index 666ff740eb5..a1bab8d128c 100644 --- a/packages/multichain/src/scope/transform.ts +++ b/packages/multichain/src/scope/transform.ts @@ -4,6 +4,8 @@ import { cloneDeep } from 'lodash'; import type { ExternalScopeObject, ExternalScopesObject, + InternalScopesObject, + InternalScopeString, NormalizedScopeObject, NormalizedScopesObject, } from './types'; @@ -101,11 +103,12 @@ export const mergeScopeObject = ( /** * Merges two NormalizedScopeObjects - * @param scopeA - The first scope object to merge. - * @param scopeB - The second scope object to merge. - * @returns The merged scope object. + * + * @param scopeA - The first normalized scope object to merge. + * @param scopeB - The second normalized scope object to merge. + * @returns The merged normalized scope object from the [CAIP-25](https://github.com/ChainAgnostic/CAIPs/blob/main/CAIPs/caip-25.md) request. */ -export const mergeScopes = ( +export const mergeNormalizedScopes = ( scopeA: NormalizedScopesObject, scopeB: NormalizedScopesObject, ): NormalizedScopesObject => { @@ -134,6 +137,37 @@ export const mergeScopes = ( return scope; }; +/** + * Merges two InternalScopeObjects + * + * @param scopeA - The first internal scope object to merge. + * @param scopeB - The second internal scope object to merge. + * @returns The merged internal scope object from the [CAIP-25](https://github.com/ChainAgnostic/CAIPs/blob/main/CAIPs/caip-25.md) request. + */ +export const mergeInternalScopes = ( + scopeA: InternalScopesObject, + scopeB: InternalScopesObject, +): InternalScopesObject => { + const resultScope = cloneDeep(scopeA); + + Object.entries(scopeB).forEach(([scopeString, rightScopeObject]) => { + const internalScopeString = scopeString as InternalScopeString; + const leftRequiredScopeObject = resultScope[internalScopeString]; + if (!leftRequiredScopeObject) { + resultScope[internalScopeString] = rightScopeObject; + } else { + resultScope[internalScopeString] = { + accounts: getUniqueArrayItems([ + ...leftRequiredScopeObject.accounts, + ...rightScopeObject.accounts, + ]), + }; + } + }); + + return resultScope; +}; + /** * Normalizes and merges a set of ExternalScopesObjects into a NormalizedScopesObject (i.e. a set of NormalizedScopeObjects where references are flattened). * @param scopes - The external scopes to normalize and merge. @@ -145,7 +179,7 @@ export const normalizeAndMergeScopes = ( let mergedScopes: NormalizedScopesObject = {}; Object.keys(scopes).forEach((scopeString) => { const normalizedScopes = normalizeScope(scopeString, scopes[scopeString]); - mergedScopes = mergeScopes(mergedScopes, normalizedScopes); + mergedScopes = mergeNormalizedScopes(mergedScopes, normalizedScopes); }); return mergedScopes; From 40bec464802a1cc2923156043c3b3235dda3c197 Mon Sep 17 00:00:00 2001 From: ffmcgee Date: Wed, 12 Feb 2025 00:25:24 +0100 Subject: [PATCH 11/17] lint --- packages/multichain/src/scope/transform.ts | 2 ++ packages/multichain/src/scope/types.ts | 1 + 2 files changed, 3 insertions(+) diff --git a/packages/multichain/src/scope/transform.ts b/packages/multichain/src/scope/transform.ts index a1bab8d128c..3109b73d72a 100644 --- a/packages/multichain/src/scope/transform.ts +++ b/packages/multichain/src/scope/transform.ts @@ -61,6 +61,7 @@ export const normalizeScope = ( /** * Merges two NormalizedScopeObjects + * * @param scopeObjectA - The first scope object to merge. * @param scopeObjectB - The second scope object to merge. * @returns The merged scope object. @@ -170,6 +171,7 @@ export const mergeInternalScopes = ( /** * Normalizes and merges a set of ExternalScopesObjects into a NormalizedScopesObject (i.e. a set of NormalizedScopeObjects where references are flattened). + * * @param scopes - The external scopes to normalize and merge. * @returns The normalized and merged scopes. */ diff --git a/packages/multichain/src/scope/types.ts b/packages/multichain/src/scope/types.ts index b13b5edae75..8993eb0cabb 100644 --- a/packages/multichain/src/scope/types.ts +++ b/packages/multichain/src/scope/types.ts @@ -91,6 +91,7 @@ export type ScopedProperties = Record> & { /** * Parses a scope string into a namespace and reference. + * * @param scopeString - The scope string to parse. * @returns An object containing the namespace and reference. */ From 1f9e0de2186175795e50a1db936c4017f640daa0 Mon Sep 17 00:00:00 2001 From: ffmcgee Date: Wed, 12 Feb 2025 00:37:50 +0100 Subject: [PATCH 12/17] lint:eslint --- eslint-warning-thresholds.json | 6 ------ 1 file changed, 6 deletions(-) diff --git a/eslint-warning-thresholds.json b/eslint-warning-thresholds.json index e777270690c..bfaeceb3350 100644 --- a/eslint-warning-thresholds.json +++ b/eslint-warning-thresholds.json @@ -374,12 +374,6 @@ "@typescript-eslint/no-unsafe-enum-comparison": 4, "jsdoc/tag-lines": 4 }, - "packages/multichain/src/scope/transform.ts": { - "jsdoc/tag-lines": 3 - }, - "packages/multichain/src/scope/types.ts": { - "jsdoc/tag-lines": 1 - }, "packages/multichain/src/scope/validation.ts": { "jsdoc/tag-lines": 2 }, From dbaeb6e0d25d10c2feb9e19513ff5c122c8e80be Mon Sep 17 00:00:00 2001 From: ffmcgee Date: Wed, 12 Feb 2025 18:11:52 +0100 Subject: [PATCH 13/17] code review changes --- .../multichain/src/caip25Permission.test.ts | 30 ------------------- packages/multichain/src/caip25Permission.ts | 8 +---- packages/multichain/src/index.ts | 3 +- 3 files changed, 3 insertions(+), 38 deletions(-) diff --git a/packages/multichain/src/caip25Permission.test.ts b/packages/multichain/src/caip25Permission.test.ts index 0839c37f2b2..df5b6883421 100644 --- a/packages/multichain/src/caip25Permission.test.ts +++ b/packages/multichain/src/caip25Permission.test.ts @@ -45,40 +45,10 @@ describe('caip25EndowmentBuilder', () => { endowmentGetter: expect.any(Function), allowedCaveats: [Caip25CaveatType], validator: expect.any(Function), - factory: expect.any(Function), }); expect(specification.endowmentGetter()).toBeNull(); }); - - describe('factory', () => { - const { factory } = caip25EndowmentBuilder.specificationBuilder({ - methodHooks: { - findNetworkClientIdByChainId: jest.fn(), - listAccounts: jest.fn(), - }, - }); - - it('should call `constructPermission` with provided options', () => { - const mockRequestData = { origin: 'test.com' }; - const mockOptions: PermissionOptions = { - caveats: [ - { - type: Caip25CaveatType, - value: { - requiredScopes: {}, - optionalScopes: {}, - isMultichainOrigin: true, - }, - }, - ], - target: Caip25EndowmentPermissionName, - invoker: 'test.com', - }; - factory(mockOptions, mockRequestData); - expect(constructPermission).toHaveBeenCalledWith({ ...mockOptions }); - }); - }); }); describe('createCaip25Caveat', () => { diff --git a/packages/multichain/src/caip25Permission.ts b/packages/multichain/src/caip25Permission.ts index d203fad04d4..c13126666b4 100644 --- a/packages/multichain/src/caip25Permission.ts +++ b/packages/multichain/src/caip25Permission.ts @@ -172,7 +172,7 @@ export const caip25CaveatBuilder = ({ const mergedValue: Caip25CaveatValue = { requiredScopes: mergedRequiredScopes, optionalScopes: mergedOptionalScopes, - isMultichainOrigin: rightValue.isMultichainOrigin, + isMultichainOrigin: leftValue.isMultichainOrigin, }; const partialDiff = diffScopesForCaip25CaveatValue( @@ -197,7 +197,6 @@ type Caip25EndowmentSpecification = ValidPermissionSpecification<{ targetName: typeof Caip25EndowmentPermissionName; endowmentGetter: (_options?: EndowmentGetterParams) => null; validator: PermissionValidatorConstraint; - factory: PermissionFactory>; allowedCaveats: Readonly> | null; }>; @@ -227,11 +226,6 @@ const specificationBuilder: PermissionSpecificationBuilder< ); } }, - factory: (permissionOptions, _requestData) => { - return constructPermission({ - ...permissionOptions, - }); - }, }; }; diff --git a/packages/multichain/src/index.ts b/packages/multichain/src/index.ts index 267bc895bc4..76b41aec33e 100644 --- a/packages/multichain/src/index.ts +++ b/packages/multichain/src/index.ts @@ -49,7 +49,8 @@ export { parseScopeString } from './scope/types'; export { normalizeScope, mergeScopeObject, - mergeNormalizedScopes as mergeScopes, + mergeNormalizedScopes, + mergeInternalScopes, normalizeAndMergeScopes, } from './scope/transform'; From c32d5c79cb77463ac6fafa2b2bdbb65afe0af1cf Mon Sep 17 00:00:00 2001 From: ffmcgee Date: Wed, 12 Feb 2025 18:16:14 +0100 Subject: [PATCH 14/17] ci --- packages/multichain/src/index.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/multichain/src/index.test.ts b/packages/multichain/src/index.test.ts index 8465d5a24fd..7ceafeea0cb 100644 --- a/packages/multichain/src/index.test.ts +++ b/packages/multichain/src/index.test.ts @@ -28,7 +28,8 @@ describe('@metamask/multichain', () => { "parseScopeString", "normalizeScope", "mergeScopeObject", - "mergeScopes", + "mergeNormalizedScopes", + "mergeInternalizedScopes", "normalizeAndMergeScopes", "caip25CaveatBuilder", "Caip25CaveatType", From 8d5691703c6704da41a294062524b1122b44318b Mon Sep 17 00:00:00 2001 From: ffmcgee Date: Wed, 12 Feb 2025 18:20:01 +0100 Subject: [PATCH 15/17] fix --- packages/multichain/src/index.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/multichain/src/index.test.ts b/packages/multichain/src/index.test.ts index 7ceafeea0cb..4e384e43b4f 100644 --- a/packages/multichain/src/index.test.ts +++ b/packages/multichain/src/index.test.ts @@ -29,7 +29,7 @@ describe('@metamask/multichain', () => { "normalizeScope", "mergeScopeObject", "mergeNormalizedScopes", - "mergeInternalizedScopes", + "mergeInternalScopes", "normalizeAndMergeScopes", "caip25CaveatBuilder", "Caip25CaveatType", From 7fa8016c064f23d168b1b02f86a3fe2cea3750b1 Mon Sep 17 00:00:00 2001 From: ffmcgee Date: Wed, 12 Feb 2025 18:25:23 +0100 Subject: [PATCH 16/17] lint:eslint --- packages/multichain/src/caip25Permission.test.ts | 3 --- packages/multichain/src/caip25Permission.ts | 2 -- 2 files changed, 5 deletions(-) diff --git a/packages/multichain/src/caip25Permission.test.ts b/packages/multichain/src/caip25Permission.test.ts index df5b6883421..86b328174be 100644 --- a/packages/multichain/src/caip25Permission.test.ts +++ b/packages/multichain/src/caip25Permission.test.ts @@ -1,9 +1,6 @@ import { CaveatMutatorOperation, - type PermissionConstraint, - type PermissionOptions, PermissionType, - constructPermission, } from '@metamask/permission-controller'; import type { Caip25CaveatValue } from './caip25Permission'; diff --git a/packages/multichain/src/caip25Permission.ts b/packages/multichain/src/caip25Permission.ts index c13126666b4..eab0e751ccc 100644 --- a/packages/multichain/src/caip25Permission.ts +++ b/packages/multichain/src/caip25Permission.ts @@ -4,14 +4,12 @@ import type { EndowmentGetterParams, ValidPermissionSpecification, PermissionValidatorConstraint, - PermissionFactory, PermissionConstraint, EndowmentCaveatSpecificationConstraint, } from '@metamask/permission-controller'; import { CaveatMutatorOperation, PermissionType, - constructPermission, } from '@metamask/permission-controller'; import type { CaipAccountId, Json } from '@metamask/utils'; import { From 0bb68777feb8cc9eb14c83a7736f0974322abaa4 Mon Sep 17 00:00:00 2001 From: ffmcgee Date: Wed, 12 Feb 2025 19:46:55 +0100 Subject: [PATCH 17/17] code review --- .../multichain/src/caip25Permission.test.ts | 343 +++++++++++------- packages/multichain/src/caip25Permission.ts | 17 +- .../multichain/src/scope/transform.test.ts | 119 +++++- 3 files changed, 339 insertions(+), 140 deletions(-) diff --git a/packages/multichain/src/caip25Permission.test.ts b/packages/multichain/src/caip25Permission.test.ts index 86b328174be..a52127b901a 100644 --- a/packages/multichain/src/caip25Permission.test.ts +++ b/packages/multichain/src/caip25Permission.test.ts @@ -20,11 +20,6 @@ jest.mock('./scope/supported', () => ({ })); const MockScopeSupported = jest.mocked(ScopeSupported); -jest.mock('@metamask/permission-controller', () => ({ - ...jest.requireActual('@metamask/permission-controller'), - constructPermission: jest.fn(), -})); - const { removeAccount, removeScope } = Caip25CaveatMutators[Caip25CaveatType]; describe('caip25EndowmentBuilder', () => { @@ -705,145 +700,236 @@ describe('caip25CaveatBuilder', () => { }); describe('permission merger', () => { - const initLeftValue: Caip25CaveatValue = { - optionalScopes: { - 'eip155:1': { - accounts: ['eip155:1:0xdead'], - }, - }, - requiredScopes: {}, - isMultichainOrigin: false, - }; - - it.each<{ - rightValue: Caip25CaveatValue; - expectedMergedValue: Caip25CaveatValue; - expectedDiff: Caip25CaveatValue; - }>([ - { - rightValue: { - optionalScopes: { - 'eip155:1': { - accounts: ['eip155:1:0xbeef'], + describe('optionalScopes', () => { + it.each<{ + description: string; + rightValue: Caip25CaveatValue; + expectedMergedValue: Caip25CaveatValue; + expectedDiff: Caip25CaveatValue; + }>([ + { + description: + 'incremental request existing scope with a new account - should return merged scope with existing chain and both accounts', + rightValue: { + optionalScopes: { + 'eip155:1': { + accounts: ['eip155:1:0xbeef'], + }, }, + requiredScopes: {}, + isMultichainOrigin: false, }, - requiredScopes: {}, - isMultichainOrigin: false, - }, - expectedMergedValue: { - optionalScopes: { - 'eip155:1': { accounts: ['eip155:1:0xdead', 'eip155:1:0xbeef'] }, + expectedMergedValue: { + optionalScopes: { + 'eip155:1': { accounts: ['eip155:1:0xdead', 'eip155:1:0xbeef'] }, + }, + requiredScopes: {}, + isMultichainOrigin: false, }, - requiredScopes: {}, - isMultichainOrigin: false, - }, - expectedDiff: { - optionalScopes: { - 'eip155:1': { - accounts: ['eip155:1:0xbeef'], + expectedDiff: { + optionalScopes: { + 'eip155:1': { + accounts: ['eip155:1:0xbeef'], + }, }, + isMultichainOrigin: false, + requiredScopes: {}, }, - isMultichainOrigin: false, - requiredScopes: {}, }, - }, - { - rightValue: { - optionalScopes: { - 'eip155:10': { - accounts: [], + { + description: + 'incremental request a whole new scope without accounts - should return merged scope with previously existing chain and accounts, plus new requested chain with no accounts', + rightValue: { + optionalScopes: { + 'eip155:10': { + accounts: [], + }, }, + requiredScopes: {}, + isMultichainOrigin: false, }, - requiredScopes: {}, - isMultichainOrigin: false, - }, - expectedMergedValue: { - optionalScopes: { - 'eip155:1': { accounts: ['eip155:1:0xdead'] }, - 'eip155:10': { - accounts: [], + expectedMergedValue: { + optionalScopes: { + 'eip155:1': { accounts: ['eip155:1:0xdead'] }, + 'eip155:10': { + accounts: [], + }, }, + requiredScopes: {}, + isMultichainOrigin: false, }, - requiredScopes: {}, - isMultichainOrigin: false, - }, - expectedDiff: { - optionalScopes: { - 'eip155:10': { - accounts: [], + expectedDiff: { + optionalScopes: { + 'eip155:10': { + accounts: [], + }, }, + isMultichainOrigin: false, + requiredScopes: {}, }, - isMultichainOrigin: false, - requiredScopes: {}, }, - }, - { - rightValue: { - optionalScopes: { - 'eip155:10': { - accounts: ['eip155:10:0xbeef'], + { + description: + 'incremental request a whole new scope with accounts - should return merged scope with previously existing chain and accounts, plus new requested chain with new account', + rightValue: { + optionalScopes: { + 'eip155:10': { + accounts: ['eip155:10:0xbeef'], + }, }, + requiredScopes: {}, + isMultichainOrigin: false, }, - requiredScopes: {}, - isMultichainOrigin: false, - }, - expectedMergedValue: { - optionalScopes: { - 'eip155:1': { accounts: ['eip155:1:0xdead'] }, - 'eip155:10': { accounts: ['eip155:10:0xbeef'] }, + expectedMergedValue: { + optionalScopes: { + 'eip155:1': { accounts: ['eip155:1:0xdead'] }, + 'eip155:10': { accounts: ['eip155:10:0xbeef'] }, + }, + requiredScopes: {}, + isMultichainOrigin: false, }, - requiredScopes: {}, - isMultichainOrigin: false, - }, - expectedDiff: { - optionalScopes: { - 'eip155:10': { - accounts: ['eip155:10:0xbeef'], + expectedDiff: { + optionalScopes: { + 'eip155:10': { + accounts: ['eip155:10:0xbeef'], + }, }, + isMultichainOrigin: false, + requiredScopes: {}, }, - isMultichainOrigin: false, - requiredScopes: {}, }, - }, - { - rightValue: { - optionalScopes: { - 'eip155:1': { - accounts: ['eip155:1:0xdead', 'eip155:1:0xbeef'], + { + description: + 'incremental request an existing scope with new accounts, and whole new scope with accounts - should return merged scope with previously existing chain and accounts, plus new requested chain with new accounts', + rightValue: { + optionalScopes: { + 'eip155:1': { + accounts: ['eip155:1:0xdead', 'eip155:1:0xbeef'], + }, + 'eip155:10': { + accounts: ['eip155:10:0xdead', 'eip155:10:0xbeef'], + }, }, - 'eip155:10': { - accounts: ['eip155:10:0xdead', 'eip155:10:0xbeef'], + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedMergedValue: { + optionalScopes: { + 'eip155:1': { accounts: ['eip155:1:0xdead', 'eip155:1:0xbeef'] }, + 'eip155:10': { + accounts: ['eip155:10:0xdead', 'eip155:10:0xbeef'], + }, }, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedDiff: { + optionalScopes: { + 'eip155:1': { + accounts: ['eip155:1:0xbeef'], + }, + 'eip155:10': { + accounts: ['eip155:10:0xdead', 'eip155:10:0xbeef'], + }, + }, + isMultichainOrigin: false, + requiredScopes: {}, }, - requiredScopes: {}, - isMultichainOrigin: false, }, - expectedMergedValue: { - optionalScopes: { - 'eip155:1': { accounts: ['eip155:1:0xdead', 'eip155:1:0xbeef'] }, - 'eip155:10': { - accounts: ['eip155:10:0xdead', 'eip155:10:0xbeef'], + { + description: + 'incremental request an existing scope with new accounts, and 2 whole new scope with accounts - should return merged scope with previously existing chain and accounts, plus new requested chains with new accounts', + rightValue: { + optionalScopes: { + 'eip155:1': { + accounts: ['eip155:1:0xdead', 'eip155:1:0xbadd'], + }, + 'eip155:10': { + accounts: ['eip155:10:0xbeef', 'eip155:10:0xbadd'], + }, + 'eip155:426161': { + accounts: [ + 'eip155:426161:0xdead', + 'eip155:426161:0xbeef', + 'eip155:426161:0xbadd', + ], + }, }, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedMergedValue: { + optionalScopes: { + 'eip155:1': { accounts: ['eip155:1:0xdead', 'eip155:1:0xbadd'] }, + 'eip155:10': { + accounts: ['eip155:10:0xbeef', 'eip155:10:0xbadd'], + }, + 'eip155:426161': { + accounts: [ + 'eip155:426161:0xdead', + 'eip155:426161:0xbeef', + 'eip155:426161:0xbadd', + ], + }, + }, + requiredScopes: {}, + isMultichainOrigin: false, + }, + expectedDiff: { + optionalScopes: { + 'eip155:1': { accounts: ['eip155:1:0xbadd'] }, + 'eip155:10': { + accounts: ['eip155:10:0xbeef', 'eip155:10:0xbadd'], + }, + 'eip155:426161': { + accounts: [ + 'eip155:426161:0xdead', + 'eip155:426161:0xbeef', + 'eip155:426161:0xbadd', + ], + }, + }, + isMultichainOrigin: false, + requiredScopes: {}, }, - requiredScopes: {}, - isMultichainOrigin: false, }, - expectedDiff: { - optionalScopes: { - 'eip155:1': { - accounts: ['eip155:1:0xbeef'], + ])( + '$description', + async ({ rightValue, expectedMergedValue, expectedDiff }) => { + const initLeftValue: Caip25CaveatValue = { + optionalScopes: { + 'eip155:1': { + accounts: ['eip155:1:0xdead'], + }, }, - 'eip155:10': { - accounts: ['eip155:10:0xdead', 'eip155:10:0xbeef'], + requiredScopes: {}, + isMultichainOrigin: false, + }; + + const [newValue, diff] = merger(initLeftValue, rightValue); + + expect(newValue).toStrictEqual( + expect.objectContaining(expectedMergedValue), + ); + expect(diff).toStrictEqual(expect.objectContaining(expectedDiff)); + }, + ); + }); + + describe('requiredScopes', () => { + it('incremental request an existing scope with new accounts, and 2 whole new scope with accounts - should return merged scope with previously existing chain and accounts, plus new requested chains with new accounts', () => { + const initLeftValue: Caip25CaveatValue = { + requiredScopes: { + 'eip155:1': { + accounts: ['eip155:1:0xdead'], }, }, + optionalScopes: {}, isMultichainOrigin: false, - requiredScopes: {}, - }, - }, - { - rightValue: { - optionalScopes: { + }; + + const rightValue: Caip25CaveatValue = { + requiredScopes: { 'eip155:1': { accounts: ['eip155:1:0xdead', 'eip155:1:0xbadd'], }, @@ -858,11 +944,12 @@ describe('caip25CaveatBuilder', () => { ], }, }, - requiredScopes: {}, + optionalScopes: {}, isMultichainOrigin: false, - }, - expectedMergedValue: { - optionalScopes: { + }; + + const expectedMergedValue: Caip25CaveatValue = { + requiredScopes: { 'eip155:1': { accounts: ['eip155:1:0xdead', 'eip155:1:0xbadd'] }, 'eip155:10': { accounts: ['eip155:10:0xbeef', 'eip155:10:0xbadd'], @@ -875,11 +962,11 @@ describe('caip25CaveatBuilder', () => { ], }, }, - requiredScopes: {}, + optionalScopes: {}, isMultichainOrigin: false, - }, - expectedDiff: { - optionalScopes: { + }; + const expectedDiff: Caip25CaveatValue = { + requiredScopes: { 'eip155:1': { accounts: ['eip155:1:0xbadd'] }, 'eip155:10': { accounts: ['eip155:10:0xbeef', 'eip155:10:0xbadd'], @@ -892,20 +979,16 @@ describe('caip25CaveatBuilder', () => { ], }, }, + optionalScopes: {}, isMultichainOrigin: false, - requiredScopes: {}, - }, - }, - ])( - 'should return the expected merged value $expectedMergedValue and expected diff $expectedDiff', - async ({ rightValue, expectedMergedValue, expectedDiff }) => { + }; const [newValue, diff] = merger(initLeftValue, rightValue); expect(newValue).toStrictEqual( expect.objectContaining(expectedMergedValue), ); expect(diff).toStrictEqual(expect.objectContaining(expectedDiff)); - }, - ); + }); + }); }); }); diff --git a/packages/multichain/src/caip25Permission.ts b/packages/multichain/src/caip25Permission.ts index eab0e751ccc..efd7880abe9 100644 --- a/packages/multichain/src/caip25Permission.ts +++ b/packages/multichain/src/caip25Permission.ts @@ -386,16 +386,17 @@ function removeScope( } /** + * Returns the differential between two provided CAIP-25 permission caveat value scopes. * * @param originalValue - The existing CAIP-25 permission caveat value. * @param mergedValue - The result from merging existing and incoming CAIP-25 permission caveat values. - * @param scopeToMerge - The required or optional scopes from the [CAIP-25](https://github.com/ChainAgnostic/CAIPs/blob/main/CAIPs/caip-25.md) request. + * @param scopeToDiff - The required or optional scopes from the [CAIP-25](https://github.com/ChainAgnostic/CAIPs/blob/main/CAIPs/caip-25.md) request. * @returns The differential between original and merged CAIP-25 permission caveat values. */ function diffScopesForCaip25CaveatValue( originalValue: Caip25CaveatValue, mergedValue: Caip25CaveatValue, - scopeToMerge: keyof Pick< + scopeToDiff: keyof Pick< Caip25CaveatValue, 'optionalScopes' | 'requiredScopes' >, @@ -403,27 +404,27 @@ function diffScopesForCaip25CaveatValue( const diff = cloneDeep(originalValue); for (const [scopeString, mergedScopeObject] of Object.entries( - mergedValue[scopeToMerge], + mergedValue[scopeToDiff], )) { const internalScopeString = scopeString as InternalScopeString; - const originalScopeObject = diff[scopeToMerge][internalScopeString]; + const originalScopeObject = diff[scopeToDiff][internalScopeString]; if (originalScopeObject) { const newAccounts = mergedScopeObject.accounts.filter( (account) => - !diff[scopeToMerge][ + !diff[scopeToDiff][ scopeString as InternalScopeString ]?.accounts.includes(account), ); if (newAccounts.length > 0) { - diff[scopeToMerge][internalScopeString] = { + diff[scopeToDiff][internalScopeString] = { accounts: newAccounts, }; continue; } - delete diff[scopeToMerge][internalScopeString]; + delete diff[scopeToDiff][internalScopeString]; } else { - diff[scopeToMerge][internalScopeString] = mergedScopeObject; + diff[scopeToDiff][internalScopeString] = mergedScopeObject; } } diff --git a/packages/multichain/src/scope/transform.test.ts b/packages/multichain/src/scope/transform.test.ts index 4efc3d4b10e..0132920488e 100644 --- a/packages/multichain/src/scope/transform.test.ts +++ b/packages/multichain/src/scope/transform.test.ts @@ -1,10 +1,15 @@ import { normalizeScope, mergeNormalizedScopes, + mergeInternalScopes, mergeScopeObject, normalizeAndMergeScopes, } from './transform'; -import type { ExternalScopeObject, NormalizedScopeObject } from './types'; +import type { + ExternalScopeObject, + NormalizedScopeObject, + InternalScopesObject, +} from './types'; const externalScopeObject: ExternalScopeObject = { methods: [], @@ -252,7 +257,117 @@ describe('Scope Transform', () => { }); }); - describe('mergeScopes', () => { + describe('mergeInternalScopes', () => { + it.each<{ + description: string; + rightValue: InternalScopesObject; + expectedMergedValue: InternalScopesObject; + }>([ + { + description: + 'incremental request existing scope with a new account - should return merged scope with existing chain and both accounts', + rightValue: { + 'eip155:1': { + accounts: ['eip155:1:0xbeef'], + }, + }, + expectedMergedValue: { + 'eip155:1': { accounts: ['eip155:1:0xdead', 'eip155:1:0xbeef'] }, + }, + }, + { + description: + 'incremental request a whole new scope without accounts - should return merged scope with previously existing chain and accounts, plus new requested chain with no accounts', + rightValue: { + 'eip155:10': { + accounts: [], + }, + }, + expectedMergedValue: { + 'eip155:1': { accounts: ['eip155:1:0xdead'] }, + 'eip155:10': { + accounts: [], + }, + }, + }, + { + description: + 'incremental request a whole new scope with accounts - should return merged scope with previously existing chain and accounts, plus new requested chain with new account', + rightValue: { + 'eip155:10': { + accounts: ['eip155:10:0xbeef'], + }, + }, + expectedMergedValue: { + 'eip155:1': { accounts: ['eip155:1:0xdead'] }, + 'eip155:10': { accounts: ['eip155:10:0xbeef'] }, + }, + }, + { + description: + 'incremental request an existing scope with new accounts, and whole new scope with accounts - should return merged scope with previously existing chain and accounts, plus new requested chain with new accounts', + rightValue: { + 'eip155:1': { + accounts: ['eip155:1:0xdead', 'eip155:1:0xbeef'], + }, + 'eip155:10': { + accounts: ['eip155:10:0xdead', 'eip155:10:0xbeef'], + }, + }, + expectedMergedValue: { + 'eip155:1': { accounts: ['eip155:1:0xdead', 'eip155:1:0xbeef'] }, + 'eip155:10': { + accounts: ['eip155:10:0xdead', 'eip155:10:0xbeef'], + }, + }, + }, + { + description: + 'incremental request an existing scope with new accounts, and 2 whole new scope with accounts - should return merged scope with previously existing chain and accounts, plus new requested chains with new accounts', + rightValue: { + 'eip155:1': { + accounts: ['eip155:1:0xdead', 'eip155:1:0xbadd'], + }, + 'eip155:10': { + accounts: ['eip155:10:0xbeef', 'eip155:10:0xbadd'], + }, + 'eip155:426161': { + accounts: [ + 'eip155:426161:0xdead', + 'eip155:426161:0xbeef', + 'eip155:426161:0xbadd', + ], + }, + }, + expectedMergedValue: { + 'eip155:1': { accounts: ['eip155:1:0xdead', 'eip155:1:0xbadd'] }, + 'eip155:10': { + accounts: ['eip155:10:0xbeef', 'eip155:10:0xbadd'], + }, + 'eip155:426161': { + accounts: [ + 'eip155:426161:0xdead', + 'eip155:426161:0xbeef', + 'eip155:426161:0xbadd', + ], + }, + }, + }, + ])('$description', async ({ rightValue, expectedMergedValue }) => { + const initLeftValue: InternalScopesObject = { + 'eip155:1': { + accounts: ['eip155:1:0xdead'], + }, + }; + const mergedValue = mergeInternalScopes(initLeftValue, rightValue); + + expect(mergedValue).toStrictEqual( + expect.objectContaining(expectedMergedValue), + ); + }); + }); + + describe('mergeNormalizedScopes', () => { it('merges the scopeObjects with matching scopeString', () => { expect( mergeNormalizedScopes(