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

Jl/caip multichain/fix connection flow for permitted chains #27471

Merged
merged 19 commits into from
Oct 3, 2024
Merged

Jl/caip multichain/fix connection flow for permitted chains #27471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
6c2a842
sync eth accounts on chain add
jiexi Sep 27, 2024
6e22497
add all known eip155 methods and notifications on chain add
jiexi Sep 27, 2024
ae59871
read default accounts out of permission request on Connect Page
jiexi Sep 27, 2024
ea1b9bd
Pass supported eth accounts and chains defaults to approval in wallet…
jiexi Sep 27, 2024
4011f66
Set permitted chains and eth accounts from approved request in wallet…
jiexi Sep 27, 2024
bb41fdd
Fix specs
jiexi Sep 27, 2024
89c82ee
lint
jiexi Sep 27, 2024
73dacf0
Merge branch 'caip-multichain' into jl/caip-multichain/fix-connection…
jiexi Sep 27, 2024
97ca5c5
Fix connect page permission default optional chain
jiexi Sep 27, 2024
eed8930
Stop sending currently selected chainId in permittedChains approval r…
jiexi Sep 27, 2024
3633097
remove dead hook
jiexi Sep 27, 2024
296a2ae
Merge branch 'caip-multichain' into jl/caip-multichain/fix-connection…
jiexi Sep 30, 2024
037d0ad
Fix account address case comparison
jiexi Sep 30, 2024
60f7518
Fix PermissionsConnectPermissionsList restricted network selector
jiexi Sep 30, 2024
35c92b1
Merge branch 'caip-multichain' into jl/caip-multichain/fix-connection…
jiexi Sep 30, 2024
5099d98
Merge branch 'caip-multichain' into jl/caip-multichain/fix-connection…
jiexi Sep 30, 2024
171a19a
Merge branch 'caip-multichain' into jl/caip-multichain/fix-connection…
jiexi Oct 1, 2024
1b4deb6
Merge branch 'caip-multichain' into jl/caip-multichain/fix-connection…
jiexi Oct 1, 2024
7915c28
Merge branch 'caip-multichain' into jl/caip-multichain/fix-connection…
jiexi Oct 3, 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
28 changes: 9 additions & 19 deletions app/scripts/controllers/permissions/background-api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,12 @@ import {
getPermittedEthChainIds,
setPermittedEthChainIds,
} from '../../lib/multichain-api/adapters/caip-permission-adapter-permittedChains';
import {
CaveatTypes,
RestrictedMethods,
} from '../../../../shared/constants/permissions';
import { RestrictedMethods } from '../../../../shared/constants/permissions';
import { PermissionNames } from './specifications';

export function getPermissionBackgroundApiMethods({
permissionController,
approvalController,
networkController,
}) {
// To add more than one account when already connected to the dapp
const addMoreAccounts = (origin, accounts) => {
Expand Down Expand Up @@ -75,17 +71,23 @@ export function getPermissionBackgroundApiMethods({
throw new Error('tried to add chains when none have been permissioned'); // TODO: better error
}

// get the list of permitted eth accounts before we modify the permitted chains and potentially lose some
const ethAccounts = getEthAccounts(caip25Caveat.value);

const ethChainIds = getPermittedEthChainIds(caip25Caveat.value);

const updatedEthChainIds = Array.from(
new Set([...ethChainIds, ...chainIds]),
);

const updatedCaveatValue = setPermittedEthChainIds(
let updatedCaveatValue = setPermittedEthChainIds(
caip25Caveat.value,
updatedEthChainIds,
);

// ensure that the list of permitted eth accounts is intact after permitted chain updates
updatedCaveatValue = setEthAccounts(updatedCaveatValue, ethAccounts);

permissionController.updateCaveat(
origin,
Caip25EndowmentPermissionName,
Expand All @@ -95,11 +97,6 @@ export function getPermissionBackgroundApiMethods({
};

const requestAccountsAndChainPermissionsWithId = (origin) => {
const { chainId } =
networkController.getNetworkConfigurationByNetworkClientId(
networkController.state.selectedNetworkClientId,
);

const id = nanoid();
// NOTE: the eth_accounts/permittedChains approvals will be combined in the future.
// Until they are actually combined, when testing, you must request both
Expand All @@ -115,14 +112,7 @@ export function getPermissionBackgroundApiMethods({
},
permissions: {
[RestrictedMethods.eth_accounts]: {},
[PermissionNames.permittedChains]: {
caveats: [
{
type: CaveatTypes.restrictNetworkSwitching,
value: [chainId],
},
],
},
[PermissionNames.permittedChains]: {},
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this change may be breaking something about the UI in the wallet_switchEthereumChain permissions flow on the legacy API?

Screen.Recording.2024-09-30.at.3.07.51.PM.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don' think I understand what is broken here. If I am correct, you start without Ethereum mainnet permissioned, and then switch to it. After approving the switch, Ethereum mainnet is checked inside the EditNetworkModal. I assume this dapp has EIP-1193 granted permissions, otherwise the switch chain call would have failed (rather than prompting the user to approve or not)

Copy link
Contributor

@adonesky1 adonesky1 Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Oh sorry I'm specifically highlighting that that switch chain permission UI no longer shows the ethereum avatar or network name
Screenshot 2024-09-30 at 4 00 11 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh gotcha. missed that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed here 60f7518

will pull into PR against dev as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#27518

},
},
type: MethodNames.requestPermissions,
Expand Down
110 changes: 29 additions & 81 deletions app/scripts/controllers/permissions/background-api.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import { MethodNames } from '@metamask/permission-controller';
import {
CaveatTypes,
RestrictedMethods,
} from '../../../../shared/constants/permissions';
import { RestrictedMethods } from '../../../../shared/constants/permissions';
import {
Caip25CaveatType,
Caip25EndowmentPermissionName,
} from '../../lib/multichain-api/caip25permissions';
import { flushPromises } from '../../../../test/lib/timer-helpers';
import {
KnownNotifications,
KnownRpcMethods,
} from '../../lib/multichain-api/scope';
import { getPermissionBackgroundApiMethods } from './background-api';
import { PermissionNames } from './specifications';

Expand Down Expand Up @@ -469,45 +470,7 @@ describe('permission background API methods', () => {
});

describe('requestAccountsAndChainPermissionsWithId', () => {
it('gets the networkConfiguration for the current globally selected network client', () => {
const networkController = {
state: {
selectedNetworkClientId: 'mainnet',
},
getNetworkConfigurationByNetworkClientId: jest.fn().mockReturnValue({
chainId: '0x1',
}),
};
const approvalController = {
addAndShowApprovalRequest: jest.fn().mockResolvedValue({
approvedChainIds: ['0x1', '0x5'],
approvedAccounts: ['0xdeadbeef'],
}),
};
const permissionController = {
grantPermissions: jest.fn(),
};

getPermissionBackgroundApiMethods({
networkController,
approvalController,
permissionController,
}).requestAccountsAndChainPermissionsWithId('foo.com');

expect(
networkController.getNetworkConfigurationByNetworkClientId,
).toHaveBeenCalledWith('mainnet');
});

it('requests eth_accounts and permittedChains approval and returns the request id', async () => {
const networkController = {
state: {
selectedNetworkClientId: 'mainnet',
},
getNetworkConfigurationByNetworkClientId: jest.fn().mockReturnValue({
chainId: '0x1',
}),
};
const approvalController = {
addAndShowApprovalRequest: jest.fn().mockResolvedValue({
approvedChainIds: ['0x1', '0x5'],
Expand All @@ -519,7 +482,6 @@ describe('permission background API methods', () => {
};

const result = getPermissionBackgroundApiMethods({
networkController,
approvalController,
permissionController,
}).requestAccountsAndChainPermissionsWithId('foo.com');
Expand All @@ -539,14 +501,7 @@ describe('permission background API methods', () => {
},
permissions: {
[RestrictedMethods.eth_accounts]: {},
[PermissionNames.permittedChains]: {
caveats: [
{
type: CaveatTypes.restrictNetworkSwitching,
value: ['0x1'],
},
],
},
[PermissionNames.permittedChains]: {},
},
},
type: MethodNames.requestPermissions,
Expand All @@ -555,14 +510,6 @@ describe('permission background API methods', () => {
});

it('grants a legacy CAIP-25 permission (isMultichainOrigin: false) with the approved eip155 chainIds and accounts', async () => {
const networkController = {
state: {
selectedNetworkClientId: 'mainnet',
},
getNetworkConfigurationByNetworkClientId: jest.fn().mockReturnValue({
chainId: '0x1',
}),
};
const approvalController = {
addAndShowApprovalRequest: jest.fn().mockResolvedValue({
approvedChainIds: ['0x1', '0x5'],
Expand All @@ -574,7 +521,6 @@ describe('permission background API methods', () => {
};

getPermissionBackgroundApiMethods({
networkController,
approvalController,
permissionController,
}).requestAccountsAndChainPermissionsWithId('foo.com');
Expand All @@ -594,13 +540,13 @@ describe('permission background API methods', () => {
requiredScopes: {},
optionalScopes: {
'eip155:1': {
methods: [],
notifications: [],
methods: KnownRpcMethods.eip155,
notifications: KnownNotifications.eip155,
accounts: ['eip155:1:0xdeadbeef'],
},
'eip155:5': {
methods: [],
notifications: [],
methods: KnownRpcMethods.eip155,
notifications: KnownNotifications.eip155,
accounts: ['eip155:5:0xdeadbeef'],
},
},
Expand Down Expand Up @@ -649,7 +595,7 @@ describe('permission background API methods', () => {
);
});

it('calls updateCaveat with the chain added', () => {
it('calls updateCaveat with the chain added and all eip155 accounts synced', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

What does "all eip155 accounts synced" mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the eip155 scopeObjects, gather the unique list of eip155 accounts, and then assign that list back to each eip155 scopeObject (overwriting any accounts array value that was previous there)

const permissionController = {
getCaveat: jest.fn().mockReturnValue({
value: {
Expand All @@ -661,7 +607,7 @@ describe('permission background API methods', () => {
'eip155:10': {
methods: [],
notifications: [],
accounts: ['eip155:10:0x1', 'eip155:10:0x2'],
accounts: ['eip155:10:0x2'],
},
},
optionalScopes: {
Expand All @@ -675,7 +621,7 @@ describe('permission background API methods', () => {
'eip155:1': {
methods: [],
notifications: [],
accounts: ['eip155:1:0x2', 'eip155:1:0x3'],
accounts: ['eip155:1:0x1'],
},
},
isMultichainOrigin: true,
Expand All @@ -698,6 +644,7 @@ describe('permission background API methods', () => {
'eip155:1': {
methods: [],
notifications: [],
accounts: ['eip155:1:0x1', 'eip155:1:0x2'],
},
'eip155:10': {
methods: [],
Expand All @@ -716,12 +663,12 @@ describe('permission background API methods', () => {
'eip155:1': {
methods: [],
notifications: [],
accounts: ['eip155:1:0x2', 'eip155:1:0x3'],
accounts: ['eip155:1:0x1', 'eip155:1:0x2'],
},
'eip155:1337': {
methods: [],
notifications: [],
accounts: [],
methods: KnownRpcMethods.eip155,
notifications: KnownNotifications.eip155,
accounts: ['eip155:1337:0x1', 'eip155:1337:0x2'],
},
},
isMultichainOrigin: true,
Expand Down Expand Up @@ -765,7 +712,7 @@ describe('permission background API methods', () => {
);
});

it('calls updateCaveat with the chains added', () => {
it('calls updateCaveat with the chains added and all eip155 accounts synced', () => {
const permissionController = {
getCaveat: jest.fn().mockReturnValue({
value: {
Expand All @@ -777,7 +724,7 @@ describe('permission background API methods', () => {
'eip155:10': {
methods: [],
notifications: [],
accounts: ['eip155:10:0x1', 'eip155:10:0x2'],
accounts: ['eip155:10:0x2'],
},
},
optionalScopes: {
Expand All @@ -791,7 +738,7 @@ describe('permission background API methods', () => {
'eip155:1': {
methods: [],
notifications: [],
accounts: ['eip155:1:0x2', 'eip155:1:0x3'],
accounts: ['eip155:1:0x1'],
},
},
isMultichainOrigin: true,
Expand All @@ -814,6 +761,7 @@ describe('permission background API methods', () => {
'eip155:1': {
methods: [],
notifications: [],
accounts: ['eip155:1:0x1', 'eip155:1:0x2'],
},
'eip155:10': {
methods: [],
Expand All @@ -832,17 +780,17 @@ describe('permission background API methods', () => {
'eip155:1': {
methods: [],
notifications: [],
accounts: ['eip155:1:0x2', 'eip155:1:0x3'],
accounts: ['eip155:1:0x1', 'eip155:1:0x2'],
},
'eip155:4': {
methods: [],
notifications: [],
accounts: [],
methods: KnownRpcMethods.eip155,
notifications: KnownNotifications.eip155,
accounts: ['eip155:4:0x1', 'eip155:4:0x2'],
},
'eip155:5': {
methods: [],
notifications: [],
accounts: [],
methods: KnownRpcMethods.eip155,
notifications: KnownNotifications.eip155,
accounts: ['eip155:5:0x1', 'eip155:5:0x2'],
},
},
isMultichainOrigin: true,
Expand Down
9 changes: 5 additions & 4 deletions app/scripts/lib/multichain-api/adapters/caip-permission-adapter-permittedChains.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Caip25CaveatValue } from '../caip25permissions';
import { KnownNotifications, KnownRpcMethods } from '../scope';
import {
addPermittedEthChainId,
getPermittedEthChainIds,
Expand Down Expand Up @@ -89,8 +90,8 @@ describe('CAIP-25 permittedChains adapters', () => {
accounts: ['eip155:100:0x100'],
},
'eip155:101': {
methods: [],
notifications: [],
methods: KnownRpcMethods.eip155,
notifications: KnownNotifications.eip155,
accounts: [],
},
},
Expand Down Expand Up @@ -272,8 +273,8 @@ describe('CAIP-25 permittedChains adapters', () => {
accounts: ['eip155:100:0x100'],
},
'eip155:101': {
methods: [],
notifications: [],
methods: KnownRpcMethods.eip155,
notifications: KnownNotifications.eip155,
accounts: [],
},
},
Expand Down
6 changes: 4 additions & 2 deletions app/scripts/lib/multichain-api/adapters/caip-permission-adapter-permittedChains.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import { Hex, KnownCaipNamespace } from '@metamask/utils';
import { toHex } from '@metamask/controller-utils';
import { Caip25CaveatValue } from '../caip25permissions';
import {
KnownNotifications,
KnownRpcMethods,
mergeScopes,
parseScopeString,
ScopesObject,
Expand Down Expand Up @@ -44,8 +46,8 @@ export const addPermittedEthChainId = (
optionalScopes: {
...caip25CaveatValue.optionalScopes,
[scopeString]: {
methods: [],
notifications: [],
methods: KnownRpcMethods.eip155,
notifications: KnownNotifications.eip155,
accounts: [],
},
},
Expand Down
Loading