Use SDK positive output vault filter before quoting#84
Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughUpdates include a rain.orderbook submodule bump, added positive-output-vault filters to order listing endpoints, swap route call updated to pass ChangesRaindex Integration & Order Filtering Upgrade
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
## Motivation Three commits were pushed directly to `main` without the normal PR review flow: - `543a103` - Add order caching and fix trades-by-address timeout - `50e7fd9` - Add metaboard neutralization, order caching, direct trades, quote chunking, and nginx hardening - `b1ebea3` - Skip RPC quotes for zero-balance orders and add stage timing Branch protection currently prevents rewriting `main`, so this PR restores the reviewed tree state with a normal revert commit instead. The original changes are preserved for review in #82, #83, and #84. ## Solution - Revert the three direct-pushed commits in newest-to-oldest order. - Fold the revert into a single commit for review clarity. - Leave the original changes available in the stacked review PRs. ## Checks - `nix develop -c cargo fmt` - `nix develop -c rainix-rs-static` Note: `rainix-rs-static` passed, with existing warnings reported in the nested `rain.orderbook` crate. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Removed Features** * Disabled trade-related API endpoints (`GET /trades/by-tx`, `GET /trades/by-address`, batch trade queries). * Removed in-memory caching layer for order listings. * **Improvements** * Simplified application architecture by removing unnecessary caching mechanisms and dependencies. * Streamlined order retrieval logic. * **Infrastructure** * Updated server configuration and system settings. * Removed local database storage layer integration. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
findolor
changed the title
findolor
changed the base branch from
review/alastair-metaboard-direct-trades
to
graphite-base/84
findolor
force-pushed
the
review/alastair-zero-balance-stage-timing
branch
from
77697aa to
65f9508
Compare
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. |
findolor
force-pushed
the
review/alastair-zero-balance-stage-timing
branch
4 times, most recently
from
ce96866 to
068864e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/swap/mod.rs (1)
87-94:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
has_positive_output_vault_balancefilter on the swap quoting path.The list endpoints (
get_by_owner.rs,get_by_token.rs) both includehas_positive_output_vault_balance: Some(true)in theirGetOrdersFilters, butget_orders_for_pairinswap/mod.rs(lines 87-94) does not. This inconsistency means the swap quote path will still consider zero-output-balance orders, defeating the purpose of delegating this filtering to the SDK.🔧 Proposed fix
let filters = GetOrdersFilters { active: Some(true), tokens: Some(GetOrdersTokenFilter { inputs: Some(vec![input_token]), outputs: Some(vec![output_token]), }), + has_positive_output_vault_balance: Some(true), ..Default::default() };🤖 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/routes/swap/mod.rs` around lines 87 - 94, The GetOrdersFilters used in get_orders_for_pair is missing the has_positive_output_vault_balance flag so zero-output-balance orders are still considered; update the filters construction in swap::get_orders_for_pair to include has_positive_output_vault_balance: Some(true) (alongside the existing active and tokens fields) so the SDK performs the positive-output-balance filtering; modify the GetOrdersFilters literal where filters is created to add the has_positive_output_vault_balance field.
🤖 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.
Outside diff comments:
In `@src/routes/swap/mod.rs`:
- Around line 87-94: The GetOrdersFilters used in get_orders_for_pair is missing
the has_positive_output_vault_balance flag so zero-output-balance orders are
still considered; update the filters construction in swap::get_orders_for_pair
to include has_positive_output_vault_balance: Some(true) (alongside the existing
active and tokens fields) so the SDK performs the positive-output-balance
filtering; modify the GetOrdersFilters literal where filters is created to add
the has_positive_output_vault_balance field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 422a18f3-435b-4ead-90ff-68a7d215935d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
flake.nixlib/rain.orderbooksrc/routes/orders/get_by_owner.rssrc/routes/orders/get_by_token.rssrc/routes/swap/mod.rssrc/routes/tokens.rssrc/test_helpers.rs
Merge activity
|
## Motivation This PR preserves direct-pushed commit `b1ebea3` for review after removing it from the intended `main` history. It is stacked on #83 and contains the third original direct-pushed commit. The zero-balance quote skip now belongs in Raindex so order pagination and totals are filtered before the REST API fetches quotes. This REST PR now delegates that predicate to the SDK instead of parsing/filtering balances locally. ## Depends On - rainlanguage/raindex#2562 (`feat: add positive output vault balance order filter`) - This PR bumps `lib/rain.orderbook` to `014016c699a2da4aa27d335bbf565e7347c2f762`, which is from that Raindex work and exposes `has_positive_output_vault_balance`. ## Solution - Pass `has_positive_output_vault_balance: Some(true)` when listing orders by owner or token. - Remove REST-side output balance parsing/filtering and quote the SDK-filtered orders directly. - Keep stage timing instrumentation from the original commit. - Update REST compatibility for the newer SDK bindings, take-order candidate signature, signed context field, and spec version 5 test fixtures. ## Checks - `nix develop -c cargo fmt` - `nix develop -c cargo check` - `nix develop -c cargo test routes::orders` - `nix develop -c cargo test` - `nix develop -c rainix-rs-static` fails on existing clippy `too_many_arguments` errors in `src/routes/trades.rs` route handlers. Stack note: merge/review after #82 and #83. This PR is based on `review/alastair-metaboard-direct-trades`. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Orders now filtered to include only those with a positive output vault balance * Added support for version-5 Raindex registry configuration * **Tests** * Expanded token listing tests and remote-token scenario coverage * Added/updated tests for version-5 Raindex registry compatibility * **Chores** * Updated submodule reference and adjusted build/prep configuration paths <!-- end of auto-generated comment: release notes by coderabbit.ai -->
graphite-app
Bot
force-pushed
the
review/alastair-zero-balance-stage-timing
branch
from
068864e to
6fb902f
Compare
3 participants
Motivation
This PR preserves direct-pushed commit
b1ebea3for review after removing it from the intendedmainhistory. It is stacked on #83 and contains the third original direct-pushed commit.The zero-balance quote skip now belongs in Raindex so order pagination and totals are filtered before the REST API fetches quotes. This REST PR now delegates that predicate to the SDK instead of parsing/filtering balances locally.
Depends On
feat: add positive output vault balance order filter)lib/rain.orderbookto014016c699a2da4aa27d335bbf565e7347c2f762, which is from that Raindex work and exposeshas_positive_output_vault_balance.Solution
has_positive_output_vault_balance: Some(true)when listing orders by owner or token.Checks
nix develop -c cargo fmtnix develop -c cargo checknix develop -c cargo test routes::ordersnix develop -c cargo testnix develop -c rainix-rs-staticfails on existing clippytoo_many_argumentserrors insrc/routes/trades.rsroute handlers.Stack note: merge/review after #82 and #83. This PR is based on
review/alastair-metaboard-direct-trades.Summary by CodeRabbit
New Features
Tests
Chores