feat: add keyring state migration framework#505
Open
danroc wants to merge 39 commits into
Open
Conversation
danroc
force-pushed
the
dr/kering-migrations
branch
4 times, most recently
from
ad96b17 to
0679d1f
Compare
ccharly
reviewed
Apr 10, 2026
|
|
||
| - **`schema`**: Validates the **output** of a migration at runtime. | ||
| - **`inputSchema`**: (Optional) Validates the **input** before the `migrate` function is called. | ||
| - **`defineMigration<Output, Input>`**: Ensures the `migrate` function's return type matches the `schema` type at compile time. |
Contributor
There was a problem hiding this comment.
I would have expected to have input first, then output? But I guess it's because we want to have the type for the right version "first"?
Like:
const migrations = defineMigrations(
// version 1 -> HdStateV1
defineMigration<HdStateV1, HdStateV0>({
version: 1,
inputSchema: HdStateV0Schema,
schema: HdStateV1Schema,
migrate: (state) => ({
accountCount: state.numberOfAccounts,
mnemonic: state.mnemonic,
hdPath: state.hdPath,
}),
}),
...
Contributor
Author
There was a problem hiding this comment.
It's intentional since Output is the primary type. Input can be omitted and it will default to Json.
|
|
||
| class MyKeyring { | ||
| async deserialize(state: Json): Promise<void> { | ||
| const { data } = await applyMigrations(state, migrations); |
Contributor
There was a problem hiding this comment.
WDYT about renaming this just migrate?
Contributor
Author
There was a problem hiding this comment.
migrate is already a field of KeyringMigration, I didn't re-use it because I thought it could be confusing.
Comment on lines
+62
to
+70
| defineMigration<HdStateV1, HdStateV0>({ | ||
| version: 1, | ||
| inputSchema: HdStateV0Schema, | ||
| schema: HdStateV1Schema, | ||
| migrate: (state) => ({ | ||
| accountCount: state.numberOfAccounts, | ||
| mnemonic: state.mnemonic, | ||
| hdPath: state.hdPath, | ||
| }), |
Contributor
There was a problem hiding this comment.
Maybe we could auto-Infer input and output based on inputSchema and schema? 🤔
Contributor
Author
There was a problem hiding this comment.
It's already the case, they aren't necessary in this case, I can remove the types from the example.
Comment on lines
+33
to
+53
| const HdStateV0Schema = object({ | ||
| numberOfAccounts: number(), // legacy field name | ||
| mnemonic: array(number()), | ||
| hdPath: string(), | ||
| }); | ||
| type HdStateV0 = Infer<typeof HdStateV0Schema>; | ||
|
|
||
| const HdStateV1Schema = object({ | ||
| accountCount: number(), // renamed from numberOfAccounts | ||
| mnemonic: array(number()), | ||
| hdPath: string(), | ||
| }); | ||
| type HdStateV1 = Infer<typeof HdStateV1Schema>; | ||
|
|
||
| const HdStateV2Schema = object({ | ||
| accountCount: number(), | ||
| mnemonic: array(number()), | ||
| hdPath: string(), | ||
| createdAt: number(), // new field | ||
| }); | ||
| type HdStateV2 = Infer<typeof HdStateV2Schema>; |
Contributor
There was a problem hiding this comment.
Nit: But would be nice to enforce that all schemas are JSON-compatible too (not sure we have something ready for this).
Maybe we don't need to enforce this in the superstruct itself, but in the defineMigration* we could make sure T extends Json? Just to avoid allowing state that could not be serialized to JSON?
Contributor
Author
There was a problem hiding this comment.
It's already the case (at least in compile-time), Input and Output must extend Json.
danroc
force-pushed
the
dr/kering-migrations
branch
from
03a6c5b to
13c9dd8
Compare
|
This PR is marked as stale because it has been open for 60 days with no activity. Please remove the stale label or leave a comment, or it will be closed in 14 days. |
|
This PR was closed due to no follow-up activity in the last 14 days. Thank you for your contributions. |
Add a versioned migration framework that allows keyrings to evolve their
serialized state format over time. Migrations run during deserialize()
and wrap state in a { version, data } envelope.
- KeyringMigration type with generic Output extends Json
- defineMigration() helper for compile-time type binding between
migrate and superstruct schema
- applyMigrations() runner with sequential version validation
- isVersionedState() type guard and getLatestVersion() helper
- Developer documentation with integration examples
- Add jest types to tsconfig.packages.json for IDE support
Replace manual type checks in isVersionedState with a VersionedStateStruct schema using superstruct. Derive VersionedState type from the struct. Use JsonStruct from @metamask/utils for the data field.
…raint - applyMigrations now returns MigrationResult with a migrated boolean so callers can detect when migrations ran and schedule a persist - defineMigration gains an optional inputSchema (Struct<Input>) that validates state before migrate is called; the Input type parameter removes the need for manual casts inside migrate - migrations.md updated: inputSchema example, migrated usage in deserialize, output-as-input validation chain note, and a new Idempotent migrations constraint
defineMigrations uses rest parameters to infer a tuple type, which lets
applyMigrations resolve data as the last migration's output without any
cast or as const:
const migrations = defineMigrations(
defineMigration<V1, V0>({ version: 1, ... }),
defineMigration<V2, V1>({ version: 2, ... }),
);
const { data } = await applyMigrations(state, migrations); // data: V2
Falls back to Json when the array is typed as KeyringMigration[].
…emas When both `schema` and `inputSchema` are provided to `defineMigration`, TypeScript infers `Output` and `Input` automatically. Remove the explicit generic arguments and redundant `migrate` return type annotations in those cases across docs, tests, and JSDoc examples. The `@ts-expect-error` test for intentionally invalid output retains its explicit `<OutputState>` type param since the compile-time check only fires when TypeScript knows the expected output type.
…rn for migrations
Replace optional validate with an always-present method that is a no-op when no schema is provided, and update tests to assert behavior rather than presence.
- Remove broken {@link defineMigrations} reference from JSDoc
- Add test: version-0 versioned state triggers all migrations
- Add test: default JsonStruct validation fires without inputSchema
- Fix serialize() example to use VersionedState<HdStateV2> return type
- Add `as const` guidance to Best Practices in migrations.md
danroc
force-pushed
the
dr/kering-migrations
branch
from
13c9dd8 to
28a63ce
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
@metamask/keyring-sdkfor evolving keyring serialized statedeserialize()and wrap state in a{ version, data }envelopedefineMigration()helper provides compile-time type binding betweenmigrateand superstructschematsconfig.packages.jsonfor IDE supportTest plan
yarn workspace @metamask/keyring-sdk testpasses (85 tests, 100% coverage)yarn workspace @metamask/keyring-sdk buildsucceedsNote
Medium Risk
Framework targets vault/keyring serialized state; incorrect migrations or adoption could corrupt wallets, though this PR only adds the SDK and does not wire it into keyrings yet.
Overview
Adds a versioned migration framework to
@metamask/keyring-sdkso keyrings can evolve serialized vault state duringdeserialize().Legacy unversioned payloads are treated as version 0; migrated state is wrapped in
{ version, data }.defineMigrationtiesmigrateto optional superstructschema/inputSchema(compile-time + runtime validation).applyMigrationsruns only pending steps (sync/async), validates each step, returnsmigratedfor re-persist, and infers the finaldatatype when migrations are declaredas const. HelpersisVersionedState,getLatestVersion, and typesVersionedState/MigrationResultare exported from the package entry.Includes
docs/migrations.md, changelog note, and broad Jest coverage for envelope detection, ordering, and error cases.Reviewed by Cursor Bugbot for commit 28a63ce. Bugbot is set up for automated code reviews on this repo. Configure here.