SIP-34: Introduce endowment:keyring.version

[ccharly]

Description

Introducing a new version field in the endowment:keyring configuration object, to allow conditional logic when introducing new breaking change to the keyring API.

[ccharly]

Question 1: I initially wanted to use keyringApiVersion, but that does sounds a bit "too much" (since we're already in a "keyring context", but maybe apiVersion could work?)

---

Question 2: I thought using number would make it easier to write runtime-logic like:

if (manifest['endowment:keyring'].version >= api.version) {
  // Then we can use breaking features from version `api.version`
}

We could also go with a "version-string", like 'v1', 'v2', etc... But they might not be really practical to use (unless we are converting them to actual number under the hood)

[GuillaumeRx]

I would prefer using a semVer

[Mrtenz]

I don't think semver makes much sense here. The main purpose of this version is to introduce breaking changes. Non-breaking changes don't need a version increment.

[GuillaumeRx]

Makes sense ! I was wondering if we would not ever do something like we did for snaps-sdk in the manifest for the keyring-api but I guess this is unrelated.

[Mrtenz]

Yeah, we could potentially leverage the platformVersion as well instead of having a version for each permission separately.

[Mrtenz]

Given that the keyring-api is not "related" to the platform itself, IDK if the platformVersion could be used in our use-case. 🤔

It's somewhat related, since the Snaps keyring API is built on the Snaps platform 😅 I wonder if instead of adding a version for specific permissions, we should check if the platform version is newer than a certain version instead. It would simplify implementation a bit as well, since the platform version is already implemented.

[ccharly]

@danroc any thoughts on this?

my only worry is, if we make a new keyring API breaking change (like for the old submitRequest example with the origin), we would make a new major version of the keyring API, but the platform version won't be bumped at the same time, since the keyring API is not really packaged/bundled/part of the platform itself? (the endowment:keyring is part of it though)

NOTE: I have no idea how/when we bump the platform version 😅

[danroc]

I don't have a strong opinion on this. However, we will probably need to make another breaking change at some point later this year (we want to split the Keyring API into smaller interfaces to cover different use cases).

In my opinion, it's simpler to handle a single platform version. It would prevent us from having to check for different combinations of endowment X versus endowment Y, but it also means we might need to bump the global version more often. That said, this shouldn't happen too frequently.

[ccharly]

I also agree with using the platform/global version too.

My only "worry" is that the keyring-api is not really "part of" the platform (the endowment is, yes), but we are not using keyring-api anywhere in the snaps monorepo/platform and thus we don't have an explicit way to be part of a platform version bump 😄

But we could probably find a trick for this, I guess.

[Mrtenz]

The platform version is tied to the version of the SDK. There are some other breaking changes we can (and want to) make in the SDK to work around this.

[Mrtenz]

I wonder if we should make this more generic, and instead describe a way for any endowment, or permission in general, to specify a version (where necessary).

[ccharly]

I'm definitely ok with that, as long as we have a way to use that version, I'm fine with this.

This would probably allow breaking changes within the endowments themselves yes!

[Mrtenz]

Maybe this should be a separate SIP, only describing the behaviour for version 2.

[ccharly]

Sounds like a good plan to me yes! First, the versioning, second, the actual change introduced with that version.

I'll get rid of this detailed example then, thanks!

[ccharly]

922ce85

[Mrtenz]

I'm confused by why this is a breaking change. Adding new fields to a request should not be considered breaking.

[ccharly]

Yes.. So you are right, new field shouldn't be considered breaking, however, in our current "Keyring API SDK", we are exposing some functions to dispatch incoming requests into the right "handler" using handleKeyringRequest:

• https://github.com/MetaMask/accounts/blob/main/packages/keyring-snap-sdk/src/rpc-handler.ts#L190-L223
• https://github.com/MetaMask/accounts/blob/main/packages/keyring-snap-sdk/src/rpc-handler.ts#L163-L166

And right now, those functions are "strict" and will superstruct.assert, so if we have any extra fields that are not part of those structs, assert will throw.

That's probably a bad design choice from us here 😅 but if we start to add new field, older Snaps that use those functions will end up with an error.

An improvement to this could be to allow additional fields and mention to the Snap developers to allow extra-fields as well (in case they are doing their own dispatching and do not rely on our SDK).

So that's why we consider this a breaking change in this context.