Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: implement From<BigUint> for address type #2488

Merged
merged 1 commit into from
Sep 30, 2024
+24 −11
Merged

chore: implement From<BigUint> for address type #2488

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions crates/katana/controller/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,7 @@
storage: Some(get_contract_storage(credential_id, public_key)?),
};

let address =
ContractAddress::from(Felt::from_bytes_be(&user.contract_address.to_bytes_be()));
let address = ContractAddress::from(user.contract_address);

(address, GenesisAllocation::Contract(account))
};
Expand Down Expand Up @@ -97,9 +96,7 @@
storage: Some(get_contract_storage(credential_id, public_key)?),
};

let address = ContractAddress::from(Felt::from_bytes_be(
&user.account.contract_address.to_bytes_be(),
));
let address = ContractAddress::from(user.account.contract_address);

Check warning on line 99 in crates/katana/controller/src/lib.rs

View check run for this annotation

Codecov / codecov/patch

crates/katana/controller/src/lib.rs#L99 

Added line #L99 was not covered by tests

(address, account)
};
Expand Down
13 changes: 13 additions & 0 deletions crates/katana/primitives/src/contract.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::fmt;

use num_bigint::BigUint;
use starknet::core::utils::normalize_address;

use crate::class::ClassHash;
Expand Down Expand Up @@ -50,6 +51,18 @@
}
}

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()))
}

Check warning on line 63 in crates/katana/primitives/src/contract.rs

View check run for this annotation

Codecov / codecov/patch

crates/katana/primitives/src/contract.rs#L61-L63 

Added lines #L61 - L63 were not covered by tests
}
Comment on lines +54 to +64
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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)
}
}


#[macro_export]
macro_rules! address {
($value:expr) => {
Expand Down
9 changes: 5 additions & 4 deletions crates/katana/primitives/src/da/encoding.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ pub fn decode_state_updates(value: &[BigUint]) -> Result<StateUpdates, EncodingE

for _ in 0..total_updates {
let address = iter.next().ok_or(EncodingError::MissingAddress)?;
let address: ContractAddress = Felt::from(address).into();
let address = ContractAddress::from(address);

let metadata = iter.next().ok_or(EncodingError::MissingMetadata)?;
let metadata = Metadata::decode(metadata)?;
Expand Down Expand Up @@ -308,6 +308,7 @@ mod tests {
use starknet::macros::felt;

use super::*;
use crate::address;

macro_rules! biguint {
($s:expr) => {
Expand Down Expand Up @@ -352,9 +353,9 @@ mod tests {
assert_eq!(state_updates.declared_classes.len(), 1);
assert_eq!(state_updates.deployed_contracts.len(), 0);

let address: ContractAddress =
felt!("2019172390095051323869047481075102003731246132997057518965927979101413600827")
.into();
let address = address!(
"2019172390095051323869047481075102003731246132997057518965927979101413600827"
);

assert_eq!(state_updates.nonce_updates.get(&address), Some(&Felt::ONE));

Expand Down
3 changes: 2 additions & 1 deletion crates/katana/storage/provider/src/providers/fork/state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,7 @@ impl ContractClassProvider for ForkedSnapshot {
mod tests {
use std::collections::BTreeMap;

use katana_primitives::address;
use katana_primitives::state::{StateUpdates, StateUpdatesWithDeclaredClasses};
use starknet::macros::felt;

Expand All @@ -229,7 +230,7 @@ mod tests {
fn test_get_nonce() {
let backend = create_forked_backend("http://localhost:8080", 1);

let address: ContractAddress = felt!("1").into();
let address = address!("1");
let class_hash = felt!("11");
let remote_nonce = felt!("111");
let local_nonce = felt!("1111");
Expand Down
3 changes: 2 additions & 1 deletion crates/katana/storage/provider/src/test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

use alloy_primitives::U256;
use katana_db::mdbx::{test_utils, DbEnvKind};
use katana_primitives::address;
use katana_primitives::block::{BlockHash, FinalityStatus};
use katana_primitives::class::CompiledClass;
use katana_primitives::contract::ContractAddress;
Expand Down Expand Up @@ -49,7 +50,7 @@
/// - 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");

Check warning on line 53 in crates/katana/storage/provider/src/test_utils.rs

View check run for this annotation

Codecov / codecov/patch

crates/katana/storage/provider/src/test_utils.rs#L53 

Added line #L53 was not covered by tests
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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


// TODO: we should have a genesis builder that can do all of this for us.
let class = {
Expand Down
Loading