fix(antseed): narrow attempted to what provably never broadcast - #97
Merged
Conversation
The keeper's first armed top-up burnt $5 of a $10 daily cap on a deposit that failed with an RPC 403. `_outcome_for` only reaches `failed` (which consumes neither cap nor cooldown) on `attempted: false`, and control.js marked every non-zero CLI exit `attempted: true` — correct, but blunt. Narrow it in the SIDECAR, not the keeper. The keeper is handed `(stderr || stdout)[:600]`: one stream, truncated, and @antseed/cli prints its transaction hash with console.log while ora writes the failure line to stderr — so on the failure path the keeper gets the stream that cannot carry the hash and never sees the one that can. Any judgement made there would be made on strictly less evidence than the sidecar already had. New antseed/broadcast.js (dependency-free, like amount.js / ids.js) answers `attempted: false` only for a RECOGNISED pre-RPC failure: a process that never spawned (execFile ENOENT/EACCES/...), a module graph that would not load, the CLI's own argument guard — and only with an empty stdout, since `buyer deposit` prints its Wallet:/Amount: preamble before the deposits client exists. Everything else stays `attempted: true`, including silence and every unrecognised shape, so a future CLI degrades toward the safe answer. The 403 that prompted this deliberately still classifies `unknown`, and the module documents why at length. `buyer deposit` broadcasts TWICE (unconditional approve, then the deposit) and polls for a receipt after each; the CLI drops the TransactionResponse before rethrowing, so a receipt poll that 403s prints exactly what a 403 before signing prints — no hash, same message. The suggested "no tx hash + transport-level error" heuristic would call that `failed` and move real USDC with the ledger recording nothing. Resolving that class needs evidence from outside the CLI's stdio (nonce around the run, or the escrow delta a cycle later). Also closes a hole the incident exposed: every `unknown` consumes the cap, so at the shipped knobs (cap 10, amount 5) only two deposits fit in 24h and the error breaker's third strike could never be written. The cap silently absorbed the failure and the keeper went quiet for a day with no halt and nothing above WARNING. It now halts on two strikes once the cap can no longer admit an attempt — forfeiting nothing it could still have done. Every comment asserting the old "did the CLI run" contract is updated; `failed` now means "no transaction could have reached Base mainnet".
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe sidecar classifies CLI failures by broadcast evidence. The control layer returns this classification. The wallet keeper uses ChangesBroadcast outcome classification
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant WalletKeeper
participant AntseedControl
participant classifyCliFailure
WalletKeeper->>AntseedControl: Submit wallet operation
AntseedControl->>classifyCliFailure: Classify failed CLI result
classifyCliFailure-->>AntseedControl: Return attempted and why
AntseedControl-->>WalletKeeper: Return operation response
WalletKeeper->>WalletKeeper: Map attempted false to failed
WalletKeeper->>WalletKeeper: Map other failures to unknown
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@wallet_keeper.py`:
- Around line 909-923: Derive the capped halt threshold in the top-up handling
flow around _error_strikes and _halt_topups from knobs.topup_daily_cap_usdc
divided by knobs.topup_amount_usdc, limiting it to TOPUP_ERROR_STRIKES_TO_HALT
and ensuring a minimum threshold of one. Use this derived threshold instead of
the fixed TOPUP_ERROR_STRIKES_TO_HALT_CAPPED value so a one-attempt cap halts
after its first unknown result, and add coverage for that one-attempt daily-cap
scenario.
🪄 Autofix
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: 6eae452d-1bb2-4a04-9ba8-78049dcbeb5e
📒 Files selected for processing (8)
antseed/broadcast.jsantseed/broadcast.test.jsantseed/control.jsdocs/PROVIDERS.mdhost_store.pytests/test_antseed_node.pytests/test_wallet_keeper.pywallet_keeper.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context. The wallet keeper was enabled in prod for the first time today. Its top-up produced:
wallet_op_spend_sincethen read{'spent_usdc': 5.0}— $5 of a $10 daily cap consumed by a transaction that never landed.What this PR does NOT do
It does not narrow that case. The obvious rule — "no tx hash in the output + a transport-level error ⇒ nothing broadcast" — is unsound, and implementing it would have been a money bug.
buyer depositprints onlyerr.messageon failure (deposit.js:32-34). The hash lives insidedepositsClient.deposit()and is never attached to the thrown error, so a 403 hit while polling for a receipt — after broadcast — is byte-identical to a 403 hit before signing. That rule would have recorded a live, money-moving deposit as costing nothing: the one error direction theattemptedcontract exists to prevent. The incident's own shape still classifiesunknown, and the module documents why.Two further reasons the naive rule fails:
403is a bad key (the FallbackProvider rewrites a single-endpoint 403 intoquorum not met, alsoSERVER_ERROR), and connection reset is not pre-broadcast — only refused and DNS failure are, and neither is affirmatively recognisable here.What it does do
antseed/broadcast.js—classifyCliFailure({code, killed, stdout, stderr}), first match wins, affirmative not residual:attempted: falserequires a recognised pre-RPC shape, so silence, drift, and anything unrecognised stayunknown.depositing usdc, …) ⇒ attempted, unconditionallynonce,already known,execution reverted, …) ⇒ attemptedWallet:/Amount:preamble precedes the deposits client)Amount must be a positive number,Cannot find module) ⇒ not attemptedIt lives in the sidecar, not the keeper, on evidence grounds: the keeper receives
(r.stderr || r.stdout)truncated to 600 chars — one stream. ora writes the failure to stderr; the hash isconsole.log'd to stdout. On the failure path the keeper is handed the stream that cannot contain a hash and never sees the one that can.The real bug this surfaced
failedwas already safe. The hole was on theunknownside: everyunknownconsumes the cap, so at the shipped knobs (cap 10, amount 5) exactly two rows fit in 24h — while the error breaker waits for three. The third row can never be written. The cap silently absorbs the failure and the keeper goes quiet for a day: no halt, nothing above WARNING, nobody told.That is exactly what prod did today.
TOPUP_ERROR_STRIKES_TO_HALT_CAPPED = 2halts the shorter run once the cap can no longer admit an attempt, converting a silent day-long stall into a persisted, operator-cleared alarm. A cap reached by deposits that worked still just returnsdaily_cap, pinned by its own test.Tests
6 Python + 16 node. 4 fail before the change, verified by stashing the source. Classifier fixtures are the real prod
wallet_opsdetail, byte for byte.Honest scope
This does not stop the prod failure recurring — it stops it being invisible. The actual fix is genlayerlabs/devexp-argocd-apps#534, which gives the CLI a keyed Base RPC;
ANTSEED_BASE_RPC_URLis unset, so it has been transacting through free public endpoints. Resolvingunknownrows from on-chain evidence (escrow delta, or a nonce witness) is the only sound path to the incident class and wants a design pass first.Summary by CodeRabbit
Bug Fixes
Documentation
Tests