Repository navigation
fix: let healthy swap orders survive oracle failures (RAI-3008) - #197
Conversation
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. 6 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 8 reviews per hour. WalkthroughV2 quote and calldata processing now preserve healthy candidates when an oracle context fetch fails. The routes return Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No issue remains that requires a change before merge; proceed with normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 43.90% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 41 functions across 4 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai review Please review the updated head 092c901. It adds a fresh SDK fill check for mixed-book up-to calldata (wrapped spend input and buy output via private effective-price metadata), preserves approval-only and ordinary healthy partial-fill behavior, and adds regression coverage. All 533 tests, strict static checks, production check and docs build pass. |
✅ Action performedReview finished.
|
|
@coderabbitai You completed review of 092c901 with no actionable comments and reported no issue requiring a change before merge. The GitHub APPROVED review still references c96d59f. Please confirm whether the latest reviewed head meets your approval requirements and submit an updated approval only if it does; otherwise identify what needs fixing. Please do not override review or pre-merge requirements. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="ST0x-Technology/st0x.rest.api"
gh pr view 197 --repo "$repo" --json number,state,isDraft,headRefOid,baseRefName,reviewDecision,mergeable,mergeStateStatus,statusCheckRollup,reviews,comments
gh api --paginate "repos/$repo/pulls/197/comments" --jq '.[] | {user: .user.login, commit_id, path, line, body, html_url}'
gh api graphql -f query='
query {
repository(owner: "ST0x-Technology", name: "st0x.rest.api") {
pullRequest(number: 197) {
headRefOid
baseRef {
branchProtectionRule {
requiresApprovingReviews
requiredApprovingReviewCount
requiresCodeOwnerReviews
requiresStatusChecks
requiredStatusCheckContexts
requiresStrictStatusChecks
requiresConversationResolution
}
}
reviewThreads(first: 100) {
pageInfo { hasNextPage endCursor }
nodes {
isResolved
isOutdated
comments(first: 20) {
nodes { author { login } body url }
}
}
}
}
}
}'Length of output: 16765 ⚙️ CodeRabbit configuration inspected Length of output: 97744
I will request an updated approval without overriding any requirements. This does not confirm merge readiness. At inspection, ✅ Action performedChanges approved. Review threads were not changed. |
|
@rain-marvin review Please review the complete PR at head 092c901. CodeRabbit has reviewed and approved the current change with no actionable findings. This fixes the production mixed-oracle swap veto while preserving price/reference guards, incomplete mixed-book errors, selected signed-context refresh, attribution and preflight. It also checks the SDK fresh up-to fill in wrapped units. All 533 local tests (including two executable EVM fixtures), strict static checks, production check and docs build pass. Please report any remaining issues and approve only when satisfied. |
|
🔎 Reviewing |
|
@rain-marvin The branch now includes CI-only commit db27309 after the production implementation you are reviewing (092c901). The only change is the CI test filter: it now runs both oracle_integration_tests instead of selecting only the older attribution test. The exact command passed both EVM fixtures, and formatter/static checks and commit hooks pass. Please include this one-line CI change when concluding the review and approval; there are no production-code changes since your review started. |
![rain-marvin[bot]](https://avatars.githubusercontent.com/in/5077017?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR stops one broken oracle-backed order from blocking every swap on its pair. Quote and calldata now drop the unavailable order, route through the healthy orders, and still return SWAP_ORACLE_UNAVAILABLE when the healthy orders cannot fill the full request, up-to modes included. Calldata repeats the fill check on the SDK's fresh route in wrapped units before it returns executable data. Price caps, reference guards, deployment selection, signed-context refresh, attribution and preflight are unchanged.
Overall the change is correct and well tested. I found no blocking issue. Three small comments: a malformed explicit price cap on a mixed book now gives a 500 where it used to give a 400, the new EVM regression is not run in CI because of the CI name filter, and the mixed-book fill check copies the quote v2 policy.
Known trade-off, not raised as a finding: a slippage-only request (no referenceIoRatio) on a mixed book now sets its cap from the orders that remain. If the failed order was the best-priced one, the caller gets the worse surviving price with no signal that an order was excluded. The PR description states this choice. A response flag for excluded orders could be a follow-up. The fill checks also trust the SDK's simulated legs, so they share the SDK's estimation limits, as healthy books already do.
claude-opus-5-5 · high · 19 min
|
I checked commit
This job only answered your comment; it does not post the review. The review of claude-opus-5-5 · high · 21 min |
|
@rain-marvin approve Please verify the latest head c5337be against your review of 092c901. All three comments are addressed and replied to with evidence: db27309 makes CI run both oracle integration fixtures; c5337be preserves HTTP 400 for malformed and negative explicit caps with all-mode regressions, and shares quote/calldata fill selection through simulate_request_fill. All 534 feature-enabled tests pass, including both executable EVM fixtures; formatter, strict all-target/all-feature static checks and cargo check pass. Please approve only if the fixes and latest diff satisfy your review, or report any remaining issues. |
|
🔎 Verifying that my findings from |
There was a problem hiding this comment.
This PR keeps swap quote and calldata working on a mixed order book: when some orders have an unavailable oracle, the API still quotes and builds calldata from the healthy orders if they can fill the request, and returns the oracle error only when they cannot. The head is still c5337be, the commit my last review covered, so there is no new diff to review. I checked each earlier finding against the code again and all three are fixed.
Earlier findings:
- Malformed explicit cap returned 500 on a mixed book (
src/routes/swap/calldata.rs): resolved. The mixed-route pre-check now maps aFloat::parsefailure to 400invalid price_capand rejects a negative cap throughmap_raindex_error(NegativePriceCap), the same 400 the SDK path gives. A regression test covers both caps in all three modes. - CI did not run the mixed-book EVM test (
src/routes/swap/calldata/oracle_integration_tests.rs): resolved..github/workflows/ci.yaml:115now runscargo test --features oracle-integration-tests oracle_integration_tests, which matches both tests in the module. - Duplicated fill policy between quote and calldata (nit): resolved. Both endpoints now call
slippage::simulate_request_fill, which picks the deployment, the buy/spend achieved amount and the exact/up-to comparison in one place.
All three threads were resolved by me after checking the fixes; none was resolved by hand by someone else. No open issues remain.
claude-opus-5-5 · high · 25 s
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
ueco-jb
left a comment
ueco-jb
left a comment
There was a problem hiding this comment.
The mixed-book path is conservative: healthy candidates must fill the request at both initial simulation and fresh SDK calldata, while exact modes still rely on SDK minimums and approval-only responses are rechecked on retry. Price caps, deployment isolation, wrapped-unit checks, signed-context refresh, attribution, and preflight remain intact. One test-style nit; no blocking issues.
2 participants
Healthy orders can serve swap quotes and calldata when another same-pair order's oracle fails. This fixes the production wtNVDA requests where a retired Fly endpoint returned 404 and vetoed a successfully quoted order. Fixes RAI-3008.
Live effect: valid mixed-order swaps stop returning blanket oracle-unavailable errors · Risk: medium (money path) · Ships: after normal service deployment
Decisions
SWAP_ORACLE_UNAVAILABLE, including up-to requests. Ordinary healthy up-to books retain partial-fill behavior. This fixes the unconditional veto without broadening the existing partial-fill policy.Risks
Proof
cargo check, docs build, and whitespace checks pass.Rollout