-
Notifications
You must be signed in to change notification settings - Fork 26
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.
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
test(blockifier_reexecution): add support for declare and deploy account tx with rpc #1604
test(blockifier_reexecution): add support for declare and deploy account tx with rpc #1604
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1604 +/- ##
==========================================
+ Coverage 40.10% 41.96% +1.85%
==========================================
Files 26 193 +167
Lines 1895 23005 +21110
Branches 1895 23005 +21110
==========================================
+ Hits 760 9653 +8893
- Misses 1100 12898 +11798
- Partials 35 454 +419 ☔ View full report in Codecov by Sentry. |
move to top Code quote: use starknet_api::transaction::DeployAccountTransaction; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @aner-starkware and @AvivYossef-starkware)
crates/blockifier_reexecution/src/state_reader/raw_rpc_json_test.rs
line 113 at r2 (raw file):
assert_matches!(declare_tx_v3, Transaction::Declare(DeclareTransaction::V3(..))); }
suggestion (non-blocking).
you can do something similar in the deploy_account txs test
Suggestion:
#[rstest]
fn deserialize_declare_txs(#[values("declare_v1", "declare_v2", "declare_v3")] key: &str) {
let declare_tx = deserialize_transaction_json_to_starknet_api_tx(
read_json_file("raw_rpc_json_objects/transactions.json")[key].clone(),
)
.expect(format!("Failed to deserialize {key} tx"));
match key {
"declare_v1" => assert_matches!(declare_tx, Transaction::Declare(DeclareTransaction::V1(..))),
"declare_v2" => assert_matches!(declare_tx, Transaction::Declare(DeclareTransaction::V2(..))),
"declare_v3" => assert_matches!(declare_tx, Transaction::Declare(DeclareTransaction::V3(..))),
_ => panic!("Unknown scenario '{key}'"),
}
}
crates/blockifier_reexecution/src/state_reader/rpc_https_test.rs
line 130 at r2 (raw file):
case(EXAMPLE_DEPLOY_ACCOUNT_V3_BLOCK_NUMBER, EXAMPLE_DEPLOY_ACCOUNT_V3_TX_HASH, 3) )] pub fn test_get_deploy_account_tx_by_hash(block_number: u64, tx_hash: &str, version: u64) {
this is the standard syntax for test cases in our repo
Suggestion:
#[rstest]
#[case(EXAMPLE_DEPLOY_ACCOUNT_V1_BLOCK_NUMBER, EXAMPLE_DEPLOY_ACCOUNT_V1_TX_HASH, 1)]
#[case(EXAMPLE_DEPLOY_ACCOUNT_V3_BLOCK_NUMBER, EXAMPLE_DEPLOY_ACCOUNT_V3_TX_HASH, 3)]
pub fn test_get_deploy_account_tx_by_hash(#[case] block_number: u64, #[case] tx_hash: &str, #[case] version: u64) {
crates/blockifier_reexecution/src/state_reader/rpc_https_test.rs
line 130 at r2 (raw file):
case(EXAMPLE_DEPLOY_ACCOUNT_V3_BLOCK_NUMBER, EXAMPLE_DEPLOY_ACCOUNT_V3_TX_HASH, 3) )] pub fn test_get_deploy_account_tx_by_hash(block_number: u64, tx_hash: &str, version: u64) {
we have a type for this.
you will not be able to match
(working on a fix) but use if-else for now
Suggestion:
version: TransactionVersion
crates/blockifier_reexecution/src/state_reader/rpc_https_test.rs
line 153 at r2 (raw file):
case(EXAMPLE_DECLARE_V2_BLOCK_NUMBER, EXAMPLE_DECLARE_V2_TX_HASH, 2), case(EXAMPLE_DECLARE_V3_BLOCK_NUMBER, EXAMPLE_DECLARE_V3_TX_HASH, 3) )]
see above
Code quote:
#[rstest(
block_number,
tx_hash,
expected_version,
case(EXAMPLE_DECLARE_V1_BLOCK_NUMBER, EXAMPLE_DECLARE_V1_TX_HASH, 1),
case(EXAMPLE_DECLARE_V2_BLOCK_NUMBER, EXAMPLE_DECLARE_V2_TX_HASH, 2),
case(EXAMPLE_DECLARE_V3_BLOCK_NUMBER, EXAMPLE_DECLARE_V3_TX_HASH, 3)
)]
crates/blockifier_reexecution/src/state_reader/rpc_https_test.rs
line 154 at r2 (raw file):
case(EXAMPLE_DECLARE_V3_BLOCK_NUMBER, EXAMPLE_DECLARE_V3_TX_HASH, 3) )] pub fn test_get_declare_tx_by_hash(block_number: u64, tx_hash: &str, expected_version: u64) {
see above
Suggestion:
expected_version: TransactionVersion)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @aner-starkware and @AvivYossef-starkware)
a discussion (no related file):
please amend the commit message to start with test(blockifier_reexecution): ...
df80f6e
to
148ad69
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 4 files reviewed, 7 unresolved discussions (waiting on @aner-starkware and @dorimedini-starkware)
a discussion (no related file):
Previously, dorimedini-starkware wrote…
please amend the commit message to start with
test(blockifier_reexecution): ...
Done.
crates/blockifier_reexecution/src/state_reader/raw_rpc_json_test.rs
line 113 at r2 (raw file):
Previously, dorimedini-starkware wrote…
suggestion (non-blocking).
you can do something similar in the deploy_account txs test
nice
crates/blockifier_reexecution/src/state_reader/rpc_https_test.rs
line 130 at r2 (raw file):
Previously, dorimedini-starkware wrote…
this is the standard syntax for test cases in our repo
Done.
crates/blockifier_reexecution/src/state_reader/rpc_https_test.rs
line 130 at r2 (raw file):
Previously, dorimedini-starkware wrote…
we have a type for this.
you will not be able tomatch
(working on a fix) but use if-else for now
Done.
crates/blockifier_reexecution/src/state_reader/rpc_https_test.rs
line 132 at r2 (raw file):
Previously, aner-starkware wrote…
move to top
Done.
crates/blockifier_reexecution/src/state_reader/rpc_https_test.rs
line 153 at r2 (raw file):
Previously, dorimedini-starkware wrote…
see above
Done.
crates/blockifier_reexecution/src/state_reader/rpc_https_test.rs
line 154 at r2 (raw file):
Previously, dorimedini-starkware wrote…
see above
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @aner-starkware and @AvivYossef-starkware)
crates/blockifier_reexecution/src/state_reader/raw_rpc_json_test.rs
line 78 at r3 (raw file):
#[values("deploy_account_v1", "deploy_account_v3")] deploy_account_version: &str, ) { let deploy_account_tx_v1 = deserialize_transaction_json_to_starknet_api_tx(
rename (not always v1)
Suggestion:
deploy_account
148ad69
to
fac29c1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @aner-starkware and @dorimedini-starkware)
crates/blockifier_reexecution/src/state_reader/raw_rpc_json_test.rs
line 78 at r3 (raw file):
Previously, dorimedini-starkware wrote…
rename (not always v1)
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aner-starkware)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware)
test: blockifier reececution test getting deploy account tx with rpc
build: blockifier reexecution add support for declare tx