feat: managed write path (TxPipeline wired in) with stuck-tx gas-bump - #34
Conversation
The SDK had a TxPipeline (nonce/gas prepare, stuck detection, prepareBump)
but context writes bypassed it via wallet.sendTransaction, so the
escalation-on-resend loop -- the fix for a liquidator whose tx is stuck at
too-low gas -- was unreachable through the SDK. This wires it in.
- ChainClient.sendManaged(to, data, value, SendParams{nonce, gas_limit,
max_fee, max_priority}): a send with fully explicit nonce+gas (no
nonce/gas RPC), so a TxPipeline owns them. EthChainClient forwards to
wallet.sendTransaction; the mock records the params.
- context managed-write API (over a heap-owned nonce_mgr + gas_cache +
TxPipeline): enableManagedWrites(starting_nonce, gas_cfg, cfg),
refreshBaseFee(base_fee, now_ms), sendManaged(request, now_ms) ->
{hash,nonce}, stuckWrites(now_ms), resendBumped(request, hash, mult) ->
new hash at the SAME nonce with fees scaled, and confirmWrite/failWrite.
All timing via explicit now_ms (no OS clock in lib code). A send failure
releases the nonce so it is not skipped.
resendBumped leaves the original as the in-flight tracker (avoids
double-tracking the reused nonce); the caller confirms/fails the original
when either version mines.
Tests exercise the full flow via the mock: GasPriceUnavailable before a
base fee, nonce assignment (5), explicit gas + non-zero critical-urgency
fees, stuck-timeout detection, a 2x bump resend at the SAME nonce with
exactly-doubled fees, and confirm-then-bump -> TxNotInFlight. Plus the
not-enabled / already-enabled guards.
zig build test 504, contract-test 103 (Debug + ReleaseFast).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds managed transaction submission with explicit nonces and EIP-1559 fees, context-level nonce/gas pipeline management, stuck-write replacement, lifecycle tracking, mock support, and contract tests. ChangesManaged write pipeline
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant PerpCityContext
participant TxPipeline
participant ChainClient
participant Wallet
Caller->>PerpCityContext: sendManaged(request)
PerpCityContext->>TxPipeline: prepare nonce and fees
PerpCityContext->>ChainClient: sendManaged(transaction, SendParams)
ChainClient->>Wallet: sendTransaction(explicit nonce and EIP-1559 fees)
Wallet-->>PerpCityContext: transaction hash
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/context.zig (1)
871-878: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
confirmWrite/failWritesilently no-op when managed writes are disabled, unlike the other managed APIs.
sendManaged,stuckWrites,resendBumped, andenableManagedWritesall returnerror.ManagedWritesNotEnabledwhenself.managed == null, butconfirmWrite/failWritejust do nothing. A caller invoking either on a disabled context gets no signal that the confirm/fail was dropped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/context.zig` around lines 871 - 878, Update confirmWrite and failWrite to return error.ManagedWritesNotEnabled when self.managed is null, matching sendManaged, stuckWrites, resendBumped, and enableManagedWrites. Preserve the existing pipeline.confirmTx and pipeline.failTx calls when managed writes are enabled, and adjust the function return types and propagation accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/context.zig`:
- Around line 821-843: Update sendManaged so errors from
mw.pipeline.recordSubmission are handled separately from client.sendManaged
failures: do not release the nonce or return an error after the transaction has
been broadcast, and still return the successful TxResult while handling the
tracking failure through the established mechanism.
---
Nitpick comments:
In `@src/context.zig`:
- Around line 871-878: Update confirmWrite and failWrite to return
error.ManagedWritesNotEnabled when self.managed is null, matching sendManaged,
stuckWrites, resendBumped, and enableManagedWrites. Preserve the existing
pipeline.confirmTx and pipeline.failTx calls when managed writes are enabled,
and adjust the function return types and propagation accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 05c3dabb-2c13-487c-bb74-3803d0aee2f8
📒 Files selected for processing (5)
src/chain_client.zigsrc/context.zigsrc/testing/mock_chain_client.zigtests/contract/context_managed_write_test.zigtests/contract_tests.zig
CodeRabbit (Critical): in sendManaged the tx is already broadcast when
recordSubmission runs, so a tracking error (e.g. OOM) propagated out and
made a live send look failed -- a caller could retry with a fresh nonce
and double-send. Track best-effort (catch {}) and return the TxResult; the
only cost of a miss is that this tx isn't covered by stuck-detection.
What
Wires the SDK's existing
TxPipelineinto a managed write path on the context, so a stuck transaction can be resent at the same nonce with escalated gas -- through the SDK, not around it.Why
The SDK already had a
TxPipeline(nonce/gas prepare, stuck detection,prepareBump), but context writes bypassed it viawallet.sendTransaction(which fills nonce/gas from RPC). So the escalation-on-resend loop -- the fix for a liquidator whose tx is stuck at too-low gas (the gator's prod OOG-resend history) -- was built but unreachable through the SDK. This connects it.Changes
ChainClient.sendManaged(to, data, value, SendParams{nonce, gas_limit, max_fee, max_priority}): a send with fully explicit nonce + EIP-1559 gas (no nonce/gas RPC), so aTxPipelineowns them.EthChainClientforwards towallet.sendTransaction; the mock records the params.contextmanaged-write API over a heap-ownednonce_mgr+gas_cache+TxPipeline:enableManagedWrites(starting_nonce, gas_cfg, cfg),refreshBaseFee(base_fee, now_ms)sendManaged(request, now_ms) -> {tx_hash, nonce}(prepare -> explicit send -> track; a send failure releases the nonce so it isn't skipped)stuckWrites(now_ms),resendBumped(request, original_hash, multiplier) -> new hash(same nonce, fees scaled),confirmWrite/failWritenow_ms(no OS clock in lib code)resendBumpedleaves the original as the in-flight tracker (avoids double-tracking the reused nonce); the caller confirms/fails the original when either version mines.Tests
Full flow via the mock:
GasPriceUnavailablebefore a base fee, nonce assignment, explicit gas + non-zero critical-urgency fees, stuck-timeout detection (not-yet vs stuck), a 2x bump resend at the same nonce with exactly-doubled fees, confirm-then-bump ->TxNotInFlight, plus the not-enabled / already-enabled guards.zig build test: 504/504zig build contract-test(Debug + ReleaseFast): 103/103zig fmt --check: cleanSummary by CodeRabbit
PerpCityContextAPIs to enable managed writes, refresh base fee, send managed requests, and confirm/fail submissions, includingresendBumpedsupport.