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: add entropySource to non-evm accounts #30549

Merged
merged 6 commits into from
Feb 27, 2025

Conversation

PatrykLucka
Copy link
Contributor

@PatrykLucka PatrykLucka commented Feb 25, 2025

Description

Adds entropySource to non-EVM accounts to ensure that the primary keyring ID is provided to the Solana snap during account creation. This will help maintain compatibility for the multi-SRP feature, even though we are not using it yet.

Manual testing steps

  • Create a local flask build with yarn start:flask
  • Create a solana account
  • Dump state and look in AccountsController store (metamask.accounts)

Screenshots/Recordings

Before

After

Expected behaviour

entropySource should be present in account.options

{
        "97efa6e1-4de8-486f-94d6-14707ce91024": {
          "type": "solana:data-account",
          "id": "97efa6e1-4de8-486f-94d6-14707ce91024",
          "address": "4QoCH9mHHaUMmJBHMFdeAn6DLJV7qcCYrLy7dAKDdee6",
          "options": {
            "scope": "solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp",
            "entropySource": "01JN1FPDGHYKA2K6JQBKEGV56T"
          },
          "methods": [
            "sendAndConfirmTransaction"
          ],
          "scopes": [
            "solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp",
            "solana:4uhcVJyU9pJkvQyS88uRDiswHXSCkY3z",
            "solana:EtWTRABZaYq6iMfeYKouRu166VU2xqa1"
          ],
          "metadata": {
            "name": "Solana Account 1",
            "importTime": 1740585525303,
            "keyring": {
              "type": "Snap Keyring"
            },
            "snap": {
              "id": "npm:@metamask/solana-wallet-snap",
              "name": "Solana",
              "enabled": true
            },
            "lastSelected": 1740585525308,
            "nameLastUpdatedAt": 1740585525310
          }
        }
      },
      "selectedAccount": "97efa6e1-4de8-486f-94d6-14707ce91024"
    },

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.

@PatrykLucka PatrykLucka self-assigned this Feb 25, 2025
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.

@metamaskbot metamaskbot added the team-mmi PRs from the MMI team label Feb 25, 2025
@metamaskbot
Copy link
Collaborator

Builds ready [ccfc68d]
Page Load Metrics (1762 ± 88 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14862094175518991
domContentLoaded14332078172818589
load14952096176218288
domInteractive279041178
backgroundConnect980342110
firstReactRender1575302110
getState573262311
initialActions00000
loadScripts10231564126615876
setupStore861232010
uiStartup16572349201119393

@PatrykLucka PatrykLucka force-pushed the add-entropy-srouce-for-non-evm-accounts branch from ccfc68d to a6ccbb8 Compare February 26, 2025 15:48
@PatrykLucka PatrykLucka changed the base branch from multi-srp-mvp-2 to main February 26, 2025 15:48
@PatrykLucka PatrykLucka force-pushed the add-entropy-srouce-for-non-evm-accounts branch from a6ccbb8 to b7e2608 Compare February 26, 2025 15:58
@PatrykLucka PatrykLucka force-pushed the add-entropy-srouce-for-non-evm-accounts branch from b7e2608 to 0979f7a Compare February 26, 2025 16:11
@PatrykLucka PatrykLucka marked this pull request as ready for review February 26, 2025 16:35
@PatrykLucka PatrykLucka requested a review from a team as a code owner February 26, 2025 16:35
@shane-t shane-t self-requested a review February 26, 2025 16:35
shane-t
shane-t previously approved these changes Feb 26, 2025
Copy link
Member

@shane-t shane-t left a comment

Choose a reason for hiding this comment

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

Passes manual tests

@shane-t shane-t requested a review from danroc February 26, 2025 16:38
@metamaskbot
Copy link
Collaborator

Builds ready [0979f7a]
Page Load Metrics (1595 ± 53 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint34217831521288138
domContentLoaded13831782156211354
load13941788159511153
domInteractive247832126
backgroundConnect11131392713
firstReactRender1478362411
getState45914157
initialActions01000
loadScripts1020135911709746
setupStore75211105
uiStartup16092193181214871
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 30 Bytes (0.00%)
  • common: 113 Bytes (0.00%)

@PatrykLucka PatrykLucka force-pushed the add-entropy-srouce-for-non-evm-accounts branch from bc9ebbf to 857d9e1 Compare February 26, 2025 17:20
Co-authored-by: Charly Chevalier <[email protected]>
@PatrykLucka PatrykLucka force-pushed the add-entropy-srouce-for-non-evm-accounts branch from 857d9e1 to 2b0dda9 Compare February 26, 2025 17:22
await bitcoinWalletSnapClient.createAccount(network);
await bitcoinWalletSnapClient.createAccount(
network,
primaryKeyring.metadata.id,
Copy link
Contributor

Choose a reason for hiding this comment

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

Current Bitcoin Snap does not support this entropySource and will throw an error if we pass it there.

Since we're about to use our new version of the Snap, we might wanna support this too.

For now, Bitcoin support is disabled, so this won't break anything (unless we use the bitcoin build flag + create an account).

cc: @danroc @gantunesr @darioAnongba

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should leave it unchanged for Bitcoin for now

@ccharly
Copy link
Contributor

ccharly commented Feb 26, 2025

Tested on my side, not approving the PR for now (just waiting for some internal decision about the Bitcoin case).

$ jq '.metamask | {keyrings, keyringsMetadata}'
{
  "keyrings": [
    {
      "type": "HD Key Tree",
      "accounts": [
        "0xb5e5196de890271ba5f6c696fb0abbc7d3a5c6c5"
      ]
    },
    {
      "type": "Snap Keyring",
      "accounts": [
        "4wRTxSXApFzKS4tk4fqQiEcSYe43NyYX1Xvh6VhzfkkE",
        "JDSQGLPQ9RCcXrvaxZiayoWzMo4FktaywDDJcBTQktyr"
      ]
    }
  ],
  "keyringsMetadata": [
    {
      "id": "01JN1M20V7KEFB8W68WH7A9173",
      "name": ""
    },
    {
      "id": "01JN1M29DZ4052VGS2GFMEWP97",
      "name": ""
    }
  ]
}
$ jq '.metamask.internalAccounts.accounts | to_entries[] | .value.options' state.json
{}
{
  "scope": "solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp",
  "entropySource": "01JN1M20V7KEFB8W68WH7A9173"
}
{
  "scope": "solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp",
  "entropySource": "01JN1M20V7KEFB8W68WH7A9173"
}

@metamaskbot
Copy link
Collaborator

Builds ready [2b0dda9]
Page Load Metrics (1592 ± 57 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14491902158911153
domContentLoaded14321858156210350
load14581955159211957
domInteractive248635178
backgroundConnect1199392110
firstReactRender145620126
getState45214157
initialActions01000
loadScripts1042141511509847
setupStore75713147
uiStartup16632279180713465
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 28 Bytes (0.00%)
  • common: 113 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [ca84bad]
Page Load Metrics (1746 ± 63 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint27120091515512246
domContentLoaded15081995172113665
load15202012174613263
domInteractive26117442512
backgroundConnect1371292010
firstReactRender1586402412
getState45515178
initialActions01000
loadScripts10861536128712158
setupStore86317189
uiStartup17332321200214469
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 28 Bytes (0.00%)
  • common: 113 Bytes (0.00%)

@ccharly ccharly added this pull request to the merge queue Feb 27, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 27, 2025
@danroc danroc added this pull request to the merge queue Feb 27, 2025
Merged via the queue into main with commit b3eec46 Feb 27, 2025
79 checks passed
@danroc danroc deleted the add-entropy-srouce-for-non-evm-accounts branch February 27, 2025 10:05
@github-actions github-actions bot locked and limited conversation to collaborators Feb 27, 2025
@metamaskbot metamaskbot added the release-12.14.0 Issue or pull request that will be included in release 12.14.0 label Feb 27, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.14.0 Issue or pull request that will be included in release 12.14.0 team-mmi PRs from the MMI team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants