Skip to content

Conversation

@TzahiTaub
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@github-actions
Copy link

github-actions bot commented Jan 12, 2025

Artifacts upload workflows:

Copy link
Contributor Author

TzahiTaub commented Jan 12, 2025

@TzahiTaub TzahiTaub marked this pull request as ready for review January 12, 2025 12:56
@TzahiTaub TzahiTaub force-pushed the 01-12-test_blockifier_refactor_account_transaction_test branch from 1e46d10 to 5881e4d Compare January 12, 2025 14:13
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, 3 unresolved discussions (waiting on @TzahiTaub)


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

    let chain_info = &block_context.chain_info;
    let TestInitData { mut state, account_address, contract_address, .. } =
        create_test_init_data(chain_info, CairoVersion::Cairo1(RunnableCairo1::Casm));

the test contract was previously cairo0. don't we need cairo0 coverage here?

Code quote:

    let TestInitData { mut state, account_address, contract_address, .. } =
        create_test_init_data(chain_info, CairoVersion::Cairo1(RunnableCairo1::Casm));

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

    let chain_info = &block_context.chain_info;
    let TestInitData { mut state, account_address, contract_address, .. } =
        create_test_init_data(chain_info, CairoVersion::Cairo1(RunnableCairo1::Casm));

same remark (was cairo0 test contract)

Code quote:

    let TestInitData { mut state, account_address, contract_address, .. } =
        create_test_init_data(chain_info, CairoVersion::Cairo1(RunnableCairo1::Casm));

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

    block_context.versioned_constants.enable_reverts = enable_reverts;
    let TestInitData { mut state, account_address, contract_address, mut nonce_manager } =
        create_test_init_data(&block_context.chain_info, cairo_version);

similar comment: I think that even if you are testing a cairo0 contract you would want a cairo1 account for this test

Code quote:

    block_context.versioned_constants.enable_reverts = enable_reverts;
    let TestInitData { mut state, account_address, contract_address, mut nonce_manager } =
        create_test_init_data(&block_context.chain_info, cairo_version);

Copy link
Contributor Author

@TzahiTaub TzahiTaub 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, 3 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)


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

Previously, dorimedini-starkware wrote…

the test contract was previously cairo0. don't we need cairo0 coverage here?

It doesn't seem to. This test doesn't check anything about the test contract; it just makes a trivial call to have some base tx.


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

Previously, dorimedini-starkware wrote…

same remark (was cairo0 test contract)

Same as above.


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

Previously, dorimedini-starkware wrote…

similar comment: I think that even if you are testing a cairo0 contract you would want a cairo1 account for this test

This wasn't cairo0, but the account contract was cairo1-casm for the native run, and it will now be native for that run. The address remains the same, it's the same computation for both. This is the TODO @Yoni-Starkware wrote.

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:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)

@TzahiTaub TzahiTaub merged commit a16b2ce into main-v0.13.4 Jan 13, 2025
10 checks passed
@TzahiTaub TzahiTaub deleted the 01-12-test_blockifier_refactor_account_transaction_test branch January 13, 2025 11:09
@github-actions github-actions bot locked and limited conversation to collaborators Jan 15, 2025
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