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.

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

refactor: connection Flow to use CAIP25 Permission format #29824

Open
wants to merge 46 commits into
base: main
Choose a base branch
from

Conversation

ffmcgee725
Copy link
Member

@ffmcgee725 ffmcgee725 commented Jan 21, 2025

For part of our collaboration with the Snaps team to enable Non EVM multichain API we are refactoring the connection flow UI.

The first step to enable this to modify what the connection confirmation UI components receive from the ApprovalController.
Currently they receive an object like this:

{
    "eth_accounts": {
        "caveats": [
            {
                "type": "restrictReturnedAccounts",
                "value": []
            }
        ]
    },
    "endowment:permitted-chains": {
        "caveats": [
            {
                "type": "restrictNetworkSwitching",
                "value": []
            }
        ]
    }
}

Basically the params for a wallet_requestPermission request.

We want to change this so that instead they receive (and know how to interpret) a CAIP25 formatted request like:

{
    "eip155:1": {
        "accounts": []  // there may be accounts here but often will not be
    },
    "eip155:10": {
        "accounts": []
    },
}

Description

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Send the following request
await window.ethereum.request({
 "method": "wallet_requestPermissions",
 "params": [
  {
    
    "eth_accounts": {
        "caveats": [
            {
                "type": "restrictReturnedAccounts",
                "value": []
            }
        ]
    },
    "endowment:permitted-chains": {
        "caveats": [
            {
                "type": "restrictNetworkSwitching",
                "value": []
            }
        ]
    }
  }
],
});


  1. Wallet extension UI should prompt user to give permissions to accounts / chains and edit if desired or confirm to proceed.

Screenshots/Recordings

Screen.Recording.2025-01-23.at.17.18.17.mov

NOTE: Don't mind different screens in the UI, this vid is outdated and all UI screens are same as main branch

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@ffmcgee725 ffmcgee725 changed the base branch from jl/caip-multichain-migrate-core to main January 23, 2025 16:13
@ffmcgee725 ffmcgee725 force-pushed the jc/caip25-permission-refactor branch from 5218ea4 to f771a58 Compare January 23, 2025 16:13
@ffmcgee725 ffmcgee725 changed the title Refactor Connection Flow to use CAIP25 Permission format Refactor: Connection Flow to use CAIP25 Permission format Jan 23, 2025
@ffmcgee725 ffmcgee725 changed the title Refactor: Connection Flow to use CAIP25 Permission format refactor: Connection Flow to use CAIP25 Permission format Jan 23, 2025
@ffmcgee725 ffmcgee725 changed the title refactor: Connection Flow to use CAIP25 Permission format refactor: connection Flow to use CAIP25 Permission format Jan 23, 2025
@ffmcgee725 ffmcgee725 requested review from jiexi and adonesky1 January 24, 2025 19:20
* @param hexChainIds - The list of permitted chains.
* @returns The granted permissions with the target name of the {@link Caip25EndowmentPermissionName}.
*/
export function parseCaip25PermissionsResponse(
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] - I think parse maybe is a confusing word for this.

maybe just keep it pretty generic/straightforward:

Suggested change
export function parseCaip25PermissionsResponse(
export function getApprovedSessionsScopes(

Copy link
Member Author

Choose a reason for hiding this comment

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

resolved in ecad6ee

jiexi
jiexi previously approved these changes Jan 24, 2025
jiexi
jiexi previously approved these changes Jan 24, 2025
@ffmcgee725 ffmcgee725 requested a review from adonesky1 January 24, 2025 19:54
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

adonesky1
adonesky1 previously approved these changes Jan 24, 2025
Copy link
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

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

LGTM

@ffmcgee725 ffmcgee725 dismissed stale reviews from adonesky1 and jiexi via e49c99f January 25, 2025 01:33
@metamaskbot
Copy link
Collaborator

Builds ready [ae0bdb4]
Page Load Metrics (1747 ± 70 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint15412120174414569
domContentLoaded15312063171713665
load15432118174714570
domInteractive24206443919
backgroundConnect987272110
firstReactRender17168483718
getState561202110
initialActions01000
loadScripts11001531127111354
setupStore86117178
uiStartup17132481203420799
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 490 Bytes (0.01%)
  • ui: 1.24 KiB (0.02%)
  • common: -409 Bytes (-0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [bfccd6e]
Page Load Metrics (1666 ± 78 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint46620631607309148
domContentLoaded14862003164315675
load14972057166616378
domInteractive248735157
backgroundConnect85628189
firstReactRender1598472713
getState45312136
initialActions01000
loadScripts10571494119013766
setupStore7521294
uiStartup16932329190618589
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 490 Bytes (0.01%)
  • ui: 1.13 KiB (0.01%)
  • common: -259 Bytes (-0.00%)

@ffmcgee725 ffmcgee725 requested review from adonesky1 and jiexi January 26, 2025 02:32
@metamaskbot
Copy link
Collaborator

Builds ready [80611dc]
Page Load Metrics (1643 ± 64 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint22218811361564271
domContentLoaded14101861162112962
load14181867164313464
domInteractive237939178
backgroundConnect76625189
firstReactRender1578342512
getState45614178
initialActions01000
loadScripts10031366117810852
setupStore787232512
uiStartup16082292193819996
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 490 Bytes (0.01%)
  • ui: 1.13 KiB (0.01%)
  • common: -259 Bytes (-0.00%)

Comment on lines +59 to +78
[Caip25EndowmentPermissionName]: ({ t, permissionValue }) => {
const caveatValue = getRequestedSessionScopes({
permissions: permissionValue,
});
const requestedAccounts = getEthAccounts(caveatValue);
const isLegacySwitchChain = Boolean(!requestedAccounts.length);

if (isLegacySwitchChain) {
return {
label: t('permission_walletSwitchEthereumChain'),
leftIcon: IconName.Wifi,
weight: PermissionWeight.permittedChains,
};
}
return {
label: t('permission_ethereumAccounts'),
leftIcon: IconName.Eye,
weight: PermissionWeight.eth_accounts,
};
},
Copy link
Member Author

Choose a reason for hiding this comment

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

To make sure we render the proper UI on wallet_switchEthereumChain request, we add an edge case.

Fortunately for us, this code is not triggered on:

{
 "method": "wallet_requestPermissions",
 "params": [
  {
    
    "eth_accounts": {
        "caveats": [
            {
                "type": "restrictReturnedAccounts",
                "value": []
            }
        ]
    },
    "endowment:permitted-chains": {
        "caveats": [
            {
                "type": "restrictNetworkSwitching",
                "value": ["0x1"]
            }
        ]
    }
  }
],

So we are safeguarded that the proper UI will render on every covered case (no UI changes after the refactor, all e2e tests make sure of that ✅ )

Comment on lines +63 to +74
const lastPendingPermission = getLatestPendingPermissionFromOrigin(
state,
origin,
);
const isLegacySwitchEthChainRequest =
lastPendingPermission?.method === 'wallet_switchEthereumChain';

const isRequestingAccounts = Boolean(
permissionsRequest?.permissions?.[Caip25EndowmentPermissionName] &&
!isLegacySwitchEthChainRequest,
);

Copy link
Member Author

@ffmcgee725 ffmcgee725 Jan 26, 2025

Choose a reason for hiding this comment

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

Here I decided to check state object to get latest pending permission from current origin, so we can see the method to see if wallet_switchEthereumChain was used to make the call.

This way, if it was used to make the call, we are sure isRequestingAccounts (perhaps this variable name should be re-thinked...) is evaluated to false.
So, downstream on ui/pages/permissions-connect/permissions-connect.component.js we make sure to trigger

if (history.location.pathname === connectPath && !isRequestingAccounts) {
     /* `isRequestingAccounts` is now false, so negating it becomes true and we enter the switch case */
      switch (requestType) {
        case 'wallet_installSnap':
          history.replace(snapInstallPath);
          break;
        case 'wallet_updateSnap':
          history.replace(snapUpdatePath);
          break;
        case 'wallet_installSnapResult':
          history.replace(snapResultPath);
          break;
        case 'wallet_connectSnaps':
          history.replace(snapsConnectPath);
          break;
        default:
          history.replace(confirmPermissionPath); /* <---- We want the flow to come here to render the proper UI */
      }

This seems to be a consistent way of making UI rendering behaviour stay the same after the refactor. But I'm open to discussion if you guys have a better approach to solving this problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants