-
Notifications
You must be signed in to change notification settings - Fork 68
Add nonce checking #845
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add nonce checking #845
Conversation
📝 WalkthroughWalkthroughAdds nonce-aware checks and adjusted gas estimation to the transaction send flow: verifies pending vs confirmed nonces before send, estimates gas with NoSend then multiplies by 1.5 for final GasLimit, logs nonce/gas details, and retains existing receipt-waiting behavior and error paths. Changes
Sequence Diagram(s)(Skipped — changes are localized to a single file and do not introduce a multi-component sequential feature that warrants a diagram.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
token-price-oracle/updater/tx_manager.go
🧰 Additional context used
🧬 Code graph analysis (1)
token-price-oracle/updater/tx_manager.go (2)
token-price-oracle/client/l2_client.go (1)
L2Client(15-19)bindings/hardhat/types.go (1)
Receipt(32-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (6)
token-price-oracle/updater/tx_manager.go (6)
17-33: LGTM!The constants are well-documented and the 15% bump exceeds the EIP-1559 minimum of 10%. The
PendingTxInfostruct captures all necessary fields for tracking pending transactions.
35-39: LGTM!The
pendingTxfield is properly added to track the last pending transaction, and the existing mutex will protect concurrent access.
55-70: LGTM!The pending transaction handling logic correctly differentiates between confirmed transactions (clears state) and errors (continues to replacement logic). The error case appropriately continues to attempt replacement rather than failing outright.
153-183: LGTM!The
handlePendingTxmethod correctly handles three scenarios:
- Transaction receipt found → return receipt
- Nonce confirmed (possibly by replacement) → clear state, return nil
- Transaction still pending → return error for replacement logic
185-216: LGTM with caveat.The gas price bump logic correctly implements a 15% increase with a 3x cap. The arithmetic is sound. Note that this method assumes non-nil inputs; ensure callers always pass valid
*big.Intvalues (see earlier comment about EIP-1559 transactions).
138-150: LGTM!The decision to preserve
pendingTxon receipt timeout (line 144) is a sound design choice—it allows the nextSendTransactioncall to attempt replacement. The pending state is correctly cleared only on successful confirmation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
token-price-oracle/updater/tx_manager.go (1)
106-184: Critical: Indefinite waiting without upper bound timeout creates operational risk.The function now waits indefinitely for a transaction receipt with no built-in timeout. This design has severe operational implications:
Problem: A transaction stuck in the mempool (due to insufficient gas, network congestion, or node issues) will cause the system to wait forever until:
- Context is cancelled (only if caller sets timeout)
- Transaction is dropped from pool (timing depends on node configuration)
- Transaction is eventually mined (could be hours or days)
Impact:
- During this indefinite wait, the mutex remains locked, blocking all other transactions
- If context timeout occurs, the next send attempt will be blocked by the pending nonce check (see lines 51-59)
- System becomes deadlocked without external intervention
- No self-recovery mechanism
Recommendations:
- Add a reasonable upper bound timeout (e.g., 10-15 minutes) independent of context
- After timeout, return an error with details for transaction replacement
- Implement transaction replacement in SendTransaction (see previous comment)
- Document that callers should set context timeouts
Suggested bounded waiting with replacement hint
func (m *TxManager) waitForReceipt(ctx context.Context, txHash common.Hash, pollInterval time.Duration) (*types.Receipt, error) { ticker := time.NewTicker(pollInterval) defer ticker.Stop() startTime := time.Now() maxWaitTime := 15 * time.Minute // Upper bound for { elapsed := time.Since(startTime) // Hard timeout check if elapsed > maxWaitTime { return nil, fmt.Errorf("transaction %s not confirmed after %v, may need replacement with higher gas", txHash.Hex(), maxWaitTime) } // ... rest of the logic } }
🧹 Nitpick comments (1)
token-price-oracle/updater/tx_manager.go (1)
167-172: Optional: Logging condition may skip minute markers due to timing.The condition
int(elapsed.Seconds())%60 == 0only logs when the poll happens to hit an exact 60-second mark. With a 2-second polling interval, this is likely but not guaranteed. You might miss logging at 60s if the poll occurs at 59s and the next at 61s.More reliable logging approach
+ lastLogMinute := 0 for { // ... existing code ... } else { - // Transaction found, log progress every minute elapsed := time.Since(startTime) - if int(elapsed.Seconds()) > 0 && int(elapsed.Seconds())%60 == 0 { + currentMinute := int(elapsed.Minutes()) + if currentMinute > lastLogMinute { log.Info("Still waiting for transaction receipt", "tx_hash", txHash.Hex(), "is_pending", isPending, "waited", elapsed) + lastLogMinute = currentMinute } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
token-price-oracle/updater/tx_manager.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
token-price-oracle/updater/tx_manager.go (2)
5-5: LGTM!The new imports are necessary for the enhanced error handling with
errors.Is()andethereum.NotFoundchecks.Also applies to: 10-10
69-81: LGTM!The two-phase gas estimation approach is solid: estimate with
NoSend=true, then apply a 1.5x safety margin for the actual send. This helps prevent out-of-gas errors while avoiding excessive gas limits.
Summary by CodeRabbit
New Features
Reliability & UX
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.