Repository navigation
Conversation
…arden batch options The Sorokit#808 merge left main red (CI run Sorokit#509: 57 failing tests) and changed both test expectations and source behaviour. This restores the pre-Sorokit#808 public contracts while keeping the new behaviour that can be kept additively. - submitBatch: the submission overload is declared first, so a call with {horizonUrl, networkPassphrase} resolves to a Promise; planning options are disjoint and stay synchronous. - batch helpers: one stable return type per function (no return-type-only overloads); structured variants live under explicit new names. - EventIndex: has/add/deduplicate/eviction use a collision-safe canonical identity (length-framed parts, structural payloads) instead of a compact digest. - decodeContractEvent: pre-Sorokit#808 generic contract restored, names are case-insensitive, aliases accepted; ABI decoding stays on its explicit entry point. - timeLocks: legacy StrKey validation and error codes restored; the sequence-aware overload stays explicit. - submitBatch options: atomic fails closed instead of claiming atomicity; continueOnError and timeout are enforced. - tests: REV9 regression suite plus compile-time contract assertions. No assertion was weakened, no test removed, no skips introduced.
Collaborator
|
@kiyeps Close this issue the right way with "closes #number" |
…ubmissions - submitBatch now rejects mode: "parallel" with continueOnError: false (VALIDATION) instead of starting every submission up front and implying that remaining entries could be skipped. - submitBatch honours the per-attempt timeout by aborting the in-flight request via the AbortSignal that submitTransaction already accepts, and reports one deterministic message: the outcome is unknown, so the signed XDR must not be auto-retried - reconcile by transaction hash. - tests cover both semantics; the no-timeout path keeps the legacy call arity so existing assertions remain valid.
Collaborator
|
@kiyeps dont you undertsand, youre meant to Close the pr with "closes #number" edit your pr description with the close #number |
An integration probe against a local Horizon-compatible endpoint showed an already-aborted AbortSignal did not prevent submissions: retries still issued HTTP requests (3/3 attempts went out), while a mid-flight abort did stop them. - submitTransactionImpl short-circuits inside the retry closure when options.signal is already aborted, so no request is sent and a follow-up retry cannot re-broadcast a signed envelope. - Regression test spies on Horizon.Server.prototype.submitTransaction: pre-aborted -> no submit attempt; mid-flight abort -> attempts === 1.
Contributor
Author
|
Thanks for the guidance. I checked the original issues related to the #808 changes: #706, #723, #724, and #725 are already closed by #808, and #814 is a corrective compatibility follow-up rather than completion of an open issue. |
Collaborator
|
message me on telegram @bamford_jnr |
This branch has not been deployed
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.
Context
main(0a7025e2) is currently red: CI run #509 fails the Test job with 57 failing tests, while Lint/Build/Typecheck/Security Audit pass. The failures come from the #808 merge, which changed test expectations and source behaviour for several public APIs without migrating the pre-existing suites.This PR restores the pre-#808 public contracts while keeping the #808 behaviour that can be kept additively. No assertion was weakened, no test was removed, and no
.skip/.onlywas introduced.What changed
horizonUrl+networkPassphrase) is declared first, so such a call resolves toPromise<SorokitResult<...>>; planning options are disjoint (horizonUrl?: never) and stay synchronous. Asserted at compile time insrc/tests/submitBatch.typecheck.ts.SorokitResult-returning helpers keep their pre-existing shape; the structured variants moved to explicit names (buildRollbackInstructions,isBatchAtomicComplete,recommendRetryStrategy).has/add/deduplicate/eviction key on a collision-safe canonical identity (length-framed identifier parts + structural payload serialization) instead of a 32-bit digest, so distinct nested payloads and ambiguous identifier boundaries can no longer be conflated.hashStringkeeps its publicnumberreturn type and its docs are corrected (32-bit, not 53-bit).DecodedContractEvent<TType, TData>,ContractEventDecoder<T>, requiredraw) is restored, names are case-insensitive, aliases (paircreated,created,swapped,swap_executed) are accepted, and the trailing-topic fallback is back. ABI decoding stays on the explicitdecodeContractEventWithAbientry point.StrKey.isValidEd25519PublicKey→INVALID_ADDRESS, bounds checks) and error codes; the sequence-aware overload is explicit and is the only one returning a Promise.atomic: truefails closed in submission mode instead of returning a result that claims atomicity;continueOnError: falsestops after the first failure and marks the restskipped;timeoutis enforced per attempt.Test files touched (all pure API renames)
contractEvents.test.ts(28 lines),contractEvents.performance.test.ts(2),batchSubmitter.test.ts(26),timeLocks.test.ts(40) — renames required by the restored/bifurcated contracts (size()→size,validateTimeBounds→isTimeBoundsValid,getTimeLockedTransactionStatus→getTimeLockStatus, helper renames). Assertion count is unchanged: the diff pairs 28 removedexpect(lines with 28 added, and 3 removedit/describewith 3 added. New files:src/tests/api-compat-rev8.test.ts,src/tests/api-compat-rev9.test.ts(22 tests),src/tests/submitBatch.typecheck.ts(compile-time contract checks, no runtime assertions).Local verification (raw results)
npm run typecheck: exit 0 (includes the compile-time contract file)npm run build: exit 0npm test(full, coverage thresholds active): 181 files passed | 3 skipped (184); 3502 passed | 80 skipped (3582); exit 0; coverage81.27 / 79.87 / 77.55 / 81.27git diff --check: exit 0; no workflow/package/tsconfig/coverage-config changes; no deleted files; no secretsLocal results are not a substitute for CI — this PR's own run should be the source of truth. Happy to adapt anything the maintainers prefer, including dropping
src/tests/submitBatch.typecheck.tsif a non-test compile-time file is not wanted in the tree.