fix: report an expired transaction as expired - #10
Draft
ganymedio wants to merge 2 commits into
Draft
Conversation
waitForTransaction retries a 404, so once a node drops an expired transaction from its mempool the wait keeps polling a hash that no longer exists and ends with "timed out in pending state after 20 seconds". That names the wrong cause: it points at timeoutSecs, when no amount of waiting can help, and it is indistinguishable from a transaction that is merely slow. The wait now compares the transaction's expiration_timestamp_secs against the ledger timestamp before deciding which failure to report, and says the transaction expired and was dropped, naming the two ways to widen the window. Chain time rather than local time, because the node decides what has expired and a skewed client clock would otherwise misreport a transaction that is still valid. The extra request runs only on the failure path, and a failure to reach the node falls back to the previous message rather than replacing one unclear error with another. The comparison is a pure exported function so it can be unit tested without a network, matching how the rest of tests/unit is written.
pnpm _fmt --check is a CI job and the new file was not formatted.
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.
A transaction that expires is reported as a poll timeout, which names the wrong cause and sends you to the wrong lever.
What happens today
waitForTransactiontreats a 404 as retryable, inhandleAPIError. That is correct while a transaction is merely not visible yet, but it is also what happens after a node garbage-collects an expired transaction: the hash stops resolving. The loop keeps polling something that no longer exists untiltimeoutSecsruns out, then throws:The natural reading is "poll for longer", so the reader reaches for
timeoutSecs. No amount of waiting can help, because the transaction is gone and will never commit. Nothing in the loop comparesexpiration_timestamp_secsagainst ledger time, so a slow transaction and a dropped one are indistinguishable.The defaults hide it further:
DEFAULT_TXN_TIMEOUT_SECandDEFAULT_TXN_EXP_SEC_FROM_NOWare both 20, so the wait gives up at almost exactly the moment the transaction expires.What this changes
Only the failure path. Before choosing which error to throw, the wait asks the node for the ledger timestamp and compares it against the expiry it already holds on
lastTxn, which is retained precisely for this kind of post-loop decision. If chain time is past the expiry:Otherwise the previous message is unchanged.
Three deliberate choices:
No behaviour changes: the same calls fail and succeed as before, and the same error type is thrown. Only the message differs, and only when the transaction actually expired.
Scope
This is deliberately not a change to
DEFAULT_TXN_EXP_SEC_FROM_NOW. The complaint that motivated it is that the failure is unexplained, and that is a diagnosis problem. Raising the default is a separate proposal with its own tradeoff, since sequence-number ordering means a submitted transaction that never commits blocks later transactions from the same account until it expires, so a longer default turns a short stall into a long one for every consumer.Also out of scope: neither
WaitForTransactionErrornorFailedTransactionErroris on the public surface today, so callers cannot branch on the type and have to read the message. A distinct error class would be the better interface, butsrc/errors/index.tsis the natural home andinternal/transaction.tsalready importsMovementApiErrorfrom it, so re-exporting there introduces a cycle. Worth doing separately.Verification
The comparison is extracted as
hasTransactionExpired, a pure function, so it is unit tested without a network. That matches howtests/unitis written, since nothing there mocks the client layer. Cases covered: expired, not expired, the exact expiry second, the unit conversion between seconds and microseconds, string, number and bigint inputs, and unparseable input.I have not run the suite. This checkout has no
node_modules, sopnpm installis needed first. Both changed files were syntax-checked with the TypeScript parser, which is not a substitute. Worth runningpnpm unit-testbefore this leaves draft.