feat: Allow complex calldata when initializing contracts #2625
Conversation
WalkthroughOhayo, sensei! This pull request introduces updates primarily focused on dependency management in the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 0
🧹 Outside diff range and nitpick comments (2)
bin/sozo/src/commands/call.rs (1)
14-14: Ohayo sensei! Good architectural decision on the module reorganization!Moving
calldata_decodertodojo_world::configimproves the codebase organization by:
- Making the decoder more accessible to other crates
- Placing it in a more appropriate location given its configuration-related purpose
crates/sozo/ops/src/migrate/mod.rs (1)
171-172: Consider enhancing error context for better debugging.Sensei, the current error handling using
map_err(|_| MigrationError::InitCallArgs)discards valuable context about what failed during decoding. Consider preserving the original error details.- decode_calldata(&args.join(",")) - .map_err(|_| MigrationError::InitCallArgs)? + decode_calldata(&args.join(",")) + .map_err(|e| MigrationError::InitCallArgs(format!("Failed to decode calldata: {}", e)))?This would require updating the
MigrationErrorenum to include the error context:pub enum MigrationError<E> { InitCallArgs(String), // ... other variants }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.toml(2 hunks)bin/sozo/src/commands/call.rs(1 hunks)bin/sozo/src/commands/execute.rs(1 hunks)bin/sozo/src/commands/mod.rs(0 hunks)crates/dojo/world/Cargo.toml(1 hunks)crates/dojo/world/src/config/mod.rs(1 hunks)crates/sozo/ops/src/migrate/mod.rs(2 hunks)
💤 Files with no reviewable changes (1)
- bin/sozo/src/commands/mod.rs
✅ Files skipped from review due to trivial changes (1)
- bin/sozo/src/commands/execute.rs
🔇 Additional comments (8)
crates/dojo/world/src/config/mod.rs (1)
1-1: Ohayo sensei! LGTM - Clean module organization.
The addition of calldata_decoder as a public module aligns well with the PR objectives to support complex calldata types during initialization. The module organization follows Rust best practices.
Let's verify the module's integration:
✅ Verification successful
Ohayo sensei! LGTM - Module integration verified successfully
The calldata_decoder module is properly integrated and used across the codebase:
- Located at expected path:
crates/dojo/world/src/config/calldata_decoder.rs - Actively used in key command implementations:
bin/sozo/src/commands/execute.rsbin/sozo/src/commands/call.rscrates/sozo/ops/src/migrate/mod.rs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the calldata_decoder module integration and usage
# Test 1: Check if the module file exists
fd --type f "calldata_decoder\.rs$" crates/dojo/world/src/config/
# Test 2: Verify the module's usage across the codebase
rg -l "use.*config::calldata_decoder"
Length of output: 254
crates/dojo/world/Cargo.toml (1)
27-29: Ohayo! The new dependencies look good, sensei!
The addition of hex, hex-literal, and num-bigint aligns perfectly with the PR's objective of supporting felt252 short strings in calldata. These are essential for hex string processing and arbitrary-precision integer operations.
Let's verify these dependencies are being used appropriately:
✅ Verification successful
Ohayo! Dependencies are being used effectively, sensei!
The verification shows that the new dependencies are actively used in the codebase, particularly in calldata_decoder.rs where:
hexis used for decoding hex strings in U256 processinghex-literalis used for constant definition (u128_max_plus_1)num-bigint::BigUintis used for handling large numbers in U256 decoding
The dependencies are not just added but are properly integrated into the codebase's functionality, especially for handling complex calldata types as intended.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of new dependencies in the codebase
# Expected: Find usages in calldata processing related code
echo "Checking hex/hex-literal usage..."
rg -l 'use hex::|use hex_literal::|hex::decode|hex::encode'
echo "Checking num-bigint usage..."
rg -l 'use num_bigint::|BigInt|BigUint'
Length of output: 831
Script:
#!/bin/bash
# Let's check the actual usage in calldata_decoder.rs since it's the main file we're interested in
echo "Checking detailed usage in calldata_decoder.rs..."
rg -A 5 'use (hex|hex_literal|num_bigint)' crates/dojo/world/src/config/calldata_decoder.rs
echo "\nChecking implementations using these dependencies..."
rg -C 10 '(hex::|BigInt|BigUint)' crates/dojo/world/src/config/calldata_decoder.rs
Length of output: 3113
bin/sozo/src/commands/call.rs (1)
Line range hint 24-35: Ohayo sensei! Perfect implementation aligning with PR objectives!
The calldata handling implementation successfully:
- Supports felt252 short strings through the
sstrprefix - Provides a generic solution for various types (u256, str, int)
- Maintains consistency with
sozo --calldatacommand style
Cargo.toml (3)
172-172: Ohayo sensei! The hex-literal dependency addition looks good!
The hex-literal crate is a perfect fit for handling felt252 short strings in calldata, which aligns well with the PR objectives.
182-182: The num-bigint dependency is well-chosen, sensei!
The num-bigint crate is essential for handling arbitrary-precision integers needed for felt252 operations. The version 0.4.3 is stable and maintained.
Line range hint 172-182: Verify dependency compatibility with existing crates
Let's ensure these new dependencies don't conflict with existing ones, particularly with the crypto and numeric libraries already in use.
crates/sozo/ops/src/migrate/mod.rs (2)
25-25: LGTM! Import addition aligns with feature objectives.
Ohayo! The new import for decode_calldata is well-placed and supports the PR's goal of enabling complex calldata types during contract initialization.
171-172: Implementation looks good, but let's verify the comma handling.
The new implementation using decode_calldata successfully enables complex calldata types. However, we should verify how it handles arguments that might contain commas themselves.
✅ Verification successful
Implementation is safe for comma handling, sensei! 🍜
After reviewing the decode_calldata implementation, I can confirm that commas in arguments are properly handled through type prefixes. The decoder supports:
- String values with
str:prefix (e.g.,str:hello, world) - Short strings with
sstr:prefix - Numbers with
u256:orint:prefix - Default hex/decimal values
The test case test_str_decoder_long demonstrates that strings containing spaces and commas are correctly processed. The implementation is robust and handles complex calldata types safely.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential comma-containing arguments in init_call_args
# Look for string literals in init_call_args that might contain commas
rg -A 3 'init_call_args.*=.*".*,.*"'
Length of output: 38
Script:
#!/bin/bash
# Let's search for any usage of init_call_args to understand the context better
rg -A 5 "init_call_args"
# Also check for any tests or examples using decode_calldata
rg -A 5 "decode_calldata"
# Look for the implementation of decode_calldata to understand its behavior
ast-grep --pattern 'fn decode_calldata($$$) { $$$ }'
Length of output: 13481
Script:
#!/bin/bash
# Let's check the implementation of decode_calldata and its item delimiter
cat crates/dojo/world/src/config/calldata_decoder.rs
# Also check for any test cases that might involve commas
rg -A 10 "test_.*decoder.*" crates/dojo/world/src/config/calldata_decoder.rs
Length of output: 14152
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2625 +/- ##
==========================================
- Coverage 57.25% 56.89% -0.36%
==========================================
Files 396 397 +1
Lines 49162 49464 +302
==========================================
- Hits 28148 28143 -5
- Misses 21014 21321 +307 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks @edisontim for tackling that!
Some comments that require changes, and then let's move on!
| // The injection of class hash and addresses is no longer supported since the | ||
| // world contains an internal DNS. | ||
| let args = if let Some(args) = init_call_args { |
There was a problem hiding this comment.
The comment can be updated where we can remove that "only felts are supported".
We can keep the comment related to the DNS, since injection of other contract addresses are not yet supported again.
| hex.workspace = true | ||
| hex-literal.workspace = true | ||
| num-bigint.workspace = true |
There was a problem hiding this comment.
those weren't removed from the sozo/Cargo.toml.
There was a problem hiding this comment.
Actually, could you uncomment and add to the sozo/main.rs this?
#![warn(unused_crate_dependencies)]This will ensure we removed unused crates. 👍
There was a problem hiding this comment.
My bad, I forgot to check afterwards what I had taken out!
There was a problem hiding this comment.
Thanks for the cleanup @edisontim! Good feature to have better init_calldata passed to contracts. :)
Closes #2587
Summary by CodeRabbit
New Features
hex-literal,num-bigint, andhexin theCargo.tomlfiles.calldata_decoderto expand functionality.Improvements
Refactor
calldata_decoderto reflect a reorganization in the codebase.Bug Fixes
snapboxmodule from tests.