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

Added a test for the scenario when a non deployed user invokes a transaction #971

Conversation

nimrod-starkware
Copy link
Contributor

@nimrod-starkware nimrod-starkware commented Oct 5, 2023

tests the same scenario as test_internal_invoke_tx_failure in the Python tests


This change is Reviewable

@codecov-commenter
Copy link

codecov-commenter commented Oct 5, 2023

Codecov Report

Merging #971 (131240d) into main (d93b237) will increase coverage by 1.23%.
Report is 5 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #971      +/-   ##
==========================================
+ Coverage   68.79%   70.03%   +1.23%     
==========================================
  Files          47       47              
  Lines        5987     6384     +397     
  Branches     5987     6384     +397     
==========================================
+ Hits         4119     4471     +352     
- Misses       1493     1544      +51     
+ Partials      375      369       -6     

see 7 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a 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 r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @nimrod-starkware)


crates/blockifier/src/transaction/account_transactions_test.rs line 235 at r1 (raw file):

#[case(TransactionVersion::ZERO)]
#[case(TransactionVersion::ONE)]

remove newline between the #[case] and the TODO


crates/blockifier/src/transaction/account_transactions_test.rs line 271 at r1 (raw file):

    );
    match tx_result {
        Ok(info) => assert!(info.revert_error.is_some()),

as with the Err case, need to explicitly check for the expected error.
The error is a string in this case, so just assert that some indicative string is a substring of the error (maybe "is not deployed"? not sure if it's the same text).
If it's the same text you're searching for, please put it in a variable

Code quote:

revert_error.is_some()

crates/blockifier/src/transaction/account_transactions_test.rs line 274 at r1 (raw file):

        Err(err) => {
            assert!(matches!(err, TransactionExecutionError::ExecutionError(e) if
            matches!(&e, EntryPointExecutionError::VirtualMachineExecutionErrorWithTrace { trace, .. } if trace.contains("is not deployed.")))); // make sure the error is because the account wasn't deployed

I think this is better, if possible; full destructuring instead of two ifs

Suggestion:

            assert!(matches!(err, TransactionExecutionError::ExecutionError(
                EntryPointExecutionError::VirtualMachineExecutionErrorWithTrace { trace, .. }
                if trace.contains("is not deployed.")
            )));

crates/blockifier/src/transaction/account_transactions_test.rs line 275 at r1 (raw file):

            assert!(matches!(err, TransactionExecutionError::ExecutionError(e) if
            matches!(&e, EntryPointExecutionError::VirtualMachineExecutionErrorWithTrace { trace, .. } if trace.contains("is not deployed.")))); // make sure the error is because the account wasn't deployed
            assert!(matches!(tx_version, TransactionVersion::ZERO)); // we excpect to get an error only when tx_version is 0, on other versions to revert

@nimrod-starkware we start comments with capital letters and end with periods.
Also, they should be on the line above the line of code.
@giladchase shouldn't the formatter have failed this PR? Maybe your recent change did something...? These lines are over 100 chars long

Suggestion:

            // Make sure the error is because the account wasn't deployed.
            matches!(&e, EntryPointExecutionError::VirtualMachineExecutionErrorWithTrace { trace, .. } if trace.contains("is not deployed."))));
            // We excpect to get an error only when tx_version is 0, on other versions to revert.
            assert!(matches!(tx_version, TransactionVersion::ZERO));

Copy link
Collaborator

@giladchase giladchase left a 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, 4 unresolved discussions (waiting on @nimrod-starkware)


crates/blockifier/src/transaction/account_transactions_test.rs line 275 at r1 (raw file):

@giladchase shouldn't the formatter have failed this PR? Maybe your recent change did something...? These lines are over 100 chars long

rustfmt doesn't work for items that include macros: rust-lang/rustfmt#8
What recent change?

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a 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, 4 unresolved discussions (waiting on @nimrod-starkware)


crates/blockifier/src/transaction/account_transactions_test.rs line 275 at r1 (raw file):

Previously, giladchase wrote…

@giladchase shouldn't the formatter have failed this PR? Maybe your recent change did something...? These lines are over 100 chars long

rustfmt doesn't work for items that include macros: rust-lang/rustfmt#8
What recent change?

ah... nevermind then... damn
@nimrod-starkware please ensure no line is over 100 chars
(recent change was in SN-API repo.. my bad)

@nimrod-starkware nimrod-starkware force-pushed the nimrod/move_test_internal_invoke_tx_failure_from_python_to_rust branch 2 times, most recently from 6da1f7d to 93261a4 Compare October 5, 2023 10:06
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a 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 r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nimrod-starkware)


crates/blockifier/src/transaction/account_transactions_test.rs line 276 at r2 (raw file):

        Ok(info) => {
            //  Make sure the error is because the account wasn't deployed.
            assert!(info.revert_error.is_some_and(|err_str| err_str.contains("is not deployed.")));

please add let expected_error = "is not deployed" before the match, and use it twice

Code quote:

"is not deployed."

crates/blockifier/src/transaction/account_transactions_test.rs line 284 at r2 (raw file):

                if trace.contains("is not deployed.")
            ));
            // We excpect to get an error only when tx_version is 0, on other versions to revert.

typo

Code quote:

 excpect

@nimrod-starkware nimrod-starkware force-pushed the nimrod/move_test_internal_invoke_tx_failure_from_python_to_rust branch 2 times, most recently from 30abc37 to 28b6f91 Compare October 5, 2023 10:12
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a 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 r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nimrod-starkware)

@nimrod-starkware nimrod-starkware force-pushed the nimrod/move_test_internal_invoke_tx_failure_from_python_to_rust branch from 28b6f91 to 2fb206c Compare October 5, 2023 10:14
@nimrod-starkware nimrod-starkware force-pushed the nimrod/move_test_internal_invoke_tx_failure_from_python_to_rust branch from 2fb206c to 131240d Compare October 5, 2023 10:16
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @nimrod-starkware)

@nimrod-starkware nimrod-starkware merged commit f15130f into main Oct 5, 2023
@nimrod-starkware nimrod-starkware deleted the nimrod/move_test_internal_invoke_tx_failure_from_python_to_rust branch October 5, 2023 10:23
gswirski pushed a commit to reilabs/blockifier that referenced this pull request Jun 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants