Split take-orders endpoint into buy and sell routes - #2399
Merged
Conversation
Contributor
WalkthroughThe PR replaces a single multi-purpose take-orders API endpoint with two dedicated endpoints (buy and sell). TakeOrdersApiRequest and the local TakeOrdersMode enum were removed and replaced with BuyRequest and SellRequest request types. Route handlers were split into separate buy() and sell() functions, and OpenAPI definitions were updated to reflect the new paths and schemas. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Add utoipa and utoipa-swagger-ui with rocket features to enable OpenAPI specification generation and Swagger UI for the REST API.
- Add ToSchema derive to ApiErrorResponse with examples - Add TakeOrdersMode enum with From impl for type-safe conversion - Add ToSchema derives to TakeOrdersApiRequest and TakeOrdersApiResponse - Include realistic examples using Base network configuration
- Add OpenApi derive with API info and schema components - Mount Swagger UI at /swagger/ and OpenAPI JSON at /swagger/openapi.json - Add tests for Swagger UI HTML response - Add tests for OpenAPI spec structure, paths, schemas, and response codes
- Replace single /take-orders with /take-orders/buy and /take-orders/sell - Rename fields for clarity: sellToken/buyToken → tokenIn/tokenOut, priceCap → maxRatio - Replace TakeOrdersMode enum with simpler `exact` boolean flag - Add comprehensive field descriptions for OpenAPI documentation - Add full test coverage for both endpoints including swagger schema tests
findolor
force-pushed
the
2026-01-07-split-take-orders-buy-sell-routes
branch
from
January 14, 2026 09:37
2a730d8 to
dbe1e7c
Compare
…-take-orders-buy-sell-routes - Keep buy/sell route split with tokenIn/tokenOut/maxRatio naming - Remove unnecessary allow_credentials from CORS config - Add spawn_blocking comments to both endpoints
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/rest_api/src/routes/take_orders.rs`:
- Around line 157-190: Extract the duplicated spawn_blocking + single-thread
runtime logic into a shared helper (e.g., run_on_current_thread or
run_blocking_single_thread) and call it from buy (and the other handler) instead
of duplicating the block; the helper should accept a closure or async function
that receives cloned yaml_content and take_request and internally build the
current_thread tokio runtime, call block_on on the provided future, and map
runtime or task errors into ApiError (same messages used today), so you can
replace the tokio::task::spawn_blocking(...) { let rt =
tokio::runtime::Builder::new_current_thread()...
rt.block_on(execute_take_orders(yaml_content, take_request)) } pattern in buy
(and the sibling handler) with a single call to that helper which returns
Result<_, ApiError>.
- Around line 74-95: The serde_json json! macro is used unqualified in the
TakeOrdersApiResponse struct (prices field example) which will fail if json! is
not imported; update the macro invocation to the fully qualified path
serde_json::json! (i.e., replace json!(...) with serde_json::json!(...)) so the
example compiles without adding a module-level import, and verify the prices
field in TakeOrdersApiResponse still serializes correctly.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
crates/rest_api/src/main.rscrates/rest_api/src/routes/take_orders.rspackages/webapp/src/lib/constants.tstauri-app/src/lib/services/loadRemoteSettings.ts
🧰 Additional context used
📓 Path-based instructions (9)
packages/webapp/**/*.{svelte,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
For Webapp (
packages/webapp), run lints and format checks usingnix develop -c npm run svelte-lint-format-check -w@rainlanguage/webapp``
Files:
packages/webapp/src/lib/constants.ts
packages/webapp/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
For Webapp (
packages/webapp), run tests usingnix develop -c npm run test -w@rainlanguage/webapp``
Files:
packages/webapp/src/lib/constants.ts
packages/{webapp,ui-components}/**/*.{svelte,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
If you modify frontend code or functionality affecting the frontend, you MUST provide a screenshot of the built webapp reflecting your change.
Files:
packages/webapp/src/lib/constants.ts
packages/**/*.{js,ts,svelte}
📄 CodeRabbit inference engine (AGENTS.md)
JavaScript/Svelte organized as
packages/*including webapp, ui-components, and orderbook (wasm wrapper published to npm)
Files:
packages/webapp/src/lib/constants.ts
**/*.{ts,tsx,svelte}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,svelte}: TypeScript/Svelte: format withnix develop -c npm run format
TypeScript/Svelte: lint withnix develop -c npm run lint
TypeScript/Svelte: type-check withnix develop -c npm run check
Files:
packages/webapp/src/lib/constants.tstauri-app/src/lib/services/loadRemoteSettings.ts
tauri-app/**
📄 CodeRabbit inference engine (AGENTS.md)
Desktop app located in
tauri-appwith Rust and Svelte; excludesrc-taurifrom Cargo workspace
Files:
tauri-app/src/lib/services/loadRemoteSettings.ts
crates/**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
crates/**/*.rs: For Rust crates incrates/*, run lints usingnix develop -c cargo clippy --workspace --all-targets --all-features -D warnings
For Rust crates incrates/*, run tests usingnix develop -c cargo test --workspaceor--package <crate>
Files:
crates/rest_api/src/main.rscrates/rest_api/src/routes/take_orders.rs
**/crates/**
📄 CodeRabbit inference engine (AGENTS.md)
Rust workspace organized as
crates/*with subdirectories: cli, common, bindings, js_api, quote, subgraph, settings, math, integration_tests
Files:
crates/rest_api/src/main.rscrates/rest_api/src/routes/take_orders.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Rust: format code withnix develop -c cargo fmt --all
Rust: lint withnix develop -c rainix-rs-static(preconfigured flags included)
Rust: crates and modules usesnake_case; types usePascalCase
Files:
crates/rest_api/src/main.rscrates/rest_api/src/routes/take_orders.rs
🧠 Learnings (24)
📓 Common learnings
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2391
File: crates/rest_api/src/main.rs:66-201
Timestamp: 2026-01-16T11:13:10.390Z
Learning: In crates/rest_api, happy-path integration tests for the /take-orders endpoint are intentionally omitted because they would require mocking blockchain and subgraph infrastructure, which is considered too complex for the value provided. The test suite focuses on error-path validation instead.
📚 Learning: 2025-11-06T12:08:39.643Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2298
File: packages/webapp/src/lib/constants.ts:3-4
Timestamp: 2025-11-06T12:08:39.643Z
Learning: In the rain.orderbook repository, the REMOTE_SETTINGS_URL constant in packages/webapp/src/lib/constants.ts and tauri-app/src/lib/services/loadRemoteSettings.ts should remain duplicated rather than being consolidated into a single import, as this duplication is intentional.
Applied to files:
packages/webapp/src/lib/constants.tstauri-app/src/lib/services/loadRemoteSettings.ts
📚 Learning: 2025-04-09T12:58:03.399Z
Learnt from: hardingjam
Repo: rainlanguage/rain.orderbook PR: 1565
File: packages/webapp/src/lib/services/loadRegistryUrl.ts:5-19
Timestamp: 2025-04-09T12:58:03.399Z
Learning: In the rain.orderbook application, URL validation for registry URLs is handled at the frontend/component level before the `loadRegistryUrl` function is called, so additional validation within this function is not necessary.
Applied to files:
packages/webapp/src/lib/constants.ts
📚 Learning: 2025-04-09T13:00:24.640Z
Learnt from: hardingjam
Repo: rainlanguage/rain.orderbook PR: 1565
File: packages/webapp/src/__tests__/registryManager.test.ts:227-238
Timestamp: 2025-04-09T13:00:24.640Z
Learning: In the rain.orderbook project, error handling was added to the RegistryManager to handle localStorage exceptions, addressing part of the verification agent's suggestions. The team should also consider testing extremely long URLs that might approach browser limits.
Applied to files:
packages/webapp/src/lib/constants.ts
📚 Learning: 2026-01-16T11:13:10.390Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2391
File: crates/rest_api/src/main.rs:66-201
Timestamp: 2026-01-16T11:13:10.390Z
Learning: In crates/rest_api, happy-path integration tests for the /take-orders endpoint are intentionally omitted because they would require mocking blockchain and subgraph infrastructure, which is considered too complex for the value provided. The test suite focuses on error-path validation instead.
Applied to files:
crates/rest_api/src/main.rscrates/rest_api/src/routes/take_orders.rs
📚 Learning: 2026-01-16T06:20:40.644Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2358
File: crates/common/src/local_db/query/fetch_orders/mod.rs:17-21
Timestamp: 2026-01-16T06:20:40.644Z
Learning: In crates/common/src/local_db/query/fetch_orders/mod.rs, for internal structs like FetchOrdersTokensFilter that are never compared directly (tests compare individual fields instead), PartialEq and Eq derives are not needed. Documentation can be skipped when field names are self-explanatory, especially for internal types.
Applied to files:
crates/rest_api/src/main.rscrates/rest_api/src/routes/take_orders.rs
📚 Learning: 2026-01-16T11:17:10.568Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2391
File: crates/rest_api/src/routes/take_orders.rs:75-93
Timestamp: 2026-01-16T11:17:10.568Z
Learning: In crates/rest_api, the take_orders handler uses spawn_blocking with a dedicated single-threaded Tokio runtime per request because RaindexClient contains Rc<RefCell<...>> internally which is not Send, while Rocket requires handler futures to be Send. This pattern is intentional and required to satisfy Rust's thread-safety requirements.
Applied to files:
crates/rest_api/src/main.rs
📚 Learning: 2025-12-18T08:19:55.397Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2363
File: crates/common/src/take_orders/preflight.rs:80-107
Timestamp: 2025-12-18T08:19:55.397Z
Learning: In crates/common/src/take_orders/preflight.rs, the simulate_take_orders function intentionally returns () and discards the simulation result data because it only needs to verify that the transaction doesn't revert with the given parameters, without requiring access to output amounts or other returndata.
Applied to files:
crates/rest_api/src/main.rscrates/rest_api/src/routes/take_orders.rs
📚 Learning: 2025-06-17T16:21:24.384Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1903
File: crates/settings/src/yaml/orderbook.rs:371-377
Timestamp: 2025-06-17T16:21:24.384Z
Learning: In crates/settings/src/yaml/orderbook.rs tests, the user findolor considers RPC ordering in Vec<Url> assertions to be intentional and not a test brittleness issue. The ordering of RPCs in tests should be preserved as specified.
Applied to files:
crates/rest_api/src/main.rscrates/rest_api/src/routes/take_orders.rs
📚 Learning: 2025-07-16T14:33:45.887Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2001
File: crates/common/src/raindex_client/vaults.rs:0-0
Timestamp: 2025-07-16T14:33:45.887Z
Learning: In the rainlanguage/rain.orderbook codebase, findolor considers using Address::random() in tests to be acceptable when the mock server doesn't validate the address parameter and returns a fixed response, making the test deterministic regardless of the address value used.
Applied to files:
crates/rest_api/src/main.rs
📚 Learning: 2025-07-21T16:34:04.947Z
Learnt from: 0xgleb
Repo: rainlanguage/rain.orderbook PR: 1911
File: crates/common/src/raindex_client/orders.rs:720-720
Timestamp: 2025-07-21T16:34:04.947Z
Learning: In the rainlanguage/rain.orderbook codebase, user 0xgleb prefers using `.unwrap()` in test code rather than `.expect()` with descriptive messages, considering the direct unwrap approach acceptable for test contexts where failures should be fast and clear.
Applied to files:
crates/rest_api/src/main.rs
📚 Learning: 2025-05-13T20:06:22.602Z
Learnt from: 0xgleb
Repo: rainlanguage/rain.orderbook PR: 1713
File: crates/settings/src/remote/chains/mod.rs:43-226
Timestamp: 2025-05-13T20:06:22.602Z
Learning: When writing tests for collections of complex objects in Rust, prefer item-by-item comparison over direct vector comparison to get more specific error messages that pinpoint exactly which item and field has a mismatch.
Applied to files:
crates/rest_api/src/main.rs
📚 Learning: 2025-09-25T12:19:33.736Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2099
File: crates/common/src/raindex_client/sqlite_web/fetch.rs:1203-1209
Timestamp: 2025-09-25T12:19:33.736Z
Learning: httpmock json_body_partial expects Into<String> (JSON string), while json_body expects Into<serde_json::Value> (JSON object). Using .to_string() with json_body_partial is the correct usage.
Applied to files:
crates/rest_api/src/main.rs
📚 Learning: 2026-01-16T11:10:05.073Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2391
File: crates/rest_api/src/main.rs:8-19
Timestamp: 2026-01-16T11:10:05.073Z
Learning: In crates/rest_api/src/main.rs, the REST API is implemented as stateless and unauthenticated: no cookies, sessions, or auth headers; the taker address is passed as a request parameter rather than using session-based context. Maintain this design if it is an intentional security model; if future security requirements require authentication, add explicit tokens/headers and document the change.
Applied to files:
crates/rest_api/src/main.rs
📚 Learning: 2025-07-16T10:40:05.717Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2000
File: crates/common/src/raindex_client/vaults.rs:183-183
Timestamp: 2025-07-16T10:40:05.717Z
Learning: In the rainlanguage/rain.orderbook codebase, user findolor considers breaking changes from Option<U256> to U256 for required fields like decimals in RaindexVaultToken to be acceptable and safe, even when they affect multiple usage sites across the codebase.
Applied to files:
crates/rest_api/src/routes/take_orders.rs
📚 Learning: 2026-01-16T11:17:10.568Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2391
File: crates/rest_api/src/routes/take_orders.rs:75-93
Timestamp: 2026-01-16T11:17:10.568Z
Learning: When a handler or code path in crates/rest_api/src/routes uses non-Send interior mutability (e.g., Rc<RefCell<...>>), and the framework requires Send futures (e.g., Rocket), spawn blocking work with a dedicated per-request single-thread Tokio runtime via spawn_blocking. This pattern isolates non-Send state from the async executor and sidelines Send requirements, preserving thread-safety. Apply this guidance to similar handlers in crates/rest_api/src/routes (not just take_orders) where non-Send types are used within Rocket or similar frameworks.
Applied to files:
crates/rest_api/src/routes/take_orders.rs
📚 Learning: 2025-12-03T10:40:25.429Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2344
File: crates/common/src/local_db/pipeline/runner/mod.rs:18-31
Timestamp: 2025-12-03T10:40:25.429Z
Learning: In `crates/common/src/local_db/pipeline/runner/mod.rs`, the `TargetSuccess` struct does not need separate `ob_id` or `orderbook_key` fields because the contained `SyncOutcome` already includes orderbook identification information such as chain_id and orderbook_address. This avoids redundant data duplication.
Applied to files:
crates/rest_api/src/routes/take_orders.rs
📚 Learning: 2025-07-04T09:02:57.301Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1956
File: crates/common/src/fuzz/mod.rs:64-64
Timestamp: 2025-07-04T09:02:57.301Z
Learning: In rainlanguage/rain.orderbook, user findolor prefers to limit type consistency changes to only the parts directly related to the current work scope. For example, when updating chain_id fields from u64 to u32 in fuzz-related code, unrelated files like tauri-app wallet commands can remain as u64 if they serve different purposes and aren't part of the current changes.
Applied to files:
crates/rest_api/src/routes/take_orders.rs
📚 Learning: 2025-06-04T10:21:01.388Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1907
File: packages/orderbook/test/common/test.test.ts:75-77
Timestamp: 2025-06-04T10:21:01.388Z
Learning: The DotrainOrder.create API in packages/orderbook/test/common/test.test.ts is internal and not used directly in consumer applications, so API changes here don't require external breaking change documentation.
Applied to files:
crates/rest_api/src/routes/take_orders.rs
📚 Learning: 2025-07-21T16:34:31.193Z
Learnt from: 0xgleb
Repo: rainlanguage/rain.orderbook PR: 1911
File: crates/subgraph/src/types/impls.rs:7-15
Timestamp: 2025-07-21T16:34:31.193Z
Learning: In the rainlanguage/rain.orderbook codebase, user 0xgleb considers breaking changes that remove unsafe default behaviors to be intentional and acceptable. Specifically, the get_decimals() method in crates/subgraph/src/types/impls.rs was intentionally changed to return MissingDecimals error instead of defaulting to 18 decimals, as defaulting to 18 is considered unsafe and should never have been done.
Applied to files:
crates/rest_api/src/routes/take_orders.rs
📚 Learning: 2025-09-30T21:18:01.636Z
Learnt from: hardyjosh
Repo: rainlanguage/rain.orderbook PR: 2167
File: crates/virtual-raindex/src/engine/take.rs:131-138
Timestamp: 2025-09-30T21:18:01.636Z
Learning: In the virtual-raindex take order flow (crates/virtual-raindex/src/engine/take.rs), balance diffs are written from the order's perspective where the taker is the counterparty: the order's input column receives taker_output (what the taker provides to the order) and the order's output column receives taker_input (what the taker requests from the order).
Applied to files:
crates/rest_api/src/routes/take_orders.rs
📚 Learning: 2025-10-02T19:17:20.332Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2163
File: crates/common/src/raindex_client/orders.rs:738-741
Timestamp: 2025-10-02T19:17:20.332Z
Learning: In crates/common/src/raindex_client/orders.rs, fetch_dotrain_source() is intentionally called in try_from_sg_order for every order conversion because the dotrain source information is needed immediately. A future optimization with local DB logic is planned to eliminate the network round-trip concern.
Applied to files:
crates/rest_api/src/routes/take_orders.rs
📚 Learning: 2025-07-04T10:27:22.544Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1956
File: crates/common/src/raindex_client/orders.rs:609-609
Timestamp: 2025-07-04T10:27:22.544Z
Learning: In the rainlanguage/rain.orderbook codebase, user findolor prefers not to implement overflow protection for trades count casting (usize to u16) at this time, considering it unnecessary for the current scope since the practical risk of orders having 65,535+ trades is extremely low.
Applied to files:
crates/rest_api/src/routes/take_orders.rs
📚 Learning: 2025-08-26T14:52:37.000Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2099
File: crates/common/src/hyper_rpc.rs:3-7
Timestamp: 2025-08-26T14:52:37.000Z
Learning: In the rain.orderbook codebase, creating new reqwest::Client instances per request in HyperRpcClient is not considered an issue by the maintainers, despite potential performance benefits of client reuse.
Applied to files:
crates/rest_api/src/routes/take_orders.rs
🧬 Code graph analysis (2)
crates/rest_api/src/main.rs (1)
crates/rest_api/src/routes/take_orders.rs (3)
routes(241-243)buy(158-191)sell(206-239)
crates/rest_api/src/routes/take_orders.rs (1)
crates/rest_api/src/main.rs (1)
rocket(43-57)
🔇 Additional comments (13)
packages/webapp/src/lib/constants.ts (1)
1-4: The REMOTE_SETTINGS_URL has already been updated in sync with tauri-app/src/lib/services/loadRemoteSettings.ts to use the same commit hash. Both URLs resolve correctly.crates/rest_api/src/routes/take_orders.rs (6)
10-39: BuyRequest schema and naming are clear.Direction-agnostic
token_in/token_outplusmax_ratioand theexactdefault read cleanly and map well to the buy semantics.
42-72: SellRequest mirrors BuyRequest semantics cleanly.Field naming and docs stay consistent with the buy payload while clarifying sell-specific intent.
205-239: Sell handler wiring and mode selection look correct.The request mapping and response handling align cleanly with the sell endpoint behavior.
241-243: Route export updated to buy/sell endpoints.Clear and minimal change to expose both handlers.
249-295: BuyRequest deserialization tests cover default vs exact behavior.Nice coverage of default
exact = falseand explicitexact = true.
297-343: SellRequest deserialization tests cover default vs exact behavior.Mirrors buy coverage and ensures symmetry of the new payload.
crates/rest_api/src/main.rs (5)
8-24: OpenAPI wiring reflects the new buy/sell schema types.Paths and components align with the updated route surface.
74-108: CORS preflight coverage for buy/sell endpoints looks good.
110-239: Buy endpoint error-path tests remain thorough after field renames.
241-370: Sell endpoint error-path tests remain thorough after field renames.
396-569: OpenAPI spec assertions cover paths, schemas, and field descriptions.tauri-app/src/lib/services/loadRemoteSettings.ts (1)
1-2: The REMOTE_SETTINGS_URL constants in bothtauri-app/src/lib/services/loadRemoteSettings.tsandpackages/webapp/src/lib/constants.tsare correctly aligned with the commit hash7b2a499474ba7ea3d798909e93021fb51aa3b3b9, and the URL is valid and accessible. No changes needed.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Comment on lines
74
to
95
| #[derive(Debug, Clone, Serialize, Deserialize, ToSchema)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct TakeOrdersApiResponse { | ||
| /// Address of the orderbook contract to call | ||
| #[schema(example = "0xd2938e7c9fe3597f78832ce780feb61945c377d7")] | ||
| pub orderbook: String, | ||
| /// ABI-encoded calldata for the takeOrders4 function | ||
| #[schema(example = "0x...")] | ||
| pub calldata: String, | ||
| /// Blended effective price across all selected orders (tokenIn per 1 tokenOut) | ||
| #[schema(example = "0.00045")] | ||
| pub effective_price: String, | ||
| /// Individual prices for each order leg, sorted from best to worst | ||
| #[schema(example = json!(["0.00044", "0.00046"]))] | ||
| pub prices: Vec<String>, | ||
| /// Expected amount of tokenIn to spend based on current quotes | ||
| #[schema(example = "450")] | ||
| pub expected_sell: String, | ||
| /// Maximum tokenIn that could be spent (worst-case based on maxRatio) | ||
| #[schema(example = "500")] | ||
| pub max_sell_cap: String, | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the imports in the file
head -50 crates/rest_api/src/routes/take_orders.rsRepository: rainlanguage/rain.orderbook
Length of output: 2707
🏁 Script executed:
# Search for json! usage and serde_json imports in the file
rg -n "use.*serde_json|use.*json|json!\(" crates/rest_api/src/routes/take_orders.rs | head -20Repository: rainlanguage/rain.orderbook
Length of output: 161
🏁 Script executed:
# Check if there's a prelude or glob import that brings json! into scope
rg -n "use.*\*" crates/rest_api/src/routes/take_orders.rsRepository: rainlanguage/rain.orderbook
Length of output: 92
🏁 Script executed:
# Check Cargo.toml to see if serde_json is a dependency
cat crates/rest_api/Cargo.toml | grep -A 30 "\[dependencies\]"Repository: rainlanguage/rain.orderbook
Length of output: 656
🏁 Script executed:
# Check if there's a prelude or if utoipa brings json! into scope
rg -n "json!" crates/rest_api/src/routes/ | head -20Repository: rainlanguage/rain.orderbook
Length of output: 170
🏁 Script executed:
# Let's check the entire take_orders.rs file around line 87 to see if json! is imported elsewhere
sed -n '1,100p' crates/rest_api/src/routes/take_orders.rs | tail -50Repository: rainlanguage/rain.orderbook
Length of output: 2144
🏁 Script executed:
# Let's check if there's any prelude or macro_use at the crate level
head -20 crates/rest_api/src/lib.rs 2>/dev/null || head -20 crates/rest_api/src/main.rs 2>/dev/nullRepository: rainlanguage/rain.orderbook
Length of output: 644
🏁 Script executed:
# Check if there's a prelude module that might re-export json!
find crates/rest_api/src -name "prelude.rs" -o -name "mod.rs" | xargs rg -l "json!" 2>/dev/nullRepository: rainlanguage/rain.orderbook
Length of output: 55
Qualify the json! macro to make the dependency explicit.
The json! macro from serde_json is not imported at the module level, so the unqualified usage at line 87 would fail to compile. Use the fully qualified path serde_json::json!.
♻️ Suggested tweak
- #[schema(example = json!(["0.00044", "0.00046"]))]
+ #[schema(example = serde_json::json!(["0.00044", "0.00046"]))]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[derive(Debug, Clone, Serialize, Deserialize, ToSchema)] | |
| #[serde(rename_all = "camelCase")] | |
| pub struct TakeOrdersApiResponse { | |
| /// Address of the orderbook contract to call | |
| #[schema(example = "0xd2938e7c9fe3597f78832ce780feb61945c377d7")] | |
| pub orderbook: String, | |
| /// ABI-encoded calldata for the takeOrders4 function | |
| #[schema(example = "0x...")] | |
| pub calldata: String, | |
| /// Blended effective price across all selected orders (tokenIn per 1 tokenOut) | |
| #[schema(example = "0.00045")] | |
| pub effective_price: String, | |
| /// Individual prices for each order leg, sorted from best to worst | |
| #[schema(example = json!(["0.00044", "0.00046"]))] | |
| pub prices: Vec<String>, | |
| /// Expected amount of tokenIn to spend based on current quotes | |
| #[schema(example = "450")] | |
| pub expected_sell: String, | |
| /// Maximum tokenIn that could be spent (worst-case based on maxRatio) | |
| #[schema(example = "500")] | |
| pub max_sell_cap: String, | |
| } | |
| #[derive(Debug, Clone, Serialize, Deserialize, ToSchema)] | |
| #[serde(rename_all = "camelCase")] | |
| pub struct TakeOrdersApiResponse { | |
| /// Address of the orderbook contract to call | |
| #[schema(example = "0xd2938e7c9fe3597f78832ce780feb61945c377d7")] | |
| pub orderbook: String, | |
| /// ABI-encoded calldata for the takeOrders4 function | |
| #[schema(example = "0x...")] | |
| pub calldata: String, | |
| /// Blended effective price across all selected orders (tokenIn per 1 tokenOut) | |
| #[schema(example = "0.00045")] | |
| pub effective_price: String, | |
| /// Individual prices for each order leg, sorted from best to worst | |
| #[schema(example = serde_json::json!(["0.00044", "0.00046"]))] | |
| pub prices: Vec<String>, | |
| /// Expected amount of tokenIn to spend based on current quotes | |
| #[schema(example = "450")] | |
| pub expected_sell: String, | |
| /// Maximum tokenIn that could be spent (worst-case based on maxRatio) | |
| #[schema(example = "500")] | |
| pub max_sell_cap: String, | |
| } |
🤖 Prompt for AI Agents
In `@crates/rest_api/src/routes/take_orders.rs` around lines 74 - 95, The
serde_json json! macro is used unqualified in the TakeOrdersApiResponse struct
(prices field example) which will fail if json! is not imported; update the
macro invocation to the fully qualified path serde_json::json! (i.e., replace
json!(...) with serde_json::json!(...)) so the example compiles without adding a
module-level import, and verify the prices field in TakeOrdersApiResponse still
serializes correctly.
Collaborator
Author
There was a problem hiding this comment.
The json! used in utoipa's #[schema(example = ...)] attributes is not the serde_json::json! macro - it's parsed internally by utoipa's proc-macro at compile time. The code compiles and all tests pass without any use serde_json::json; import in this file. Qualifying it as serde_json::json! is unnecessary and may not work since utoipa handles this syntax internally within its attribute parser.
Contributor
There was a problem hiding this comment.
@findolor, thank you for the clarification! You're absolutely right. Utoipa's schema attributes support json!(...) syntax as part of its own proc-macro parsing, not as a direct invocation of serde_json::json!. My suggestion was incorrect—the code as written is correct and doesn't need any qualification.
I appreciate you taking the time to explain this!
✏️ Learnings added
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2399
File: crates/rest_api/src/routes/take_orders.rs:74-95
Timestamp: 2026-01-16T12:07:15.521Z
Learning: In utoipa's #[schema(...)] attributes (e.g., #[schema(example = json!(...))]), the json! syntax is parsed internally by utoipa's proc-macro at compile time and does not require importing serde_json::json!. The code compiles without any json macro import because utoipa handles this within its attribute parser.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2391
File: crates/rest_api/src/routes/take_orders.rs:75-93
Timestamp: 2026-01-16T11:17:10.568Z
Learning: In crates/rest_api, the take_orders handler uses spawn_blocking with a dedicated single-threaded Tokio runtime per request because RaindexClient contains Rc<RefCell<...>> internally which is not Send, while Rocket requires handler futures to be Send. This pattern is intentional and required to satisfy Rust's thread-safety requirements.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2363
File: crates/common/src/take_orders/preflight.rs:80-107
Timestamp: 2025-12-18T08:19:55.397Z
Learning: In crates/common/src/take_orders/preflight.rs, the simulate_take_orders function intentionally returns () and discards the simulation result data because it only needs to verify that the transaction doesn't revert with the given parameters, without requiring access to output amounts or other returndata.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2391
File: crates/rest_api/src/main.rs:66-201
Timestamp: 2026-01-16T11:13:10.390Z
Learning: In crates/rest_api, happy-path integration tests for the /take-orders endpoint are intentionally omitted because they would require mocking blockchain and subgraph infrastructure, which is considered too complex for the value provided. The test suite focuses on error-path validation instead.
Learnt from: hardyjosh
Repo: rainlanguage/rain.orderbook PR: 2167
File: crates/virtual-raindex/src/engine/take.rs:131-138
Timestamp: 2025-09-30T21:18:01.636Z
Learning: In the virtual-raindex take order flow (crates/virtual-raindex/src/engine/take.rs), balance diffs are written from the order's perspective where the taker is the counterparty: the order's input column receives taker_output (what the taker provides to the order) and the order's output column receives taker_input (what the taker requests from the order).
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2163
File: crates/common/src/raindex_client/orders.rs:738-741
Timestamp: 2025-10-02T19:17:20.332Z
Learning: In crates/common/src/raindex_client/orders.rs, fetch_dotrain_source() is intentionally called in try_from_sg_order for every order conversion because the dotrain source information is needed immediately. A future optimization with local DB logic is planned to eliminate the network round-trip concern.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2198
File: crates/common/src/raindex_client/local_db/query/fetch_store_addresses/mod.rs:7-8
Timestamp: 2025-10-13T12:57:05.900Z
Learning: In the rain.orderbook repository, using unqualified serde helper names (e.g., `#[serde(with = "serde_address")]`) that rely on glob imports is acceptable and does not need to be changed to fully qualified paths.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2008
File: crates/common/src/raindex_client/add_orders.rs:85-86
Timestamp: 2025-07-18T10:31:05.498Z
Learning: In the rainlanguage/rain.orderbook codebase, cfg-guarded imports like `#[cfg(not(target_family = "wasm"))] use super::*;` in test modules are sometimes needed to fix compiler warnings, even when similar imports exist in nested sub-modules. These should not be removed as they serve a specific purpose in the compilation process.
Learnt from: 0xgleb
Repo: rainlanguage/rain.orderbook PR: 1911
File: crates/subgraph/src/utils/mod.rs:1-1
Timestamp: 2025-07-21T16:37:20.599Z
Learning: In the rainlanguage/rain.orderbook codebase, user 0xgleb prefers explicit module imports over re-exporting symbols at higher levels. Specifically for the float constants in crates/subgraph/src/utils/float.rs, they prefer using `utils::float::*` rather than re-exporting with `pub use float::*` in the utils module, as the explicit import makes it clearer what is being imported and why it's needed.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2358
File: crates/common/src/local_db/query/fetch_orders/mod.rs:17-21
Timestamp: 2026-01-16T06:20:40.644Z
Learning: In crates/common/src/local_db/query/fetch_orders/mod.rs, for internal structs like FetchOrdersTokensFilter that are never compared directly (tests compare individual fields instead), PartialEq and Eq derives are not needed. Documentation can be skipped when field names are self-explanatory, especially for internal types.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2000
File: crates/common/src/raindex_client/vaults.rs:183-183
Timestamp: 2025-07-16T10:40:05.717Z
Learning: In the rainlanguage/rain.orderbook codebase, user findolor considers breaking changes from Option<U256> to U256 for required fields like decimals in RaindexVaultToken to be acceptable and safe, even when they affect multiple usage sites across the codebase.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2099
File: crates/common/src/raindex_client/sqlite_web/fetch.rs:1203-1209
Timestamp: 2025-09-25T12:19:33.736Z
Learning: httpmock json_body_partial expects Into<String> (JSON string), while json_body expects Into<serde_json::Value> (JSON object). Using .to_string() with json_body_partial is the correct usage.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1956
File: crates/common/src/fuzz/mod.rs:64-64
Timestamp: 2025-07-04T09:02:57.301Z
Learning: In rainlanguage/rain.orderbook, user findolor prefers to limit type consistency changes to only the parts directly related to the current work scope. For example, when updating chain_id fields from u64 to u32 in fuzz-related code, unrelated files like tauri-app wallet commands can remain as u64 if they serve different purposes and aren't part of the current changes.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1474
File: crates/js_api/src/yaml/mod.rs:31-34
Timestamp: 2025-03-31T13:57:59.660Z
Learning: The OrderbookYaml constructor in crates/js_api/src/yaml/mod.rs does not need early YAML validation. The author prefers to validate YAML only when it's actually used rather than during initialization.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2245
File: crates/common/src/raindex_client/local_db/executor.rs:17-46
Timestamp: 2025-10-19T15:52:58.239Z
Learning: When formatting JsValue errors in Rust wasm code using `{:?}`, the Debug output is sufficiently informative and does not produce unhelpful "[object Object]" strings in practice.
Learnt from: 0xgleb
Repo: rainlanguage/rain.orderbook PR: 1911
File: crates/subgraph/src/types/impls.rs:7-15
Timestamp: 2025-07-21T16:34:31.193Z
Learning: In the rainlanguage/rain.orderbook codebase, user 0xgleb considers breaking changes that remove unsafe default behaviors to be intentional and acceptable. Specifically, the get_decimals() method in crates/subgraph/src/types/impls.rs was intentionally changed to return MissingDecimals error instead of defaulting to 18 decimals, as defaulting to 18 is considered unsafe and should never have been done.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1907
File: packages/orderbook/test/common/test.test.ts:75-77
Timestamp: 2025-06-04T10:21:01.388Z
Learning: The DotrainOrder.create API in packages/orderbook/test/common/test.test.ts is internal and not used directly in consumer applications, so API changes here don't require external breaking change documentation.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:24:53.708Z
Learning: For PR `#1884` in rainlanguage/rain.orderbook, provide a separate comment after each review with PR size statistics in the format: `TOTAL=number ADDITIONS=number DELETIONS=number`, excluding lock files like package-lock.json and cargo.lock.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:24:53.708Z
Learning: For PR `#1884` in rainlanguage/rain.orderbook, provide a separate comment after each review with PR size statistics in the format: `TOTAL=number ADDITIONS=number DELETIONS=number`, excluding lock files like package-lock.json and cargo.lock.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:24:53.708Z
Learning: For PR `#1884` in rainlanguage/rain.orderbook, provide a separate comment after each review with PR size statistics in the format: `TOTAL=number ADDITIONS=number DELETIONS=number`, excluding lock files like package-lock.json and cargo.lock.
Comment on lines
+157
to
+190
| #[post("/take-orders/buy", data = "<request>")] | ||
| pub async fn buy(request: Json<BuyRequest>) -> Result<Json<TakeOrdersApiResponse>, ApiError> { | ||
| let mode = if request.exact { | ||
| TakeOrdersMode::BuyExact | ||
| } else { | ||
| TakeOrdersMode::BuyUpTo | ||
| }; | ||
|
|
||
| let yaml_content = request.yaml_content.clone(); | ||
| let take_request = TakeOrdersRequest { | ||
| taker: request.taker.clone(), | ||
| chain_id: request.chain_id, | ||
| sell_token: request.token_in.clone(), | ||
| buy_token: request.token_out.clone(), | ||
| mode, | ||
| amount: request.amount.clone(), | ||
| price_cap: request.max_ratio.clone(), | ||
| }; | ||
|
|
||
| // RaindexClient contains Rc<RefCell<...>> which is not Send, but Rocket requires | ||
| // Send futures. We use spawn_blocking with a dedicated runtime to run everything | ||
| // on a single thread where Rc<RefCell> is safe. | ||
| let response = tokio::task::spawn_blocking(move || { | ||
| let rt = tokio::runtime::Builder::new_current_thread() | ||
| .enable_all() | ||
| .build() | ||
| .map_err(|e| ApiError::Internal(format!("Failed to create runtime: {}", e)))?; | ||
|
|
||
| rt.block_on(execute_take_orders(yaml_content, take_request)) | ||
| }) | ||
| .await | ||
| .map_err(|e| ApiError::Internal(format!("Task execution failed: {}", e)))??; | ||
|
|
||
| Ok(Json(response)) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract a shared helper for the spawn_blocking wrapper.
Both handlers duplicate the same single-thread runtime/spawn_blocking logic. A small helper keeps the pattern in one place and reduces divergence risk.
♻️ Suggested refactor
async fn execute_take_orders(
yaml_content: String,
request: TakeOrdersRequest,
) -> Result<TakeOrdersApiResponse, ApiError> {
let client = RaindexClient::new(vec![yaml_content], None)?;
@@
Ok(TakeOrdersApiResponse {
orderbook: result.orderbook.to_string(),
calldata: result.calldata.to_string(),
effective_price,
prices: prices?,
expected_sell,
max_sell_cap,
})
}
+
+async fn execute_take_orders_blocking(
+ yaml_content: String,
+ request: TakeOrdersRequest,
+) -> Result<TakeOrdersApiResponse, ApiError> {
+ tokio::task::spawn_blocking(move || {
+ let rt = tokio::runtime::Builder::new_current_thread()
+ .enable_all()
+ .build()
+ .map_err(|e| ApiError::Internal(format!("Failed to create runtime: {}", e)))?;
+
+ rt.block_on(execute_take_orders(yaml_content, request))
+ })
+ .await
+ .map_err(|e| ApiError::Internal(format!("Task execution failed: {}", e)))?
+}
@@
- let response = tokio::task::spawn_blocking(move || {
- let rt = tokio::runtime::Builder::new_current_thread()
- .enable_all()
- .build()
- .map_err(|e| ApiError::Internal(format!("Failed to create runtime: {}", e)))?;
-
- rt.block_on(execute_take_orders(yaml_content, take_request))
- })
- .await
- .map_err(|e| ApiError::Internal(format!("Task execution failed: {}", e)))??;
+ let response = execute_take_orders_blocking(yaml_content, take_request).await?;
@@
- let response = tokio::task::spawn_blocking(move || {
- let rt = tokio::runtime::Builder::new_current_thread()
- .enable_all()
- .build()
- .map_err(|e| ApiError::Internal(format!("Failed to create runtime: {}", e)))?;
-
- rt.block_on(execute_take_orders(yaml_content, take_request))
- })
- .await
- .map_err(|e| ApiError::Internal(format!("Task execution failed: {}", e)))??;
+ let response = execute_take_orders_blocking(yaml_content, take_request).await?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[post("/take-orders/buy", data = "<request>")] | |
| pub async fn buy(request: Json<BuyRequest>) -> Result<Json<TakeOrdersApiResponse>, ApiError> { | |
| let mode = if request.exact { | |
| TakeOrdersMode::BuyExact | |
| } else { | |
| TakeOrdersMode::BuyUpTo | |
| }; | |
| let yaml_content = request.yaml_content.clone(); | |
| let take_request = TakeOrdersRequest { | |
| taker: request.taker.clone(), | |
| chain_id: request.chain_id, | |
| sell_token: request.token_in.clone(), | |
| buy_token: request.token_out.clone(), | |
| mode, | |
| amount: request.amount.clone(), | |
| price_cap: request.max_ratio.clone(), | |
| }; | |
| // RaindexClient contains Rc<RefCell<...>> which is not Send, but Rocket requires | |
| // Send futures. We use spawn_blocking with a dedicated runtime to run everything | |
| // on a single thread where Rc<RefCell> is safe. | |
| let response = tokio::task::spawn_blocking(move || { | |
| let rt = tokio::runtime::Builder::new_current_thread() | |
| .enable_all() | |
| .build() | |
| .map_err(|e| ApiError::Internal(format!("Failed to create runtime: {}", e)))?; | |
| rt.block_on(execute_take_orders(yaml_content, take_request)) | |
| }) | |
| .await | |
| .map_err(|e| ApiError::Internal(format!("Task execution failed: {}", e)))??; | |
| Ok(Json(response)) | |
| #[post("/take-orders/buy", data = "<request>")] | |
| pub async fn buy(request: Json<BuyRequest>) -> Result<Json<TakeOrdersApiResponse>, ApiError> { | |
| let mode = if request.exact { | |
| TakeOrdersMode::BuyExact | |
| } else { | |
| TakeOrdersMode::BuyUpTo | |
| }; | |
| let yaml_content = request.yaml_content.clone(); | |
| let take_request = TakeOrdersRequest { | |
| taker: request.taker.clone(), | |
| chain_id: request.chain_id, | |
| sell_token: request.token_in.clone(), | |
| buy_token: request.token_out.clone(), | |
| mode, | |
| amount: request.amount.clone(), | |
| price_cap: request.max_ratio.clone(), | |
| }; | |
| let response = execute_take_orders_blocking(yaml_content, take_request).await?; | |
| Ok(Json(response)) | |
| } |
🤖 Prompt for AI Agents
In `@crates/rest_api/src/routes/take_orders.rs` around lines 157 - 190, Extract
the duplicated spawn_blocking + single-thread runtime logic into a shared helper
(e.g., run_on_current_thread or run_blocking_single_thread) and call it from buy
(and the other handler) instead of duplicating the block; the helper should
accept a closure or async function that receives cloned yaml_content and
take_request and internally build the current_thread tokio runtime, call
block_on on the provided future, and map runtime or task errors into ApiError
(same messages used today), so you can replace the
tokio::task::spawn_blocking(...) { let rt =
tokio::runtime::Builder::new_current_thread()...
rt.block_on(execute_take_orders(yaml_content, take_request)) } pattern in buy
(and the sibling handler) with a single call to that helper which returns
Result<_, ApiError>.
Collaborator
Author
There was a problem hiding this comment.
We only have two endpoints currently. If we need to add more endpoints in the future that require this pattern, we can revisit extracting a shared helper. For now, the duplication is minimal and the inline code with the explanatory comment is clear enough.
Contributor
There was a problem hiding this comment.
@findolor, understood! That makes sense—with just two endpoints, the inline pattern with the explanatory comment is clear and maintainable. I'll keep this preference in mind for future reviews. 👍
✏️ Learnings added
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2399
File: crates/rest_api/src/routes/take_orders.rs:157-190
Timestamp: 2026-01-16T12:07:10.582Z
Learning: In crates/rest_api, findolor prefers to keep duplicated spawn_blocking + single-thread runtime patterns inline (with explanatory comments) when there are only 2 instances, rather than extracting a helper function prematurely. They will revisit extraction if more endpoints require this pattern in the future.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2391
File: crates/rest_api/src/routes/take_orders.rs:75-93
Timestamp: 2026-01-16T11:17:10.568Z
Learning: In crates/rest_api, the take_orders handler uses spawn_blocking with a dedicated single-threaded Tokio runtime per request because RaindexClient contains Rc<RefCell<...>> internally which is not Send, while Rocket requires handler futures to be Send. This pattern is intentional and required to satisfy Rust's thread-safety requirements.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2363
File: crates/common/src/take_orders/preflight.rs:80-107
Timestamp: 2025-12-18T08:19:55.397Z
Learning: In crates/common/src/take_orders/preflight.rs, the simulate_take_orders function intentionally returns () and discards the simulation result data because it only needs to verify that the transaction doesn't revert with the given parameters, without requiring access to output amounts or other returndata.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2391
File: crates/rest_api/src/main.rs:66-201
Timestamp: 2026-01-16T11:13:10.390Z
Learning: In crates/rest_api, happy-path integration tests for the /take-orders endpoint are intentionally omitted because they would require mocking blockchain and subgraph infrastructure, which is considered too complex for the value provided. The test suite focuses on error-path validation instead.
Learnt from: hardyjosh
Repo: rainlanguage/rain.orderbook PR: 2167
File: crates/virtual-raindex/src/engine/take.rs:131-138
Timestamp: 2025-09-30T21:18:01.636Z
Learning: In the virtual-raindex take order flow (crates/virtual-raindex/src/engine/take.rs), balance diffs are written from the order's perspective where the taker is the counterparty: the order's input column receives taker_output (what the taker provides to the order) and the order's output column receives taker_input (what the taker requests from the order).
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1903
File: crates/cli/src/commands/order/calldata.rs:47-57
Timestamp: 2025-06-17T16:46:19.035Z
Learning: In the CLI command `crates/cli/src/commands/order/calldata.rs`, the user prefers to let lower-level errors from `try_into_call()` bubble up when the RPCs list is empty, rather than adding early validation checks with custom error messages.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1938
File: crates/settings/src/yaml/orderbook.rs:180-199
Timestamp: 2025-06-18T18:24:32.049Z
Learning: In crates/settings/src/yaml/orderbook.rs, the user prefers to avoid refactoring duplicate search logic between get_orderbook_by_address and get_orderbook_by_network_key when there are only 2 functions, indicating they would consider it if more similar functions are added in the future.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1994
File: crates/common/src/raindex_client/vaults.rs:282-292
Timestamp: 2025-07-15T08:01:38.534Z
Learning: In the rainlanguage/rain.orderbook codebase, findolor prefers to avoid concurrency optimizations like using `futures::future::try_join_all` for parallel processing of balance changes, considering such optimizations "not that critical at the moment" when the performance impact is minimal.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1950
File: crates/common/src/raindex_client/orders.rs:301-301
Timestamp: 2025-06-24T13:36:28.797Z
Learning: In the RaindexClient codebase, when Arc::new(RwLock::new(self.clone())) is used (e.g., in get_orders and get_order_by_hash methods), this creates new Arc/RwLock wrappers around a cloned RaindexClient, but the underlying client data is functionally the same. This pattern is used to provide the correct Arc<RwLock<RaindexClient>> type expected by RaindexOrder::try_from_sg_order() method.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2370
File: crates/common/src/raindex_client/vaults.rs:896-973
Timestamp: 2026-01-07T15:38:42.640Z
Learning: In the rainlanguage/rain.orderbook codebase, findolor considers duplication acceptable when dealing with separate subgraph types (like SgDeposit, SgWithdrawal, SgClearBounty) in match arms, as extracting a helper function would require passing 10+ individual fields and would be less clean than self-contained match arms. This structural duplication is preferred over complex helper functions.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2344
File: crates/common/src/local_db/pipeline/runner/mod.rs:18-31
Timestamp: 2025-12-03T10:40:25.429Z
Learning: In `crates/common/src/local_db/pipeline/runner/mod.rs`, the `TargetSuccess` struct does not need separate `ob_id` or `orderbook_key` fields because the contained `SyncOutcome` already includes orderbook identification information such as chain_id and orderbook_address. This avoids redundant data duplication.
Learnt from: 0xgleb
Repo: rainlanguage/rain.orderbook PR: 1859
File: crates/quote/src/quote_debug.rs:472-492
Timestamp: 2025-05-20T10:20:08.206Z
Learning: In the Rain Orderbook codebase, the `#[tokio::test(flavor = "multi_thread")]` annotation is specifically needed for tests that use `LocalEvm`, not just for consistency across all async tests.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2012
File: crates/js_api/src/registry.rs:485-512
Timestamp: 2025-07-30T07:41:39.271Z
Learning: In crates/js_api/src/registry.rs, findolor considers the current concurrent fetching of order files using futures::future::join_all without concurrency limits to be acceptable, preferring the simple approach over adding concurrency limiting mechanisms for the DotrainRegistry implementation.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2099
File: crates/common/src/hyper_rpc.rs:3-7
Timestamp: 2025-08-26T14:52:37.000Z
Learning: In the rain.orderbook codebase, creating new reqwest::Client instances per request in HyperRpcClient is not considered an issue by the maintainers, despite potential performance benefits of client reuse.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2000
File: crates/common/src/raindex_client/vaults.rs:183-183
Timestamp: 2025-07-16T10:40:05.717Z
Learning: In the rainlanguage/rain.orderbook codebase, user findolor considers breaking changes from Option<U256> to U256 for required fields like decimals in RaindexVaultToken to be acceptable and safe, even when they affect multiple usage sites across the codebase.
Learnt from: 0xgleb
Repo: rainlanguage/rain.orderbook PR: 1911
File: crates/subgraph/src/types/impls.rs:7-15
Timestamp: 2025-07-21T16:34:31.193Z
Learning: In the rainlanguage/rain.orderbook codebase, user 0xgleb considers breaking changes that remove unsafe default behaviors to be intentional and acceptable. Specifically, the get_decimals() method in crates/subgraph/src/types/impls.rs was intentionally changed to return MissingDecimals error instead of defaulting to 18 decimals, as defaulting to 18 is considered unsafe and should never have been done.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1956
File: crates/common/src/fuzz/mod.rs:64-64
Timestamp: 2025-07-04T09:02:57.301Z
Learning: In rainlanguage/rain.orderbook, user findolor prefers to limit type consistency changes to only the parts directly related to the current work scope. For example, when updating chain_id fields from u64 to u32 in fuzz-related code, unrelated files like tauri-app wallet commands can remain as u64 if they serve different purposes and aren't part of the current changes.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2358
File: crates/common/src/local_db/query/fetch_orders/mod.rs:17-21
Timestamp: 2026-01-16T06:20:40.644Z
Learning: In crates/common/src/local_db/query/fetch_orders/mod.rs, for internal structs like FetchOrdersTokensFilter that are never compared directly (tests compare individual fields instead), PartialEq and Eq derives are not needed. Documentation can be skipped when field names are self-explanatory, especially for internal types.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1907
File: packages/orderbook/test/common/test.test.ts:75-77
Timestamp: 2025-06-04T10:21:01.388Z
Learning: The DotrainOrder.create API in packages/orderbook/test/common/test.test.ts is internal and not used directly in consumer applications, so API changes here don't require external breaking change documentation.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:24:53.708Z
Learning: For PR `#1884` in rainlanguage/rain.orderbook, provide a separate comment after each review with PR size statistics in the format: `TOTAL=number ADDITIONS=number DELETIONS=number`, excluding lock files like package-lock.json and cargo.lock.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:24:53.708Z
Learning: For PR `#1884` in rainlanguage/rain.orderbook, provide a separate comment after each review with PR size statistics in the format: `TOTAL=number ADDITIONS=number DELETIONS=number`, excluding lock files like package-lock.json and cargo.lock.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:24:53.708Z
Learning: For PR `#1884` in rainlanguage/rain.orderbook, provide a separate comment after each review with PR size statistics in the format: `TOTAL=number ADDITIONS=number DELETIONS=number`, excluding lock files like package-lock.json and cargo.lock.
Collaborator
Author
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
Combines: - Enum response type (NeedsApproval/Ready) with approval checking - Split buy/sell endpoints with renamed fields (tokenIn/tokenOut, maxRatio) - OpenAPI examples for both response variants on both endpoints - Updated tests for new schema structure
hardyjosh
approved these changes
Feb 11, 2026
Contributor
There was a problem hiding this comment.
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)
crates/rest_api/src/routes/take_orders.rs (1)
143-208: 🧹 Nitpick | 🔵 TrivialRepeated error-mapping boilerplate in
execute_take_orders.The
map_err(|e| ApiError::Raindex(…::Float(e)))closure appears four times (lines 153-157, 168-172, 178-181, 186-190, 192-196). A small helper would reduce noise:♻️ Suggested refactor
+fn float_err(e: impl std::fmt::Display) -> ApiError { + ApiError::Raindex(rain_orderbook_common::raindex_client::RaindexError::Float(e)) +} + async fn execute_take_orders( yaml_content: String, request: TakeOrdersRequest, ) -> Result<TakeOrdersApiResponse, ApiError> { ... - let amount = approval_info.amount.format().map_err(|e| { - ApiError::Raindex(rain_orderbook_common::raindex_client::RaindexError::Float( - e, - )) - })?; + let amount = approval_info.amount.format().map_err(float_err)?;
🤖 Fix all issues with AI agents
In `@crates/rest_api/src/routes/take_orders.rs`:
- Around line 378-424: Add unit tests that assert the `exact` boolean maps to
the correct TakeOrdersMode for both endpoints: when parsing a buy request
`exact: true` it must yield TakeOrdersMode::BuyExact and `exact: false` yields
BuyUpTo; when parsing a sell request `exact: true` it must yield
TakeOrdersMode::SpendExact and `exact: false` yields SpendUpTo. To implement,
either (a) extract the mode-selection logic used in the buy/sell handlers into a
small helper (e.g., map_buy_mode(exact: bool) -> TakeOrdersMode and
map_sell_mode(exact: bool) -> TakeOrdersMode) and add unit tests for those
helpers, or (b) add unit tests that construct the same request struct used by
the handlers (BuyRequest/SellRequest or TakeOrdersRequest) and assert the
handler/mapping produces the expected TakeOrdersMode; reference the
TakeOrdersMode enum and the buy/sell request parsing logic in take_orders.rs
when adding tests.
Comment on lines
378
to
424
| #[test] | ||
| fn test_request_deserialization_buy_up_to() { | ||
| fn test_buy_request_deserialization() { | ||
| let json = r#"{ | ||
| "yamlContent": "version: 1", | ||
| "taker": "0x1111111111111111111111111111111111111111", | ||
| "chainId": 1, | ||
| "sellToken": "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", | ||
| "buyToken": "0xbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", | ||
| "mode": "buyUpTo", | ||
| "tokenIn": "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", | ||
| "tokenOut": "0xbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", | ||
| "amount": "100", | ||
| "priceCap": "2.5" | ||
| "maxRatio": "2.5" | ||
| }"#; | ||
|
|
||
| let request: TakeOrdersApiRequest = serde_json::from_str(json).unwrap(); | ||
| let request: BuyRequest = serde_json::from_str(json).unwrap(); | ||
|
|
||
| assert_eq!(request.yaml_content, "version: 1"); | ||
| assert_eq!(request.taker, "0x1111111111111111111111111111111111111111"); | ||
| assert_eq!(request.chain_id, 1); | ||
| assert_eq!( | ||
| request.sell_token, | ||
| request.token_in, | ||
| "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" | ||
| ); | ||
| assert_eq!( | ||
| request.buy_token, | ||
| request.token_out, | ||
| "0xbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" | ||
| ); | ||
| assert_eq!(request.amount, "100"); | ||
| assert_eq!(request.price_cap, "2.5"); | ||
| assert!(matches!(request.mode, TakeOrdersMode::BuyUpTo)); | ||
| assert_eq!(request.max_ratio, "2.5"); | ||
| assert!(!request.exact); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_request_deserialization_buy_exact() { | ||
| fn test_buy_request_deserialization_exact() { | ||
| let json = r#"{ | ||
| "yamlContent": "version: 1", | ||
| "taker": "0x1111111111111111111111111111111111111111", | ||
| "chainId": 137, | ||
| "sellToken": "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", | ||
| "buyToken": "0xbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", | ||
| "mode": "buyExact", | ||
| "tokenIn": "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", | ||
| "tokenOut": "0xbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", | ||
| "amount": "50.5", | ||
| "priceCap": "1.0" | ||
| "maxRatio": "1.0", | ||
| "exact": true | ||
| }"#; | ||
|
|
||
| let request: TakeOrdersApiRequest = serde_json::from_str(json).unwrap(); | ||
| let request: BuyRequest = serde_json::from_str(json).unwrap(); | ||
|
|
||
| assert!(matches!(request.mode, TakeOrdersMode::BuyExact)); | ||
| assert!(request.exact); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding unit tests that verify exact flag maps to the correct TakeOrdersMode.
The deserialization tests confirm field parsing, but there's no unit-level assertion that exact: true on /buy produces BuyExact (not SpendExact) and exact: true on /sell produces SpendExact (not BuyExact). Since the mode mapping is the core differentiator between the two endpoints, a focused test would guard against accidental swap.
One approach: extract the mode-selection logic into a small testable helper, or use Rocket's local client to inspect the resulting TakeOrdersRequest (though that's harder without mocks). Even a simple standalone test like:
#[test]
fn test_buy_mode_mapping() {
assert_eq!(
if true { TakeOrdersMode::BuyExact } else { TakeOrdersMode::BuyUpTo },
TakeOrdersMode::BuyExact
);
assert_eq!(
if false { TakeOrdersMode::BuyExact } else { TakeOrdersMode::BuyUpTo },
TakeOrdersMode::BuyUpTo
);
}
#[test]
fn test_sell_mode_mapping() {
assert_eq!(
if true { TakeOrdersMode::SpendExact } else { TakeOrdersMode::SpendUpTo },
TakeOrdersMode::SpendExact
);
assert_eq!(
if false { TakeOrdersMode::SpendExact } else { TakeOrdersMode::SpendUpTo },
TakeOrdersMode::SpendUpTo
);
}would make the intent explicit, though extracting the mapping into a method and testing that method would be stronger.
Also applies to: 426-472
🤖 Prompt for AI Agents
In `@crates/rest_api/src/routes/take_orders.rs` around lines 378 - 424, Add unit
tests that assert the `exact` boolean maps to the correct TakeOrdersMode for
both endpoints: when parsing a buy request `exact: true` it must yield
TakeOrdersMode::BuyExact and `exact: false` yields BuyUpTo; when parsing a sell
request `exact: true` it must yield TakeOrdersMode::SpendExact and `exact:
false` yields SpendUpTo. To implement, either (a) extract the mode-selection
logic used in the buy/sell handlers into a small helper (e.g.,
map_buy_mode(exact: bool) -> TakeOrdersMode and map_sell_mode(exact: bool) ->
TakeOrdersMode) and add unit tests for those helpers, or (b) add unit tests that
construct the same request struct used by the handlers (BuyRequest/SellRequest
or TakeOrdersRequest) and assert the handler/mapping produces the expected
TakeOrdersMode; reference the TakeOrdersMode enum and the buy/sell request
parsing logic in take_orders.rs when adding tests.
Contributor
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
Contributor
|
Tip For best results, initiate chat on the files or code changes. SIZE=L 🧠 Learnings used |
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.
Chained PRs
Motivation
See issues:
The original
/take-ordersendpoint with a mode enum was less intuitive for API consumers. Splitting into separate/take-orders/buyand/take-orders/sellroutes provides a cleaner, more RESTful API design that better communicates intent.Solution
/take-ordersendpoint with/take-orders/buyand/take-orders/sellroutessellToken/buyToken→tokenIn/tokenOut(direction-agnostic naming)priceCap→maxRatio(more descriptive)TakeOrdersModeenum with simplerexactboolean flag:/take-orders/buy+exact: false→ BuyUpTo/take-orders/buy+exact: true→ BuyExact/take-orders/sell+exact: false→ SpendUpTo/take-orders/sell+exact: true→ SpendExactChecks
By submitting this for review, I'm confirming I've done the following:
fix #2393
Summary by CodeRabbit
Refactor
Documentation