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

SIP-27: Accounts Metadata Request #148

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

SIP-27: Accounts Metadata Request #148

wants to merge 5 commits into from

Conversation

ritave
Copy link
Contributor

@ritave ritave commented Sep 18, 2024

This SIP proposes extension to the Keyring API, with a method called keyring_listAccountsAll that exposes all accounts usable by the user in the extension.

It's result is based on keyring_listAccounts

@ritave ritave requested review from Montoya, ziad-saab and a team as code owners September 18, 2024 10:55
Copy link
Member

@Mrtenz Mrtenz left a comment

Choose a reason for hiding this comment

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

It's a bit unclear from this SIP whether this applies to all accounts in MetaMask (including Ethereum accounts not created through an account Snap), or just account Snap accounts. Maybe you can clarify that too.

SIPS/sip-27.md Outdated Show resolved Hide resolved
SIPS/sip-27.md Outdated Show resolved Hide resolved
SIPS/sip-27.md Outdated

### Snap Manifest

This SIP specifies a permission named `keyring_listAccountsAll`. This permission grants a snap the ability to retrieve account metadata through an RPC call.
Copy link
Member

Choose a reason for hiding this comment

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

What do you think of this?

Suggested change
This SIP specifies a permission named `keyring_listAccountsAll`. This permission grants a snap the ability to retrieve account metadata through an RPC call.
This SIP specifies a permission named `keyring_listAllAccounts`. This permission grants a snap the ability to retrieve account metadata through an RPC call.

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 specifically don't want it to be mistaken with existing keyring_listAccounts, which only lists accounts managed by the Snap. IMHO, by appending it at the end it's more visible,

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it makes sense to prefix this with keyring_ in the first place. So far these APIs are only used by account Snaps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want to display all accounts, we have to support how keyring manages and stores them in the extension, so this API mimics keyring internals almost 1:1.

SIPS/sip-27.md Outdated Show resolved Hide resolved
SIPS/sip-27.md Outdated
```typescript
type KeyringAccount = {
id: string; // An extension-specific unique ID
address: string; // A blockchain specific public address for the account.
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a CAIP-10 address?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This address can exist on multiple chains at the same time. Keyrings initially had a chains property, but it's not implemented

Copy link
Member

Choose a reason for hiding this comment

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

Then how would you identify which network the address is tied to? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only data available in MetaMask currently is the type property.

Copy link
Member

Choose a reason for hiding this comment

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

I guess my question is, should we combine type and address into one field with a CAIP-10 address.

Copy link
Contributor Author

@ritave ritave Sep 20, 2024

Choose a reason for hiding this comment

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

type exposes more information than just the network the address is on.

Accounts team envisioned the following types:

type AccountType =
	| 'eip155:eoa'
	| 'eip155:sca:erc4337'
	| 'bip122:p2pk'
	| 'bip122:p2pkh'
	| 'bip122:p2sh'
	| 'bip122:p2wpkh'
	| 'bip122:p2tr';

Notice they are all on mainnet while still differing

Copy link
Contributor

Choose a reason for hiding this comment

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

The main goal of type is to differentiate between accounts that may behave differently, such as EOAs and Smart Contract Accounts.

For multichain, we are still working on a solution that addresses two key issues:

  • Storing past Bitcoin addresses
  • Supporting multiple addresses for multiple chains (e.g., Cosmos)

We are considering approaches like addresses: Map<Caip2ChainId, Address[]> or addresses: Caip10Address[] (PR draft). Which is aligned with what you said above.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ritave, I need to double-check, but the reason we dropped the chains field was that CAIP-2 doesn't support wildcards. We considered using eip155:*, even though it wouldn’t be compliant, but ultimately decided to replace it with a filterAccountChains method. This method can be called with a list of CAIP-2 chain IDs, and the Snap will return the list of supported chains from the input where the account can be used.

I'm not super happy with the decision and think we can reconsided the chains field.

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, we were also considering using just the CAIP namespace as an alternative to wildcard. This way we can regroup "things" on the same namespace, so instead of eip155:*, we would use eip155.

We started using this pattern in the extension/core IIRC. But this is still not 100% compliant with a true CAIP-2 identifier...

SIPS/sip-27.md Outdated Show resolved Hide resolved
SIPS/sip-27.md Outdated

```typescript
type KeyringAccount = {
id: string; // An extension-specific unique ID
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to expose this? What do you envision we use it for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you have two accounts with the same address (eg 2 different hardware wallets with the same seed), you need to differentiate them somehow.

Copy link
Member

Choose a reason for hiding this comment

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

I feel like this just complicates things. The Snap has no information about how the address was created, right? Is it even relevant for the Snap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This provides a stable identity for an account.

For example, you could want to map accounts to a user editable notes about those accounts.

You can't use the name since it's user editable and can change, and you can't use address because there are multiple accounts with the same address.

By providing a stable identity (similar to React's key property) you can easily manipulate those accounts.

Co-authored-by: Maarten Zuidhoorn <[email protected]>
@danroc
Copy link
Contributor

danroc commented Sep 20, 2024

I'll give it a deeper read, but here are a few quick comments:

  • We already have an internal account type that includes a metadata field, but we deliberately chose not to expose that metadata to the Snaps, following the need-to-know principle.

  • In the case of the send flow, I think it would make more sense to remove the burden of filtering the accounts from the Snap and have the component handle it based on a prop---something like <AccountsList include="chainId:bip122:000000000019d6689c085ae165831e93" />.

  • The approach above also eliminates the need to expose all accounts to the Snap, which aligns with the first point.

  • The Keyring API defines an API implemented by the Snap. For example, MetaMask or the companion dapp can call the keyring_listAccounts method to list the accounts managed by the Snap. From the usage of this new proposed method, it seems more like a method that should be exposed by the wallet to be called by the Snap, rather than being part of the Keyring API.

address: string; // A blockchain specific public address for the account.
name: string; // User-given nickname for the account in the extension
type: string; // Blockchain specific type of the account. For example "eip155:erc4337"
};
Copy link
Contributor

Choose a reason for hiding this comment

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

If we wish to have a similar UI than the native one to list the accounts, we might needs some more information coming from the metadata field from InternalAccount (maybe not everything, but I was thinking about hardware-wallet accounts).

In case of HW, the account.type will be eip155:eoa, but we don't encode any other information regarding which HW if comes from. We do have this information in the account.metadata.keyring.type field like:

{
  "id": "651e5769-86b1-4717-ae7b-63a00d7b3872",
  "address": "0x9f753769a4b45e8e2ed61108f1174b5a0939e08e",
  "options": {},
  "methods": [
    ...
  ],
  "type": "eip155:eoa",
  "metadata": {
    "name": "Ledger 1",
    "importTime": 1727081928281,
    "keyring": {
      "type": "Ledger Hardware"
    },
    "lastSelected": 1727081928285,
    "nameLastUpdatedAt": 1727081928286
  }
}

And I believe, if we implement a similar AccountList component to display the list of accounts, we will need this keyring.type information to be able to display the small pill Ledger alongside the account entry.

Screenshot 2024-09-23 at 11 16 55

But I think @danroc had a different ideas regarding rendering a AccountList component, so maybe we won't even need to expose any of those metadata :).

I just wanted to be explicit here that type lacks some context/information.

@eriknson
Copy link
Member

  • In the case of the send flow, I think it would make more sense to remove the burden of filtering the accounts from the Snap and have the component handle it based on a prop---something like <AccountsList include="chainId:bip122:000000000019d6689c085ae165831e93" />.
  • The approach above also eliminates the need to expose all accounts to the Snap, which aligns with the first point.

Created an issue to start definition of this component here, feel free to drop ideas, requirements, and thoughts etc. directly on there

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.

5 participants