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

refactor(katana-primitives): replace HashMap with BTreeMap in state updates and genesis types #2469

Merged
merged 4 commits into from
Sep 24, 2024
Merged

refactor(katana-primitives): replace HashMap with BTreeMap in state updates and genesis types #2469

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4c3fd65
use btreemap everywhere in genesis and state updates
kariy Sep 23, 2024
16913d4
fmt
kariy Sep 23, 2024
f46c011
fix test
kariy Sep 23, 2024
d9cb0a4
fix test
kariy Sep 24, 2024
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
9 changes: 5 additions & 4 deletions bin/saya/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ async fn test_program_input_from_program_output() -> anyhow::Result<()> {
],
state_updates: StateUpdates {
nonce_updates: {
let mut map = std::collections::HashMap::new();
let mut map = std::collections::BTreeMap::new();
map.insert(
ContractAddress::from(Felt::from_str("1111").unwrap()),
Felt::from_str("22222").unwrap(),
Expand All @@ -84,19 +84,20 @@ async fn test_program_input_from_program_output() -> anyhow::Result<()> {
)]
.into_iter()
.collect(),
contract_updates: {
let mut map = std::collections::HashMap::new();
deployed_contracts: {
let mut map = std::collections::BTreeMap::new();
map.insert(
ContractAddress::from(Felt::from_str("66666").unwrap()),
Felt::from_str("7777").unwrap(),
);
map
},
declared_classes: {
let mut map = std::collections::HashMap::new();
let mut map = std::collections::BTreeMap::new();
map.insert(Felt::from_str("88888").unwrap(), Felt::from_str("99999").unwrap());
map
},
..Default::default()
},
world_da: None,
};
Expand Down
6 changes: 3 additions & 3 deletions crates/katana/controller/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::collections::HashMap;
use std::collections::BTreeMap;

use alloy_primitives::U256;
use anyhow::Result;
Expand Down Expand Up @@ -131,7 +131,7 @@ pub mod json {
fn get_contract_storage(
credential_id: CredentialID,
public_key: CoseKey,
) -> Result<HashMap<StorageKey, StorageValue>> {
) -> Result<BTreeMap<StorageKey, StorageValue>> {
use slot::account_sdk::signers::DeviceError;
use webauthn_rs_proto::auth::PublicKeyCredentialRequestOptions;
use webauthn_rs_proto::{
Expand Down Expand Up @@ -186,7 +186,7 @@ fn get_contract_storage(
let storage = get_storage_var_address(MULTIPLE_OWNERS_COMPONENT_SUB_STORAGE, &[guid])?;

// 1 for boolean True in Cairo. Refer to the provided link above.
let storage_mapping = HashMap::from([(storage, Felt::ONE)]);
let storage_mapping = BTreeMap::from([(storage, Felt::ONE)]);

Ok(storage_mapping)
}
Expand Down
36 changes: 21 additions & 15 deletions crates/katana/executor/src/implementation/blockifier/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::collections::{BTreeMap, HashMap};
use std::collections::BTreeMap;
use std::num::NonZeroU128;
use std::sync::Arc;

Expand Down Expand Up @@ -403,12 +403,15 @@ pub(super) fn state_update_from_cached_state<S: StateDb>(

let state_diff = state.0.lock().inner.to_state_diff().unwrap();

let mut declared_compiled_classes: HashMap<katana_primitives::class::ClassHash, CompiledClass> =
HashMap::new();
let mut declared_sierra_classes: HashMap<
let mut declared_compiled_classes: BTreeMap<
katana_primitives::class::ClassHash,
CompiledClass,
> = BTreeMap::new();

let mut declared_sierra_classes: BTreeMap<
katana_primitives::class::ClassHash,
FlattenedSierraClass,
> = HashMap::new();
> = BTreeMap::new();

for class_hash in state_diff.compiled_class_hashes.keys() {
let hash = class_hash.0;
Expand All @@ -427,27 +430,29 @@ pub(super) fn state_update_from_cached_state<S: StateDb>(
.nonces
.into_iter()
.map(|(key, value)| (to_address(key), value.0))
.collect::<HashMap<
.collect::<BTreeMap<
katana_primitives::contract::ContractAddress,
katana_primitives::contract::Nonce,
>>();

let storage_updates =
state_diff.storage.into_iter().fold(HashMap::new(), |mut storage, ((addr, key), value)| {
let entry: &mut HashMap<
let storage_updates = state_diff.storage.into_iter().fold(
BTreeMap::new(),
|mut storage, ((addr, key), value)| {
let entry: &mut BTreeMap<
katana_primitives::contract::StorageKey,
katana_primitives::contract::StorageValue,
> = storage.entry(to_address(addr)).or_default();
entry.insert(*key.0.key(), value);
storage
});
},
);

let contract_updates =
let deployed_contracts =
state_diff
.class_hashes
.into_iter()
.map(|(key, value)| (to_address(key), value.0))
.collect::<HashMap<
.collect::<BTreeMap<
katana_primitives::contract::ContractAddress,
katana_primitives::class::ClassHash,
>>();
Expand All @@ -457,7 +462,7 @@ pub(super) fn state_update_from_cached_state<S: StateDb>(
.compiled_class_hashes
.into_iter()
.map(|(key, value)| (key.0, value.0))
.collect::<HashMap<
.collect::<BTreeMap<
katana_primitives::class::ClassHash,
katana_primitives::class::CompiledClassHash,
>>();
Expand All @@ -468,8 +473,9 @@ pub(super) fn state_update_from_cached_state<S: StateDb>(
state_updates: StateUpdates {
nonce_updates,
storage_updates,
contract_updates,
deployed_contracts,
declared_classes,
..Default::default()
},
}
}
Expand Down Expand Up @@ -651,7 +657,7 @@ fn to_l2_l1_messages(
#[cfg(test)]
mod tests {

use std::collections::HashSet;
use std::collections::{HashMap, HashSet};

use katana_cairo::cairo_vm::types::builtin_name::BuiltinName;
use katana_cairo::cairo_vm::vm::runners::cairo_runner::ExecutionResources;
Expand Down
11 changes: 6 additions & 5 deletions crates/katana/executor/tests/executor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
mod fixtures;

use std::collections::HashMap;
use std::collections::BTreeMap;

use fixtures::{state_provider, valid_blocks};
use katana_executor::{ExecutionOutput, ExecutionResult, ExecutorFactory};
Expand Down Expand Up @@ -283,16 +283,17 @@ fn test_executor_with_valid_blocks_impl<EF: ExecutorFactory>(
assert_eq!(actual_txs, expected_txs);

let actual_nonce_updates = states.state_updates.nonce_updates;
let expected_nonce_updates = HashMap::from([(main_account, felt!("3")), (new_acc, felt!("1"))]);
let expected_nonce_updates =
BTreeMap::from([(main_account, felt!("3")), (new_acc, felt!("1"))]);

let actual_declared_classes = states.state_updates.declared_classes;
let expected_declared_classes = HashMap::from([(
let expected_declared_classes = BTreeMap::from([(
felt!("0x420"),
felt!("0x016c6081eb34ad1e0c5513234ed0c025b3c7f305902d291bad534cd6474c85bc"),
)]);

let actual_contract_deployed = states.state_updates.contract_updates;
let expected_contract_deployed = HashMap::from([
let actual_contract_deployed = states.state_updates.deployed_contracts;
let expected_contract_deployed = BTreeMap::from([
(new_acc, DEFAULT_OZ_ACCOUNT_CONTRACT_CLASS_HASH),
(deployed_contract.into(), DEFAULT_LEGACY_ERC20_CONTRACT_CLASS_HASH),
]);
Expand Down
2 changes: 1 addition & 1 deletion crates/katana/executor/tests/simulate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ fn test_simulate_tx_impl<EF: ExecutorFactory>(

assert!(states.state_updates.nonce_updates.is_empty(), "no state updates");
assert!(states.state_updates.storage_updates.is_empty(), "no state updates");
assert!(states.state_updates.contract_updates.is_empty(), "no state updates");
assert!(states.state_updates.deployed_contracts.is_empty(), "no state updates");
Copy link

Choose a reason for hiding this comment

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

Tip

Codebase Verification

🔍 Terminology Consistency Issue Identified

Ohayo, sensei! While the change from contract_updates to deployed_contracts aligns with the PR objectives in the reviewed file, there are still multiple instances of contract_updates across the codebase. This inconsistency might lead to confusion and maintenance challenges.

  • Files with remaining contract_updates:
    • crates/saya/core/src/prover/state_diff.rs
    • crates/saya/core/src/prover/program_input.rs
    • bin/saya/src/tests.rs
    • crates/saya/provider/src/rpc/state.rs
    • crates/katana/primitives/src/genesis/mod.rs
    • (and others as per the script output)

To maintain clarity and consistency, please ensure that all instances of contract_updates are reviewed and appropriately updated throughout the codebase.

Analysis chain

Ohayo, sensei! The change looks good, but let's verify terminology consistency.

The replacement of contract_updates with deployed_contracts aligns with the PR objectives and maintains the test logic. However, we should ensure this terminology change is consistent across the codebase.

Let's run a quick check to verify the terminology usage:

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining instances of 'contract_updates' and new instances of 'deployed_contracts'

echo "Checking for remaining 'contract_updates':"
rg --type rust "contract_updates"

echo "\nChecking for new 'deployed_contracts':"
rg --type rust "deployed_contracts"

Length of output: 6989

assert!(states.state_updates.declared_classes.is_empty(), "no state updates");

assert!(states.declared_sierra_classes.is_empty(), "no new classes should be declared");
Expand Down
2 changes: 1 addition & 1 deletion crates/katana/primitives/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ flate2 = { workspace = true, optional = true }
katana-cairo.workspace = true

[dev-dependencies]
assert_matches.workspace = true
num-traits.workspace = true
assert_matches.workspace = true
similar-asserts.workspace = true

[features]
Expand Down
10 changes: 5 additions & 5 deletions crates/katana/primitives/src/genesis/allocation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::collections::HashMap;
use std::collections::{BTreeMap, HashMap};
use std::fmt::Debug;

use alloy_primitives::U256;
Expand Down Expand Up @@ -59,7 +59,7 @@ impl GenesisAllocation {
}

/// Get the storage values for this contract allocation.
pub fn storage(&self) -> Option<&HashMap<StorageKey, StorageValue>> {
pub fn storage(&self) -> Option<&BTreeMap<StorageKey, StorageValue>> {
match self {
Self::Contract(contract) => contract.storage.as_ref(),
Self::Account(account) => account.storage(),
Expand Down Expand Up @@ -107,7 +107,7 @@ impl GenesisAccountAlloc {
}
}

pub fn storage(&self) -> Option<&HashMap<StorageKey, StorageValue>> {
pub fn storage(&self) -> Option<&BTreeMap<StorageKey, StorageValue>> {
match self {
Self::Account(account) => account.storage.as_ref(),
Self::DevAccount(account) => account.storage.as_ref(),
Expand Down Expand Up @@ -136,7 +136,7 @@ pub struct GenesisContractAlloc {
pub nonce: Option<Felt>,
/// The initial storage values of the contract.
#[serde(skip_serializing_if = "Option::is_none")]
pub storage: Option<HashMap<StorageKey, StorageValue>>,
pub storage: Option<BTreeMap<StorageKey, StorageValue>>,
}

/// Used mainly for development purposes where the account info including the
Expand Down Expand Up @@ -191,7 +191,7 @@ pub struct GenesisAccount {
pub nonce: Option<Felt>,
/// The initial storage values of the account.
#[serde(skip_serializing_if = "Option::is_none")]
pub storage: Option<HashMap<StorageKey, StorageValue>>,
pub storage: Option<BTreeMap<StorageKey, StorageValue>>,
}

impl GenesisAccount {
Expand Down
Loading
Loading