Skip to content
This repository has been archived by the owner on Jul 22, 2024. It is now read-only.

Commit

Permalink
fix: Read before writing when executing the deprecated version of `st…
Browse files Browse the repository at this point in the history 
…orate_write` syscall (#1006)

* Read before writing

* Add test

* Fix some tests

* Fix test

* Fix test

* Fix test

* Fix test

* Fix tests

* Fix tests

* Fix tests
  • Loading branch information
@fmoletta
fmoletta authored and fannyguthmann committed Oct 2, 2023
1 parent b697c58 commit a7493d1
Show file tree
Hide file tree
Showing 9 changed files with 115 additions and 44 deletions.
36 changes: 31 additions & 5 deletions src/syscalls/deprecated_business_logic_syscall_handler.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -872,8 +872,9 @@ impl<'a, S: StateReader> DeprecatedBLSyscallHandler<'a, S> {
address: Address,
value: Felt252,
) -> Result<(), SyscallHandlerError> {
self.starknet_storage_state
.write(&felt_to_hash(&address.0), value);
let address = felt_to_hash(&address.0);
self.starknet_storage_state.read(&address)?;
self.starknet_storage_state.write(&address, value);

Ok(())
}
Expand Down Expand Up @@ -978,7 +979,7 @@ mod tests {
state::cached_state::CachedState,
state::in_memory_state_reader::InMemoryStateReader,
syscalls::syscall_handler_errors::SyscallHandlerError,
utils::{test_utils::*, Address},
utils::{felt_to_hash, test_utils::*, Address},
};
use cairo_vm::felt::Felt252;
use cairo_vm::hint_processor::hint_processor_definition::HintProcessorLogic;
Expand All @@ -993,8 +994,8 @@ mod tests {
},
vm::{errors::memory_errors::MemoryError, vm_core::VirtualMachine},
};
use num_traits::Zero;
use std::{any::Any, borrow::Cow, collections::HashMap};
use num_traits::{One, Zero};
use std::{any::Any, borrow::Cow, collections::HashMap, sync::Arc};

type DeprecatedBLSyscallHandler<'a> =
super::DeprecatedBLSyscallHandler<'a, InMemoryStateReader>;
Expand Down Expand Up @@ -1111,4 +1112,29 @@ mod tests {
Ok(value) if value == Felt252::zero()
);
}

#[test]
fn test_storage_write_update_initial_values() {
// Initialize state reader with value
let mut state_reader = InMemoryStateReader::default();
state_reader.address_to_storage.insert(
(Address(Felt252::one()), felt_to_hash(&Felt252::one())),
Felt252::zero(),
);
// Create empty-cached state
let mut state = CachedState::new(Arc::new(state_reader), HashMap::new());
let mut syscall_handler = DeprecatedBLSyscallHandler::default_with(&mut state);
// Perform write
assert!(syscall_handler
.syscall_storage_write(Address(Felt252::one()), Felt252::one())
.is_ok());
// Check that initial values have beed updated in the cache
assert_eq!(
state.cache().storage_initial_values,
HashMap::from([(
(Address(Felt252::one()), felt_to_hash(&Felt252::one())),
Felt252::zero()
)])
)
}
}
2 changes: 1 addition & 1 deletion src/transaction/l1_handler.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ mod test {
},
events: vec![],
l2_to_l1_messages: vec![],
storage_read_values: vec![0.into()],
storage_read_values: vec![0.into(), 0.into()],
accessed_storage_keys: HashSet::from([[
4, 40, 11, 247, 0, 35, 63, 18, 141, 159, 101, 81, 182, 2, 213, 216, 100, 110,
5, 5, 101, 122, 13, 252, 204, 72, 77, 8, 58, 226, 194, 24,
Expand Down
12 changes: 11 additions & 1 deletion tests/complex_contracts/amm_contracts/amm.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ fn amm_init_pool_test() {
},
class_hash: Some(class_hash),
accessed_storage_keys,
storage_read_values: vec![Felt252::zero(), Felt252::zero()],
..Default::default()
};

Expand Down Expand Up @@ -182,7 +183,12 @@ fn amm_add_demo_tokens_test() {
},
class_hash: Some(class_hash),
accessed_storage_keys: accessed_storage_keys_add_demo_token,
storage_read_values: vec![Felt252::zero(), Felt252::zero()],
storage_read_values: vec![
Felt252::zero(),
Felt252::zero(),
Felt252::zero(),
Felt252::zero(),
],
..Default::default()
};

Expand Down Expand Up @@ -351,6 +357,10 @@ fn amm_swap_test() {
10000.into(),
100.into(),
100.into(),
10000.into(),
100.into(),
100.into(),
10000.into(),
]
.to_vec(),
..Default::default()
Expand Down
18 changes: 15 additions & 3 deletions tests/complex_contracts/amm_contracts/amm_proxy.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::complex_contracts::utils::*;
use cairo_vm::felt::Felt252;
use cairo_vm::vm::runners::cairo_runner::ExecutionResources;
use num_traits::Zero;
use starknet_crypto::FieldElement;
use starknet_in_rust::definitions::block_context::BlockContext;
use starknet_in_rust::services::api::contract_classes::deprecated_contract_class::ContractClass;
Expand Down Expand Up @@ -84,6 +85,7 @@ fn amm_proxy_init_pool_test() {
},
class_hash: Some(contract_class_hash),
accessed_storage_keys,
storage_read_values: vec![Felt252::zero(), Felt252::zero()],
..Default::default()
}];

Expand Down Expand Up @@ -288,7 +290,7 @@ fn amm_proxy_add_demo_token_test() {
entry_point_selector: Some(amm_entrypoint_selector),
entry_point_type: Some(EntryPointType::External),
calldata: calldata.clone()[1..].to_vec(),
storage_read_values: vec![0.into(), 0.into()],
storage_read_values: vec![0.into(), 0.into(), 0.into(), 0.into()],
execution_resources: ExecutionResources {
n_steps: 397,
n_memory_holes: 42,
Expand Down Expand Up @@ -539,8 +541,18 @@ fn amm_proxy_swap() {
entry_point_type: Some(EntryPointType::External),
calldata: calldata.clone()[1..].to_vec(),
retdata: [90.into()].to_vec(),
storage_read_values: [100.into(), 1000.into(), 1000.into(), 100.into(), 200.into()]
.to_vec(),
storage_read_values: [
100.into(),
1000.into(),
1000.into(),
100.into(),
100.into(),
1000.into(),
200.into(),
200.into(),
1000.into(),
]
.to_vec(),
execution_resources: ExecutionResources {
n_steps: 826,
n_memory_holes: 92,
Expand Down
26 changes: 16 additions & 10 deletions tests/complex_contracts/nft/erc721.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ fn erc721_test_approve() {
let expected_read_result = vec![];

// Checks only the storage variable ERC721_operator_approvals
let storage_read_values = vec![Felt252::from(666), Felt252::from(666)];
let storage_read_values = vec![Felt252::from(666), Felt252::zero(), Felt252::from(666)];

// Ask for the owner of the token
let mut accessed_storage_keys =
Expand Down Expand Up @@ -577,7 +577,7 @@ fn erc721_set_approval_for_all() {
let expected_read_result = vec![];

// Only writes in operator_approvals
let storage_read_values = vec![];
let storage_read_values = vec![Felt252::zero()];

// Writes to the operator the new set value
let accessed_storage_keys = get_accessed_keys(
Expand Down Expand Up @@ -696,14 +696,20 @@ fn erc721_transfer_from_test() {
accessed_storage_keys.insert(balance_to);

let expected_read_values = vec![
Felt252::from(666),
Felt252::from(666),
Felt252::from(666),
Felt252::from(666),
Felt252::from(1),
Felt252::zero(),
Felt252::zero(),
Felt252::zero(),
666.into(),
666.into(),
666.into(),
0.into(),
666.into(),
1.into(),
0.into(),
1.into(),
0.into(),
0.into(),
0.into(),
0.into(),
0.into(),
666.into(),
];

let approval_event_hash = Felt252::from_bytes_be(&calculate_sn_keccak("Approval".as_bytes()));
Expand Down
2 changes: 1 addition & 1 deletion tests/increase_balance.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ fn hello_starknet_increase_balance() {

let mut expected_accessed_storage_keys = HashSet::new();
expected_accessed_storage_keys.insert(expected_key);
let expected_storage_read_values = vec![Felt252::zero()];
let expected_storage_read_values = vec![Felt252::zero(), Felt252::zero()];

let expected_call_info = CallInfo {
caller_address: Address(0.into()),
Expand Down
48 changes: 32 additions & 16 deletions tests/internals.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,10 @@ fn expected_fee_transfer_call_info(
storage_read_values: vec![
INITIAL_BALANCE.clone(),
Felt252::zero(),
INITIAL_BALANCE.clone(),
Felt252::zero(),
Felt252::zero(),
Felt252::zero(),
Felt252::zero(),
Felt252::zero(),
],
Expand Down Expand Up @@ -636,23 +640,27 @@ fn expected_fee_transfer_info(fee: u128) -> CallInfo {
],
}],
storage_read_values: vec![
INITIAL_BALANCE.clone(),
Felt252::zero(),
INITIAL_BALANCE.clone(),
Felt252::zero(),
Felt252::zero(),
Felt252::zero(),
Felt252::zero(),
Felt252::zero(),
],
accessed_storage_keys: HashSet::from([
[
7, 35, 151, 50, 8, 99, 155, 120, 57, 206, 41, 143, 127, 254, 166, 30, 63, 149, 51,
135, 45, 239, 215, 171, 219, 145, 2, 61, 180, 101, 136, 19,
135, 45, 239, 215, 171, 219, 145, 2, 61, 180, 101, 136, 18,
],
[
2, 162, 196, 156, 77, 186, 13, 145, 179, 79, 42, 222, 133, 212, 29, 9, 86, 31, 154,
119, 136, 76, 21, 186, 42, 176, 242, 36, 27, 8, 13, 236,
],
[
7, 35, 151, 50, 8, 99, 155, 120, 57, 206, 41, 143, 127, 254, 166, 30, 63, 149, 51,
135, 45, 239, 215, 171, 219, 145, 2, 61, 180, 101, 136, 18,
135, 45, 239, 215, 171, 219, 145, 2, 61, 180, 101, 136, 19,
],
[
2, 162, 196, 156, 77, 186, 13, 145, 179, 79, 42, 222, 133, 212, 29, 9, 86, 31, 154,
Expand Down Expand Up @@ -698,26 +706,30 @@ fn expected_fib_fee_transfer_info(fee: u128) -> CallInfo {
storage_read_values: vec![
INITIAL_BALANCE.clone() - Felt252::from(2476),
Felt252::zero(),
INITIAL_BALANCE.clone() - Felt252::from(2476),
Felt252::zero(),
Felt252::from(2476),
Felt252::zero(),
Felt252::from(2476),
Felt252::zero(),
],
accessed_storage_keys: HashSet::from([
[
2, 162, 196, 156, 77, 186, 13, 145, 179, 79, 42, 222, 133, 212, 29, 9, 86, 31, 154,
119, 136, 76, 21, 186, 42, 176, 242, 36, 27, 8, 13, 235,
],
[
7, 35, 151, 50, 8, 99, 155, 120, 57, 206, 41, 143, 127, 254, 166, 30, 63, 149, 51,
135, 45, 239, 215, 171, 219, 145, 2, 61, 180, 101, 136, 19,
135, 45, 239, 215, 171, 219, 145, 2, 61, 180, 101, 136, 18,
],
[
7, 35, 151, 50, 8, 99, 155, 120, 57, 206, 41, 143, 127, 254, 166, 30, 63, 149, 51,
135, 45, 239, 215, 171, 219, 145, 2, 61, 180, 101, 136, 18,
2, 162, 196, 156, 77, 186, 13, 145, 179, 79, 42, 222, 133, 212, 29, 9, 86, 31, 154,
119, 136, 76, 21, 186, 42, 176, 242, 36, 27, 8, 13, 235,
],
[
2, 162, 196, 156, 77, 186, 13, 145, 179, 79, 42, 222, 133, 212, 29, 9, 86, 31, 154,
119, 136, 76, 21, 186, 42, 176, 242, 36, 27, 8, 13, 236,
],
[
7, 35, 151, 50, 8, 99, 155, 120, 57, 206, 41, 143, 127, 254, 166, 30, 63, 149, 51,
135, 45, 239, 215, 171, 219, 145, 2, 61, 180, 101, 136, 19,
],
]),
}
}
Expand Down Expand Up @@ -832,27 +844,32 @@ fn expected_declare_fee_transfer_info(fee: u128) -> CallInfo {
storage_read_values: vec![
INITIAL_BALANCE.clone(),
Felt252::zero(),
INITIAL_BALANCE.clone(),
Felt252::zero(),
Felt252::zero(),
Felt252::zero(),
Felt252::zero(),
Felt252::zero(),
],
accessed_storage_keys: HashSet::from([
[
7, 35, 151, 50, 8, 99, 155, 120, 57, 206, 41, 143, 127, 254, 166, 30, 63, 149, 51,
135, 45, 239, 215, 171, 219, 145, 2, 61, 180, 101, 136, 18,
],
[
2, 162, 196, 156, 77, 186, 13, 145, 179, 79, 42, 222, 133, 212, 29, 9, 86, 31, 154,
119, 136, 76, 21, 186, 42, 176, 242, 36, 27, 8, 13, 235,
135, 45, 239, 215, 171, 219, 145, 2, 61, 180, 101, 136, 19,
],
[
7, 35, 151, 50, 8, 99, 155, 120, 57, 206, 41, 143, 127, 254, 166, 30, 63, 149, 51,
135, 45, 239, 215, 171, 219, 145, 2, 61, 180, 101, 136, 19,
135, 45, 239, 215, 171, 219, 145, 2, 61, 180, 101, 136, 18,
],
[
2, 162, 196, 156, 77, 186, 13, 145, 179, 79, 42, 222, 133, 212, 29, 9, 86, 31, 154,
119, 136, 76, 21, 186, 42, 176, 242, 36, 27, 8, 13, 236,
],
[
2, 162, 196, 156, 77, 186, 13, 145, 179, 79, 42, 222, 133, 212, 29, 9, 86, 31, 154,
119, 136, 76, 21, 186, 42, 176, 242, 36, 27, 8, 13, 235,
],
]),

execution_resources: ExecutionResources {
n_steps: 525,
n_memory_holes: 59,
Expand Down Expand Up @@ -1287,7 +1304,6 @@ fn test_invoke_tx() {
// transaction.
let result = invoke_tx.execute(state, block_context, 0).unwrap();
let expected_execution_info = expected_transaction_execution_info(block_context);

assert_eq!(result, expected_execution_info);
}

Expand Down
2 changes: 1 addition & 1 deletion tests/storage.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ fn integration_storage_test() {
..Default::default()
},
class_hash: Some(class_hash),
storage_read_values: vec![42.into()],
storage_read_values: vec![0.into(), 42.into()],
accessed_storage_keys: expected_accessed_storage_keys,
..Default::default()
};
Expand Down
Loading

0 comments on commit a7493d1

Please # to comment.