Repository navigation
[RAI-2943] Bound the orchestrator-approve receipt wait and return the tx hash - #454
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
How to use the Graphite Merge QueueAdd the label add-to-gt-merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
938e8b8 to
1258b55
Compare
b8a5c0b to
0f81f67
Compare
3a0c4b3 to
83d5bed
Compare
|
@rain-marvin review |
|
🔎 Reviewing |
There was a problem hiding this comment.
This PR puts a deadline on capital approve-orchestrator. The route signs the approval, broadcasts it, and waits for its receipt only until the deadline. When the receipt does not come in time, the route answers 202 submitted_unconfirmed with the transaction hash. Before this change the request could hang until a gateway cut it off, and the operator got no hash. The wallet lock is now released at the deadline. A node that refuses the broadcast gives a 422. A failure before broadcast gives a 503. The offline issuer approve-orchestrator uses a 300 s deadline and prints the hash. SPEC, the runbook and the ops-client text are updated to match.
Overall the change is sound. The deadline covers each step: the wallet lock, the fill and sign, the broadcast, the receipt wait and the allowance re-read. The bot does not broadcast a transaction that it signed after the deadline. The concurrency and the lock handling look correct. The mapping to HTTP status codes is complete. No finding blocks the merge. The main point: the 422 "refused" answer is a little too sure of itself. Some broadcast errors do not prove that the node refused the transaction, and in those cases the operator gets a 422 without the hash. Two smaller points concern log assertions in the new tests, and a single RPC error that ends the receipt wait early.
claude-opus-5-5 · high · 15 min
…y failed confirmation reads until the deadline
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (7)
Included review availability: This review used your included allowance. 4 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 6 reviews per hour. WalkthroughApproval processing now uses deadlines for offline CLI and IAP requests. It distinguishes approvals that are confirmed, already unlimited, submitted but unconfirmed, or definitively refused. The IAP route applies an 18-second deadline and returns separate HTTP outcomes. The CLI waits up to five minutes and reports the transaction hash if confirmation is incomplete. Tests cover deadline handling, broadcast classification, confirmation retries, and route response mapping. Documentation describes operator actions for unconfirmed transactions. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The change bounds the approval route to an 18-second deadline and returns the transaction hash when an approval is submitted but unconfirmed. No concrete merge-blocking issue is identified. Live-chain validation through Turnkey has not been run. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 44 functions across 5 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
Comment |
findolor
left a comment
There was a problem hiding this comment.
Reviewed locally against the stack parent with two independent Codex reviewers. No blocking findings. Workspace tests, Clippy, and formatting passed.
|
Nonblocking nit: the retry guidance in Suggested wording: "A missing hash is inconclusive; reconcile the wallet nonce and allowance before retrying." This is an operator-guidance correction, not a merge blocker. |
|
@rain-marvin approve |
|
🔎 Verifying that my findings from |
There was a problem hiding this comment.
This PR bounds the orchestrator approval wait during vault onboarding. The approval is broadcast and confirmed under a deadline. A 422 BroadcastRejected is returned only when the node proves it refused the bytes. Any other outcome goes to confirm by hash and answers 202 submitted_unconfirmed with the hash.
The head is still 397e13e, the commit my last review checked, so nothing changed since then. I checked the code again at that commit, and it agrees with what I confirmed in each thread.
Earlier findings:
- 422 for any JSON-RPC error (
src/vault/onboarding.rs,broadcast_approval): resolved.BroadcastRejectednow comes only fromOk(Ok(None))afteris_pool_refusal. "Already known", other server errors, transport errors, and a lookup that fails or times out all returnOk(()). The checks reuse theservice.rspredicates, and the SPEC bullets match. - Early
SubmittedUnconfirmedon one transient error (confirm_approval): resolved. The receipt poll and the allowance re-read now retry at the poll interval until the outer deadline and keep the last error.PendingTransactionis removed, and the CLI message is now accurate. A mocked-node test covers one failed read of each kind. - Missing log assertions and coverage (refused-broadcast test): resolved. The test now runs under
#[traced_test]and asserts the WARN with the hash. A table test covers real refusals, "already known", -32603, and a failed lookup. The deadline tests have no log line to assert because that path logs nothing. One branch is still untested and does not block: a pool refusal whose lookup finds the transaction.
Threads resolved by hand: none. I resolved all three threads after I confirmed the fixes.
No new changes to review and no open findings.
claude-opus-5-5 · high · 17 min
Merge activity
|

Bounds
capital approve-orchestratorto one 18-second request deadline and returns the signed transaction hash when confirmation is still pending. This is part 3 of 5; #456 makes live external burn-excess safe for the poller. RAI-2943Live effect: the route returns 202 with a transaction hash, 503 before broadcast, or 422 for a proven refusal instead of hanging · Risk: medium (money path and wallet signing; a sent approval is on-chain, but the route remains operator-triggered) · Blocks: #456
Decisions
Risks
Proof
testandstaticpassed. Regression tests cover an unmined approval, a refused broadcast, a late signature, and each pre-broadcast deadline.Rollout
outcome, not only the exit code. Look up every 202 hash before retrying.