feat(torii): add eth address to ty and remove usize #2989
Conversation
|
Ohayo, sensei! WalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant PrimitiveInstance
Caller->>PrimitiveInstance: set_eth_address(Some(address))
PrimitiveInstance-->>Caller: Ok/Result
Caller->>PrimitiveInstance: as_eth_address()
PrimitiveInstance-->>Caller: Option(address)
Possibly related PRs
Suggested reviewers
Tip 🌐 Web search-backed reviews and chat
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/dojo/types/src/primitive.rs (2)
147-162: Consider adding validation for Ethereum address format.Ohayo sensei! While the numeric mapping is correct, consider adding validation to ensure the Ethereum address follows the standard format (20 bytes).
impl Primitive { + fn validate_eth_address(felt: &Felt) -> Result<(), PrimitiveError> { + let bytes = felt.to_bytes_be(); + if bytes[..12].iter().any(|&b| b != 0) { + return Err(PrimitiveError::InvalidByteLength(32, 20)); + } + Ok(()) + }
482-484: Enhance test coverage for EthAddress validation.Ohayo sensei! The basic tests are good, but consider adding tests for:
- Invalid Ethereum addresses (>20 bytes)
- Edge cases (zero address, max value address)
- Common Ethereum addresses (e.g., null address 0x0000...0000)
#[test] fn test_eth_address_validation() { // Test invalid address (>20 bytes) let mut primitive = Primitive::EthAddress(None); let invalid_felt = Felt::from_str("0x1234567890123456789012345678901234567890").unwrap(); assert!(primitive.set_eth_address(Some(invalid_felt)).is_err()); // Test zero address let zero_felt = Felt::ZERO; assert!(primitive.set_eth_address(Some(zero_felt)).is_ok()); }Also applies to: 510-513
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/dojo/types/src/primitive.rs(9 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/dojo/types/src/primitive.rs
[warning] 141-141: Formatting issue detected. Please ensure proper formatting.
[warning] 507-507: Formatting issue detected. Please ensure proper formatting.
🔇 Additional comments (4)
crates/dojo/types/src/primitive.rs (4)
47-48: LGTM! The EthAddress variant is properly defined.Ohayo! The new
EthAddressvariant is correctly added with proper attributes and type definition.
126-126: LGTM! Getter and setter macros are properly implemented.The
as_eth_addressandset_eth_addressmacros are correctly generated using the existing macro templates.Also applies to: 143-143
189-190: LGTM! SQL type mapping is appropriate.The
EthAddressis correctly mapped toSqlType::Textwhich is appropriate for storing Ethereum addresses.
141-141: Fix formatting issues flagged by CI.Ohayo! The CI pipeline has flagged formatting issues. Please run
cargo fmtto fix them.Also applies to: 507-507
🧰 Tools
🪛 GitHub Actions: ci
[warning] 141-141: Formatting issue detected. Please ensure proper formatting.
| Primitive::EthAddress(ref mut value) => { | ||
| *value = Some(felts.remove(0)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding address validation in serialization/deserialization.
Ohayo sensei! The serialization/deserialization logic should validate the Ethereum address format.
Primitive::EthAddress(ref mut value) => {
- *value = Some(felts.remove(0));
+ let felt = felts.remove(0);
+ Self::validate_eth_address(&felt)?;
+ *value = Some(felt);
}Also applies to: 397-399
Larkooo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/torii/typed-data/src/test.rs (1)
65-68: Enhance test coverage with real Ethereum addresses.Ohayo sensei! The test should include actual Ethereum addresses to ensure proper handling of real-world data.
Add these test cases:
+ // Test with a real Ethereum address + let mut ty = Ty::Primitive(Primitive::EthAddress(None)); + let value = PrimitiveType::String("0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045".to_string()); + parse_value_to_ty(&value, &mut ty).unwrap(); + assert_eq!(ty, Ty::Primitive(Primitive::EthAddress(Some(Felt::from_hex_be("0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045").unwrap())))); + + // Test with an invalid Ethereum address + let mut ty = Ty::Primitive(Primitive::EthAddress(None)); + let value = PrimitiveType::String("invalid".to_string()); + assert!(parse_value_to_ty(&value, &mut ty).is_err());crates/dojo/types/src/primitive.rs (1)
47-48: Document the EthAddress variant.Ohayo sensei! Consider adding documentation to explain the expected format and validation rules for Ethereum addresses.
Add this documentation:
#[strum(serialize = "EthAddress")] +/// Represents an Ethereum address as a Felt. +/// The address must be 20 bytes (40 hex characters) long. EthAddress(Option<Felt>),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
crates/dojo/types/src/primitive.rs(9 hunks)crates/dojo/types/src/schema.rs(3 hunks)crates/torii/grpc/proto/schema.proto(1 hunks)crates/torii/grpc/src/types/schema.rs(2 hunks)crates/torii/sqlite/src/model.rs(1 hunks)crates/torii/typed-data/src/test.rs(2 hunks)crates/torii/typed-data/src/typed_data.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build
- GitHub Check: ensure-wasm
- GitHub Check: clippy
- GitHub Check: docs
🔇 Additional comments (8)
crates/dojo/types/src/primitive.rs (1)
331-333: Consider adding address validation in serialization/deserialization.crates/dojo/types/src/schema.rs (3)
364-364: Ohayo! The EthAddress JSON serialization looks good, sensei!The implementation follows the same pattern as other address types, using hex formatting.
481-485: The EthAddress deserialization is well-implemented, sensei!The implementation correctly parses the string representation into a Felt value.
742-746: Nice formatting implementation for EthAddress, sensei!The formatting follows the consistent pattern used for other address types.
crates/torii/typed-data/src/typed_data.rs (3)
610-612: Ohayo! The EthAddress parsing looks good, sensei!The implementation correctly handles string-to-Felt conversion for Ethereum addresses.
683-685: Clean implementation of EthAddress mapping, sensei!The conversion to string representation follows the same pattern as other address types.
740-743: Well-structured type mapping for EthAddress, sensei!The field type is correctly mapped to "EthAddress" string.
crates/torii/grpc/proto/schema.proto (1)
28-32: Ohayo! The protobuf field changes look good, sensei!The changes maintain backward compatibility by:
- Removing the usize field
- Shifting subsequent fields (bool, felt252, class_hash, contract_address)
- Adding eth_address as the last field
Let's verify that there are no breaking changes in the field numbers:
✅ Verification successful
Ohayo sensei! It looks like our initial inspection hit a snag due to the file type issue. Let's re-run the verification using a revised shell script that avoids the proto file type limitation and directly targets the expected changes.
Please run this script for manual verification. Once confirmed that there are no stray
"usize"references and the fields inschema.protoreflect the intended update and ordering, we can conclude that the changes are consistent and maintain backward compatibility.
Ohayo sensei! The schema changes are looking solid. We verified that:
- There are no stray references to
usizein any.protofiles.- The relevant section of
schema.proto(lines 25-35) shows the expected field ordering.- The fields have the correct numbering:
bool = 12,felt252 = 13,class_hash = 14,contract_address = 15, andeth_address = 16.Everything checks out as intended.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any references to the old field numbers # This helps ensure that all client code has been updated rg -A 5 "field.*=.*1[2-6]" --type protoLength of output: 71
Script:
#!/bin/bash # 1. Check for any residual "usize" references in all .proto files. echo "Searching for 'usize' in .proto files..." rg "usize" --glob "**/*.proto" # 2. Display the relevant section of schema.proto to verify the field ordering and numbering. echo "Showing lines 25-35 of crates/torii/grpc/proto/schema.proto:" sed -n '25,35p' crates/torii/grpc/proto/schema.proto # 3. Verify that the expected field numbers are present in schema.proto. echo "Verifying field numbering in schema.proto for expected fields:" rg "bool = 12|felt252 = 13|class_hash = 14|contract_address = 15|eth_address = 16" crates/torii/grpc/proto/schema.protoLength of output: 955
| Primitive::EthAddress(felt) => proto::types::primitive::PrimitiveType::EthAddress( | ||
| felt.unwrap_or_default().to_bytes_be().to_vec(), | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for Ethereum address serialization.
The serialization should ensure the address is properly formatted.
Apply this diff to add validation:
Primitive::EthAddress(felt) => proto::types::primitive::PrimitiveType::EthAddress(
- felt.unwrap_or_default().to_bytes_be().to_vec(),
+ {
+ let bytes = felt.unwrap_or_default().to_bytes_be();
+ // Take only the last 20 bytes for Ethereum address
+ bytes[12..].to_vec()
+ }
),📝 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.
| Primitive::EthAddress(felt) => proto::types::primitive::PrimitiveType::EthAddress( | |
| felt.unwrap_or_default().to_bytes_be().to_vec(), | |
| ), | |
| Primitive::EthAddress(felt) => proto::types::primitive::PrimitiveType::EthAddress( | |
| { | |
| let bytes = felt.unwrap_or_default().to_bytes_be(); | |
| // Take only the last 20 bytes for Ethereum address | |
| bytes[12..].to_vec() | |
| } | |
| ), |
| proto::types::primitive::PrimitiveType::EthAddress(felt) => { | ||
| Primitive::EthAddress(Some(Felt::from_bytes_be_slice(felt.as_slice()))) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding Ethereum address validation.
Ohayo sensei! The conversion from bytes to Ethereum address should validate the address format to prevent invalid addresses.
Apply this diff to add validation:
proto::types::primitive::PrimitiveType::EthAddress(felt) => {
- Primitive::EthAddress(Some(Felt::from_bytes_be_slice(felt.as_slice())))
+ let address = Felt::from_bytes_be_slice(felt.as_slice());
+ // Validate that the address is 20 bytes (40 hex chars)
+ if felt.len() != 20 {
+ return Err(SchemaError::InvalidByteLength(felt.len(), 20));
+ }
+ Primitive::EthAddress(Some(address))
}📝 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.
| proto::types::primitive::PrimitiveType::EthAddress(felt) => { | |
| Primitive::EthAddress(Some(Felt::from_bytes_be_slice(felt.as_slice()))) | |
| } | |
| proto::types::primitive::PrimitiveType::EthAddress(felt) => { | |
| let address = Felt::from_bytes_be_slice(felt.as_slice()); | |
| // Validate that the address is 20 bytes (40 hex chars) | |
| if felt.len() != 20 { | |
| return Err(SchemaError::InvalidByteLength(felt.len(), 20)); | |
| } | |
| Primitive::EthAddress(Some(address)) | |
| } |
crates/torii/sqlite/src/model.rs
Outdated
| Primitive::EthAddress(_) => { | ||
| let value = row.try_get::<String, &str>(column_name)?; | ||
| if !value.is_empty() { | ||
| primitive.set_eth_address(Some( | ||
| Felt::from_str(&value).map_err(ParseError::FromStr)?, | ||
| ))?; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add Ethereum address validation when mapping from SQL.
Ohayo sensei! The mapping should validate that the string from SQL represents a valid Ethereum address.
Apply this diff to add validation:
Primitive::EthAddress(_) => {
let value = row.try_get::<String, &str>(column_name)?;
if !value.is_empty() {
+ // Validate Ethereum address format (0x followed by 40 hex chars)
+ if !value.starts_with("0x") || value.len() != 42 {
+ return Err(Error::Parse(ParseError::FromStr(
+ "Invalid Ethereum address format".into(),
+ )));
+ }
primitive.set_eth_address(Some(
Felt::from_str(&value).map_err(ParseError::FromStr)?,
))?;
}
}📝 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.
| Primitive::EthAddress(_) => { | |
| let value = row.try_get::<String, &str>(column_name)?; | |
| if !value.is_empty() { | |
| primitive.set_eth_address(Some( | |
| Felt::from_str(&value).map_err(ParseError::FromStr)?, | |
| ))?; | |
| } | |
| } | |
| Primitive::EthAddress(_) => { | |
| let value = row.try_get::<String, &str>(column_name)?; | |
| if !value.is_empty() { | |
| // Validate Ethereum address format (0x followed by 40 hex chars) | |
| if !value.starts_with("0x") || value.len() != 42 { | |
| return Err(Error::Parse(ParseError::FromStr( | |
| "Invalid Ethereum address format".into(), | |
| ))); | |
| } | |
| primitive.set_eth_address(Some( | |
| Felt::from_str(&value).map_err(ParseError::FromStr)?, | |
| ))?; | |
| } | |
| } |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2989 +/- ##
==========================================
- Coverage 56.97% 56.96% -0.02%
==========================================
Files 426 426
Lines 56376 56381 +5
==========================================
- Hits 32119 32116 -3
- Misses 24257 24265 +8 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
New Features
EthAddressvariant to thePrimitiveenum, replacing the previousUSizevariant.Bug Fixes
USizetype across various functionalities, ensuring consistent handling of primitive types.Tests
USizetype.