One unsolved issue is extending already existing chains. For example, adding support for a hardware wallet account for Ethereum Mainnet chain.

In current specification, the wallet can't join multiple Snaps to provide support for singular namespace.

I currently have no idea on how to solve this problem. Any help appreciated.

Hi @ritave, so my reading of this is that snaps should implement their own version of the SnapKeyring interface and export it; the MM snap RPC controller code will forward certain DApp requests to the keyring handleRequest implementation when appropriate (permission has been granted). Is that correct?

For my MPC/TSS use case I think this would work assuming the origin in handleRequest() is the DApp origin and not the snapId.

Is it worth me updating my work on snap-keyring to provide a default implementation of the interface that corresponds to this SSIP?

I am a bit concerned about the signature of the importAccount function for creating a generic implementation:

importAccount?(chainId: ChainId, data: unknown): Promise<AccountId>;

Could we pass the AccountId here instead:

importAccount?(accountId: AccountId, data: unknown): Promise<void>;

Then the implementation doesn't need to know how to derive an AccountId from the data.

Ah yeh, I was imagining overloading the behavior of importAccount for my use case but I think we actually need another function.

When running DKG I end up with a JSON object that represents a share of the private key and I need to add it to the user's account, I thought I could use importAccount but that should only be used by the end user to import a previously exported key.

So I think we need to also expose an optional addAccount function in the interface:

addAccount?(accountId: AccountId, data: Json): Promise<void>;

What do you think?

Also, I can imagine a future where the UI needs to differentiate between single party and multi-party keys, for example:

0xffe...ead (Share #2 of a 3/5)

The type to describe the properties we would want to associate with the account (when an address is generated from a multi-party key share) would look like:

type MultiPartyShare = {
 partyNumber: number;
 threshold: number;
 parties: number;
}

Any ideas on the best way to incorporate this into the design?

@ritave, I didn't mean to imply we should remove importAccount() - that is certainly still a valid use case. I meant that we should allow for keyrings to specify the additional addAccount() function.

Putting the AccountId inside the data means the keyring needs to have knowledge of the shape of the data and makes it harder to have a generic keyring implementation. I think a generic implementation that solves 90% of use cases is better than a proliferation of different keyring implementations; but maybe a generic implementation is not feasible and I am thinking about this wrong.

If we put the multi party information inside the data then the wallet needs to know about the shape of the private key material which I don't think is a good idea as it is brittle. Also it's not sensitive information so if we could let the wallet know about it without looking at the private data then I think that is worthwhile; however, if there is no desire to display this in the UI to differentiate between single party and multi-party keys then it is not worth implementing. Personally, I think it is useful to show this as the signing flow will be significantly different. Of course, like you say we can store it in snap_manageState (which is what I do at the moment) but then the wallet does not know about it.

MetaMask Snaps had an internal meeting - here are the design notes that came out of that discussion:

• I've made the lifecycle requirements more explicit in Added lifecycle explanation to SIP-2 #21
• We would like to avoid the need for snaps being stateful due to keyring.on() and keyring.off(). There was a suggestion to make the API broadcast based, in which the snap would send a JSON-RPC notification with new data for specific event which wallet would route that to all DApps. That architecture is not possible due to subscriptions having the ability to have arguments passed down, for example eth_subscribe with specific topic listed, and so the snap needs to keep track of specific arguments for each. Events API might be revisited in the future.

To support SIP-2, MetaMask will need quite a bit of refactoring to support multiple networks at the same time. We're currently exploring what kind of architecture will be needed - I'll update this discussion when we start implementation.

Finally read through this, sorry for the delay! This is looking very good! I’ve written some feedback here, and I’m open to composing it into a PR if you’d like.

Copying that feedback here:

• I might add a bit from my above section talking about examples of new account types that are made possible by the proposal. Just because I like a motivation section that helps ignite the imagination. (New hardware wallets, contract accounts, DAOs)

• The wallet implementation SHOULD support WalletConnect v2.0 protocol.

  • Maybe this could be [[[[EIP]] 1193: Ethereum Provider JavaScript API]]? WC is more opinionated about transport, so I think it would be best if we described the provided interface in the most transport-agnostic terms.
• is the chainId variable able to go high enough for "permissionless extensibility"? I'd like any trivial object, like a lightbulb, to be able to have a unique chain. (Maybe that's just not for evm chains?)

• If there are multiple such snaps, the choice, which one of those to use, is undefined and implementation dependent.

  • I appreciate this being undefined for now. For short term I would recommend not attempting deduplication, since there are various pitfalls to that implementation. A simple shortstop would be that during the account selection step of dapp connection, allow the user to select from a list of accounts where each one is labeled by its key provider.
    • Being explicit about which account provider can help a user "keep the books separate" between key managers that might be intended for different uses: One hardware wallet for business, one for pleasure.
    • This becomes significantly more important in an environment like [[Delegatable Eth]], where a user might have multiple delegations that allow them to take actions "on behalf of" one central account, but each of those delegation paths could represent a different reason/organization to have that privilege.
  • Another approach is to just allow the user to select an account, and at signature time let them select the account provider
    • could allow nice experiences like letting a user decide which hardware wallet is most accessible to them at that time.
• This SIP gets into talking about critical uses of a namespace before defining a namespace. I might recommend defining namespace earlier.
  • Some passages that are hard to read without reading ahead and then hopping back

    • Wallet splits connection arguments into namespaces.

    • Support for a namespace MUST NOT be split between multiple snaps. For
      example, one snap can't support one chain while other snap supports
      second chain.

• Support for a namespace MUST NOT be split between multiple snaps. For example, one snap can't support one chain while other snap supports second chain.

  • I'm unclear what this means.
    • We definitely do want to be able to have snaps that handle different namespaces, which I'm pretty sure we agree with, but seems to contradict this.
    • We also want multiple snaps to be able to provide accounts for the same protocol (we might have several eth account snaps, for example, to represent hardware wallets, contract accounts, and DAO memberships)
  • Revisiting after reading the permissions, I think I have an interpretation.
    • Once a snap-provided account is selected by a user to be exposed to a dapp, that dapp's requests for that account will all be passed through to that one snap.
      • Some reasons I think this might be unnecessary to state
        • Some account managers are not able to expose all methods (contract accounts can't perform signatures, and some hardware wallets don't support some signature methods).
        • A snap could claim it supports many methods, but actually forward some of them to another snap, and that's not really any of our business.
• I see that snap permissions are specified in the manifest. I'm pretty sure this means you can also do wallet_requestPermissions. I might suggest either:
  • Just show the permission request object, and link out to a guide that covers the two methods.
  • Show both methods to request the permissions.
• I'm ecstatic that event handlers are part of this specification.
  • I'd also be excited for snaps to have a general ability to register on and off methods, despite not exposing other account management methods.
• It's great that we have these keyrings declaring what methods they support. It would also be great if we could expose this to sites in some form. I had mentioned this in [[[[EIP]] 2255: Wallet Permissions System]], and doesn't need to be part of this proposal, but the possible value of dapp-side [[feature detection]] is even stronger with this proposal.
• Recommendations for safety
  • Since we are using a [[JSON-RPC API]] to expose these sensitive methods, there is risk of account snap developers writing a [[Confused Deputy]] vulnerability.
    • These issues occur when there are >2 parties coordinating privileges.
    • Example
      • Snap Alice might have permission to interact with several of a user's other accounts. For example, so that a contract account snap can trigger transactions from key managers.
      • A user might only want to grant Dapp Bob with access to Account Carol.
      • Snap Alice is now responsible for ensuring that any of its methods provided to Dapp Bob are restricted to interactions with Account Carol.
      • If Snap Alice exposes any methods with an accountId parameter, this offers Dapp Bob an opportunity to provide a chosen string for an account that it has not been permitted access to.
      • This means that a Snap account that enlists many other accounts to imbue it with its total authority must implement its own internal permissions scheme to ensure that it never grants unintended access to some of its capabilities.
      • Long term, alternatives to [[JSON-RPC API]] such as [[CapTP]] can allow snaps much more specific "object passing" type delegation that makes these types of vulnerabilities trivially impossible.
• Might as well include a link to the keyring protocol when mentioning it.

Anyways, very excited for this. I hope Elliott will be finding a short path to the multi-simultaneous networks, and hopefully we can get on with implementing this soon. Might be worth helping out with him on that, I swear there's a simple path to it (maybe something like this).

Great, you've addressed most of my questions, thank you.

We haven't yet explored communication between multiple snaps, neither in technical details nor ux.

Sorry I keep thinking of the original beta as a sort of reference specification. In it, each snap had a normal provider, and so was able to request communication channels with other snaps the exact same way that dapps request communication to snaps.

I think inter-process communication is a fundamental enough concept that it's worth defining an interface for early. Maybe we should just write that SIP soon.

The confused deputy issue I noted will probably apply to any JSON-RPC based inter-process communication standard (unless it uses some kind of local-identifier table the way Fuchsia does), and probably belongs in documentation somewhere until we support a form of object passing as a primary way of exposing interfaces, but that doesn't need to block this, as we're already inheriting JSON-RPC, and we're using it for v1 of our API partly for the sake of ecosystem consistency.

We're currently in a feature freeze until Snaps can be released in the main extension, and so, Keyrings are put on hold. We want to continue building them as soon as we can, but that'll come earliest in third quarter of this year or later.

There's a special build for DevCon Bogota that had a partial implementation - the PR in metamask-extension repo is #16107.

No new API changes came out of that implementation, but they UI flow is still being researched and worked on.