0xMiden / 0xMiden/web-sdk

batch waitForConfirmation is broken two ways: wrong target height, and it calls a method that does not exist

Aperta
#314 2 commenti 0 reazioni 0 assegnatari Vedi su GitHub
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

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.