Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: revoke CAIP-25 endowment if only eip155 account or scope is removed #4978

Merged
merged 12 commits into from
Nov 26, 2024
Merged

feat: revoke CAIP-25 endowment if only eip155 account or scope is removed #4978

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
4015c13
revoke if last eth account or permitted chain
jiexi Nov 25, 2024
2a430f5
Merge branch 'main' into jl/caip-multichain-revoke-in-mutators
jiexi Nov 25, 2024
f197304
stop upserting 'wallet:eip155` in setEthAccounts
jiexi Nov 25, 2024
50b4a30
update removeScope consider non-eth scopes. update removeAccounts con…
jiexi Nov 25, 2024
cf061b0
Merge remote-tracking branch 'origin/jl/caip-multichain-revoke-in-mut…
jiexi Nov 25, 2024
1e8ded3
Merge branch 'main' into jl/caip-multichain-revoke-in-mutators
jiexi Nov 25, 2024
35ce68c
update setEthAccounts jsdoc
jiexi Nov 26, 2024
402e3f8
add does not revoke if only wallet:eip155 accounts spec
jiexi Nov 26, 2024
8a7e499
Merge branch 'main' into jl/caip-multichain-revoke-in-mutators
jiexi Nov 26, 2024
0e4a1d8
Merge branch 'main' into jl/caip-multichain-revoke-in-mutators
adonesky1 Nov 26, 2024
d65e8eb
cleanup specs
jiexi Nov 26, 2024
562f847
Merge remote-tracking branch 'origin/jl/caip-multichain-revoke-in-mut…
jiexi Nov 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 0 additions & 23 deletions packages/multichain/src/adapters/caip-permission-adapter-eth-accounts.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,29 +159,6 @@ describe('CAIP-25 eth_accounts adapters', () => {
});
});

it('returns a CAIP-25 caveat value with missing "wallet:eip155" optional scope filled in, forming CAIP-10 account addresses from the accounts param', () => {
const input: Caip25CaveatValue = {
requiredScopes: {},
optionalScopes: {},
isMultichainOrigin: false,
};

const result = setEthAccounts(input, ['0x1', '0x2', '0x3']);
expect(result).toStrictEqual({
requiredScopes: {},
optionalScopes: {
'wallet:eip155': {
accounts: [
'wallet:eip155:0x1',
'wallet:eip155:0x2',
'wallet:eip155:0x3',
],
},
},
isMultichainOrigin: false,
});
});

it('does not modify the input CAIP-25 caveat value object in place', () => {
const input: Caip25CaveatValue = {
requiredScopes: {
Expand Down
7 changes: 1 addition & 6 deletions packages/multichain/src/adapters/caip-permission-adapter-eth-accounts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,12 +136,7 @@ export const setEthAccounts = (
accounts,
jiexi marked this conversation as resolved.
Show resolved Hide resolved
),
optionalScopes: setEthAccountsForScopesObject(
{
[KnownWalletScopeString.Eip155]: {
accounts: [],
},
...caip25CaveatValue.optionalScopes,
},
caip25CaveatValue.optionalScopes,
accounts,
),
};
Expand Down
107 changes: 91 additions & 16 deletions packages/multichain/src/caip25Permission.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ describe('caip25EndowmentBuilder', () => {
describe('Caip25CaveatMutators.authorizedScopes', () => {
describe('removeScope', () => {
it('returns a version of the caveat with the given scope removed from requiredScopes if it is present', () => {
const ethereumGoerliCaveat = {
const caveatValue = {
requiredScopes: {
'eip155:1': {
accounts: [],
Expand All @@ -78,7 +78,7 @@ describe('caip25EndowmentBuilder', () => {
sessionProperties: {},
isMultichainOrigin: true,
};
const result = removeScope(ethereumGoerliCaveat, 'eip155:1');
const result = removeScope(caveatValue, 'eip155:1');
expect(result).toStrictEqual({
operation: CaveatMutatorOperation.UpdateValue,
value: {
Expand All @@ -93,7 +93,7 @@ describe('caip25EndowmentBuilder', () => {
});

it('returns a version of the caveat with the given scope removed from optionalScopes if it is present', () => {
const ethereumGoerliCaveat = {
const caveatValue = {
requiredScopes: {
'eip155:1': {
accounts: [],
Expand All @@ -107,7 +107,7 @@ describe('caip25EndowmentBuilder', () => {
sessionProperties: {},
isMultichainOrigin: true,
};
const result = removeScope(ethereumGoerliCaveat, 'eip155:5');
const result = removeScope(caveatValue, 'eip155:5');
expect(result).toStrictEqual({
operation: CaveatMutatorOperation.UpdateValue,
value: {
Expand All @@ -122,7 +122,7 @@ describe('caip25EndowmentBuilder', () => {
});

it('returns a version of the caveat with the given scope removed from requiredScopes and optionalScopes if it is present', () => {
const ethereumGoerliCaveat = {
const caveatValue = {
requiredScopes: {
'eip155:1': {
accounts: [],
Expand All @@ -139,7 +139,7 @@ describe('caip25EndowmentBuilder', () => {
sessionProperties: {},
isMultichainOrigin: true,
};
const result = removeScope(ethereumGoerliCaveat, 'eip155:5');
const result = removeScope(caveatValue, 'eip155:5');
expect(result).toStrictEqual({
operation: CaveatMutatorOperation.UpdateValue,
value: {
Expand All @@ -153,8 +153,51 @@ describe('caip25EndowmentBuilder', () => {
});
});

it('revokes the permission if the only non wallet scope is removed', () => {
const caveatValue = {
requiredScopes: {},
optionalScopes: {
'eip155:5': {
accounts: [],
},
'wallet:eip155': {
accounts: [],
},
wallet: {
accounts: [],
},
},
sessionProperties: {},
isMultichainOrigin: true,
};
const result = removeScope(caveatValue, 'eip155:5');
expect(result).toStrictEqual({
operation: CaveatMutatorOperation.RevokePermission,
});
});

it('does not revoke the permission if the target scope does not exist but the permission only has wallet scopes', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: The "does not revoke" tests are a bit confusing because it's not clear what we do expect to happen. Could we re-phrase them to make clear that we expect the result to be a no-op? The same goes for those added later in this file as well

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually maybe these cases are already covered? I do see some "account does not exist" and "scope does not exist" test cases already.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done here d65e8eb

const caveatValue = {
requiredScopes: {},
optionalScopes: {
'wallet:eip155': {
accounts: [],
},
wallet: {
accounts: [],
},
},
sessionProperties: {},
isMultichainOrigin: true,
};
const result = removeScope(caveatValue, 'eip155:5');
expect(result.operation).not.toStrictEqual(
CaveatMutatorOperation.RevokePermission,
);
});

it('returns the caveat unchanged when the given scope is not found in either requiredScopes or optionalScopes', () => {
const ethereumGoerliCaveat = {
const caveatValue = {
requiredScopes: {
'eip155:1': {
accounts: [],
Expand All @@ -168,7 +211,7 @@ describe('caip25EndowmentBuilder', () => {
sessionProperties: {},
isMultichainOrigin: true,
};
const result = removeScope(ethereumGoerliCaveat, 'eip155:2');
const result = removeScope(caveatValue, 'eip155:2');
expect(result).toStrictEqual({
operation: CaveatMutatorOperation.Noop,
});
Expand All @@ -177,7 +220,7 @@ describe('caip25EndowmentBuilder', () => {

describe('removeAccount', () => {
it('returns a version of the caveat with the given account removed from requiredScopes if it is present', () => {
const ethereumGoerliCaveat: Caip25CaveatValue = {
const caveatValue: Caip25CaveatValue = {
requiredScopes: {
'eip155:1': {
accounts: ['eip155:1:0x1', 'eip155:1:0x2'],
Expand All @@ -186,7 +229,7 @@ describe('caip25EndowmentBuilder', () => {
optionalScopes: {},
isMultichainOrigin: true,
};
const result = removeAccount(ethereumGoerliCaveat, '0x1');
const result = removeAccount(caveatValue, '0x1');
expect(result).toStrictEqual({
operation: CaveatMutatorOperation.UpdateValue,
value: {
Expand All @@ -202,7 +245,7 @@ describe('caip25EndowmentBuilder', () => {
});

it('returns a version of the caveat with the given account removed from optionalScopes if it is present', () => {
const ethereumGoerliCaveat: Caip25CaveatValue = {
const caveatValue: Caip25CaveatValue = {
requiredScopes: {},
optionalScopes: {
'eip155:1': {
Expand All @@ -211,7 +254,7 @@ describe('caip25EndowmentBuilder', () => {
},
isMultichainOrigin: true,
};
const result = removeAccount(ethereumGoerliCaveat, '0x1');
const result = removeAccount(caveatValue, '0x1');
expect(result).toStrictEqual({
operation: CaveatMutatorOperation.UpdateValue,
value: {
Expand All @@ -227,7 +270,7 @@ describe('caip25EndowmentBuilder', () => {
});

it('returns a version of the caveat with the given account removed from requiredScopes and optionalScopes if it is present', () => {
const ethereumGoerliCaveat: Caip25CaveatValue = {
const caveatValue: Caip25CaveatValue = {
requiredScopes: {
'eip155:1': {
accounts: ['eip155:1:0x1', 'eip155:1:0x2'],
Expand All @@ -243,7 +286,7 @@ describe('caip25EndowmentBuilder', () => {
},
isMultichainOrigin: true,
};
const result = removeAccount(ethereumGoerliCaveat, '0x1');
const result = removeAccount(caveatValue, '0x1');
expect(result).toStrictEqual({
operation: CaveatMutatorOperation.UpdateValue,
value: {
Expand All @@ -265,8 +308,40 @@ describe('caip25EndowmentBuilder', () => {
});
});

it('revokes the permission if the only account is removed', () => {
jiexi marked this conversation as resolved.
Show resolved Hide resolved
const caveatValue: Caip25CaveatValue = {
requiredScopes: {},
optionalScopes: {
'eip155:1': {
accounts: ['eip155:1:0x1'],
},
},
isMultichainOrigin: true,
};
const result = removeAccount(caveatValue, '0x1');
expect(result).toStrictEqual({
operation: CaveatMutatorOperation.RevokePermission,
});
});

it('does not revoke the permission if the target account does not exist but the permission already has no accounts', () => {
const caveatValue: Caip25CaveatValue = {
requiredScopes: {},
optionalScopes: {
'eip155:1': {
accounts: [],
},
},
isMultichainOrigin: true,
};
const result = removeAccount(caveatValue, '0x1');
expect(result.operation).not.toStrictEqual(
CaveatMutatorOperation.RevokePermission,
);
});

it('returns the caveat unchanged when the given account is not found in either requiredScopes or optionalScopes', () => {
const ethereumGoerliCaveat: Caip25CaveatValue = {
const caveatValue: Caip25CaveatValue = {
requiredScopes: {
'eip155:1': {
accounts: ['eip155:1:0x1', 'eip155:1:0x2'],
Expand All @@ -279,7 +354,7 @@ describe('caip25EndowmentBuilder', () => {
},
isMultichainOrigin: true,
};
const result = removeAccount(ethereumGoerliCaveat, '0x3');
const result = removeAccount(caveatValue, '0x3');
expect(result).toStrictEqual({
operation: CaveatMutatorOperation.Noop,
});
Expand Down
65 changes: 47 additions & 18 deletions packages/multichain/src/caip25Permission.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
import type { CaipAccountId, Json } from '@metamask/utils';
import {
hasProperty,
KnownCaipNamespace,
parseCaipAccountId,
type Hex,
type NonEmptyArray,
Expand All @@ -23,6 +24,7 @@ import { getEthAccounts } from './adapters/caip-permission-adapter-eth-accounts'
import { assertIsInternalScopesObject } from './scope/assert';
import { isSupportedScopeString } from './scope/supported';
import {
parseScopeString,
type ExternalScopeString,
type InternalScopeObject,
type InternalScopesObject,
Expand Down Expand Up @@ -230,32 +232,44 @@ function removeAccount(
caip25CaveatValue: Caip25CaveatValue,
targetAddress: Hex,
) {
const copyOfCaveatValue = cloneDeep(caip25CaveatValue);
const updatedCaveatValue = cloneDeep(caip25CaveatValue);

[copyOfCaveatValue.requiredScopes, copyOfCaveatValue.optionalScopes].forEach(
(scopes) => {
Object.entries(scopes).forEach(([, scopeObject]) => {
removeAccountFromScopeObject(scopeObject, targetAddress);
});
},
);
[
updatedCaveatValue.requiredScopes,
updatedCaveatValue.optionalScopes,
].forEach((scopes) => {
Object.entries(scopes).forEach(([, scopeObject]) => {
removeAccountFromScopeObject(scopeObject, targetAddress);
});
});

const noChange = isEqual(copyOfCaveatValue, caip25CaveatValue);
const noChange = isEqual(updatedCaveatValue, caip25CaveatValue);

if (noChange) {
return {
operation: CaveatMutatorOperation.Noop,
};
}

const hasAccounts = [
...Object.values(updatedCaveatValue.requiredScopes),
...Object.values(updatedCaveatValue.optionalScopes),
].some(({ accounts }) => accounts.length > 0);

if (hasAccounts) {
return {
operation: CaveatMutatorOperation.UpdateValue,
value: updatedCaveatValue,
};
}

return {
operation: CaveatMutatorOperation.UpdateValue,
value: copyOfCaveatValue,
operation: CaveatMutatorOperation.RevokePermission,
};
}

/**
* Removes the target account from the value arrays of the given
* Removes the target scope from the value arrays of the given
* `endowment:caip25` caveat. No-ops if the target scopeString is not in
* the existing scopes.
*
Expand Down Expand Up @@ -283,17 +297,32 @@ function removeScope(
newOptionalScopes.length !==
Object.keys(caip25CaveatValue.optionalScopes).length;

if (requiredScopesRemoved || optionalScopesRemoved) {
if (!requiredScopesRemoved && !optionalScopesRemoved) {
return {
operation: CaveatMutatorOperation.Noop,
};
}

const updatedCaveatValue = {
requiredScopes: Object.fromEntries(newRequiredScopes),
optionalScopes: Object.fromEntries(newOptionalScopes),
};

const hasNonWalletScopes = [...newRequiredScopes, ...newOptionalScopes].some(
([scopeString]) => {
const { namespace } = parseScopeString(scopeString);
return namespace !== KnownCaipNamespace.Wallet;
},
);

if (hasNonWalletScopes) {
return {
operation: CaveatMutatorOperation.UpdateValue,
value: {
requiredScopes: Object.fromEntries(newRequiredScopes),
optionalScopes: Object.fromEntries(newOptionalScopes),
},
value: updatedCaveatValue,
};
}

return {
operation: CaveatMutatorOperation.Noop,
operation: CaveatMutatorOperation.RevokePermission,
};
}
Loading