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

feat: SIP-26 Integration #29887

Merged
merged 115 commits into from
Apr 9, 2025
Merged

feat: SIP-26 Integration #29887

merged 115 commits into from
Apr 9, 2025

Conversation

jiexi
Copy link
Contributor

@jiexi jiexi commented Jan 23, 2025

Open in GitHub Codespaces

General Testing:

  • Run this in a flask build
  • Create Solana Accounts
  • Go to the Multichain Test Dapp (which now supports Solana!):
Screen.Recording.2025-04-03.at.5.00.51.PM.mov

Related issues

Ticket: https://github.com/orgs/MetaMask/projects/146/views/6?pane=issue&itemId=94617458&issue=MetaMask%7CMetaMask-planning%7C3989
Upstream Extension: #27782
Core: MetaMask/core#5191

Screenshots/Recordings

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.

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

This link was pointing to a dead site, this fixes it to point to the
correct URL.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29733?quickstart=1)

## **Related issues**

N/A

## **Manual testing steps**

1. Go to
https://github.com/MetaMask/metamask-extension/blob/develop/docs/README.md
2. Click the link to Developer Docs 

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **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.
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.

@jiexi
Copy link
Contributor Author

jiexi commented Feb 5, 2025

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

Policies updated.
👀 Please review the diff for suspicious new powers.

🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff

Copy link

socket-security bot commented Feb 11, 2025

All alerts resolved. Learn more about Socket for GitHub ↗

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

View full report ↗

@jiexi
Copy link
Contributor Author

jiexi commented Feb 11, 2025

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

Policies updated.
👀 Please review the diff for suspicious new powers.

🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff

@metamaskbot
Copy link
Collaborator

❌ API Spec Test Failed. View the report here.

@metamaskbot
Copy link
Collaborator

❌ Multichain API Spec Test Failed. View the report here.

@metamaskbot
Copy link
Collaborator

❌ API Spec Test Failed. View the report here.

@metamaskbot
Copy link
Collaborator

❌ Multichain API Spec Test Failed. View the report here.

@mcmire
Copy link
Contributor

mcmire commented Mar 6, 2025

A couple of notes:

  • The Bitcoin integration was disabled in chore(bitcoin): add bitcoin build feature + disable it temporarily #30477, so we have to use the Solana integration for testing. I think this could work:

    const extensionPort = chrome.runtime.connect(YOUR_EXTENSION_ID)
    
    extensionPort.onMessage.addListener((msg) => {
      console.log(msg.data)
    })
    
    extensionPort.postMessage({
      type: "caip-x",
      data: {
        jsonrpc: "2.0",
        method: "wallet_createSession",
        params: {
          requiredScopes: {},
          optionalScopes: {
            "solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp": {
              methods: ["getAccount"],
              notifications: [],
              accounts: []
            }
          },
        },
      }
    })
    
    extensionPort.postMessage({
      type: "caip-x",
      data: {
        jsonrpc: "2.0",
        method: "wallet_invokeMethod",
        params: {
          scope: "solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp",
          request: {
            method: "getAccount",
            params: [YOUR_SOLANA_ACCOUNT_ID]
          }
        }
      }
    })
  • However, the above does not work, and in fact, the new changes introduced in this PR are breaking createSession, getSession, etc. I'm not sure why, I'll have to investigate.

Copy link

socket-security bot commented Mar 6, 2025

Review the following changes in direct dependencies. Learn more about Socket for GitHub ↗.

Package Supply Chain
Security
Vulnerability Quality Maintenance License
Added@metamask/[email protected]731007292100
Updated[email protected] ⏵ 1.0.300016691001007498100
Added@metamask/[email protected]761007593100
Updated@metamask/[email protected] ⏵ 10.1.075 -10100100 +191 +3100

View full report ↗

@metamaskbot
Copy link
Collaborator

Builds ready [552e273]
Page Load Metrics (1671 ± 105 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint153323391676218104
domContentLoaded150822901645216104
load153223471671219105
domInteractive259539189
backgroundConnect126232157
firstReactRender1465342211
getState5471094
initialActions01000
loadScripts10821805122619594
setupStore712811
uiStartup169725451880215103

@metamaskbot
Copy link
Collaborator

Builds ready [c6b5bc7]
Page Load Metrics (1912 ± 62 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint39821501749459221
domContentLoaded17232086185010752
load17512190191212962
domInteractive257536147
backgroundConnect13191635828
firstReactRender1673382211
getState6141394120
initialActions01000
loadScripts1295159113988239
setupStore860292010
uiStartup201231842326359172

Base automatically changed from jl/caip-multichain-migrate-core to main March 11, 2025 20:58
@adonesky1 adonesky1 changed the base branch from main to jl/caip-multichain-migrate-core March 12, 2025 18:24
@jiexi jiexi changed the base branch from jl/caip-multichain-migrate-core to main March 12, 2025 20:24
@jiexi jiexi changed the title DRAFT: SIP-26 Integration feat: SIP-26 Integration Mar 12, 2025
@adonesky1
Copy link
Contributor

Per @mcmire 's comment above

The Bitcoin integration was disabled in #30477, so we have to use the Solana integration for testing:

In order to test with solana you will need to:

  1. build this branch on flask
  2. create a solana account
  3. The script below will work to create a session/connection with a site and the wallet including Solana. The connection flow is not yet wired up to show Solana accounts, but you can connect for read only purposes.
const extensionPort = chrome.runtime.connect(YOUR_EXTENSION_ID)

extensionPort.onMessage.addListener((msg) => {
  console.log(msg.data)
})

extensionPort.postMessage({
  type: "caip-x",
  data: {
    jsonrpc: "2.0",
    method: "wallet_createSession",
    params: {
      requiredScopes: {},
      optionalScopes: {
        "solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp": {
          methods: ["getAccount"],
          notifications: [],
          accounts: []
        }
      },
    },
  }
})
  1. The Solana snap is currently broken but a fix was recently merged and should be released and merged to main soon.
  2. Once that fix is added and absorbed into this branch you'll be able to call to the Solana snap with this script:
extensionPort.postMessage({
  type: "caip-x",
  data: {
    jsonrpc: "2.0",
    method: "wallet_invokeMethod",
    params: {
      scope: "solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp",
      request: {
        method: "getGenesisHash",
        params: []
      }
    }
  }

@metamaskbot
Copy link
Collaborator

Builds ready [547fb5d]
Page Load Metrics (1874 ± 126 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint156624391852220106
domContentLoaded15572159175514871
load156726601874262126
domInteractive2311333209
backgroundConnect1359211413264
firstReactRender1488362110
getState5220485828
initialActions01000
loadScripts11631610133010450
setupStore76419178
uiStartup178342952338619297
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 1.4 KiB (0.02%)
  • ui: 158 Bytes (0.00%)
  • common: 9.53 KiB (0.09%)

@metamaskbot
Copy link
Collaborator

Builds ready [21627b6]
Page Load Metrics (1751 ± 85 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint23621711657360173
domContentLoaded15131991169113364
load15282293175117785
domInteractive229035157
backgroundConnect9356637737
firstReactRender1785462110
getState4198254321
initialActions01000
loadScripts10871508124811354
setupStore870252311
uiStartup178836282094401192
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 1.4 KiB (0.02%)
  • ui: 158 Bytes (0.00%)
  • common: 9.53 KiB (0.09%)

adonesky1 and others added 6 commits April 9, 2025 10:55
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/31736?quickstart=1)

## **Related issues**

Fixes:

## **Manual testing steps**

1. Go to this page...
2.
3.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **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.
Attempt to address [some concerns about time complexity raised by
@FrederikBolding](#29887 (comment))

rename and reorganize these helpers for easier navigation.

I have [an open PR against core
here](MetaMask/core#5609) where I've made the
improvements + name changes that can be reviewed and merged immediately
after 12.17.0 cut
const supportedRequestedAccounts = requestedCaipAccountIds.reduce(
(acc, account) => {
const supportedRequestedAccount =
supportedAccountsForRequestedNamespaces.find(
Copy link
Member

Choose a reason for hiding this comment

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

Could these be EIP155 addresses that should be compared without casing? Just wondering if this might cause a bug in the EVM flow

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I considered this and thought it wasn't an issue, but I think it may be so I'll add that check for EVM addresses only

Copy link
Contributor

Choose a reason for hiding this comment

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

The consequences here are pretty low, it would just mean a requested account (rare that specific accounts are requested at all) wouldn't get default pre-checked in case mismatch

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but seems worth it to make sure since we specifically added this for certain dapps that asked for it 😄

Copy link
Member

Choose a reason for hiding this comment

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

Probably not for now, but perhaps worth considering in the future to move some of this to a more shared place. So we don't have multiple implementations of checking for EIP155 etc

Copy link
Contributor

Choose a reason for hiding this comment

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

done here: bc2852a

FrederikBolding
FrederikBolding previously approved these changes Apr 9, 2025
mcmire
mcmire previously approved these changes Apr 9, 2025
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

LGTM!

darkwing
darkwing previously approved these changes Apr 9, 2025
@metamaskbot
Copy link
Collaborator

Builds ready [308b7ce]
UI Startup Metrics (1219 ± 70 ms)
PlatformBuildTypePageMetricMean (ms)Min (ms)Max (ms)Std Dev (ms)P 75 (ms)P 95 (ms)
ChromeBrowserifyHomeuiStartup1219109314257012621368
load10679701251631166994
domContentLoaded10629651244631181992
domInteractive17136271627
firstPaint7111351248419253984
backgroundConnect7421279
firstReactRender20144952027
getState13534769
initialActions001001
loadScripts820732101463856941
setupStore7418278
WebpackHomeuiStartup21511739276518622762418
load17211265244417918021969
domContentLoaded17151258243417817981962
domInteractive181285131453
firstPaint17869183717621488
backgroundConnect299242322956
firstReactRender155534661126088
getState11335678
initialActions317135
loadScripts17071256239417517931952
setupStore2662994779
FirefoxBrowserifyHomeuiStartup13761189183111814351644
load12331058171012012881514
domContentLoaded12331057170912012881514
domInteractive10738318409297
firstPaintNaNNaNNaNNaNNaNNaN
backgroundConnect19136771934
firstReactRender23185062332
getState941571679
initialActions001001
loadScripts12171038169612012761498
setupStore6433367
WebpackHomeuiStartup15261377196410015911677
load1319119316809113841460
domContentLoaded1318119216799113841459
domInteractive9650229259097
firstPaintNaNNaNNaNNaNNaNNaN
backgroundConnect21154652234
firstReactRender35295053746
getState8345689
initialActions102111
loadScripts1300117416539013621443
setupStore8529489

@adonesky1 adonesky1 enabled auto-merge April 9, 2025 18:24
@jiexi jiexi dismissed stale reviews from darkwing, mcmire, and FrederikBolding via 13492c2 April 9, 2025 18:38
@metamaskbot
Copy link
Collaborator

Builds ready [13492c2]
UI Startup Metrics (1239 ± 56 ms)
PlatformBuildTypePageMetricMean (ms)Min (ms)Max (ms)Std Dev (ms)P 75 (ms)P 95 (ms)
ChromeBrowserifyHomeuiStartup1239112414215612711334
load10839731199511132973
domContentLoaded10779691182501128984
domInteractive18136071731
firstPaint7391491196427231281
backgroundConnect7420279
firstReactRender20164762036
getState14574978
initialActions001001
loadScripts83473993348870916
setupStore8523378
WebpackHomeuiStartup21691717261016722792383
load16941295214115417651980
domContentLoaded16871288213515217541971
domInteractive171270121455
firstPaint155663125120691
backgroundConnect339398443665
firstReactRender197554911216086
getState19430338549
initialActions318135
loadScripts16821286213314917521940
setupStore26630846339
FirefoxBrowserifyHomeuiStartup13561196166711214341606
load12111073153710912751435
domContentLoaded12111073153610912751435
domInteractive9539201279096
firstPaintNaNNaNNaNNaNNaNNaN
backgroundConnect211363102051
firstReactRender23196462327
getState7320279
initialActions001001
loadScripts11921056151710912561419
setupStore931771867
WebpackHomeuiStartup15491387194111615981809
load13331178164810513811556
domContentLoaded13321177164810513801556
domInteractive9540318299198
firstPaintNaNNaNNaNNaNNaNNaN
backgroundConnect22165362337
firstReactRender36295453947
getState10442789
initialActions102111
loadScripts13131159162610413591539
setupStore8549589

@adonesky1 adonesky1 added this pull request to the merge queue Apr 9, 2025
Merged via the queue into main with commit 3ab2865 Apr 9, 2025
165 of 166 checks passed
@adonesky1 adonesky1 deleted the sip-26 branch April 9, 2025 20:52
@github-actions github-actions bot locked and limited conversation to collaborators Apr 9, 2025
@metamaskbot metamaskbot added the release-12.17.0 Issue or pull request that will be included in release 12.17.0 label Apr 9, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
INVALID-PR-TEMPLATE PR's body doesn't match template release-12.17.0 Issue or pull request that will be included in release 12.17.0 team-wallet-api-platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants