Repository navigation
fix: overlap USDT send preflight reads - #177
Conversation
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Runs the nonce, EIP-7702 authorization and balance reads in send_with_deadline concurrently with tokio::try_join!, under the existing operation lock, and adds a barrier test that needs all three reads to be in flight at once. Reviewed 2285d83, full tier, reasoned from the code and CI (not built locally).
What I checked, and 3 candidates I ruled out
Read in full: send_with_deadline and the read helpers in src/modules/usdt/wallet.rs, Rpc::call_with_deadline and reserve_request in src/modules/usdt/rpc.rs, the mock RPC and the new test in src/modules/usdt/tests.rs
CI: usdt-tests, SwiftPM and Cargo/Android manifests green at this head. iOS and Android Bindgen were still pending, and the PR does not change any public type or export.
Ruled out
- Rate limiter under concurrency:
reserve_requestholds a sharedtokio::sync::Mutexonly long enough to claim a slot. The three reads (four requests, becauseauthorization()makes two) fit inside the 20-request burst, and a dropped future intry_join!only uses up a slot it had already claimed. - Error precedence: before this change, a stale nonce returned
QuoteExpiredbefore the balance was read. Now, if the nonce is stale and the balance is also short, the call can returnInsufficientBalanceinstead. Both errors stop the send before signing, and neither is retried, so no money moves either way. Every check that ran before still runs beforederive_owner_key. - Test validity: the barrier counts only
eth_callandeth_getCode. Before the barrier is armed,sendsends onlyeth_chainId, so the three requests that reach it are the getNonce call, the getCode call and the balanceOf call. With the old sequential code, the first read waits at the barrier and the 10 s timeout fails the test, which matches what the PR says.
Merge confidence: 5/5, usdt-tests green and the overlap is covered by send_preflight_reads_overlap_before_signing.
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: None.
QA for 2285d83:
Test 1: Preflight overlap ✅ The barrier regression passed and submitted once.
Checked with the repository regression. Keeping the new test and substituting the base implementation made it time out at the barrier, as expected.
Test 2: Send safeguards and recovery ✅ The USDT module checks passed.
Checked with cargo test --locked --lib modules::usdt, including nonce, authorization, balance, expiry, backup and recovery checks.
Test 3: Deployed contracts ✅ Fee collection and atomic bridge rollback passed.
Ran the repository's ignored Arbitrum-fork test, including successful bridging and helper-allowance revocation.
Test 4: Swift simulator send ✅ A 1 USDT payment confirmed; repeating its quote ID caused no second debit.
An iOS 18.5 test harness linked to the library built at this head checked the fee cap, principal-plus-fee balance debit, and identical operation on repeat. This uses the repository fork provider and a fixture backup callback.
simulator2x.2x.mp4
Full review of fab768db..2285d83, covering both changed files. Native Bitkit UI journeys and end-to-end payment latency were not tested.
The earlier code review also found no defects at this head.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
USDT sends currently wait for nonce, account authorization and balance reads in sequence. Run these independent reads concurrently under the existing operation lock to reduce pre-signing latency without caching their results or adding RPC calls.
Description
The final nonce, authorization, balance, bridge, gas and expiry checks remain in place. Signing, durable recording, acknowledged backup and broadcast keep their existing order. No public API, binding or version change.
QA Notes
fab768db).cargo fmt --checkpasses.cargo clippyhas the same 40 warnings as unchanged master, with no added warnings.This removes sequential network waits; it does not establish an end-to-end payment or merchant-unlock latency guarantee.