feat: add createBatchStreams for bulk stream creation - #391
Conversation
|
@Viky207 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Jaydbrown
left a comment
Jaydbrown
left a comment
There was a problem hiding this comment.
Thanks for this — the chunking/validation structure is solid, but there's a real correctness bug in the multi-chunk case that needs fixing before merge:
Sequence number collision across chunks: createBatchStreams() calls buildBatchContractCallTx() once per chunk, and each call independently does server.getAccount(caller) to fetch a fresh Account object:
// src/soroban.ts, buildBatchContractCallTx
const server = createRpcServer(rpcUrl);
let account;
try {
account = await server.getAccount(caller);
} catch (err) { ... }
...
const builder = new TransactionBuilder(account, { ... });TransactionBuilder.build() reads source.sequenceNumber() + 1 and only calls source.incrementSequenceNumber() on that same Account object afterward (see node_modules/@stellar/stellar-base/lib/transaction_builder.js:565,623). Since none of these transactions are submitted on-chain before the next chunk's getAccount() call runs, every chunk fetches the same last-confirmed on-chain sequence number and produces a transaction with the same sequence + 1.
Concretely: with MAX_BATCH_SIZE = 10, calling createBatchStreams() with 11+ configs (2 chunks) returns transactions that all carry the identical sequence number. A caller who signs and submits them in order will have the first one succeed and every subsequent one rejected on-chain with a bad-sequence error — silently defeating the entire point of a "bulk stream creation" API, since it doesn't actually work past 10 streams.
The existing test ('chunks configs into groups of MAX_BATCH_SIZE') only exercises exactly 10 configs → 1 chunk, so it can't catch this — there's no test with 11+ configs / 2+ chunks asserting the returned transactions have distinct, incrementing sequence numbers.
Suggested fix: fetch the Account once in createBatchStreams() (or thread it through buildBatchContractCallTx as an optional parameter instead of having that function call getAccount() itself), and reuse the same Account object across chunks so each new TransactionBuilder(account, ...) call sees the previous chunk's incremented sequence. Please also add a regression test with >10 configs asserting the returned transactions have strictly increasing sequence numbers.
Everything else here (validation-before-work, chunking logic, doc comments) looks good — happy to take another look once this is fixed.
…t-protocol#338, conduit-protocol#339) - Add StreamConfig type alias for CreateStreamParams in types/index.ts - Add buildBatchContractCallTx helper in soroban.ts for multi-operation transactions - Add createBatchStreams(configs) method to StreamsModule that: - Validates all configs before processing - Chunks configs into groups of MAX_BATCH_SIZE (10) to respect Soroban tx size limits - Builds a single transaction with multiple invokeHostFunction operations per chunk - Simulates and assembles each transaction, returning prepared Transaction[] - Returns empty array for empty input - Add comprehensive tests for createBatchStreams in streams-success.test.ts
The original approach here (from before this session's `main` gained a proper multi-transaction batch primitive) built one transaction per chunk of MAX_BATCH_SIZE configs, packing multiple invoke_host_function operations into it via repeated addOperation() calls -- Soroban permits exactly one invoke_host_function per transaction, so that would have been rejected by the network outright, on top of a real sequence-number collision bug (fetching a fresh Account per chunk, so every chunk after the first got the same last-confirmed sequence number and only the first chunk's transaction could ever land on-chain). Reimplemented as a thin StreamsModule method that builds each config's create_stream args exactly like create() does, then delegates the actual transaction construction to buildBatchTransactions() (batch-tx.ts, already merged) -- which builds one real transaction per operation and fetches the sender's sequence number once, incrementing it per index, so every transaction in the batch stays independently submittable regardless of batch size. Per-config client-side validation is isolated (one bad config doesn't block the rest), mirroring batchWithdraw()'s existing independent-per-item reporting. Adds a regression test asserting 15 configs (spanning what would have been 2 chunks under the old MAX_BATCH_SIZE=10 design) get 15 distinct, strictly increasing sequence numbers and that the source account is fetched exactly once.
add136d to
ae6a3ed
Compare
3 participants
Overview
This PR adds a batch stream creation API that allows callers to create many streams while minimizing the number of user signatures required.
Changes
StreamsModule.createBatchStreams(configs: StreamConfig[])invokeHostFunctionoperationsCloses #338
Closes #339