refactor(sync): split sync_state into sync_chain and sync_note_transport for granularity#30
Open
WiktorStarczewski wants to merge 11 commits into
Open
refactor(sync): split sync_state into sync_chain and sync_note_transport for granularity#30WiktorStarczewski wants to merge 11 commits into
WiktorStarczewski wants to merge 11 commits into
Conversation
…ort (3-way merged with CONFLICTS) Migrated from 0xMiden/miden-client#2091 (author: JereSalo) as part of the web-sdk split. Original PR: 0xMiden/miden-client#2091 The patch contains 3-way merge conflicts; resolution needed before merge.
The 3-way merge in the prior commit left <<<<<<< ours / ======= / >>>>>>> theirs markers around the legacy acquireSyncLock block. Drop the legacy implementation (the simpler withSyncLock callback design already supersedes it) and remove the markers so the file parses.
Replaces the previous "sync after failed sync works correctly" test, which only ran two successful syncs back-to-back and never actually exercised post-failure recovery. The new test calls withSyncLock directly with a rejecting fn, then a second time with the same key, and asserts the second fn actually executes — proving the in-flight entry was cleared rather than coalesced onto the rejected promise. Exposes withSyncLock from the SDK index alongside the existing internal exports used by integration tests.
Resolves the divergence with current next:
- crates/web-client/test/sync_lock.test.ts: take theirs — next added
a new 'Sync Lock Timeout Race Condition' test that wasn't on this
branch.
- Cargo.toml: temporarily pin miden-client (and miden-client-sqlite-
store) at upstream PR branch 'jere/syncstate-ntl-decouple-1930' so
this PR's sync.rs (which calls Client::sync_chain() and
Client::sync_note_transport(), added by miden-client#2091) can
compile against the new methods.
KNOWN LIMITATION: the upstream branch is currently rebased onto a point
of next that includes #2100 (peaks-table removal), and web-sdk's
idxdb-store has not yet migrated its Store impl to the new API. So
`cargo check --workspace` fails with 9 errors in idxdb-store. Two
clean paths forward:
1. Rebase miden-client#2091 onto miden-client@dab6cf7b (the snapshot
web-sdk currently pins) — same workflow used to unblock #1835.
2. Migrate web-sdk's idxdb-store to the new Store trait — bigger
workstream, blocks this and any other PR retargeting at a post-
#2100 miden-client snapshot.
Until one of those happens, this PR's Rust-side CI will stay red on
miden-idxdb-store. JS / type / lint checks should be unaffected.
Direct retarget at miden-client#2091's tip (jere/syncstate-ntl-decouple-1930)
fails with 9 build errors in idxdb-store: jere's branch was rebased onto
a point of next that includes #2100 (peaks-table removal), and web-sdk's
idxdb-store still implements the pre-#2100 Store API (get_partial_blockchain_peaks_by_block_num,
StateSyncUpdate.block_updates, etc.).
Created a sibling branch (wiktor/syncstate-pre-2100-rebase, 48a79f208 on
miden-client) by taking jere's pre-merge tip (e212878cb — the parent of
its merge with current next) and merging miden-client@dab6cf7b into it.
That snapshot:
- Carries jere's sync_chain / sync_note_transport additions on Client
(the API this PR's web-sdk-side code calls).
- Has the dab6cf7b-era Store trait that web-sdk's idxdb-store
implements verbatim (untrack_and_prune_irrelevant_blocks, peaks
intact, StateSyncUpdate fields unchanged).
cargo check --workspace --target wasm32-unknown-unknown is clean against
this dep snapshot.
Before merging this PR:
1. Land + release miden-client#2091 (and either rebase the migration
onto current next or land it alongside a web-sdk idxdb-store
migration to the post-#2100 Store API),
2. Revert these two lines back to `branch = "next"`.
WiktorStarczewski
added
the
no changelog
The migration of miden-client#2091 rewrote js/syncLock.js to expose a
single `withSyncLock(dbId, methodId, fn)` instead of the prior
acquireSyncLock / releaseSyncLock / releaseSyncLockWithError shape, and
removed the timeout-aware `syncStateWithTimeout` WASM method in favor
of plain `syncState` (the timeout was rejecting the caller's promise
without cancelling the underlying sync — see PR description). The
js/__tests__/ unit tests were not updated alongside, so vitest dropped
to 298 → 276 passes (15 syncLock.test.js + 7 transactions.test.js
failed) and the run reported coverage gate red.
- js/__tests__/syncLock.test.js: replaced 15 acquire/release-shaped
tests with 12 tests against withSyncLock that cover the same
behaviors (in-process fallback resolution, fn rejection
propagation, same-key coalescing, error coalescing, in-flight slot
cleanup on resolve and on reject, distinct-key non-coalescing,
multi-waiter shared result) plus the existing 4 hasWebLocks tests.
Adds a separate describe for the Web-Locks branch with a mocked
navigator.locks.request, asserting the lock name shape (`miden-sync-<dbId>`)
and that fn rejection propagates through the lock. Web Locks
semantics across tabs are still covered E2E by
crates/web-client/test/sync_lock.test.ts under Playwright.
- js/__tests__/resources/transactions.test.js: rename every
`inner.syncStateWithTimeout` reference (mock setup + spy
assertions) to `inner.syncState`. The new TransactionsResource
waitFor() uses syncState() in its polling loop; the assertions
that "waitForConfirmation triggers a sync" still hold, just on
the renamed method.
vitest now runs 298/298 with 99.71% coverage.
…te() Same migration pattern as the unit tests fixed in 40d95a6, but in the Playwright integration suite (crates/web-client/test/sync_lock.test.ts). miden-client#2091's web-sdk-side migration removed the timeout-bearing syncStateWithTimeout method (per PR description: 'Removed Timeout Logic — the timeout only rejected the caller's promise, but it never actually cancelled the underlying sync'), so client.syncStateWithTimeout is now undefined at runtime — playwright reports 'TypeError: client.syncStateWithTimeout is not a function' across ci-shard-2-sync-and-state. Drop the timeout argument (no equivalent in the new API; coalescing handles concurrency) and call client.syncState() — the same method the new client.sync() wrapper delegates to internally.
This was referenced Apr 30, 2026
Collaborator
Author
|
@JereSalo can you pull next into your client branch? that should fix the tests. i've adapted all the other PRs, but this is a fork so dont want to force push. |
Collaborator
Sure, I merged next into the miden-client |
- waitFor() polling uses syncChain instead of syncState so transaction confirmation stays alive when the note transport endpoint is unavailable; chain-only sync is sufficient for confirmation. - MockWebClient gains syncChain and syncNoteTransport overrides that serialize the main-thread mock chain + note transport node to the worker before delegating, mirroring the existing syncState override. Adds matching SYNC_CHAIN_MOCK and SYNC_NOTE_TRANSPORT_MOCK worker actions and MethodName entries. - syncLock no-Web-Locks fallback now serializes cross-method calls per-dbId via an in-process promise tail. The previous fallback relied on a non-existent WASM-side mutex; in the browser, AsyncCell is a plain RefCell, so concurrent cross-method borrows would hit the recursive-use aliasing error. Same-method coalescing is unchanged. Adds two unit tests pinning the new fallback contract: cross-method ordering on the same dbId and recovery after a prior call rejects. Updates the transactions resource unit tests for the syncChain switch.
The browser-side WebClient proxy in index.js wraps the raw napi-exported syncChainImpl / syncNoteTransportImpl with same-named JS methods that go through the sync lock. The napi-built client exposes only the *Impl forms, so test adapters and napi consumers need to alias the public names to the Impl methods — same pattern as the existing syncState → syncStateImpl alias. Without these, MidenClient.syncChain() / .syncNoteTransport() and the new TransactionsResource.waitFor() polling path throw TypeError on the napi binding (the underlying property is undefined). Updates node-adapter.ts, test-helpers.ts, and test-setup.ts.
A recent clippy version flags useless_borrows_in_formatting on &account_id.to_string() inside format!(); the reference is unnecessary because format! takes its arguments by value (via Display). Pre-existing line, surfaced now by tightened lint rules.
Collaborator
Author
Thanks - the merge would fix the CI on this pr, and I did it automatically for all the miden-client PRs I migrated, but yours was a fork so I couldn't push (would have to do the branch change dance). |
Collaborator
Great! It seems to work now. Thanks. My PR wasn't a fork though 😛 |
Collaborator
|
Quick note on merge order: this PR depends on 0xMiden/miden-client#2091, which already landed in miden-client's That bump will also pull in a few other changes from miden-client that are likely breaking on our side, so it'll probably want to be its own PR rather than a quick In the meantime this branch points to a miden-client snapshot just to validate the migration on its own. Once |
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.
Migrated from miden-client#2091 (author: @JereSalo) as part of the web-sdk split (#1992 / #2135).
Summary (per the original PR)
WebClient side-effects:
The miden-client-side changes (the bulk of the original PR) stay on 0xMiden/miden-client#2091.
Note
Unlike #23/#25/#26/#27/#31, this PR can't use the
Client PR:auto-patch marker. 0xMiden/miden-client#2091's branch was rebased onto a point ofnextthat includes #2100 (peaks-table removal), and web-sdk's idxdb-store still implements the pre-#2100Storetrait — auto-patching at 0xMiden/miden-client#2091's head leaves idxdb-store with 9 compile errors. As a workaround,Cargo.tomlhere is manually pinned at a sibling miden-client branch (wiktor/syncstate-pre-2100-rebase) that snapshots 0xMiden/miden-client#2091's pre-merge tip rebased onto miden-client@dab6cf7b (the snapshot web-sdk currently pins). When 0xMiden/miden-client#2091 is rebased onto currentnextupstream, OR web-sdk's idxdb-store migrates to the post-#2100Storetrait, this PR can switch to the auto-patch marker like the others.Closes #92