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(keyring-snap-bridge): make account creation async #207

Merged
merged 3 commits into from
Feb 14, 2025

Conversation

ccharly
Copy link
Collaborator

@ccharly ccharly commented Feb 13, 2025

We now resume the Snap execution before persisting the account into the Snap keyring state.

This allows the Snap to have an existing account (once approved or not by the user, just like before), but this delays the actual account creation on the MetaMask clients (more specifically, the AccountsController in this case), which allows other controllers to react to the AccountsController:accountCreated event and to directly fetch account informations (assets/balances) from the Snap.

In the following example, the SomeOtherController cannot use the account because the account wasn't persisted by the Snap yet:

// Snap keyring methods
snap.createAccount = () => {
  const account = ...;
  
  await emitSnapKeyringEvent(AccountCreated, { account }); // -> This will trigger keyring.handleSnapkeyringEvent
  
  // Now persists the account (since we know MetaMask has accepted it).
  state.accounts.push(account);
}

// Snap keyring (on the MetaMask client)
keyring.handleSnapKeyringEvent = (message) => {
  ...
  switch (message.method) {
    case 'AccountCreated': {
      // Show some dialogs for the user
      if (dialog.accepted) {
        // At this point, Snap execution is still pending...
        await keyring.saveState(); // This will trigger AccountsController:accountAdded (see addNewAccount method in this example) event 
      }
    }
  ...
})

AccountsController.addNewAccount = (account) => {
  this.messenger.publish(`AccountsController:accountAdded`, account); // This will trigger SomeOtherController.onAccountAdded handler
}

SomeOtherController.onAccountAdded = (account) => {
  // !
  // ! Here was the problem, since the account was still not persisted onto the Snap.
  // !
  await this.getSnapClient(account.metadata.snap.id).listAccountAssets();
}

@ccharly ccharly force-pushed the feat/async-snap-keyring-create-account branch 3 times, most recently from 0c569db to 55a0b41 Compare February 13, 2025 14:07
@ccharly ccharly force-pushed the feat/async-snap-keyring-create-account branch from 55a0b41 to d9aa015 Compare February 14, 2025 10:15
@ccharly ccharly marked this pull request as ready for review February 14, 2025 10:41
@ccharly ccharly requested a review from a team as a code owner February 14, 2025 10:41
@ccharly
Copy link
Collaborator Author

ccharly commented Feb 14, 2025

@metamaskbot publish-previews

Copy link

Preview builds have been published. See these instructions (from the core monorepo) for more information about preview builds.

Expand for full list of packages and versions.
{
  "@metamask-previews/keyring-api": "17.0.0-d9aa015",
  "@metamask-previews/eth-hd-keyring": "10.0.0-d9aa015",
  "@metamask-previews/eth-ledger-bridge-keyring": "8.0.4-d9aa015",
  "@metamask-previews/eth-simple-keyring": "8.1.0-d9aa015",
  "@metamask-previews/eth-trezor-keyring": "6.1.0-d9aa015",
  "@metamask-previews/keyring-internal-api": "4.0.2-d9aa015",
  "@metamask-previews/keyring-internal-snap-client": "4.0.0-d9aa015",
  "@metamask-previews/eth-snap-keyring": "10.0.0-d9aa015",
  "@metamask-previews/keyring-snap-client": "4.0.0-d9aa015",
  "@metamask-previews/keyring-snap-sdk": "3.0.0-d9aa015",
  "@metamask-previews/keyring-utils": "2.0.0-d9aa015"
}

@ccharly
Copy link
Collaborator Author

ccharly commented Feb 14, 2025

@metamaskbot publish-previews

Copy link

Preview builds have been published. See these instructions (from the core monorepo) for more information about preview builds.

Expand for full list of packages and versions.
{
  "@metamask-previews/keyring-api": "17.0.0-b0257dd",
  "@metamask-previews/eth-hd-keyring": "10.0.0-b0257dd",
  "@metamask-previews/eth-ledger-bridge-keyring": "8.0.4-b0257dd",
  "@metamask-previews/eth-simple-keyring": "8.1.0-b0257dd",
  "@metamask-previews/eth-trezor-keyring": "6.1.0-b0257dd",
  "@metamask-previews/keyring-internal-api": "4.0.2-b0257dd",
  "@metamask-previews/keyring-internal-snap-client": "4.0.0-b0257dd",
  "@metamask-previews/eth-snap-keyring": "10.0.0-b0257dd",
  "@metamask-previews/keyring-snap-client": "4.0.0-b0257dd",
  "@metamask-previews/keyring-snap-sdk": "3.0.0-b0257dd",
  "@metamask-previews/keyring-utils": "2.0.0-b0257dd"
}

Co-authored-by: Daniel Rocha <[email protected]>
@danroc danroc added this pull request to the merge queue Feb 14, 2025
Merged via the queue into main with commit ecdc034 Feb 14, 2025
31 checks passed
@danroc danroc deleted the feat/async-snap-keyring-create-account branch February 14, 2025 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants