Cover error cases for quote module in quote crate#1846
Conversation
WalkthroughA comprehensive set of new unit and asynchronous tests has been added to the quoting functionality in Changes
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
findolor
left a comment
findolor
left a comment
There was a problem hiding this comment.
can you also add tests for these functions?
- get_order_hash
- get_id
- validate
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
crates/quote/Cargo.toml(1 hunks)crates/quote/src/quote.rs(12 hunks)
🧰 Additional context used
🧠 Learnings (1)
crates/quote/Cargo.toml (3)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#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.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#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.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#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.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Deploy-Preview
- GitHub Check: git-clean
🔇 Additional comments (1)
crates/quote/src/quote.rs (1)
310-313: 🧹 Nitpick (assertive)Unnecessary duplicate import
use alloy::hex;brings the module (and thus thehex!macro) into scope.
use alloy::hex::ToHex;already re-exports the trait and implicitly brings the module in as well, so the first line can be dropped to avoid a redundant import.-use alloy::hex; use alloy::hex::ToHex;Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
crates/quote/src/quote.rs (4)
114-114:#[inline]on an async wrapper is still unnecessaryThe same concern raised in the previous review remains: adding
#[inline]to anasync fnthat is already a tiny wrapper offers no measurable benefit while potentially increasing binary size. Please remove the attribute (or add it consistently to every similar helper, but that is rarely warranted).
475-489: Hard-coded expected hash is brittleCommitting a literal hash string makes the test fail whenever default values inside
OrderV3change. Compute the expected value on-the-fly instead, e.g.:-let expected = - "8a3fbb9caf53f18f1f78d90c48dbe4612bcd93285ed0fc033009b4a96ea2aaed".to_string(); -let actual = quote_target.get_order_hash().encode_hex::<String>(); -assert_eq!(actual, expected); +let expected = keccak256(order.abi_encode()).encode_hex::<String>(); +assert_eq!(quote_target.get_order_hash().encode_hex::<String>(), expected);
740-770: Repeated mock-server boilerplate still not deduplicatedMultiple tests recreate identical
MockServersetups and JSON RPC/Subgraph stubs. Extract helpers (e.g.fn mock_rpc_success(server: &MockServer, path: &str, data: Vec<u8>)) or move fixtures to a shared module to cut ~40 lines per test and improve readability.
1002-1006: Exact string match on upstream error message is fragileThe assertion depends on the full wording
"buffer overrun while deserializing", which may change in the decoding crate. Match the variant and usecontains("buffer overrun")instead to keep the test resilient across patch releases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
crates/quote/src/quote.rs(12 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: test
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
crates/quote/src/quote.rs (1)
323-472: 🧹 Nitpick (assertive)Make test fixtures deterministic & fix the typo in
retrun_sg_data
get_test_datarelies onAddress::random()(multiple times). Although none of those random values are explicitly asserted against, non-determinism can make failures harder to reproduce. Consider:// use a fixed seed for reproducibility let mut rng = rand::rngs::StdRng::seed_from_u64(42); let orderbook = Address::random_using(&mut rng);(or simply populate with the zero address when the exact value is irrelevant).
- The local variable is spelled
retrun_sg_data. A quick rename toreturn_sg_data(orsg_payload) saves future head-scratching and IDE miss-hits.- let retrun_sg_data = if batch { + let return_sg_data = if batch {Follow-up: update all call-sites.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
crates/quote/src/quote.rs(11 hunks)
🧰 Additional context used
🧠 Learnings (1)
crates/quote/src/quote.rs (1)
Learnt from: 0xgleb
PR: rainlanguage/rain.orderbook#1846
File: crates/quote/src/quote.rs:0-0
Timestamp: 2025-05-19T16:03:40.904Z
Learning: For hash function testing, using precomputed example values is preferred over reimplementing the hashing logic in the test itself, as it provides better validation and avoids circular testing.
🔇 Additional comments (2)
crates/quote/src/quote.rs (2)
474-488: Pre-computed hash value test looks goodGood use of a hard-coded example to validate
get_order_hash; this avoids circular testing (re-computing the hash in the test itself) and honours the preference captured in past learnings.
1005-1048: LGTM – precise error assertion maintainedThe corrupt-data path is correctly exercised and the exact error message is asserted, matching the convention agreed earlier.
| async fn test_get_batch_quote_spec_from_subgraph_err() { | ||
| let rpc_server = MockServer::start_async().await; | ||
|
|
||
| let (orderbook, order, order_id_u256, _) = get_test_data(true); | ||
|
|
||
| rpc_server.mock(|when, then| { | ||
| when.method(POST).path("/sg"); | ||
|
|
||
| let invalid_order_json = json!({ | ||
| "id": encode_prefixed(B256::random()), | ||
| "orderBytes": encode_prefixed(order.abi_encode()), | ||
| "orderHash": encode_prefixed(B256::random()), | ||
| "owner": encode_prefixed(order.owner), | ||
| "orderbook": { "id": encode_prefixed(B256::random()) }, | ||
| "active": true, | ||
| "addEvents": [], | ||
| "meta": null, | ||
| "timestampAdded": "0", | ||
| "trades": [], | ||
| "removeEvents": [] | ||
| }); | ||
|
|
||
| then.json_body_obj(&json!({ | ||
| "data": { | ||
| "orders": [invalid_order_json] | ||
| } | ||
| })); | ||
| }); | ||
|
|
||
| let batch_quote_targets_specifiers = BatchQuoteSpec(vec![QuoteSpec { | ||
| order_hash: order_id_u256, | ||
| input_io_index: 0, | ||
| output_io_index: 0, | ||
| signed_context: vec![], | ||
| orderbook, | ||
| }]); | ||
|
|
||
| let err = batch_quote_targets_specifiers | ||
| .get_batch_quote_target_from_subgraph(rpc_server.url("/sg").as_str()) | ||
| .await | ||
| .unwrap_err(); | ||
|
|
||
| assert!(matches!( | ||
| err, | ||
| Error::SubgraphClientError(OrderbookSubgraphClientError::CynicClientError(cynic_err)) | ||
| if cynic_err.to_string().contains("error decoding response body") | ||
| )); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prefer exact message matching for consistency with previous tests
In line with the earlier decision to assert the full error string (e.g. “buffer overrun while deserializing”), consider matching the exact cynic_err message here as well instead of using contains("error decoding response body"). This keeps the style consistent across the suite and immediately flags upstream wording regressions.
🤖 Prompt for AI Agents
In crates/quote/src/quote.rs between lines 668 and 715, the test currently
asserts the error message using a partial match with contains("error decoding
response body"). To maintain consistency with previous tests, update the
assertion to match the exact error message string from cynic_err, such as
"buffer overrun while deserializing". Replace the contains check with an
equality check against the full expected error message to ensure precise
matching and consistent test style.
| #[test] | ||
| fn test_validate_err() { | ||
| let quote_target = QuoteTarget { | ||
| quote_config: Quote { | ||
| order: OrderV3::default(), | ||
| outputIOIndex: U256::from(1_u16), | ||
| ..Default::default() | ||
| }, | ||
| orderbook: Address::ZERO, | ||
| }; | ||
| assert!(quote_target.validate().is_err()); | ||
|
|
||
| let quote_target = QuoteTarget { | ||
| quote_config: Quote { | ||
| order: OrderV3::default(), | ||
| inputIOIndex: U256::from(1_u16), | ||
| ..Default::default() | ||
| }, | ||
| orderbook: Address::ZERO, | ||
| }; | ||
| assert!(quote_target.validate().is_err()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen negative-path assertions
assert!(quote_target.validate().is_err()) only checks that some error was returned.
To ensure the correct variant/index is surfaced you can pattern-match:
assert!(matches!(
quote_target.validate(),
Err(Error::InvalidQuoteTarget(index)) if index == U256::from(1_u16)
));This guards against future changes that might return the wrong error type yet still satisfy is_err().
🤖 Prompt for AI Agents
In crates/quote/src/quote.rs around lines 530 to 551, the test uses
assert!(quote_target.validate().is_err()) which only verifies that an error
occurred but not the specific error type or value. To strengthen the test,
replace these assertions with pattern matching using assert!(matches!(...)) to
check that the error returned is specifically Error::InvalidQuoteTarget with the
expected index value. This ensures the test validates the exact error variant
and guards against incorrect error returns in the future.
| use alloy::hex; | ||
| use alloy::hex::ToHex; | ||
| use alloy::primitives::{address, keccak256}; | ||
| use alloy::primitives::{hex::encode_prefixed, U256}; | ||
| use alloy::sol_types::{SolCall, SolValue}; | ||
| use alloy_ethers_typecast::multicall::IMulticall3::Result as MulticallResult; | ||
| use alloy_ethers_typecast::rpc::Response; | ||
| use alloy_ethers_typecast::transaction::ReadableClientError; | ||
| use httpmock::{Method::POST, MockServer}; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consolidate duplicate alloy::hex imports
use alloy::hex; and use alloy::hex::ToHex; can be merged to avoid redundancy:
-use alloy::hex;
-use alloy::hex::ToHex;
+use alloy::hex::{self, ToHex};This keeps the imports concise and prevents “unused import” warnings should one of them become unnecessary later.
📝 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.
| use alloy::hex; | |
| use alloy::hex::ToHex; | |
| use alloy::primitives::{address, keccak256}; | |
| use alloy::primitives::{hex::encode_prefixed, U256}; | |
| use alloy::sol_types::{SolCall, SolValue}; | |
| use alloy_ethers_typecast::multicall::IMulticall3::Result as MulticallResult; | |
| use alloy_ethers_typecast::rpc::Response; | |
| use alloy_ethers_typecast::transaction::ReadableClientError; | |
| use httpmock::{Method::POST, MockServer}; | |
| use alloy::hex::{self, ToHex}; | |
| use alloy::primitives::{address, keccak256}; | |
| use alloy::primitives::{hex::encode_prefixed, U256}; | |
| use alloy::sol_types::{SolCall, SolValue}; | |
| use alloy_ethers_typecast::multicall::IMulticall3::Result as MulticallResult; | |
| use alloy_ethers_typecast::rpc::Response; | |
| use alloy_ethers_typecast::transaction::ReadableClientError; | |
| use httpmock::{Method::POST, MockServer}; |
🤖 Prompt for AI Agents
In crates/quote/src/quote.rs around lines 309 to 317, the imports include both
`use alloy::hex;` and `use alloy::hex::ToHex;` which is redundant. Consolidate
these by removing the separate `use alloy::hex;` and only keep `use
alloy::hex::ToHex;` or combine them into a single line that imports both if
needed, to keep the imports concise and avoid redundancy.
There was a problem hiding this comment.
use alloy::hex::{self, ToHex}; isn't valid. use alloy::hex is importing the macro, not the module
There was a problem hiding this comment.
@0xgleb Thank you for the correction! You're absolutely right. I can see that use alloy::hex; is importing the hex! macro (which is used in the code like hex!("734bc71c")), while use alloy::hex::ToHex; imports the trait. These can't be combined with the {self, ToHex} syntax since macros are imported differently than modules/traits.
✏️ Learnings added
Learnt from: 0xgleb
PR: rainlanguage/rain.orderbook#1846
File: crates/quote/src/quote.rs:309-317
Timestamp: 2025-05-20T15:34:06.703Z
Learning: In Rust, when importing from the alloy crate, `use alloy::hex;` imports the hex macro, not a module, so it can't be consolidated with imports like `use alloy::hex::ToHex;` using the grouped import syntax.
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.
3 participants
Motivation
tests in quote module of quote crate don't cover error cases
Solution
add error cases to the test coverage
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit