Skip to content

feat: add shareable State management lib - #288

Merged
Julink-eth merged 6 commits into
mainfrom
feat/WPN-1636-shared-state
Sep 8, 2026
Merged

feat: add shareable State management lib#288
Julink-eth merged 6 commits into
mainfrom
feat/WPN-1636-shared-state

Conversation

@Julink-eth

@Julink-eth Julink-eth commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Explanation

Add shared snap state helpers IStateManager, State, and InMemoryState to @metamask/snap-networks-utils.
Utils-only: snaps are not migrated in this PR. Each snap keeps its own defaultState / TStateValue and will switch in follow-up PRs.
State serializes through the existing shared serialize / deserialize, merges defaultState with safeMerge, and talks to snap_getState / snap_setState / snap_manageState.
Locking is Tron-style (path writes serialized, path reads parallel, blob updates exclusive) plus an admission gate so blob and path RPCs cannot overlap.

Solana, Tron, and Stellar had drifted onto three different lock schemes. Tron’s implementation seemed like the best: serialize setKey / setKeyWith, allow concurrent path reads, and make update() wait for path ops then take exclusive blob access.

That Tron scheme still has a race between the two steps of update():

1 - Wait until no path operations are in flight.
2 - Take the blob lock and call snap_manageState.

Tron’s lock waited for path ops to finish, then started the blob write. A get / setKey could slip in between those two steps and run at the same time as snap_manageState. Stellar’s fix (take the blob lock first) can deadlock.

Shared StateLock uses a short admission gate: update() holds it until the blob write is done, so no new path op can start in that window. Path reads still run in parallel; path writes stay serialized.

Known divergence (gates the Solana migration PR): Solana's State also takes an EventEmitter and runs a #migrateState purge (drops the legacy assets key) on onStart / onUpdate / onInstall. The shared State takes only config and has no event hook. The Solana migration will keep that purge outside the shared class (a thin subclass or a hook at the State instantiation site).

References

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've communicated my changes to consumers by updating changelogs for packages I've changed
  • I've introduced breaking changes in this PR and have prepared draft pull requests for clients and consumer packages to resolve them

@Julink-eth
Julink-eth requested review from a team as code owners September 8, 2026 09:59
@Julink-eth
Julink-eth deployed to default-branch September 8, 2026 09:59 — with GitHub Actions Active

readonly #config: StateConfig<TStateValue>;

constructor(config: StateConfig<TStateValue>) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

constructor(eventEmitter: EventEmitter, config: StateConfig<TStateValue>) {

May worth a look:

Solana's eventEmitter constructor arg has no equivalent — Solana's State requires an EventEmitter and subscribes onStart/onUpdate/onInstall to a #migrateState purge (State.ts:117-126 in solana-wallet-snap). The shared class takes only config. Non-blocking for this utils-only PR, but it's the one item that will gate the Solana migration PR — worth a one-line mention in the PR description so the divergence is on record.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added the known divergence in the PR description that will be taken into account when integrating the lib in Solana snap.

this.#pendingRegularStateUpdates === 0 &&
this.#releaseRegularStateUpdateMutex
) {
this.#releaseRegularStateUpdateMutex();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

May worth a check there:

Stale releaser never nulled — State.ts:69-74. After #releaseRegularStateUpdateMutex() fires, the field keeps the spent releaser. It's safe today only because the next path op's acquire overwrites the field before any pending===0 check can re-fire it. One-line hardening: set #releaseRegularStateUpdateMutex = null immediately after releasing. Prevents an Already released throw if the pairing is ever refactored.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch added the simple fix suggested to clear the releaseRegularStateUpdateMutex field.

@sonarqubecloud

sonarqubecloud Bot commented Sep 8, 2026

Copy link
Copy Markdown

@Battambang Battambang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

@Julink-eth
Julink-eth added this pull request to the merge queue Sep 8, 2026
Merged via the queue into main with commit 3e74956 Sep 8, 2026
95 checks passed
@Julink-eth
Julink-eth deleted the feat/WPN-1636-shared-state branch September 8, 2026 14:09
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.

2 participants