BlockNumber as integer rather than Hex for Sonic RPC #72
Conversation
Enhance Quantity deserialization to accept numeric values Updated the QuantityVisitor to handle both hex strings and numeric values (u64 and i64) for deserialization. Added tests to verify the correct handling of numeric JSON values.
WalkthroughQuantity deserialization in hypersync-format now accepts numeric JSON tokens (u64/i64) in addition to hex strings via deserialize_any; Visitor handlers for u64 and i64 canonicalize big-endian bytes (i64 negatives error) and produce a Quantity. Cargo package version bumped. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant JSON as JSON Input
participant Serde as Serde Deserializer
participant Visitor as QuantityVisitor
participant Qty as Quantity
JSON->>Serde: Token (hex string | u64 | i64)
Serde->>Visitor: deserialize_any(...)
alt Hex string
Visitor->>Visitor: decode hex -> canonicalize bytes
Visitor->>Qty: construct Quantity
else u64
Visitor->>Visitor: u64.to_be_bytes() -> canonicalize bytes
Visitor->>Qty: construct Quantity
else i64
alt negative
Visitor-->>Serde: error ("expected non-negative")
else non-negative
Visitor->>Visitor: cast to u64 -> to_be_bytes() -> canonicalize bytes
Visitor->>Qty: construct Quantity
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ 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). (2)
✨ Finishing Touches🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
hypersync-format/src/types/quantity.rs (3)
27-34: ethers feature: U256 -> Quantity construction writes into a zero-length buffer
Box::new([])creates an empty slice;to_big_endianwrites nothing, yielding an empty Quantity, violating invariants elsewhere. Construct into a fixed 32-byte buffer and canonicalize.#[cfg(feature = "ethers")] impl From<ethabi::ethereum_types::U256> for Quantity { fn from(value: ethabi::ethereum_types::U256) -> Self { - let mut buf = Box::new([]); - value.to_big_endian(buf.as_mut()); - Self(buf) + let mut tmp = [0u8; 32]; + value.to_big_endian(&mut tmp); + // Strip leading zeros to maintain canonical representation + let start = tmp.iter().position(|&b| b != 0).unwrap_or(tmp.len() - 1); + Self(tmp[start..].into()) } }
49-56: ethers feature: U64 -> Quantity has the same zero-length buffer issueSame fix as U256 with an 8-byte buffer.
#[cfg(feature = "ethers")] impl From<ethabi::ethereum_types::U64> for Quantity { fn from(value: ethabi::ethereum_types::U64) -> Self { - let mut buf = Box::new([]); - value.to_big_endian(buf.as_mut()); - Self(buf) + let mut tmp = [0u8; 8]; + value.to_big_endian(&mut tmp); + let start = tmp.iter().position(|&b| b != 0).unwrap_or(tmp.len() - 1); + Self(tmp[start..].into()) } }
62-69: ethers feature: Wrong length check for U64 (should be 8, not 32)This allows oversize inputs to slip through or erroneously rejects correct ones depending on context. Fix to 8 bytes for U64.
- if value.0.len() > 32 { + if value.0.len() > 8 { return Err(()); }
🧹 Nitpick comments (4)
hypersync-format/src/types/quantity.rs (4)
98-98: Tighten the expectation message to reflect unsigned-only acceptanceWe reject negative i64s below, so the expectation can be more precise: “hex string or non-negative integer.” Small wording tweak improves error clarity.
Apply:
- formatter.write_str("hex string or numeric value for a quantity") + formatter.write_str("hex string (0x…) or non-negative integer for a quantity")
110-117: Avoid heap allocation in visit_u64; build a canonical slice and use From<&[u8]>Current code allocates a Vec and then canonicalizes. We can canonicalize by slicing the stack array produced by to_be_bytes(), cutting a heap alloc and a copy.
- // Convert the integer to big-endian bytes and canonicalize - let buf = canonicalize_bytes(value.to_be_bytes().to_vec()); - Ok(Quantity::from(buf)) + // Convert to big-endian bytes, slice off leading zeros, construct from slice + let bytes = value.to_be_bytes(); + let start = bytes.iter().position(|&b| b != 0).unwrap_or(bytes.len() - 1); + Ok(Quantity::from(&bytes[start..]))
119-129: Use structured Serde error for negatives and avoid allocation (mirror u64 path)Return E::invalid_value with Unexpected::Signed for better diagnostics, and avoid Vec allocation like in visit_u64.
- if value < 0 { - return Err(E::custom("negative quantity not allowed")); - } - let unsigned = value as u64; - let buf = canonicalize_bytes(unsigned.to_be_bytes().to_vec()); - Ok(Quantity::from(buf)) + if value < 0 { + return Err(E::invalid_value(de::Unexpected::Signed(value), &"non-negative integer quantity")); + } + let unsigned = value as u64; + let bytes = unsigned.to_be_bytes(); + let start = bytes.iter().position(|&b| b != 0).unwrap_or(bytes.len() - 1); + Ok(Quantity::from(&bytes[start..]))
301-306: Add a negative i64 test and a boundary testWe reject negatives; add an assertion to lock this in. Also add i64::MAX to verify upper-bound handling.
#[test] fn test_deserialize_numeric_i64() { assert_de_tokens(&Quantity::from(hex!("66a7c725")), &[Token::I64(0x66a7c725)]); assert_de_tokens(&Quantity::from(vec![0]), &[Token::I64(0)]); assert_de_tokens(&Quantity::from(hex!("01")), &[Token::I64(1)]); } + + #[test] + fn test_deserialize_numeric_i64_negative_rejected() { + use serde_test::assert_de_tokens_error; + assert_de_tokens_error::<Quantity>(&[Token::I64(-1)], "non-negative integer"); + } + + #[test] + fn test_deserialize_numeric_i64_max() { + assert_de_tokens( + &Quantity::from(i64::MAX.to_be_bytes().as_slice()), + &[Token::I64(i64::MAX)], + ); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
hypersync-format/src/types/quantity.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
hypersync-format/src/types/quantity.rs (2)
hypersync-format/src/types/util.rs (1)
canonicalize_bytes(11-22)hypersync-format/src/types/fixed_size_data.rs (1)
deserialize(173-178)
⏰ 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). (3)
- GitHub Check: lint
- GitHub Check: test_release
- GitHub Check: test_dev
🔇 Additional comments (2)
hypersync-format/src/types/quantity.rs (2)
137-139: Switch to deserialize_any is appropriate and scopedThis enables U64/I64 tokens without widening acceptance to floats or other token kinds (no visit_f64 implemented), matching the PR goal. Looks good.
293-299: Good coverage for U64 numeric deserializationSolid checks for zero, one, and a representative larger value. This validates canonicalization and acceptance of numeric JSON tokens.
| // Accept either hex strings (0x...) or numeric JSON values | ||
| deserializer.deserialize_any(QuantityVisitor) |
There was a problem hiding this comment.
Not a big deal but probably would be better to call deserialize_str first, match and if there's an error call deserialize_u64
Then there's no overhead in it just running all the visit functions. You control the entry point. And you don't need visit_i64 for eg.
There was a problem hiding this comment.
I actually think this would be a worthwhile thing to do since we use quantity for a lot of fields
Enhance Quantity deserialization to accept numeric values
Updated the QuantityVisitor to handle both hex strings and numeric values (u64 and i64) for deserialization. Added tests to verify the correct handling of numeric JSON values.
Summary by CodeRabbit
Improvements
Tests