batch waitForConfirmation is broken two ways: wrong target height, and it calls a method that does not exist
- Lingua principale
- TypeScript
- Stelle
- 1
- Fork
- 21
- Merge medio
- 12h 14m
- PR unite (30g)
- 41
Descrizione
`BatchOptions.waitForConfirmation` does not do what it says, for two independent reasons. Both are pre-existing; surfacing them here because #313 corrects the documented meaning of the number the option polls against, which is what makes the first one visible.
### 1. It waits on a height the client has usually already reached
`#waitForBlock` polls until `getSyncHeight() >= blockNumber`, where `blockNumber` is what `submitNewTransactionBatch` returned. That value is the node's chain tip **as of submission** — upstream `BatchBuilder::submit` says so explicitly:
> Returns the node's chain tip at submission (not the block the batch is committed). The submitted transactions are recorded locally as pending; call `sync_state` to get the block they commit in.
The batch commits at some height strictly greater than that. So the loop can exit after a single sync, before the batch has landed, and report confirmation for a block that already existed.
The strongest evidence is in our own test suite: `test/batch.browser.test.ts` does not trust the returned number at all. It polls the sender's nonce for up to 120s to decide the batch landed.
Note that `js/__tests__/resources/transactions.test.js` has a case named "polls sync height until block lands" which feeds heights `[99, 99, 100]` against a returned `100`. It encodes the wrong premise and passes only because the mock is free to lag — it should be updated with whatever fix lands here.
### 2. It calls a method that does not exist
`#waitForBlock` calls `syncStateWithTimeout`, which is not a method on the client. So `waitForConfirmation: true` on a batch throws before the polling logic above ever matters.
### Suggested direction
The V1 batch API returns only a block number, and no batch or per-transaction identifier, so there is nothing to poll per-transaction the way singular `waitFor` does. Options, roughly in order of preference:
1. Have the Rust side return something identifying — a batch id, or the transaction ids it built — and poll on that.
2. Wait for `height > blockNumber` plus a store-side check that the account state actually advanced.
3. If neither is feasible under V1, remove the option rather than ship one that cannot work, and document nonce / `transactions.list()` polling as the way to observe a batch.
Whichever way it goes, `BatchOptions.waitForConfirmation` and `#waitForBlock` need docstrings that match. #313 has already reworded them to describe the current (weak) guarantee rather than the intended one.
Guida per i contributori
Apri la guida per i contributori
Direzione di ricerca
The issue is in the batch confirmation logic. Look at the BatchOptions.waitForConfirmation implementation and the #waitForBlock method. The test file test/batch.browser.test.ts shows how the nonce is polled as a workaround. Also examine js/__tests__/resources/transactions.test.js for the mock test. The fix involves understanding the V1 batch API's return values and coordinating between the Rust backend and TypeScript SDK to implement correct polling or remove the broken option.
Scritto dal modello di indicizzazione a partire dal testo della issue.
Valutazione
- Stack tecnologico
- rust, typescript
- Ambito
- backend-api-design, testing-qa
- Tipo di issue
- Bug
- Difficoltà
- 4/5
- Tempo stimato
- 3-5 giorni
- Stato di attività
- Attiva
- Chiarezza
- Specificata chiaramente
- Idoneità per principianti
- 45/100