From 3b33660f7c5666bfa3bfff4e08ce29f4d61a5343 Mon Sep 17 00:00:00 2001 From: Ammar Arif Date: Wed, 8 May 2024 23:27:31 -0400 Subject: [PATCH 1/4] wip --- .../provider/src/providers/fork/backend.rs | 5 ++ .../provider/src/providers/fork/state.rs | 50 ++++++++++++++++++- 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/crates/katana/storage/provider/src/providers/fork/backend.rs b/crates/katana/storage/provider/src/providers/fork/backend.rs index 5fe359b8a1..57b8e704cb 100644 --- a/crates/katana/storage/provider/src/providers/fork/backend.rs +++ b/crates/katana/storage/provider/src/providers/fork/backend.rs @@ -378,6 +378,11 @@ impl SharedStateProvider { pub(crate) fn new_with_backend(backend: BackendHandle) -> Self { Self(Arc::new(CacheStateDb::new(backend))) } + + #[cfg(test)] + pub fn new_with_state(state: CacheStateDb) -> Self { + Self(Arc::new(state)) + } } impl StateProvider for SharedStateProvider { diff --git a/crates/katana/storage/provider/src/providers/fork/state.rs b/crates/katana/storage/provider/src/providers/fork/state.rs index fb69c6f9d2..1597e9ae69 100644 --- a/crates/katana/storage/provider/src/providers/fork/state.rs +++ b/crates/katana/storage/provider/src/providers/fork/state.rs @@ -39,9 +39,25 @@ impl StateProvider for ForkedStateDb { StateProvider::class_hash_of_contract(&self.db, address) } + // TODO: write unit tests for these cases + // + // When reading from local storage, we only consider entries that have non-zero nonce + // values OR non-zero class hashes. + // + // Nonce == 0 && ClassHash == 0 + // - Contract does not exist locally (so try find from remote state) + // Nonce != 0 && ClassHash == 0 + // - Contract exists and was deployed remotely but new nonce was set locally (so no need to read + // from remote state anymore) + // Nonce == 0 && ClassHash != 0 + // - Contract exists and was deployed locally (always read from local state) fn nonce(&self, address: ContractAddress) -> ProviderResult> { - if let nonce @ Some(_) = - self.contract_state.read().get(&address).map(|i| i.nonce).filter(|n| n != &Nonce::ZERO) + if let nonce @ Some(_) = self + .contract_state + .read() + .get(&address) + .filter(|c| c.nonce != Nonce::default() || c.class_hash != ClassHash::default()) + .map(|c| c.nonce) { return Ok(nonce); } @@ -199,3 +215,33 @@ impl ContractClassProvider for ForkedSnapshot { } } } + +// #[cfg(test)] +// mod tests { +// use std::collections::HashMap; + +// use katana_primitives::state::{StateUpdates, StateUpdatesWithDeclaredClasses}; + +// use crate::providers::{ +// fork::backend::{Backend, SharedStateProvider}, +// in_memory::cache::CacheStateDb, +// }; + +// use super::ForkedStateDb; + +// #[test] +// fn test_get_nonce() { +// let backend = Backend::new().unwrap(); + +// let mut state = CacheStateDb::new(backend); +// state.insert_updates(); + +// let forked_state = SharedStateProvider::new_with_state(state); + +// // setup initial state +// let states = StateUpdatesWithDeclaredClasses { +// state_updates: StateUpdates { nonce_updates: HashMap::from([()]) }, +// ..Default::default() +// }; +// } +} From 9495e6f6a18e1219586699a75e1a034845b896c8 Mon Sep 17 00:00:00 2001 From: Ammar Arif Date: Thu, 9 May 2024 15:36:53 -0400 Subject: [PATCH 2/4] unit test for nonce --- .../provider/src/providers/fork/backend.rs | 58 ++++---- .../provider/src/providers/fork/state.rs | 126 ++++++++++++++---- 2 files changed, 133 insertions(+), 51 deletions(-) diff --git a/crates/katana/storage/provider/src/providers/fork/backend.rs b/crates/katana/storage/provider/src/providers/fork/backend.rs index 57b8e704cb..ca86382b04 100644 --- a/crates/katana/storage/provider/src/providers/fork/backend.rs +++ b/crates/katana/storage/provider/src/providers/fork/backend.rs @@ -372,7 +372,7 @@ impl BackendHandle { /// cache to avoid fetching it again. This is shared across multiple instances of /// [`ForkedStateDb`](super::state::ForkedStateDb). #[derive(Clone)] -pub struct SharedStateProvider(Arc>); +pub struct SharedStateProvider(pub(crate) Arc>); impl SharedStateProvider { pub(crate) fn new_with_backend(backend: BackendHandle) -> Self { @@ -604,14 +604,11 @@ fn handle_not_found_err(result: Result) -> Result, } #[cfg(test)] -mod tests { +pub(crate) mod test_utils { use std::sync::mpsc::sync_channel; - use std::time::Duration; use katana_primitives::block::BlockNumber; - use katana_primitives::contract::GenericContractInfo; - use starknet::macros::felt; use starknet::providers::jsonrpc::HttpTransport; use starknet::providers::JsonRpcClient; use tokio::net::TcpListener; @@ -619,23 +616,14 @@ mod tests { use super::*; - const LOCAL_RPC_URL: &str = "http://localhost:5050"; - - const STORAGE_KEY: StorageKey = felt!("0x1"); - const ADDR_1: ContractAddress = ContractAddress(felt!("0xADD1")); - const ADDR_1_NONCE: Nonce = felt!("0x1"); - const ADDR_1_STORAGE_VALUE: StorageKey = felt!("0x8080"); - const ADDR_1_CLASS_HASH: StorageKey = felt!("0x1"); - - fn create_forked_backend(rpc_url: String, block_num: BlockNumber) -> BackendHandle { - let url = Url::parse(&rpc_url).expect("valid url"); + pub fn create_forked_backend(rpc_url: &str, block_num: BlockNumber) -> BackendHandle { + let url = Url::parse(rpc_url).expect("valid url"); let provider = Arc::new(JsonRpcClient::new(HttpTransport::new(url))); - let block_id = BlockHashOrNumber::Num(block_num); - Backend::new(provider, block_id).unwrap() + Backend::new(provider, block_num.into()).unwrap() } // Starts a TCP server that never close the connection. - fn start_tcp_server() { + pub fn start_tcp_server() { use tokio::runtime::Builder; let (tx, rx) = sync_channel::<()>(1); @@ -655,22 +643,36 @@ mod tests { rx.recv().unwrap(); } +} + +#[cfg(test)] +mod tests { + + use std::time::Duration; + + use katana_primitives::contract::GenericContractInfo; + use starknet::macros::felt; + + use super::test_utils::*; + use super::*; + + const LOCAL_RPC_URL: &str = "http://localhost:5050"; + + const STORAGE_KEY: StorageKey = felt!("0x1"); + const ADDR_1: ContractAddress = ContractAddress(felt!("0xADD1")); + const ADDR_1_NONCE: Nonce = felt!("0x1"); + const ADDR_1_STORAGE_VALUE: StorageKey = felt!("0x8080"); + const ADDR_1_CLASS_HASH: StorageKey = felt!("0x1"); - const ERROR_INIT_BACKEND: &str = "Failed to create backend"; const ERROR_SEND_REQUEST: &str = "Failed to send request to backend"; const ERROR_STATS: &str = "Failed to get stats"; #[test] fn handle_incoming_requests() { - let url = Url::try_from("http://127.0.0.1:8080").unwrap(); - let provider = JsonRpcClient::new(HttpTransport::new(url)); - let block_id = BlockHashOrNumber::Num(1); - // start a mock remote network start_tcp_server(); - // start backend - let handle = Backend::new(Arc::new(provider), block_id).expect(ERROR_INIT_BACKEND); + let handle = create_forked_backend("http://127.0.0.1:8080", 1); // check no pending requests let stats = handle.stats().expect(ERROR_STATS); @@ -709,7 +711,7 @@ mod tests { #[test] fn get_from_cache_if_exist() { // setup - let backend = create_forked_backend(LOCAL_RPC_URL.into(), 1); + let backend = create_forked_backend(LOCAL_RPC_URL, 1); let state_db = CacheStateDb::new(backend); state_db @@ -739,7 +741,7 @@ mod tests { #[test] fn fetch_from_fork_will_err_if_backend_thread_not_running() { - let backend = create_forked_backend(LOCAL_RPC_URL.into(), 1); + let backend = create_forked_backend(LOCAL_RPC_URL, 1); let provider = SharedStateProvider(Arc::new(CacheStateDb::new(backend))); assert!(StateProvider::nonce(&provider, ADDR_1).is_err()) } @@ -756,7 +758,7 @@ mod tests { #[test] #[ignore] fn fetch_from_fork_if_not_in_cache() { - let backend = create_forked_backend(FORKED_URL.into(), 908622); + let backend = create_forked_backend(FORKED_URL, 908622); let provider = SharedStateProvider(Arc::new(CacheStateDb::new(backend))); // fetch from remote diff --git a/crates/katana/storage/provider/src/providers/fork/state.rs b/crates/katana/storage/provider/src/providers/fork/state.rs index 1597e9ae69..8f3cbe75ba 100644 --- a/crates/katana/storage/provider/src/providers/fork/state.rs +++ b/crates/katana/storage/provider/src/providers/fork/state.rs @@ -39,8 +39,6 @@ impl StateProvider for ForkedStateDb { StateProvider::class_hash_of_contract(&self.db, address) } - // TODO: write unit tests for these cases - // // When reading from local storage, we only consider entries that have non-zero nonce // values OR non-zero class hashes. // @@ -216,32 +214,114 @@ impl ContractClassProvider for ForkedSnapshot { } } -// #[cfg(test)] -// mod tests { -// use std::collections::HashMap; +#[cfg(test)] +mod tests { + use std::collections::HashMap; + + use katana_primitives::state::{StateUpdates, StateUpdatesWithDeclaredClasses}; + use starknet::macros::felt; + + use super::*; + use crate::providers::fork::backend::test_utils::create_forked_backend; + + #[test] + fn test_get_nonce() { + let backend = create_forked_backend("http://localhost:8080", 1); + + let address: ContractAddress = felt!("1").into(); + let class_hash = felt!("11"); + let remote_nonce = felt!("111"); + let local_nonce = felt!("1111"); + + // Case: contract doesn't exist at all + { + let forked_state = SharedStateProvider::new_with_backend(backend.clone()); + let state = ForkedStateDb::new(forked_state.clone()); + + // asserts that its error for now + assert!(state.nonce(address).is_err()); + assert!(forked_state.nonce(address).is_err()); + } + + // Case: contract exist remotely + { + let remote = SharedStateProvider::new_with_backend(backend.clone()); + let local = ForkedStateDb::new(remote.clone()); + + let nonce_updates = HashMap::from([(address, remote_nonce)]); + let updates = StateUpdatesWithDeclaredClasses { + state_updates: StateUpdates { nonce_updates, ..Default::default() }, + ..Default::default() + }; + remote.0.insert_updates(updates); -// use katana_primitives::state::{StateUpdates, StateUpdatesWithDeclaredClasses}; + assert_eq!(local.nonce(address).unwrap(), Some(remote_nonce)); + assert_eq!(remote.nonce(address).unwrap(), Some(remote_nonce)); + } + + // Case: contract exist remotely but nonce was updated locally + { + let remote = SharedStateProvider::new_with_backend(backend.clone()); + let local = ForkedStateDb::new(remote.clone()); + + let nonce_updates = HashMap::from([(address, remote_nonce)]); + let contract_updates = HashMap::from([(address, class_hash)]); + let updates = StateUpdatesWithDeclaredClasses { + state_updates: StateUpdates { + nonce_updates, + contract_updates, + ..Default::default() + }, + ..Default::default() + }; + remote.0.insert_updates(updates); + + let nonce_updates = HashMap::from([(address, local_nonce)]); + let updates = StateUpdatesWithDeclaredClasses { + state_updates: StateUpdates { nonce_updates, ..Default::default() }, + ..Default::default() + }; + local.insert_updates(updates); + + assert_eq!(local.nonce(address).unwrap(), Some(local_nonce)); + assert_eq!(remote.nonce(address).unwrap(), Some(remote_nonce)); + } -// use crate::providers::{ -// fork::backend::{Backend, SharedStateProvider}, -// in_memory::cache::CacheStateDb, -// }; + // Case: contract was deployed locally only and has non-zero nonce + { + let remote = SharedStateProvider::new_with_backend(backend.clone()); + let local = ForkedStateDb::new(remote.clone()); -// use super::ForkedStateDb; + let contract_updates = HashMap::from([(address, class_hash)]); + let nonce_updates = HashMap::from([(address, local_nonce)]); + let updates = StateUpdatesWithDeclaredClasses { + state_updates: StateUpdates { + nonce_updates, + contract_updates, + ..Default::default() + }, + ..Default::default() + }; + local.insert_updates(updates); -// #[test] -// fn test_get_nonce() { -// let backend = Backend::new().unwrap(); + assert_eq!(local.nonce(address).unwrap(), Some(local_nonce)); + assert!(remote.nonce(address).is_err()); + } -// let mut state = CacheStateDb::new(backend); -// state.insert_updates(); + // Case: contract was deployed locally only and has zero nonce + { + let remote = SharedStateProvider::new_with_backend(backend.clone()); + let local = ForkedStateDb::new(remote.clone()); -// let forked_state = SharedStateProvider::new_with_state(state); + let contract_updates = HashMap::from([(address, class_hash)]); + let updates = StateUpdatesWithDeclaredClasses { + state_updates: StateUpdates { contract_updates, ..Default::default() }, + ..Default::default() + }; + local.insert_updates(updates); -// // setup initial state -// let states = StateUpdatesWithDeclaredClasses { -// state_updates: StateUpdates { nonce_updates: HashMap::from([()]) }, -// ..Default::default() -// }; -// } + assert_eq!(local.nonce(address).unwrap(), Some(Default::default())); + assert!(remote.nonce(address).is_err()); + } + } } From 25981a11963f5a9327a7a3c92e321d3b11848b34 Mon Sep 17 00:00:00 2001 From: Ammar Arif Date: Thu, 9 May 2024 15:49:40 -0400 Subject: [PATCH 3/4] assert on snapshot as well --- .../provider/src/providers/fork/state.rs | 32 +++++++++++++++---- 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/crates/katana/storage/provider/src/providers/fork/state.rs b/crates/katana/storage/provider/src/providers/fork/state.rs index 8f3cbe75ba..793dc540d2 100644 --- a/crates/katana/storage/provider/src/providers/fork/state.rs +++ b/crates/katana/storage/provider/src/providers/fork/state.rs @@ -148,8 +148,8 @@ impl StateProvider for ForkedSnapshot { .inner .contract_state .get(&address) - .map(|info| info.nonce) - .filter(|n| n != &Nonce::ZERO) + .filter(|c| c.nonce != Nonce::default() || c.class_hash != ClassHash::default()) + .map(|c| c.nonce) { return Ok(nonce); } @@ -235,12 +235,16 @@ mod tests { // Case: contract doesn't exist at all { - let forked_state = SharedStateProvider::new_with_backend(backend.clone()); - let state = ForkedStateDb::new(forked_state.clone()); + let remote = SharedStateProvider::new_with_backend(backend.clone()); + let local = ForkedStateDb::new(remote.clone()); // asserts that its error for now - assert!(state.nonce(address).is_err()); - assert!(forked_state.nonce(address).is_err()); + assert!(local.nonce(address).is_err()); + assert!(remote.nonce(address).is_err()); + + // make sure the snapshot maintains the same behavior + let snapshot = local.create_snapshot(); + assert!(snapshot.nonce(address).is_err()); } // Case: contract exist remotely @@ -257,6 +261,10 @@ mod tests { assert_eq!(local.nonce(address).unwrap(), Some(remote_nonce)); assert_eq!(remote.nonce(address).unwrap(), Some(remote_nonce)); + + // make sure the snapshot maintains the same behavior + let snapshot = local.create_snapshot(); + assert_eq!(snapshot.nonce(address).unwrap(), Some(remote_nonce)); } // Case: contract exist remotely but nonce was updated locally @@ -285,6 +293,10 @@ mod tests { assert_eq!(local.nonce(address).unwrap(), Some(local_nonce)); assert_eq!(remote.nonce(address).unwrap(), Some(remote_nonce)); + + // make sure the snapshot maintains the same behavior + let snapshot = local.create_snapshot(); + assert_eq!(snapshot.nonce(address).unwrap(), Some(local_nonce)); } // Case: contract was deployed locally only and has non-zero nonce @@ -306,6 +318,10 @@ mod tests { assert_eq!(local.nonce(address).unwrap(), Some(local_nonce)); assert!(remote.nonce(address).is_err()); + + // make sure the snapshot maintains the same behavior + let snapshot = local.create_snapshot(); + assert_eq!(snapshot.nonce(address).unwrap(), Some(local_nonce)); } // Case: contract was deployed locally only and has zero nonce @@ -322,6 +338,10 @@ mod tests { assert_eq!(local.nonce(address).unwrap(), Some(Default::default())); assert!(remote.nonce(address).is_err()); + + // make sure the snapshot maintains the same behavior + let snapshot = local.create_snapshot(); + assert_eq!(snapshot.nonce(address).unwrap(), Some(Default::default())); } } } From bad473fbdf27c20a81079cb04cc94ede80c255cc Mon Sep 17 00:00:00 2001 From: Ammar Arif Date: Thu, 9 May 2024 15:58:01 -0400 Subject: [PATCH 4/4] remove unused method --- crates/katana/storage/provider/src/providers/fork/backend.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/crates/katana/storage/provider/src/providers/fork/backend.rs b/crates/katana/storage/provider/src/providers/fork/backend.rs index ca86382b04..f1c065c444 100644 --- a/crates/katana/storage/provider/src/providers/fork/backend.rs +++ b/crates/katana/storage/provider/src/providers/fork/backend.rs @@ -378,11 +378,6 @@ impl SharedStateProvider { pub(crate) fn new_with_backend(backend: BackendHandle) -> Self { Self(Arc::new(CacheStateDb::new(backend))) } - - #[cfg(test)] - pub fn new_with_state(state: CacheStateDb) -> Self { - Self(Arc::new(state)) - } } impl StateProvider for SharedStateProvider {