Use proper types for addresses and bytes for local db related logic#2318
Merged
Use proper types for addresses and bytes for local db related logic#2318
Conversation
Contributor
WalkthroughConverted many local-db and raindex client APIs and data models from string/Bytes representations to strongly-typed primitives (Address, B256, U256, u32/ u64) and added From conversions into SqlValue; updated SQL parameter bindings and extensive tests accordingly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/common/src/local_db/query/fetch_orders/mod.rs (1)
193-245: Tests validate clause insertion and parameter shapes after type changesThe updated tests confirm:
- active filter parameterization (
?1 = 'all'/'active'/'inactive'),- replacement of all chain/orderbook markers,
- lowercase normalization of bound orderbook address params, and
- that chain IDs are deduped at the SqlValue level.
They don’t yet assert the exact bound values for owner/token/order hash params, but they do verify the presence of the clauses and the overall param count/type, which is sufficient for this refactor’s scope.
Also applies to: 257-306, 308-379
4 tasks
0xgleb
requested changes
Nov 25, 2025
| block_number: U256::from(1), | ||
| block_timestamp: U256::from(2), | ||
| transaction_hash: "0xabc".into(), | ||
| transaction_hash: Bytes::from(vec![0x0a, 0xbc]), |
Collaborator
There was a problem hiding this comment.
transaction_hash should be B256, not dynamic size Bytes
| block_number: U256::from(block), | ||
| block_timestamp: Some(U256::from(1)), | ||
| transaction_hash: format!("0x{:x}", block), | ||
| transaction_hash: B256::from_str(&format!("0x{block:064x}")).unwrap(), |
Collaborator
There was a problem hiding this comment.
that's a very odd conversion. consider simply using b256! macro
| block_timestamp: Some(U256::from(2)), | ||
| transaction_hash: "0xaabbccddeeff00112233445566778899aabbccddeeff00112233445566778899" | ||
| .to_string(), | ||
| transaction_hash: B256::from_str( |
Collaborator
There was a problem hiding this comment.
consider using macros for these kind of cases. there are address!, b256!, fixed_bytes!, and bytes! macros available
| #[serde(alias = "order_owner")] | ||
| pub order_owner: String, | ||
| #[serde(alias = "order_nonce")] | ||
| pub orderbook_address: Address, |
Collaborator
There was a problem hiding this comment.
consider dropping the _address suffix now that the type signature communicates the same info
| pub order_owner: String, | ||
| #[serde(alias = "order_nonce")] | ||
| pub orderbook_address: Address, | ||
| pub order_hash: Bytes, |
| pub order_hash: String, | ||
| pub owner: String, | ||
| #[serde(alias = "blockTimestamp")] | ||
| pub order_hash: Bytes, |
| #[serde(alias = "transaction_hash")] | ||
| pub transaction_hash: String, | ||
| #[serde(alias = "log_index")] | ||
| pub transaction_hash: Bytes, |
| exec: &E, | ||
| ob_id: &OrderbookIdentifier, | ||
| order_hash: &str, | ||
| order_hash: Bytes, |
| exec: &E, | ||
| ob_id: &OrderbookIdentifier, | ||
| order_hash: &str, | ||
| order_hash: Bytes, |
| pub struct BlockResponse { | ||
| pub timestamp: U256, | ||
| pub hash: String, | ||
| pub hash: Bytes, |
0xgleb
approved these changes
Nov 27, 2025
hardyjosh
approved these changes
Dec 1, 2025
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/common/src/raindex_client/orders.rs (1)
445-474: Parse and use the CSV id field instead of hardcoding to a constant.The
try_from_local_db_orders_csvfunction reads theidfrom CSV but discards it (via_id_str) and always setsidto0x0000000000000000000000000000000000000000000000000000000000000001for all parsed orders. This destroys per-row identity and breaks downstream code that differentiates orders by ID. The function is actively used in vault parsing and theidfield is exposed via the WASM API.Apply the suggested fix: parse
id_str, convert it toB256, and use it in the struct. Also add test assertions for theidfield to prevent regression.
| use super::*; | ||
| use crate::raindex_client::orders::RaindexOrder; | ||
| use alloy::primitives::{hex::decode, Bytes}; | ||
| use alloy::primitives::{hex::decode, Bytes, B256}; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid re‑encoding order bytes before abi_decode
TryInto<OrderV4> for &RaindexOrder currently converts Bytes → hex String → Vec<u8> just to call abi_decode, which already works on raw bytes. You can simplify and remove the extra allocation:
-use alloy::primitives::{hex::decode, Bytes, B256};
+use alloy::primitives::{Bytes, B256};
@@
impl TryInto<OrderV4> for &RaindexOrder {
type Error = OrderDetailError;
fn try_into(self) -> Result<OrderV4, Self::Error> {
- let order = OrderV4::abi_decode(&decode(self.order_bytes().to_string())?)?;
- Ok(order)
+ // Decode directly from the stored ABI bytes.
+ let order = OrderV4::abi_decode(self.order_bytes().as_ref())?;
+ Ok(order)
}
}Please confirm that the OrderV4::abi_decode(&[u8]) overload is available in your alloy version; if not, keeping the existing code is fine functionally.
Also applies to: 117-122
🤖 Prompt for AI Agents
In crates/common/src/raindex_client/remove_orders.rs around lines 3 and again at
117-122, the code currently re-encodes order bytes by converting Bytes -> hex
String -> Vec<u8> before calling OrderV4::abi_decode; instead, pass the raw byte
slice directly to OrderV4::abi_decode (e.g. call
OrderV4::abi_decode(bytes.as_ref()) or &bytes[..]) and remove the hex::decode
step and intermediate allocations; apply the same replacement at both locations
and keep the rest of the conversion logic unchanged, but only do this if your
alloy version exposes OrderV4::abi_decode(&[u8]).
Comment on lines
+142
to
+150
| #[cfg(target_family = "wasm")] | ||
| let order_hash = B256::from_str(&self.order_hash())?; | ||
| #[cfg(not(target_family = "wasm"))] | ||
| let order_hash = B256::from_str(&self.order_hash().to_string())?; | ||
|
|
||
| let local_trades = fetch_order_trades( | ||
| &local_db, | ||
| &OrderbookIdentifier::new(chain_id, orderbook), | ||
| &order_hash, | ||
| order_hash, |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unnecessary string round-trip in non-WASM path.
In the non-WASM branch, self.order_hash() already returns B256 (per the external context showing pub fn order_hash(&self) -> B256). The conversion B256::from_str(&self.order_hash().to_string()) is redundant and could fail unnecessarily.
Apply this diff:
#[cfg(target_family = "wasm")]
let order_hash = B256::from_str(&self.order_hash())?;
#[cfg(not(target_family = "wasm"))]
-let order_hash = B256::from_str(&self.order_hash().to_string())?;
+let order_hash = self.order_hash();This matches the pattern used in get_trade_count at line 265, which directly assigns self.order_hash() for non-WASM.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[cfg(target_family = "wasm")] | |
| let order_hash = B256::from_str(&self.order_hash())?; | |
| #[cfg(not(target_family = "wasm"))] | |
| let order_hash = B256::from_str(&self.order_hash().to_string())?; | |
| let local_trades = fetch_order_trades( | |
| &local_db, | |
| &OrderbookIdentifier::new(chain_id, orderbook), | |
| &order_hash, | |
| order_hash, | |
| #[cfg(target_family = "wasm")] | |
| let order_hash = B256::from_str(&self.order_hash())?; | |
| #[cfg(not(target_family = "wasm"))] | |
| let order_hash = self.order_hash(); | |
| let local_trades = fetch_order_trades( | |
| &local_db, | |
| &OrderbookIdentifier::new(chain_id, orderbook), | |
| order_hash, |
🤖 Prompt for AI Agents
In crates/common/src/raindex_client/trades.rs around lines 142 to 150, the
non-WASM branch unnecessarily converts self.order_hash() to a string and
reparses it; instead, assign the B256 directly. Replace the non-wasm path so it
sets order_hash = self.order_hash() (matching the pattern used in
get_trade_count) and remove the to_string()/from_str round-trip to avoid
redundant work and potential parse errors.
Comment on lines
33
to
44
| pub struct LogEntryResponse { | ||
| pub address: String, | ||
| pub topics: Vec<String>, | ||
| pub data: String, | ||
| pub address: Address, | ||
| pub topics: Vec<Bytes>, | ||
| pub data: Bytes, | ||
| pub block_number: U256, | ||
| pub block_timestamp: Option<U256>, | ||
| pub transaction_hash: String, | ||
| pub transaction_hash: B256, | ||
| pub transaction_index: String, | ||
| pub block_hash: String, | ||
| pub block_hash: B256, | ||
| pub log_index: U256, | ||
| pub removed: bool, | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using B256 for topics instead of Vec<Bytes>.
Ethereum log topics are always 32-byte values (keccak256 hashes or padded values). Using Vec<B256> would be more semantically correct and provide compile-time size guarantees. However, if the current Vec<Bytes> approach works with the existing deserialization and downstream usage, this is a minor concern.
pub struct LogEntryResponse {
pub address: Address,
- pub topics: Vec<Bytes>,
+ pub topics: Vec<B256>,
pub data: Bytes,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub struct LogEntryResponse { | |
| pub address: String, | |
| pub topics: Vec<String>, | |
| pub data: String, | |
| pub address: Address, | |
| pub topics: Vec<Bytes>, | |
| pub data: Bytes, | |
| pub block_number: U256, | |
| pub block_timestamp: Option<U256>, | |
| pub transaction_hash: String, | |
| pub transaction_hash: B256, | |
| pub transaction_index: String, | |
| pub block_hash: String, | |
| pub block_hash: B256, | |
| pub log_index: U256, | |
| pub removed: bool, | |
| } | |
| pub struct LogEntryResponse { | |
| pub address: Address, | |
| pub topics: Vec<B256>, | |
| pub data: Bytes, | |
| pub block_number: U256, | |
| pub block_timestamp: Option<U256>, | |
| pub transaction_hash: B256, | |
| pub transaction_index: String, | |
| pub block_hash: B256, | |
| pub log_index: U256, | |
| pub removed: bool, | |
| } |
🤖 Prompt for AI Agents
In crates/common/src/rpc_client.rs around lines 33 to 44, the LogEntryResponse
struct uses topics: Vec<Bytes> but Ethereum topics are fixed 32-byte values;
change the type to topics: Vec<B256> and update imports/usages accordingly,
ensuring serde (or your deserialization layer) can map the incoming hex/bytes
into B256 (add/from_bytes or derive Deserialize for B256 or implement a custom
serde visitor if needed), update any downstream code that expects Vec<Bytes> to
use Vec<B256> (or convert where necessary), and run cargo build/tests to confirm
deserialization and consumers compile.
Contributor
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
Contributor
|
Tip For best results, initiate chat on the files or code changes. SIZE=L 🧠 Learnings used |
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Chained PR
Motivation
Some of the data structures we had used
Stringand&strtypes in place ofAddress,BytesandU256. This PR aims to fix that by introducing the proper types and refactoring the code base.Solution
SqlStatementfile to have proper from implementations for these new typesChecks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
Refactor
Public API
Tests
✏️ Tip: You can customize this high-level summary in your review settings.