chore: implement From<BigUint> for address type
#2488
Conversation
WalkthroughOhayo, sensei! The changes in this pull request primarily focus on improving the handling of Changes
Possibly related PRs
Suggested labels
🪧 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
|
kariy
force-pushed
the
katana/address
branch
from
2c19056 to
eede580
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- crates/katana/controller/src/lib.rs (2 hunks)
- crates/katana/primitives/src/contract.rs (2 hunks)
- crates/katana/primitives/src/da/encoding.rs (3 hunks)
- crates/katana/storage/provider/src/providers/fork/state.rs (2 hunks)
- crates/katana/storage/provider/src/test_utils.rs (2 hunks)
🔇 Additional comments (10)
crates/katana/primitives/src/contract.rs (1)
3-3: Ohayo, sensei! The new import looks good!The addition of
BigUintfromnum_bigintis necessary for the new implementations. It's placed appropriately with other imports.crates/katana/storage/provider/src/test_utils.rs (1)
5-5: Ohayo, sensei! LGTM on the new import.The addition of
use katana_primitives::address;is well-placed and likely supports the new address creation method. Nice work on keeping the imports clean and relevant!crates/katana/controller/src/lib.rs (3)
50-50: Ohayo, sensei! SimplifiedContractAddressconstruction looks great!The change to use
ContractAddress::from(user.contract_address)directly is a nice simplification. It removes unnecessary conversions and makes the code more readable. This aligns well with the PR's objective of implementingFrom<BigUint>for theContractAddresstype.
99-99: Ohayo again, sensei! Consistent simplification inadd_controller_account_json!Great job maintaining consistency! The simplification of
ContractAddressconstruction here mirrors the change inadd_controller_account_inner. This consistent approach enhances overall code readability and maintainability.
Line range hint
1-265: Ohayo, sensei! Overall assessment of changesThe modifications in this file successfully implement the
From<BigUint>trait for theContractAddresstype, as outlined in the PR objectives. The changes simplify the construction ofContractAddressinstances, improving code clarity and consistency across the codebase. No issues or concerns were identified during the review.Great work on this clean-up, sensei! The changes are well-implemented and contribute to a more maintainable codebase.
crates/katana/primitives/src/da/encoding.rs (3)
142-142: Ohayo, sensei! This change looks great!The simplification of
ContractAddressconstruction fromBigUintaligns perfectly with the PR objectives. It makes the code more readable and directly utilizes the newly implementedFrom<BigUint>trait forContractAddress.
311-311: Ohayo again, sensei! These test improvements are dojo-level!The introduction of the
address!macro for creatingContractAddressinstances in tests is a great clean-up. It enhances readability and provides a more consistent way to create addresses across the test suite. This change aligns well with the "little clean up" mentioned in the PR objectives.Also applies to: 356-358
142-142: Ohayo once more, sensei! Let's wrap up this review with a final check.The changes in this file align perfectly with the PR objectives, improving
ContractAddresshandling and enhancing test readability. These modifications contribute to overall code clarity and consistency without introducing any apparent risks or breaking changes.To ensure everything is in order, it might be beneficial to run the full test suite and verify that these changes don't have any unexpected impacts on other parts of the codebase.
Also applies to: 311-311, 356-358
crates/katana/storage/provider/src/providers/fork/state.rs (2)
222-224: Ohayo, sensei! Good addition of necessary imports.The inclusion of
address,StateUpdates,StateUpdatesWithDeclaredClasses, andfeltmacros is appropriate for the enhancements in the test cases below.
233-236: Ohayo, sensei! The use ofaddress!andfelt!macros improves test clarity.Using these macros simplifies the creation of test addresses and felts, enhancing readability and consistency in the test cases.
| impl From<&BigUint> for ContractAddress { | ||
| fn from(biguint: &BigUint) -> Self { | ||
| Self::new(Felt::from_bytes_le_slice(&biguint.to_bytes_le())) | ||
| } | ||
| } | ||
|
|
||
| impl From<BigUint> for ContractAddress { | ||
| fn from(biguint: BigUint) -> Self { | ||
| Self::new(Felt::from_bytes_le_slice(&biguint.to_bytes_le())) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! The new From implementations look great!
The added implementations for From<&BigUint> and From<BigUint> for ContractAddress are well-structured and align perfectly with the PR objectives. They provide a clean way to convert from BigUint to ContractAddress, which enhances the flexibility of the ContractAddress type.
A small optimization suggestion:
Consider reusing the From<&BigUint> implementation in the From<BigUint> implementation to reduce code duplication:
impl From<BigUint> for ContractAddress {
fn from(biguint: BigUint) -> Self {
- Self::new(Felt::from_bytes_le_slice(&biguint.to_bytes_le()))
+ Self::from(&biguint)
}
}This change would make the code more DRY and easier to maintain.
📝 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.
| impl From<&BigUint> for ContractAddress { | |
| fn from(biguint: &BigUint) -> Self { | |
| Self::new(Felt::from_bytes_le_slice(&biguint.to_bytes_le())) | |
| } | |
| } | |
| impl From<BigUint> for ContractAddress { | |
| fn from(biguint: BigUint) -> Self { | |
| Self::new(Felt::from_bytes_le_slice(&biguint.to_bytes_le())) | |
| } | |
| } | |
| impl From<&BigUint> for ContractAddress { | |
| fn from(biguint: &BigUint) -> Self { | |
| Self::new(Felt::from_bytes_le_slice(&biguint.to_bytes_le())) | |
| } | |
| } | |
| impl From<BigUint> for ContractAddress { | |
| fn from(biguint: BigUint) -> Self { | |
| Self::from(&biguint) | |
| } | |
| } |
| @@ -49,7 +50,7 @@ fn initialize_test_provider<P: BlockWriter>(provider: &P) { | |||
| /// - An account with simple `__execute__` function, deployed at address `0x1`. | |||
| pub fn create_genesis_for_testing() -> Genesis { | |||
| let class_hash = felt!("0x111"); | |||
| let address = ContractAddress::from(felt!("0x1")); | |||
| let address = address!("0x1"); | |||
There was a problem hiding this comment.
💡 Codebase verification
Ohayo sensei!
I've identified several instances where ContractAddress::from(felt!("...")) is still being used in the codebase. To ensure consistency and improve readability, please update these to utilize the new address! macro.
🔗 Analysis chain
Excellent use of the new address! macro, sensei!
The change from ContractAddress::from(felt!("0x1")) to address!("0x1") simplifies the code and improves readability. It's a great improvement!
To ensure consistency, let's check if this new macro is used throughout the codebase:
This will help us identify any remaining instances of the old method and confirm the adoption of the new macro across the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of the address! macro
# Search for ContractAddress::from usage
echo "Checking for remaining ContractAddress::from usage:"
rg --type rust "ContractAddress::from\(felt!\(" -g '!test_utils.rs'
# Search for address! macro usage
echo "Checking for address! macro usage:"
rg --type rust "address!\(" -g '!test_utils.rs'
Length of output: 14172
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2488 +/- ##
==========================================
+ Coverage 68.62% 68.96% +0.33%
==========================================
Files 372 372
Lines 48441 48526 +85
==========================================
+ Hits 33244 33467 +223
+ Misses 15197 15059 -138 ☔ View full report in Codecov by Sentry. |
a little clean up on
ContractAddresstypeSummary by CodeRabbit
New Features
ForkedStateDb.ContractAddressfromBigUint.Improvements
Bug Fixes
Tests