dev: Better test state isolation between origin and forked devnet#825
dev: Better test state isolation between origin and forked devnet#825FabijanC merged 7 commits into0xSpaceShard:mainfrom
Conversation
FabijanC
left a comment
There was a problem hiding this comment.
These test functions are too long
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
Apologies, has been made less verbose |
|
Hi @FabijanC Can you please review, so I can be able to proceed to resolving another issue on this repo? |
FabijanC
left a comment
There was a problem hiding this comment.
Please re-read the described scenario in #773.
It requires forking from block m when the latest origin block is n, where n > m. It also requires state changes at m+1, where n > m+1. Can you inspect the existing tests (added by your or older) and see if these tests already exist?
|
@FabijanC as regards to the OG issue description, the first commit I made covered that scenario // Block 1 => Origin Devnet
let (_, contract_address) =
declare_v3_deploy_v3(&predeployed_account, contract_class.clone(), casm_hash, &ctor_args)
.await
.unwrap();
let entry_point_selector = get_selector_from_name("get_balance").unwrap();
let call_result = origin_devnet
.json_rpc_client
.call(
FunctionCall { contract_address, entry_point_selector, calldata: vec![] },
BlockId::Tag(BlockTag::Latest),
)
.await
.unwrap();
assert_eq!(call_result, vec![initial_value]);
// Block 2 => Origin Devnet
let increment_value = Felt::from(7_u32);
let contract_invoke = vec![Call {
to: contract_address,
selector: get_selector_from_name("increase_balance").unwrap(),
calldata: vec![increment_value, Felt::ZERO],
}];
let invoke_result = predeployed_account
.execute_v3(contract_invoke.clone())
.l1_gas(0)
.l1_data_gas(1e3 as u64)
.l2_gas(1e7 as u64)
.send()
.await
.unwrap();
assert_tx_succeeded_accepted(&invoke_result.transaction_hash, &origin_devnet.json_rpc_client)
.await;
assert_eq!(
get_contract_balance(&origin_devnet, contract_address).await,
initial_value + increment_value
);
let fork_devnet = origin_devnet.fork().await.unwrap();
assert_eq!(
get_contract_balance(&fork_devnet, contract_address).await,
initial_value + increment_value
);
// Block 3 => Origin Devnet
let increment_value_ii = Felt::from(19_u32);
let contract_invoke_ii = vec![Call {
to: contract_address,
selector: get_selector_from_name("increase_balance").unwrap(),
calldata: vec![increment_value_ii, Felt::ZERO],
}];
let invoke_result_ii = predeployed_account
.execute_v3(contract_invoke_ii.clone())
.l1_gas(0)
.l1_data_gas(1e3 as u64)
.l2_gas(1e7 as u64)
.send()
.await
.unwrap();
assert_tx_succeeded_accepted(
&invoke_result_ii.transaction_hash,
&origin_devnet.json_rpc_client,
)
.await;
assert_eq!(
get_contract_balance(&origin_devnet, contract_address).await,
initial_value + increment_value + increment_value_ii
);
// Asserting here that forked devnet balance didn't change
assert_eq!(
get_contract_balance(&fork_devnet, contract_address).await,
initial_value + increment_value
);
// Block 4 => Origin Devnet
let increment_value_iii = Felt::from(57_u32);
let contract_invoke_iii = vec![Call {
to: contract_address,
selector: get_selector_from_name("increase_balance").unwrap(),
calldata: vec![increment_value_iii, Felt::ZERO],
}];
let invoke_result_iii = predeployed_account
.execute_v3(contract_invoke_iii.clone())
.l1_gas(0)
.l1_data_gas(1e3 as u64)
.l2_gas(1e7 as u64)
.send()
.await
.unwrap();
assert_tx_succeeded_accepted(
&invoke_result_iii.transaction_hash,
&origin_devnet.json_rpc_client,
)
.await;
assert_eq!(
get_contract_balance(&origin_devnet, contract_address).await,
initial_value + increment_value + increment_value_ii + increment_value_iii
);
// Asserting here that forked devnet balance didn't change
assert_eq!(
get_contract_balance(&fork_devnet, contract_address).await,
initial_value + increment_value
);Only removed it when you complained the tests were too long And if this particular issue description has been tested before, no. The first commit in this pull request was as straight as the issue described Imagine an origin with n blocks
- We fork from block `m < n`
- The storage of a contract state is changed at `m+1 < n`
- We assert that calling the contract in the fork with block_id = latest (or pending) returns the same as origin at `m` (and not at `m+1`) |
|
Saying the test functions are too long doesn't mean you ought to remove logic, rather refactor it to be more readable. |
|
Rightly So @FabijanC done that currently, looking forward to your review |
|
Which AI tool are you using for generating the code? |
|
None, I'm being as honest as possible here. |
|
GM @FabijanC any update, on the PR Review? |
|
Hi @FabijanC I just want to say thank you for how graceful you were through out the review process, One key takeaway I got from this is to focus solely on the issue I'm resolving, and not try to include unneccessary overhead/difficulty in my implementation; |
Usage related changes
Development related changes
Checklist:
./scripts/format.sh./scripts/clippy_check.sh./scripts/check_unused_deps.sh./scripts/check_spelling.sh./website/README.md