Skip to content

SIP-34: Introduce endowment:keyring.version #172

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.

Sign up for GitHub

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

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

SIP-34: Introduce endowment:keyring.version #172

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
160d075
Introduce `endowment:keyring.version`
ccharly Apr 9, 2025
922ce85
refactor: remove mention of submitRequest example + wording
ccharly Apr 10, 2025
2d781da
chore: typo
ccharly Apr 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions SIPS/sip-34.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
---
sip: 34
title: endowment:keyring.version
status: Draft
discussions-to: https://github.com/MetaMask/SIPs/discussions/172
author: Charly Chevalier (@ccharly), Daniel Rocha (@danroc)
created: 2025-04-07
---

## Abstract

This proposal introduces a new field, `version`, within the existing configuration object used by the `endowment:keyring`.
Copy link
Member

@Mrtenz Mrtenz Apr 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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).

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'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!

This field will allow the versioning of the keyring API, providing the flexibility to define the current API version being used.
By declaring the version in the configuration, we can implement conditional behavior in the API based on the selected version.
This will allow us to modify or add new functionality in the API without breaking backward compatibility.

## Motivation

The current `endowment:keyring` configuration does not support versioning, which limits our ability to introduce breaking changes or new functionality that may require changes in behavior.
Adding a `version` field would allow us to version the API and define distinct behaviors based on the declared version, limiting the need for breaking changes when new features are added.

Existing implementations relying on previous versions will continue to work without modification.

## Specification

### Language

The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and "OPTIONAL" written in uppercase in this document are to be interpreted as described in [RFC 2119](https://www.ietf.org/rfc/rfc2119.txt).

### Proposed implementation

Add an OPTIONAL `version` field to the `endowment:keyring` configuration object.
This field will be a `number` and will defaults to `1` if not provided to maintain compatibility with existing implementations.

```json
{
"endowment:keyring": {
"version": 2,
Copy link
Contributor Author

@ccharly ccharly Apr 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer using a semVer

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@Mrtenz Mrtenz Apr 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 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.

Copy link
Member

@Mrtenz Mrtenz Apr 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

"allowedOrigins": [
...
]
}
}
```

On the keyring API implementation side, we will modify our API methods to conditionally behave based on the `version`:
- When `version` is set to `1`, the API will behave as it currently does.
- When `version` is set to `2`, the API will introduce new incompatible behaviors/features with version `1`.
- Future versions will allow additional breaking features, with conditional logic to ensure backward compatibility.

This new implementation MUST ensure backward compatibility with older Snaps.
- Snaps using the existing `endowment:keyring` configuration object will continue to function without modification.
- New functionalities, will only be enabled for Snaps that declare `"version": 2` (or above).
- As more versions are introduced, **each versions will have its own set of features or modifications, ensuring that older versions continue to operate as expected**.

## Copyright

Copyright and related rights waived via [CC0](../LICENSE).