Implement preflight simulation logic for take orders functionality in the SDK #2363
Conversation
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
findolor
changed the base branch from
main
to
2025-12-12-take-orders-sdk-impl
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR refactors the take-orders API from multi-parameter to request-driven design, introduces preflight validation (balance/allowance checks and simulation), and adds corresponding error handling. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RaindexClient
participant Preflight as Preflight<br/>Validation
participant Provider as RPC<br/>Provider
participant Orderbook
participant ERC20
Client->>RaindexClient: get_take_orders_calldata(TakeOrdersRequest)
RaindexClient->>RaindexClient: Parse request<br/>(validate addresses, floats)
RaindexClient->>RaindexClient: Fetch orders from subgraph
loop Preflight Retry Loop
RaindexClient->>Preflight: setup for best candidate
Preflight->>Provider: mk_read_provider()
Preflight->>ERC20: check_taker_balance_and_allowance()
ERC20-->>Preflight: balance/allowance check result
alt Balance/Allowance Valid
Preflight->>Provider: simulate_take_orders()
Provider->>Orderbook: call takeOrders3 (simulation)
alt Simulation Succeeds
Preflight-->>RaindexClient: ✓ preflight passed
else Simulation Fails
Preflight->>RaindexClient: ✗ preflight failed,<br/>find failing order index
RaindexClient->>RaindexClient: Remove failing candidate<br/>and retry with next
end
else Preflight Error
RaindexClient->>RaindexClient: Return PreflightError
end
end
RaindexClient->>RaindexClient: Build calldata & result
RaindexClient-->>Client: TakeOrdersCalldataResult<br/>(via accessors)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
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
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
crates/common/src/raindex_client/mod.rs(2 hunks)crates/common/src/raindex_client/take_orders/e2e_tests.rs(5 hunks)crates/common/src/raindex_client/take_orders/mod.rs(2 hunks)crates/common/src/raindex_client/take_orders/request.rs(2 hunks)crates/common/src/raindex_client/take_orders/result.rs(4 hunks)crates/common/src/take_orders/mod.rs(1 hunks)crates/common/src/take_orders/preflight.rs(1 hunks)packages/orderbook/test/js_api/raindexClient.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
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/common/src/raindex_client/mod.rscrates/common/src/take_orders/mod.rscrates/common/src/raindex_client/take_orders/request.rscrates/common/src/raindex_client/take_orders/result.rscrates/common/src/raindex_client/take_orders/e2e_tests.rscrates/common/src/raindex_client/take_orders/mod.rscrates/common/src/take_orders/preflight.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/common/src/raindex_client/mod.rscrates/common/src/take_orders/mod.rscrates/common/src/raindex_client/take_orders/request.rscrates/common/src/raindex_client/take_orders/result.rscrates/common/src/raindex_client/take_orders/e2e_tests.rscrates/common/src/raindex_client/take_orders/mod.rscrates/common/src/take_orders/preflight.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/common/src/raindex_client/mod.rscrates/common/src/take_orders/mod.rscrates/common/src/raindex_client/take_orders/request.rscrates/common/src/raindex_client/take_orders/result.rscrates/common/src/raindex_client/take_orders/e2e_tests.rscrates/common/src/raindex_client/take_orders/mod.rscrates/common/src/take_orders/preflight.rs
packages/orderbook/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/orderbook/**/*.{ts,tsx,js,jsx}: For Orderbook TypeScript (packages/orderbook), run lints usingnix develop -c npm run check -w @rainlanguage/orderbook
For Orderbook TypeScript (packages/orderbook), run tests usingnix develop -c npm run test -w @rainlanguage/orderbook
Files:
packages/orderbook/test/js_api/raindexClient.test.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/orderbook/test/js_api/raindexClient.test.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/orderbook/test/js_api/raindexClient.test.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript/Svelte: run tests with
nix develop -c npm run test(Vitest). Name test files*.test.tsor*.spec.ts
Files:
packages/orderbook/test/js_api/raindexClient.test.ts
🧠 Learnings (48)
📓 Common learnings
Learnt from: brusherru
Repo: rainlanguage/rain.orderbook PR: 2044
File: tauri-app/src/routes/orders/[chainId]-[orderbook]-[orderHash]/+page.svelte:76-78
Timestamp: 2025-07-31T19:03:56.594Z
Learning: In the rainlanguage/rain.orderbook project, when the Tauri app has issues preventing proper testing, the team prefers to defer Tauri-specific implementations to separate PRs rather than including untested code, especially for financial operations like withdrawals.
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.
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:26:31.566Z
Learning: For PR #1884 and future PRs in rainlanguage/rain.orderbook repository, provide PR size reports in a separate comment with ONLY the exact format "TOTAL=number\nADDITIONS=number\nDELETIONS=number", without any additional text or explanation. Reports should exclude irrelevant files such as lock files (package-lock.json, yarn.lock, pnpm-lock.yaml, cargo.lock).
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T22:14:22.646Z
Learning: For PR #1884 in rainlanguage/rain.orderbook, I must consistently report the PR size after EVERY new commit in the exact format "TOTAL=number\nADDITIONS=number\nDELETIONS=number" without any additional text or formatting blocks. The report must exclude lock files (package-lock.json, yarn.lock, pnpm-lock.yaml, cargo.lock) and can be included either as part of the review or as a separate comment.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T22:14:22.646Z
Learning: For PR #1884 in rainlanguage/rain.orderbook, I must consistently report the PR size after EVERY new commit in the exact format "TOTAL=number\nADDITIONS=number\nDELETIONS=number" without any formatting blocks. The report must exclude lock files (package-lock.json, yarn.lock, pnpm-lock.yaml, cargo.lock) and can be included either as part of the review or as a separate comment.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T22:47:21.927Z
Learning: For PR #1884 in rainlanguage/rain.orderbook, the PR size report must always be placed at the very beginning of any comment, before any learning used section or other content.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T22:47:21.927Z
Learning: For PR #1884 in rainlanguage/rain.orderbook, the PR size report must always be placed at the very beginning of any comment, before any learning used section or other content.
📚 Learning: 2025-08-01T07:35:13.418Z
Learnt from: brusherru
Repo: rainlanguage/rain.orderbook PR: 2036
File: crates/js_api/src/filters/errors.rs:39-46
Timestamp: 2025-08-01T07:35:13.418Z
Learning: In the Rain Orderbook project's PersistentFilterStoreError (crates/js_api/src/filters/errors.rs), the Display implementation already provides user-friendly error messages, so using err.to_string() for both msg and readable_msg in the WasmEncodedError conversion is appropriate and doesn't require separate readable message handling.
Applied to files:
crates/common/src/raindex_client/mod.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/common/src/raindex_client/mod.rscrates/common/src/raindex_client/take_orders/request.rscrates/common/src/raindex_client/take_orders/result.rscrates/common/src/raindex_client/take_orders/e2e_tests.rscrates/common/src/raindex_client/take_orders/mod.rscrates/common/src/take_orders/preflight.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/common/src/raindex_client/mod.rscrates/common/src/raindex_client/take_orders/request.rscrates/common/src/raindex_client/take_orders/result.rscrates/common/src/raindex_client/take_orders/e2e_tests.rscrates/common/src/raindex_client/take_orders/mod.rscrates/common/src/take_orders/preflight.rs
📚 Learning: 2025-05-09T05:28:22.089Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1744
File: crates/js_api/src/subgraph/order.rs:109-114
Timestamp: 2025-05-09T05:28:22.089Z
Learning: In the rain.orderbook project, SubgraphError contains or converts from OrderbookSubgraphClientError, so using OrderbookSubgraphClientError in error creation followed by a conversion to SubgraphError (via ? operator) is acceptable.
Applied to files:
crates/common/src/raindex_client/mod.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/common/src/raindex_client/mod.rscrates/common/src/raindex_client/take_orders/request.rscrates/common/src/raindex_client/take_orders/e2e_tests.rscrates/common/src/raindex_client/take_orders/mod.rs
📚 Learning: 2025-07-11T08:37:24.423Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1983
File: crates/settings/src/gui.rs:965-973
Timestamp: 2025-07-11T08:37:24.423Z
Learning: In the rainlanguage/rain.orderbook project, user findolor prefers to keep user-facing error messages generic and user-friendly rather than technically specific. For example, using "a valid number" instead of "a valid u8 (0-255)" in GUI validation error messages to maintain simplicity for end users.
Applied to files:
crates/common/src/raindex_client/mod.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:
packages/orderbook/test/js_api/raindexClient.test.tscrates/common/src/raindex_client/take_orders/e2e_tests.rs
📚 Learning: 2025-11-25T16:50:11.197Z
Learnt from: CR
Repo: rainlanguage/rain.orderbook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-25T16:50:11.197Z
Learning: Applies to packages/orderbook/**/*.{ts,tsx,js,jsx} : For Orderbook TypeScript (`packages/orderbook`), run tests using `nix develop -c npm run test -w rainlanguage/orderbook`
Applied to files:
packages/orderbook/test/js_api/raindexClient.test.ts
📚 Learning: 2025-08-02T02:34:32.237Z
Learnt from: brusherru
Repo: rainlanguage/rain.orderbook PR: 2036
File: crates/js_api/src/filters/raindex_filter_store.rs:446-533
Timestamp: 2025-08-02T02:34:32.237Z
Learning: In the rainlanguage/rain.orderbook project's RaindexFilterStore WASM tests (crates/js_api/src/filters/raindex_filter_store.rs), brusherru decided to focus on testing only methods without side effects (that don't use web_sys) due to difficulties with mocking localStorage and window APIs in the WASM test environment. This pragmatic approach tests pure logic separately from browser integration.
Applied to files:
packages/orderbook/test/js_api/raindexClient.test.tscrates/common/src/raindex_client/take_orders/request.rscrates/common/src/raindex_client/take_orders/result.rscrates/common/src/raindex_client/take_orders/e2e_tests.rscrates/common/src/raindex_client/take_orders/mod.rscrates/common/src/take_orders/preflight.rs
📚 Learning: 2025-04-04T11:25:21.518Z
Learnt from: hardingjam
Repo: rainlanguage/rain.orderbook PR: 1559
File: packages/ui-components/src/__tests__/OrderOrVaultHash.test.ts:94-94
Timestamp: 2025-04-04T11:25:21.518Z
Learning: In the rain.orderbook project, minimal test fixtures are preferred over complete mocks that implement the entire interface. Type casting (e.g., `as unknown as SgVault`) is an acceptable approach to maintain both minimal fixtures and TypeScript type compatibility.
Applied to files:
packages/orderbook/test/js_api/raindexClient.test.tscrates/common/src/raindex_client/take_orders/e2e_tests.rs
📚 Learning: 2025-04-08T12:56:03.272Z
Learnt from: hardingjam
Repo: rainlanguage/rain.orderbook PR: 1512
File: packages/ui-components/src/__tests__/OrderDetail.test.ts:125-143
Timestamp: 2025-04-08T12:56:03.272Z
Learning: The OrderDetail component in the Rain orderbook UI doesn't currently have error handling tests, but issue #1605 has been created to address this in the future.
Applied to files:
packages/orderbook/test/js_api/raindexClient.test.ts
📚 Learning: 2025-06-08T18:43:51.842Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1891
File: packages/webapp/src/routes/deploy/[strategyName]/[deploymentKey]/page.test.ts:66-80
Timestamp: 2025-06-08T18:43:51.842Z
Learning: In the rain.orderbook webapp test files, when mocking objects like the transaction manager, it's acceptable to use simple empty objects with ts-expect-error comments rather than providing complete mock implementations with all properties and methods.
Applied to files:
packages/orderbook/test/js_api/raindexClient.test.ts
📚 Learning: 2025-07-04T09:14:35.748Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1956
File: packages/ui-components/src/__tests__/VaultDetail.test.ts:177-181
Timestamp: 2025-07-04T09:14:35.748Z
Learning: In the rain.orderbook project, maintainer findolor is fine with mutating mock data directly in test files using ts-expect-error comments, preferring this pragmatic approach over immutable patterns like spread operators for test data updates.
Applied to files:
packages/orderbook/test/js_api/raindexClient.test.ts
📚 Learning: 2025-07-31T19:34:11.716Z
Learnt from: brusherru
Repo: rainlanguage/rain.orderbook PR: 2044
File: crates/common/src/raindex_client/vaults_list.rs:363-423
Timestamp: 2025-07-31T19:34:11.716Z
Learning: In the rainlanguage/rain.orderbook project, for WASM-exposed functionality like VaultsList, the team prefers to keep comprehensive tests in the non-WASM environment due to the complexity of recreating objects like RaindexVaults in WASM. WASM tests focus on basic functionality and error cases since the WASM code reuses the already-tested non-WASM implementation.
Applied to files:
packages/orderbook/test/js_api/raindexClient.test.tscrates/common/src/raindex_client/take_orders/request.rscrates/common/src/raindex_client/take_orders/e2e_tests.rscrates/common/src/raindex_client/take_orders/mod.rscrates/common/src/take_orders/preflight.rs
📚 Learning: 2025-07-31T19:03:56.594Z
Learnt from: brusherru
Repo: rainlanguage/rain.orderbook PR: 2044
File: tauri-app/src/routes/orders/[chainId]-[orderbook]-[orderHash]/+page.svelte:76-78
Timestamp: 2025-07-31T19:03:56.594Z
Learning: In the rainlanguage/rain.orderbook project, when the Tauri app has issues preventing proper testing, the team prefers to defer Tauri-specific implementations to separate PRs rather than including untested code, especially for financial operations like withdrawals.
Applied to files:
packages/orderbook/test/js_api/raindexClient.test.ts
📚 Learning: 2025-04-08T09:18:46.653Z
Learnt from: hardingjam
Repo: rainlanguage/rain.orderbook PR: 1597
File: packages/ui-components/src/__tests__/OrderDetail.test.ts:120-120
Timestamp: 2025-04-08T09:18:46.653Z
Learning: In test files for the Rain Orderbook project, it's acceptable to bypass TypeScript's strict typing using constructs like `as unknown as [Type]` to create simplified mock objects that don't need to implement the entire interface.
Applied to files:
packages/orderbook/test/js_api/raindexClient.test.ts
📚 Learning: 2025-07-21T16:37:20.599Z
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.
Applied to files:
crates/common/src/take_orders/mod.rs
📚 Learning: 2025-05-14T05:13:59.713Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1744
File: crates/subgraph/src/orderbook_client.rs:44-46
Timestamp: 2025-05-14T05:13:59.713Z
Learning: In the rain.orderbook project, WASM dependencies are intentionally made available in non-WASM targets to facilitate testing of WASM-related functionality, so conditional compilation guards like `#[cfg(target_family = "wasm")]` should not be added to imports or implementations that may be needed for tests.
Applied to files:
crates/common/src/raindex_client/take_orders/request.rscrates/common/src/raindex_client/take_orders/result.rscrates/common/src/raindex_client/take_orders/mod.rs
📚 Learning: 2025-05-14T05:52:04.270Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1715
File: crates/js_api/src/common/mod.rs:15-22
Timestamp: 2025-05-14T05:52:04.270Z
Learning: The project doesn't require `#[repr(transparent)]` for newtype wrappers in WASM contexts such as `AddOrderCalldata` and `RemoveOrderCalldata` as the current implementation is working as expected without it.
Applied to files:
crates/common/src/raindex_client/take_orders/request.rscrates/common/src/raindex_client/take_orders/result.rscrates/common/src/raindex_client/take_orders/e2e_tests.rscrates/common/src/raindex_client/take_orders/mod.rs
📚 Learning: 2025-05-17T15:32:28.733Z
Learnt from: 0xgleb
Repo: rainlanguage/rain.orderbook PR: 1790
File: tauri-app/src-tauri/src/commands/vault.rs:67-67
Timestamp: 2025-05-17T15:32:28.733Z
Learning: For the PR focused on testing Tauri commands::order module, the generic type parameter R: Runtime was selectively added where needed for the PR scope, applying the changes primarily to order.rs and related files while leaving other modules like vault.rs for potential future refactoring.
Applied to files:
crates/common/src/raindex_client/take_orders/request.rscrates/common/src/raindex_client/take_orders/e2e_tests.rscrates/common/src/raindex_client/take_orders/mod.rscrates/common/src/take_orders/preflight.rs
📚 Learning: 2025-08-01T09:07:20.383Z
Learnt from: brusherru
Repo: rainlanguage/rain.orderbook PR: 2044
File: packages/orderbook/README.md:186-189
Timestamp: 2025-08-01T09:07:20.383Z
Learning: In the rainlanguage/rain.orderbook project, Rust methods on structs like RaindexVaultsList are exported as JavaScript getters in WASM bindings using #[wasm_bindgen(getter)]. This means while the Rust code uses method calls like items(), the JavaScript/WASM API exposes them as property access like .items. The README.md correctly documents the JavaScript API surface, not the Rust implementation details.
Applied to files:
crates/common/src/raindex_client/take_orders/request.rscrates/common/src/raindex_client/take_orders/result.rscrates/common/src/raindex_client/take_orders/mod.rs
📚 Learning: 2025-07-18T10:31:05.498Z
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.
Applied to files:
crates/common/src/raindex_client/take_orders/request.rscrates/common/src/raindex_client/take_orders/result.rscrates/common/src/raindex_client/take_orders/e2e_tests.rscrates/common/src/raindex_client/take_orders/mod.rscrates/common/src/take_orders/preflight.rs
📚 Learning: 2025-04-30T09:28:36.960Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1715
File: crates/js_api/src/common/mod.rs:111-118
Timestamp: 2025-04-30T09:28:36.960Z
Learning: In the rain.orderbook repository, the WASM tests are already properly configured with conditional compilation using `#[cfg(target_family = "wasm")]` and `#[cfg(not(target_family = "wasm"))]`, and don't require additional `wasm_bindgen_test_configure!(run_in_browser)` directives.
Applied to files:
crates/common/src/raindex_client/take_orders/request.rscrates/common/src/raindex_client/take_orders/result.rscrates/common/src/raindex_client/take_orders/mod.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/common/src/raindex_client/take_orders/request.rscrates/common/src/raindex_client/take_orders/e2e_tests.rscrates/common/src/raindex_client/take_orders/mod.rs
📚 Learning: 2025-12-17T13:17:58.487Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2360
File: crates/common/src/test_helpers.rs:329-367
Timestamp: 2025-12-17T13:17:58.487Z
Learning: In the rainlanguage/rain.orderbook codebase, findolor considers hardcoded values (like decimals) acceptable in test helper functions such as deposit_to_orderbook in crates/common/src/test_helpers.rs, as test infrastructure doesn't need the same level of parameterization as production code.
Applied to files:
crates/common/src/raindex_client/take_orders/request.rscrates/common/src/raindex_client/take_orders/result.rscrates/common/src/raindex_client/take_orders/e2e_tests.rscrates/common/src/raindex_client/take_orders/mod.rscrates/common/src/take_orders/preflight.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/common/src/raindex_client/take_orders/result.rscrates/common/src/raindex_client/take_orders/e2e_tests.rs
📚 Learning: 2025-05-09T05:21:40.234Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1744
File: crates/js_api/src/subgraph/vault.rs:19-37
Timestamp: 2025-05-09T05:21:40.234Z
Learning: In the rain.orderbook project, the developer prefers to use tuple structs for WASM wrappers (like VaultCalldataResult, GetVaultsResult) rather than structs with named fields.
Applied to files:
crates/common/src/raindex_client/take_orders/result.rs
📚 Learning: 2025-08-02T03:55:25.215Z
Learnt from: brusherru
Repo: rainlanguage/rain.orderbook PR: 2036
File: packages/orderbook/test/js_api/filters.test.ts:19-36
Timestamp: 2025-08-02T03:55:25.215Z
Learning: In the rainlanguage/rain.orderbook project's WASM tests, the pattern of chaining `.value!` calls on WASM result types (like from VaultsFilterBuilder methods) is the established and preferred approach for handling WASM results, and should not be refactored into intermediate variables as it would add unnecessary verbosity without improving the code.
Applied to files:
crates/common/src/raindex_client/take_orders/result.rscrates/common/src/raindex_client/take_orders/e2e_tests.rscrates/common/src/raindex_client/take_orders/mod.rscrates/common/src/take_orders/preflight.rs
📚 Learning: 2025-05-19T13:40:56.080Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1858
File: crates/subgraph/src/orderbook_client/mod.rs:54-58
Timestamp: 2025-05-19T13:40:56.080Z
Learning: The `wasm_bindgen_utils` crate in the Rain Orderbook project handles conditional compilation for `JsValue` and `JsError` internally, allowing `impl From<Error> for JsValue` to work on non-WASM targets without explicit cfg guards.
Applied to files:
crates/common/src/raindex_client/take_orders/result.rs
📚 Learning: 2025-06-17T16:46:19.035Z
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.
Applied to files:
crates/common/src/raindex_client/take_orders/result.rscrates/common/src/raindex_client/take_orders/e2e_tests.rscrates/common/src/raindex_client/take_orders/mod.rscrates/common/src/take_orders/preflight.rs
📚 Learning: 2025-05-14T05:51:50.277Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1715
File: crates/js_api/src/common/mod.rs:55-59
Timestamp: 2025-05-14T05:51:50.277Z
Learning: In the Rain Orderbook project, the error handling for WASM exported functions uses WasmEncodedError which includes both the original error message and a human-readable version, making direct modification of the JsValue conversion unnecessary.
Applied to files:
crates/common/src/raindex_client/take_orders/result.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/common/src/raindex_client/take_orders/result.rscrates/common/src/raindex_client/take_orders/e2e_tests.rscrates/common/src/raindex_client/take_orders/mod.rscrates/common/src/take_orders/preflight.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/common/src/raindex_client/take_orders/result.rscrates/common/src/raindex_client/take_orders/e2e_tests.rscrates/common/src/take_orders/preflight.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/common/src/raindex_client/take_orders/e2e_tests.rs
📚 Learning: 2025-05-20T10:20:08.206Z
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.
Applied to files:
crates/common/src/raindex_client/take_orders/e2e_tests.rscrates/common/src/raindex_client/take_orders/mod.rscrates/common/src/take_orders/preflight.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/common/src/raindex_client/take_orders/e2e_tests.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/common/src/raindex_client/take_orders/e2e_tests.rs
📚 Learning: 2025-06-24T13:36:28.797Z
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.
Applied to files:
crates/common/src/raindex_client/take_orders/e2e_tests.rs
📚 Learning: 2025-06-07T05:19:46.330Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1872
File: packages/webapp/src/__tests__/handleVaultDeposit.test.ts:20-53
Timestamp: 2025-06-07T05:19:46.330Z
Learning: In the rain.orderbook codebase, simple wrapper/adapter functions that just delegate to other functions (like handleVaultDeposit) don't need extensive edge case testing for missing parameters or error handling - the current test coverage focusing on core functionality is sufficient.
Applied to files:
crates/common/src/raindex_client/take_orders/e2e_tests.rs
📚 Learning: 2025-10-06T11:28:30.692Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2145
File: crates/common/src/raindex_client/local_db/query/fetch_orders/query.sql:6-7
Timestamp: 2025-10-06T11:28:30.692Z
Learning: In `crates/common/src/raindex_client/local_db/query/fetch_orders/query.sql`, the orderbook_address is currently hardcoded to '0x2f209e5b67A33B8fE96E28f24628dF6Da301c8eB' because the system only supports a single orderbook at the moment. Multiorderbook logic is not yet implemented and will be added in the future.
Applied to files:
crates/common/src/raindex_client/take_orders/e2e_tests.rs
📚 Learning: 2025-06-24T08:46:03.368Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1947
File: crates/common/src/raindex_client/orders.rs:98-125
Timestamp: 2025-06-24T08:46:03.368Z
Learning: In the vault merging logic in crates/common/src/raindex_client/orders.rs, optimization isn't necessary because the maximum list items are usually around 5 items. For such small datasets, the simple three-loop approach is preferred over HashMap-based optimization due to clarity and minimal performance impact.
Applied to files:
crates/common/src/raindex_client/take_orders/e2e_tests.rs
📚 Learning: 2025-10-18T10:38:41.273Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2237
File: crates/common/src/raindex_client/local_db/sync.rs:79-89
Timestamp: 2025-10-18T10:38:41.273Z
Learning: In `crates/common/src/raindex_client/local_db/sync.rs`, the sync_database method currently only supports indexing a single orderbook per chain ID, which is why `.first()` is used to select the orderbook configuration. Multi-orderbook support per chain ID is planned for future PRs.
Applied to files:
crates/common/src/raindex_client/take_orders/e2e_tests.rs
📚 Learning: 2025-06-16T10:49:47.770Z
Learnt from: thedavidmeister
Repo: rainlanguage/rain.orderbook PR: 1926
File: test/concrete/ob/OrderBook.clear.zeroAmount.t.sol:24-32
Timestamp: 2025-06-16T10:49:47.770Z
Learning: LibTestAddOrder.conformConfig() in test/util/lib/LibTestAddOrder.sol automatically constrains OrderConfigV3 to prevent common test failures by ensuring validInputs[0].token != validOutputs[0].token, setting them to address(0) and address(1) respectively if they're equal. This prevents TokenSelfTrade errors in fuzz tests.
Applied to files:
crates/common/src/raindex_client/take_orders/e2e_tests.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/common/src/raindex_client/take_orders/e2e_tests.rs
📚 Learning: 2025-05-14T04:49:14.621Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1710
File: crates/quote/Cargo.toml:30-30
Timestamp: 2025-05-14T04:49:14.621Z
Learning: In the rain.orderbook repository, wasm-bindgen-utils is intentionally used as an unconditional dependency because non-wasm targets (like tests) sometimes need to use it.
Applied to files:
crates/common/src/raindex_client/take_orders/mod.rs
📚 Learning: 2025-05-27T06:16:12.195Z
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1874
File: tauri-app/src-tauri/src/transaction_status.rs:87-114
Timestamp: 2025-05-27T06:16:12.195Z
Learning: In tauri-app/src-tauri/src/transaction_status.rs, testing one WriteTransactionStatus variant (like PendingPrepare) is sufficient for the update_status_and_emit method since all variants follow the same code path through the From<WriteTransactionStatus<T>> implementation.
Applied to files:
crates/common/src/take_orders/preflight.rs
📚 Learning: 2025-07-15T08:01:38.534Z
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.
Applied to files:
crates/common/src/take_orders/preflight.rs
🧬 Code graph analysis (5)
packages/orderbook/test/js_api/raindexClient.test.ts (2)
crates/settings/src/orderbook.rs (1)
value(47-48)crates/js_api/src/gui/deposits.rs (1)
value(81-83)
crates/common/src/take_orders/mod.rs (1)
crates/common/src/take_orders/preflight.rs (5)
build_approval_calldata(65-68)check_taker_balance(33-50)check_taker_balance_and_allowance(70-78)find_failing_order_index(109-153)simulate_take_orders(80-107)
crates/common/src/raindex_client/take_orders/request.rs (2)
subgraph/src/float.ts (1)
Float(4-4)crates/common/src/withdraw.rs (1)
parse(70-71)
crates/common/src/raindex_client/take_orders/mod.rs (8)
crates/common/src/take_orders/config.rs (1)
build_take_orders_config_from_buy_simulation(25-73)crates/common/src/take_orders/preflight.rs (1)
check_taker_balance_and_allowance(70-78)crates/bindings/src/provider.rs (1)
mk_read_provider(24-42)crates/common/src/raindex_client/take_orders/request.rs (1)
parse_request_from_struct(32-60)crates/common/src/rpc_client.rs (2)
rpc_urls(106-108)new_with_urls(69-82)crates/common/src/raindex_client/mod.rs (1)
new(117-139)crates/common/src/raindex_client/take_orders/selection.rs (2)
build_candidates_for_chain(11-24)select_best_orderbook_simulation(42-92)crates/common/src/raindex_client/take_orders/result.rs (1)
build_calldata_result(56-89)
crates/common/src/take_orders/preflight.rs (1)
crates/common/src/erc20.rs (1)
allowance(71-79)
🪛 Gitleaks (8.30.0)
packages/orderbook/test/js_api/raindexClient.test.ts
[high] 2057-2057: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 2058-2058: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-browser-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: test
- GitHub Check: git-clean
- GitHub Check: test
- GitHub Check: Deploy-Docs-Preview
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: Deploy-Preview-Push
🔇 Additional comments (31)
crates/common/src/take_orders/preflight.rs (4)
14-26: LGTM!The
PreflightErrorenum is well-structured with appropriate error variants covering all preflight failure modes. The#[from]derives enable clean error propagation.
28-50: LGTM!The
AllowanceCheckResultstruct andcheck_taker_balancefunction are correctly implemented. The balance check properly converts the float amount to raw units using token decimals before comparison.
65-68: LGTM!The
build_approval_calldatafunction correctly encodes the ERC20 approve call using the sol types ABI encoder.
170-216: LGTM!Good unit test coverage for error display, approval calldata encoding/decoding round-trip, and allowance check result semantics.
crates/common/src/raindex_client/mod.rs (2)
341-351: LGTM!The new
NonPositivePriceCapandPreflightError(String)variants are appropriately added to handle the new preflight validation flow. The error messages are user-friendly and consistent with existing patterns.Note:
PreflightErrorwraps aStringrather than the typedtake_orders::PreflightError. This is a reasonable design choice to avoid tight coupling and simplify the error chain, though it does lose some type information.
504-516: LGTM!The
to_readable_msgimplementations for the new error variants provide clear, user-friendly messages consistent with the rest of the error handling.crates/common/src/take_orders/mod.rs (1)
3-14: LGTM!The new
preflightmodule is properly declared and its public API is re-exported following the existing pattern in this file. The exports provide a clean interface for the preflight validation functionality.packages/orderbook/test/js_api/raindexClient.test.ts (5)
2056-2059: Static analysis false positive - these are test Ethereum addresses, not API keys.The Gitleaks warnings on
TAKER_ADDRESS,SELL_TOKEN, andBUY_TOKENare false positives. These are well-formed Ethereum addresses used as test constants, not actual API keys or secrets.
2193-2196: LGTM!The
encodeUint256helper is a clean utility for generating 0x-prefixed 64-character hex values for mock RPC responses.
2263-2298: Verify mock ordering for multi-call test scenarios.The test relies on a specific ordering of RPC mock responses (block number, multicall quote, decimals, balance, allowance, simulation). If the actual implementation makes calls in a different order, the mocks will return incorrect data.
Consider using
mockServer.forPost('/rpc1').matching(...)with body matchers to make the mocks order-independent, or add a comment documenting the expected call order.
2462-2498: Good test for price cap filtering across multiple legs.This test verifies that when two orders exist with different prices, the expensive one is filtered out. The assertion on
result.value.prices.length === 1confirms only the cheap leg is used.
2055-2613: Comprehensive test coverage for the TakeOrders flow.The test suite covers key scenarios:
- Input validation (zero/negative amounts, price caps)
- Error handling (NoLiquidity, InsufficientLiquidity, PreflightError)
- Mode behavior (partial vs exact fill)
- Price cap filtering
- Preflight revert handling
crates/common/src/raindex_client/take_orders/e2e_tests.rs (5)
1-18: LGTM!Imports are properly updated to reflect the new API structure with
TakeOrdersRequestand updated test helper paths.
56-72: LGTM!The test correctly constructs a
TakeOrdersRequestand verifies that empty subgraph results produce aNoLiquidityerror.
172-231: LGTM!The happy path test properly:
- Funds and approves the taker before the call
- Uses the new accessor methods (
result.orderbook(),result.calldata(),result.prices(),result.effective_price())- Verifies the decoded config and price values
- Checks minimumInput is zero for Partial mode
234-264: LGTM!Good test for invalid address validation - verifies that a malformed
sell_tokenproduces aFromHexError.
266-296: LGTM!Good test for invalid float validation - verifies that a non-numeric
buy_amountproduces aFloaterror.crates/common/src/raindex_client/take_orders/request.rs (4)
1-20: LGTM! Clean WASM-friendly request structure.The
TakeOrdersRequeststruct is well-designed for WASM bindings with proper serialization traits and theimpl_wasm_traits!macro. The separation between the public WASM-friendlyTakeOrdersRequest(String fields) and internalParsedTakeOrdersRequest(typed fields) is a good pattern.
32-60: Validation logic is comprehensive and correct.The parsing function properly:
- Validates address formats
- Prevents same-token pairs
- Ensures positive buy_amount and price_cap
The error handling via
?operator correctly propagates toRaindexError.
62-270: Excellent test coverage!The test suite thoroughly covers:
- Valid input scenarios
- Invalid addresses (sell_token, buy_token, taker)
- Invalid numeric values
- Boundary conditions (zero, negative)
- Same-token-pair rejection
- Both
MinReceiveModevariants
41-42: Minor: Consider avoiding String clones.The
Float::parsecalls clone the strings before parsing. While not a critical issue, this could be optimized ifFloat::parseaccepts&str:let buy_amount = Float::parse(&req.buy_amount)?; let price_cap = Float::parse(&req.price_cap)?;⛔ Skipped due to learnings
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: 1687 File: crates/js_api/src/gui/state_management.rs:32-35 Timestamp: 2025-04-29T11:19:41.267Z Learning: The team prefers the `get_dotrain_hash` function to accept owned String parameters instead of string references, even though it may result in additional allocations during serialization/deserialization.crates/common/src/raindex_client/take_orders/result.rs (3)
11-54: Well-structured accessor pattern for WASM/non-WASM compatibility.The dual implementation approach cleanly separates WASM (String-based) and native Rust accessors. The private fields with public getters provide good API encapsulation.
Note: The accessors clone
calldataandprices. This is likely necessary for ownership but may have performance implications if called frequently in hot paths.
56-89: Effective price calculation is correct with proper guards.The division-by-zero check at lines 67-74 properly handles edge cases where
total_buy_amountis zero. The price collection from simulation legs is straightforward and correct.
91-198: Tests appropriately verify result construction.The tests validate:
- Valid calldata encoding/decoding
- Effective price calculation logic
- Price array alignment with simulation legs
crates/common/src/raindex_client/take_orders/mod.rs (7)
25-52: LGTM! Clean helper for fetching orders.The
fetch_orders_for_pairhelper properly constructs filters and returnsNoLiquidityerror when no orders are found.
148-160: Balance and allowance checks are appropriately guarded per orderbook.The
checked_for_orderbooktracking ensures that balance/allowance verification is performed only once per unique orderbook, avoiding redundant checks. This is a good optimization.
229-252: LGTM! WASM serialization test is appropriate.The test verifies
MinReceiveModecan round-trip through WASM serialization, which is essential for the WASM bindings.
154-158: Error message could be more specific about which token lacks allowance.The error at lines 154-158 states "Insufficient allowance for required spend" but doesn't indicate which token (sell_token) requires approval or what the required amount is. This makes it harder for users to debug.
Consider enhancing the error message:
if allowance_check.needs_approval { return Err(RaindexError::PreflightError( format!( "Insufficient allowance for sell token {:?}. Required: {}, Current allowance: {}", req.sell_token, max_sell.format()?, allowance_check.current_allowance.format()? ) )); }(Note: This assumes
AllowanceCheckResultincludes acurrent_allowancefield; adjust based on actual struct definition.)⛔ Skipped due to learnings
Learnt from: findolor Repo: rainlanguage/rain.orderbook PR: 1983 File: crates/settings/src/gui.rs:965-973 Timestamp: 2025-07-11T08:37:24.423Z Learning: In the rainlanguage/rain.orderbook project, user findolor prefers to keep user-facing error messages generic and user-friendly rather than technically specific. For example, using "a valid number" instead of "a valid u8 (0-255)" in GUI validation error messages to maintain simplicity for end users.
143-143: Performance: Consider avoiding full candidate clone on each iteration.Line 143 clones the entire
candidatesvector on every loop iteration for the simulation selection. This could be expensive if there are many candidates or the loop iterates multiple times.Consider passing
&[TakeOrderCandidate]toselect_best_orderbook_simulationif possible, or restructuring to avoid the clone.⛔ Skipped due to learnings
Learnt from: findolor Repo: rainlanguage/rain.orderbook PR: 1925 File: crates/settings/src/remote_tokens.rs:44-62 Timestamp: 2025-07-11T08:45:43.961Z Learning: findolor prefers to avoid performance micro-optimizations like changing `.values()` to `.into_values()` in HashMap iterations in the rainlanguage/rain.orderbook codebase, even when they would eliminate unnecessary cloning.Learnt from: findolor Repo: rainlanguage/rain.orderbook PR: 1947 File: crates/common/src/raindex_client/orders.rs:98-125 Timestamp: 2025-06-24T08:46:03.368Z Learning: In the vault merging logic in crates/common/src/raindex_client/orders.rs, optimization isn't necessary because the maximum list items are usually around 5 items. For such small datasets, the simple three-loop approach is preferred over HashMap-based optimization due to clarity and minimal performance impact.Learnt from: findolor Repo: rainlanguage/rain.orderbook PR: 1903 File: crates/common/src/deposit.rs:63-64 Timestamp: 2025-06-17T16:54:43.176Z Learning: In the rain.orderbook codebase, the maintainer findolor prefers to keep the current implementation that clones the rpcs vector when calling ReadableClient::new_from_urls rather than optimizing it to avoid the clone or adding early validation for empty rpcs. Performance micro-optimizations like avoiding vector clones in this context are not a priority.Learnt from: findolor Repo: rainlanguage/rain.orderbook PR: 2163 File: crates/cli/src/commands/order/calldata.rs:47-48 Timestamp: 2025-09-24T10:54:42.060Z Learning: In crates/cli/src/commands/order/calldata.rs, the config_deployment variable is used both as a parameter to AddOrderArgs::new_from_deployment() and later to extract RPC information, requiring the clone() to maintain access after ownership transfer.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: 1903 File: crates/js_api/src/gui/order_operations.rs:124-134 Timestamp: 2025-06-17T16:32:04.554Z Learning: In the rain.orderbook codebase, RPC lists are typically small (2 items maximum), so performance optimizations around cloning and converting small Vec<Url> collections are generally unnecessary.
141-207: Critical: Unbounded retry loop could cause infinite iteration.The
loopat line 141 has no maximum iteration limit. If the simulation keeps failing andfind_failing_order_indexkeeps returning indices, this could iterate until all candidates are exhausted. While line 197-199 checks for empty candidates, a malicious or buggy scenario could cause excessive iterations.Consider adding a maximum retry limit:
🔎 Suggested fix with retry limit
+ const MAX_RETRIES: usize = 10; + let mut retry_count = 0; + loop { + if retry_count >= MAX_RETRIES { + return Err(RaindexError::PreflightError( + format!("Exceeded maximum retry attempts ({}) while filtering failing orders", MAX_RETRIES) + )); + } + retry_count += 1; + let (best_orderbook, best_sim) = selection::select_best_orderbook_simulation(⛔ Skipped due to learnings
Learnt from: findolor Repo: rainlanguage/rain.orderbook PR: 1947 File: crates/common/src/raindex_client/orders.rs:98-125 Timestamp: 2025-06-24T08:46:03.368Z Learning: In the vault merging logic in crates/common/src/raindex_client/orders.rs, optimization isn't necessary because the maximum list items are usually around 5 items. For such small datasets, the simple three-loop approach is preferred over HashMap-based optimization due to clarity and minimal performance impact.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: 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.Learnt from: findolor Repo: rainlanguage/rain.orderbook PR: 2012 File: crates/js_api/src/registry.rs:233-252 Timestamp: 2025-07-23T10:51:12.278Z Learning: In the rainlanguage/rain.orderbook codebase, findolor considers the pattern of creating a partially initialized struct instance within a constructor and then populating it through async operations to be acceptable, as long as the partial state is never exposed to users and the constructor returns Result<T, E> with proper error propagation using the ? operator.
211-227: Sentinel valueu32::MAXcreates fragile filtering logic.The conversion of
U256indices tou32usingunwrap_or(u32::MAX)is problematic: if conversion overflows, the sentinel value will never match real candidate indices (which are bounded by array length), preventing removal of the failing order. While a safeguard exists (candidates.is_empty() check at line 197), this creates a fragile path that depends on eventual candidate exhaustion rather than proper filtering.Consider explicit error handling:
let input_idx: u32 = failing_order.inputIOIndex.try_into() .map_err(|_| RaindexError::PreflightError("Index overflow".to_string()))?; let output_idx: u32 = failing_order.outputIOIndex.try_into() .map_err(|_| RaindexError::PreflightError("Index overflow".to_string()))?;Alternatively, if overflow is not realistically possible, document why and why the sentinel approach is acceptable.
Comment on lines
+80
to
+107
| pub async fn simulate_take_orders( | ||
| provider: &ReadProvider, | ||
| orderbook: Address, | ||
| taker: Address, | ||
| config: &TakeOrdersConfigV4, | ||
| block_number: u64, | ||
| ) -> Result<(), String> { | ||
| let calldata = takeOrders3Call { | ||
| config: config.clone(), | ||
| } | ||
| .abi_encode(); | ||
|
|
||
| let tx = TransactionRequest::default() | ||
| .with_from(taker) | ||
| .with_to(orderbook) | ||
| .with_input(Bytes::copy_from_slice(&calldata)) | ||
| .with_gas_limit(DEFAULT_GAS_CAP); | ||
|
|
||
| let block_id = alloy::eips::BlockNumberOrTag::Number(block_number).into(); | ||
|
|
||
| provider | ||
| .call(tx.into()) | ||
| .block(block_id) | ||
| .await | ||
| .map_err(format_rpc_error)?; | ||
|
|
||
| Ok(()) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider returning simulation result data instead of discarding it.
The simulate_take_orders function performs an eth_call but discards the successful response data (line 100-104). While the current use case only needs pass/fail status, the simulation result contains useful data like output amounts that could be valuable for callers.
🔎 Optional: Return the call result for richer information:
pub async fn simulate_take_orders(
provider: &ReadProvider,
orderbook: Address,
taker: Address,
config: &TakeOrdersConfigV4,
block_number: u64,
-) -> Result<(), String> {
+) -> Result<Bytes, String> {
let calldata = takeOrders3Call {
config: config.clone(),
}
.abi_encode();
let tx = TransactionRequest::default()
.with_from(taker)
.with_to(orderbook)
.with_input(Bytes::copy_from_slice(&calldata))
.with_gas_limit(DEFAULT_GAS_CAP);
let block_id = alloy::eips::BlockNumberOrTag::Number(block_number).into();
- provider
+ let result = provider
.call(tx.into())
.block(block_id)
.await
.map_err(format_rpc_error)?;
- Ok(())
+ Ok(result)
}🤖 Prompt for AI Agents
In crates/common/src/take_orders/preflight.rs around lines 80 to 107, the
function currently performs an eth_call and discards the successful response;
change the function to return the call result so callers can inspect output
amounts or other returndata. Modify the signature to return Result<Bytes,
String> (or Result<Option<Bytes>, String> if you prefer nullable), capture the
provider.call(...).block(...).await result into a variable, map or convert the
successful response into the raw Bytes (or None) and return it instead of
ignoring it, and update any callers/tests to handle the new return type.
Collaborator
Author
There was a problem hiding this comment.
we only need the simulation to make sure the tx does not revert with the parameters that we are passing. we don't need it for anything else at this point
Contributor
There was a problem hiding this comment.
@findolor, understood! I've noted that the current implementation is intentional and sufficient for your needs.
🐰✨
✏️ Learnings added
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2363
File: crates/common/src/take_orders/preflight.rs:80-107
Timestamp: 2025-12-18T08:19:51.021Z
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.
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: 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: 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: 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: 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.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2012
File: crates/js_api/src/registry.rs:233-252
Timestamp: 2025-07-23T10:51:12.278Z
Learning: In the rainlanguage/rain.orderbook codebase, findolor considers the pattern of creating a partially initialized struct instance within a constructor and then populating it through async operations to be acceptable, as long as the partial state is never exposed to users and the constructor returns Result<T, E> with proper error propagation using the ? operator.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2360
File: crates/common/src/test_helpers.rs:329-367
Timestamp: 2025-12-17T13:17:58.487Z
Learning: In the rainlanguage/rain.orderbook codebase, findolor considers hardcoded values (like decimals) acceptable in test helper functions such as deposit_to_orderbook in crates/common/src/test_helpers.rs, as test infrastructure doesn't need the same level of parameterization as production code.
Learnt from: brusherru
Repo: rainlanguage/rain.orderbook PR: 2036
File: crates/js_api/src/filters/raindex_filter_store.rs:446-533
Timestamp: 2025-08-02T02:34:32.237Z
Learning: In the rainlanguage/rain.orderbook project's RaindexFilterStore WASM tests (crates/js_api/src/filters/raindex_filter_store.rs), brusherru decided to focus on testing only methods without side effects (that don't use web_sys) due to difficulties with mocking localStorage and window APIs in the WASM test environment. This pragmatic approach tests pure logic separately from browser integration.
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: 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.
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: thedavidmeister
Repo: rainlanguage/rain.orderbook PR: 2149
File: src/concrete/arb/RouteProcessorOrderBookV5ArbOrderTaker.sol:37-39
Timestamp: 2025-09-16T07:52:46.075Z
Learning: In the rainlanguage/rain.orderbook codebase, Slither static analysis tool prefers explicit handling of unused return values by capturing them in variables and referencing them in no-op statements (e.g., `(losslessInputAmount);`) rather than discarding them via destructuring with blank elements (e.g., `(uint256 inputTokenAmount, ) = ...`). The explicit pattern satisfies Slither's unused return value checks while the destructuring approach causes Slither to complain.
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: 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.
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: 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: 0xgleb
Repo: rainlanguage/rain.orderbook PR: 1790
File: tauri-app/src-tauri/src/commands/vault.rs:67-67
Timestamp: 2025-05-17T15:32:28.733Z
Learning: For the PR focused on testing Tauri commands::order module, the generic type parameter R: Runtime was selectively added where needed for the PR scope, applying the changes primarily to order.rs and related files while leaving other modules like vault.rs for potential future refactoring.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:26:31.566Z
Learning: For PR #1884 and future PRs in rainlanguage/rain.orderbook repository, provide PR size reports in a separate comment with ONLY the exact format "TOTAL=number\nADDITIONS=number\nDELETIONS=number", without any additional text or explanation. Reports should exclude irrelevant files such as lock files (package-lock.json, yarn.lock, pnpm-lock.yaml, cargo.lock).
Learnt from: brusherru
Repo: rainlanguage/rain.orderbook PR: 2044
File: tauri-app/src/routes/orders/[chainId]-[orderbook]-[orderHash]/+page.svelte:76-78
Timestamp: 2025-07-31T19:03:56.594Z
Learning: In the rainlanguage/rain.orderbook project, when the Tauri app has issues preventing proper testing, the team prefers to defer Tauri-specific implementations to separate PRs rather than including untested code, especially for financial operations like withdrawals.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T22:14:22.646Z
Learning: For PR #1884 in rainlanguage/rain.orderbook, I must consistently report the PR size after EVERY new commit in the exact format "TOTAL=number\nADDITIONS=number\nDELETIONS=number" without any additional text or formatting blocks. The report must exclude lock files (package-lock.json, yarn.lock, pnpm-lock.yaml, cargo.lock) and can be included either as part of the review or as a separate comment.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T22:14:22.646Z
Learning: For PR #1884 in rainlanguage/rain.orderbook, I must consistently report the PR size after EVERY new commit in the exact format "TOTAL=number\nADDITIONS=number\nDELETIONS=number" without any formatting blocks. The report must exclude lock files (package-lock.json, yarn.lock, pnpm-lock.yaml, cargo.lock) and can be included either as part of the review or as a separate comment.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T22:47:21.927Z
Learning: For PR #1884 in rainlanguage/rain.orderbook, the PR size report must always be placed at the very beginning of any comment, before any learning used section or other content.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T22:47:21.927Z
Learning: For PR #1884 in rainlanguage/rain.orderbook, the PR size report must always be placed at the very beginning of any comment, before any learning used section or other content.
Comment on lines
109
to
153
| pub async fn find_failing_order_index( | ||
| provider: &ReadProvider, | ||
| orderbook: Address, | ||
| taker: Address, | ||
| config: &TakeOrdersConfigV4, | ||
| block_number: u64, | ||
| ) -> Option<usize> { | ||
| let num_orders = config.orders.len(); | ||
| if num_orders == 0 { | ||
| return None; | ||
| } | ||
| if num_orders == 1 { | ||
| return Some(0); | ||
| } | ||
|
|
||
| let min_input = config.minimumInput; | ||
|
|
||
| let mut low: usize = 0; | ||
| let mut high: usize = num_orders; | ||
| let mut last_failing: Option<usize> = None; | ||
|
|
||
| while low < high { | ||
| let mid = (low + high) / 2; | ||
|
|
||
| let prefix_config = TakeOrdersConfigV4 { | ||
| minimumInput: min_input, | ||
| maximumInput: config.maximumInput, | ||
| maximumIORatio: config.maximumIORatio, | ||
| orders: config.orders[..=mid].to_vec(), | ||
| data: config.data.clone(), | ||
| }; | ||
|
|
||
| let result = | ||
| simulate_take_orders(provider, orderbook, taker, &prefix_config, block_number).await; | ||
|
|
||
| if result.is_ok() { | ||
| low = mid + 1; | ||
| } else { | ||
| last_failing = Some(mid); | ||
| high = mid; | ||
| } | ||
| } | ||
|
|
||
| last_failing | ||
| } |
Contributor
There was a problem hiding this comment.
Binary search logic has an edge case issue when num_orders == 1.
When there's only one order (lines 120-122), the function returns Some(0) unconditionally without actually simulating it. This means:
- If the single order would succeed, the function incorrectly reports it as failing
- The function's purpose is to find failing orders via simulation, but this bypasses simulation entirely
The early return assumes a single order must be failing (since the caller presumably already knows the full batch failed), but this assumption isn't documented and could lead to confusion.
🔎 Suggested fix - simulate the single order before returning:
if num_orders == 1 {
- return Some(0);
+ // For a single order, simulate it to verify it actually fails
+ let result =
+ simulate_take_orders(provider, orderbook, taker, config, block_number).await;
+ return if result.is_err() { Some(0) } else { None };
}Alternatively, if the assumption is intentional (caller guarantees the batch already failed), add a doc comment explaining this precondition.
🤖 Prompt for AI Agents
In crates/common/src/take_orders/preflight.rs around lines 109 to 153, the
function currently returns Some(0) when num_orders == 1 without simulating the
lone order; change this to actually run simulate_take_orders for the
single-order prefix (construct a prefix TakeOrdersConfigV4 with orders[..=0],
same minimumInput/maximumInput/maximumIORatio/data) and return Some(0) only if
the simulation returns Err, otherwise return None; alternatively, if the caller
truly guarantees the full batch already failed, replace the unconditional return
with a clear doc comment describing that precondition (but prefer the simulation
approach to preserve function semantics).
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 PR
Motivation
See issues:
In the upstream PR we have implemented the logic to have take orders functionality in
RaindexClient. This PR further enhances it by adding simulation logic to make sure we don't have reverting orders.Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.