feat: add SDK account sequence and concurrent submission safety system (#277) - #325
Merged
Merged
Conversation
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
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.
Closes #277
Summary
Adds the sequence layer the SDK did not have, and gives stale-sequence failures
their own type so they can be told apart from failures that need different
handling.
The gap
Three build paths loaded the source account independently —
sendXLM(
src/payments/index.ts:55),sendAsset(:340) andfetchNetworkState(
src/transactions/offline-preparation.ts:316) — with no cache, no freshnessmarker and no invalidation. Concurrent intents on one account read the same
sequence and build conflicting envelopes.
Offline preparation makes that structural: it deliberately splits fetching state
from building, and
NetworkStatestoredsequencewith nothing recording whenit was read.
Root cause of the unclear failure
classifySubmitError(src/errors/index.ts) collapsed everyresult_codes.transactionintoTX_FAILEDwithretryable: false. Sotx_bad_seq— recoverable by rebuilding — was indistinguishable fromtx_insufficient_feeortx_bad_auth, which are not. The real code survivedonly inside the message string, which is the pattern the error standard exists
to remove. No sequence code existed in the registry.
Changes
src/account/sequence.ts(new) —SequenceProviderwithget,refresh,invalidate,peek,loadAccountandwithSequence; snapshotscarry
fetchedAt; plusvalidateSequenceValueandisSequenceStale.src/errors/codes.ts—TX_BAD_SEQUENCEadded to the registry.src/errors/index.ts—tx_bad_seqclassified before the genericcollapse; every other result code keeps its existing classification. Adds
requiresRebuild().src/transactions/offline-preparation.ts—NetworkState.fetchedAt;manual sequences validated at preparation time;
isPreparedSequenceStale();opt-in freshness enforcement in
buildUnsignedTransaction.docs/sequence-safety.md(new) — alongsideretry-policy.mdandidempotency.md.Rebuild is not retry
retryable,isRetryableError()andisSafeToRetry()all mean "this samesigned envelope may be sent again". For
tx_bad_seqthat is false: the sequencein the envelope is spent, so resubmitting can never succeed. Marking it retryable
would send consumers into a resubmit loop.
TX_BAD_SEQUENCEis thereforeretryable: false, and recovery is expressedthrough the separate
requiresRebuild(error)helper.SubmissionOutcomeisunchanged, so no consumer's exhaustive switch breaks. A test asserts
isSafeToRetrystill returns false for this case.Scope of the concurrency guarantee
withSequenceserializes intents for one account using an in-memory promisechain — a per-process guarantee. It does not coordinate across workers,
containers or machines, and the documentation says so explicitly rather than
implying broader safety.
Sequences are deliberately not pre-allocated. Handing out
sequence + 1,sequence + 2, … would avoid serializing, but one failed submission leaves a gapand every later transaction becomes permanently invalid. Re-reading after each
use is slower and correct.
Backwards compatibility
buildUnsignedTransaction(prepared)behaves exactly as before. Freshnessenforcement requires
enforceSequenceFreshness: true, covered by a test.NetworkState.fetchedAtis optional and absent for manual sequences.tx_bad_seqnow reportsTX_BAD_SEQUENCEinstead ofTX_FAILED. Consumers branching onTX_FAILEDfor this case should switch torequiresRebuild(); the message text is unchanged.MISSING_SEQUENCE, anunregistered string in
updateWithNetworkState, also moves toTX_BAD_SEQUENCE; it had no test or documentation references.Tests
31 new tests in
tests/sequence-safety.test.ts: caching and freshness;refresh/invalidateforcing re-reads;maxAgeMs: 0disabling the cache;concurrent reads sharing a sequence without serialization (documenting the
hazard) and diverging with it; serialized ordering; a failing intent not
cascading into queued ones; different accounts not blocking each other;
tx_bad_seqclassification with every other result code staying onTX_FAILED;isSafeToRetrystaying false; and stale-snapshot detection with enforcement onand off.
This also adds the first test coverage for
offline-preparation.ts, which hadno test file.
Verification
lint,check:circular(41 modules, no cycles) andbuildall pass.Full suite: 46 failed · 737 passed · 1 skipped. The 46 failures are
pre-existing on the base commit — verified by running the suite against a clean
checkout of
ebd75d1, which gives 46 failed · 706 passed · 1 skipped: samefailures in the same five files. This branch adds exactly the 31 new passing
tests and introduces no regressions.