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

feat: add the new stack trace to ValidateTransactionError #1712

Merged
merged 2 commits into from
Mar 27, 2024

Conversation

zuphitf
Copy link
Contributor

@zuphitf zuphitf commented Mar 25, 2024

This change is Reviewable

@zuphitf zuphitf self-assigned this Mar 25, 2024
@zuphitf zuphitf force-pushed the zuphit/stack_trace/add_to_validate branch from 39968fd to c0cc493 Compare March 25, 2024 09:43
@zuphitf zuphitf changed the title feat(stack trace): add the new stack trace to ValidateTransactionError feat: add the new stack trace to ValidateTransactionError Mar 25, 2024
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 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @zuphitf)

a discussion (no related file):
Please add a regression test for this


@zuphitf zuphitf force-pushed the zuphit/stack_trace/add_to_validate branch from c0cc493 to 905946a Compare March 26, 2024 14:44
@zuphitf
Copy link
Contributor Author

zuphitf commented Mar 26, 2024

Previously, dorimedini-starkware wrote…

Please add a regression test for this

Done.

@zuphitf zuphitf force-pushed the zuphit/stack_trace/add_to_validate branch from 905946a to 8f66070 Compare March 26, 2024 14:47
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 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @zuphitf)


crates/blockifier/Cargo.toml line 57 at r3 (raw file):

pretty_assertions.workspace = true
rstest.workspace = true
test-case.workspace = true

only used in tests, right?

Suggestion:

phf.workspace = true
rstest = { workspace = true, optional = true }
serde = { workspace = true, features = ["derive"] }
serde_json = { workspace = true, features = ["arbitrary_precision"] }
sha3.workspace = true
starknet-crypto.workspace = true
starknet_api = { workspace = true, features = ["testing"] }
strum.workspace = true
strum_macros.workspace = true
thiserror.workspace = true

[dev-dependencies]
assert_matches.workspace = true
pretty_assertions.workspace = true
regex.workspace = true
rstest.workspace = true
test-case.workspace = true

crates/blockifier/src/execution/entry_point_test.rs line 1072 at r3 (raw file):

"
        ),
    };

cleaner, no need for regex, checks basically the same thing; or am I missing something?

Suggestion:

    let expected_error_prefix = format!(
        "Transaction validation has failed:
0: Error in the called contract (contract address: {contract_address}, class hash: {class_hash}, \
             selector: {selector}):"
             );
    let expected_error_suffix = match cairo_version {
        CairoVersion::Cairo0 => "An ASSERT_EQ instruction failed: 1 != 0.",
        CairoVersion::Cairo1 => "Execution failed. Failure reason: 0x496e76616c6964207363656e6172696f ('Invalid scenario')."
    };

crates/blockifier/src/execution/entry_point_test.rs line 1081 at r3 (raw file):

    let cleaned_actual_error = &re.replace_all(&actual_error_str, "pc=0:*");
    // Compare actual trace to the expected trace (sans pc locations).
    assert_eq!(cleaned_actual_error.to_string(), cleaned_expected_error.to_string());

see above

Suggestion:

    let actual_error = account_tx.execute(state, block_context, true, true).unwrap_err();
    let actual_error_str = actual_error.to_string();
    assert!(actual_error_str.startswith(expected_error_prefix));
    assert!(actual_error_str.endswith(expected_error_suffix));

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, 1 unresolved discussion (waiting on @zuphitf)


crates/blockifier/src/execution/entry_point_test.rs line 1072 at r3 (raw file):

Previously, dorimedini-starkware wrote…

cleaner, no need for regex, checks basically the same thing; or am I missing something?

ah no nevermind... asserting equality is better, you assert the number of frames as well. retracting


crates/blockifier/src/execution/entry_point_test.rs line 1081 at r3 (raw file):

Previously, dorimedini-starkware wrote…

see above

ah no nevermind... asserting equality is better, you assert the number of frames as well. retracting

@zuphitf zuphitf force-pushed the zuphit/stack_trace/add_to_validate branch from 8f66070 to c5cd9c8 Compare March 26, 2024 16:06
Copy link
Contributor Author

@zuphitf zuphitf 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, 1 unresolved discussion (waiting on @dorimedini-starkware)


crates/blockifier/Cargo.toml line 57 at r3 (raw file):

Previously, dorimedini-starkware wrote…

only used in tests, right?

Done.

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 @zuphitf)

@zuphitf zuphitf merged commit 06596a7 into main Mar 27, 2024
10 checks passed
@zuphitf zuphitf deleted the zuphit/stack_trace/add_to_validate branch March 27, 2024 12:41
gswirski pushed a commit to reilabs/blockifier that referenced this pull request Jun 26, 2024
* [sozo][auth]: revoke logic added

* test added for auth revoke

* fix: remove world reader as argument

WorldContractReader can be initialized from the inside
of the ops. Hence, we avoid passing it as an argument
and it is initialized inside the ops function.

tests added for the auth revoke

* test added for auth revoke

* fix: refacto testing functions

* fix: add missing spaces

---------

Co-authored-by: glihm <dev@glihm.net>
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.

2 participants