Skip to content
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

chore(blockifier): add test declare v0 transaction has zero fee #1503

Merged
merged 1 commit into from
Oct 30, 2024

Conversation

avivg-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/test_fee_declare_v0 branch from bcfc33e to cf4d6ac Compare October 21, 2024 12:59
Copy link

codecov bot commented Oct 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.14%. Comparing base (e3165c4) to head (14acef4).
Report is 88 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1503       +/-   ##
===========================================
+ Coverage   40.10%   69.14%   +29.03%     
===========================================
  Files          26      100       +74     
  Lines        1895    13447    +11552     
  Branches     1895    13447    +11552     
===========================================
+ Hits          760     9298     +8538     
- Misses       1100     3747     +2647     
- Partials       35      402      +367     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/test_fee_declare_v0 branch from cf4d6ac to 3647bd4 Compare October 21, 2024 14:32
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/remove_enforce_fee_from_actual_fee branch from 88e2fa5 to 5d61ccb Compare October 21, 2024 14:33
Copy link
Collaborator

@noaov1 noaov1 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, 6 unresolved discussions (waiting on @avivg-starkware)


crates/blockifier/src/transaction/transactions_test.rs line 1510 at r1 (raw file):

#[rstest]
fn test_declare_tx_v0(default_l1_resource_bounds: ValidResourceBounds) {
    let account_cairo_version = CairoVersion::Cairo0; //defoult

I think you can remove it.

Suggestion:

let account_cairo_version = CairoVersion::Cairo1;

crates/blockifier/src/transaction/transactions_test.rs line 1513 at r1 (raw file):

    let use_kzg_da = true; //default
    let tx_version = TransactionVersion::ZERO;
    let block_context = &BlockContext::create_for_account_testing_with_kzg(use_kzg_da);

And than you can remove the variable use_kzg_da :)

Suggestion:

    let block_context = &BlockContext::create_for_account_testing();

crates/blockifier/src/transaction/transactions_test.rs line 1515 at r1 (raw file):

    let block_context = &BlockContext::create_for_account_testing_with_kzg(use_kzg_da);
    let versioned_constants = &block_context.versioned_constants;
    let empty_contract = FeatureContract::Empty(CairoVersion::Cairo0);

Suggestion:

let empty_contract = FeatureContract::Empty(CairoVersion::Cairo1);

crates/blockifier/src/transaction/transactions_test.rs line 1516 at r1 (raw file):

    let versioned_constants = &block_context.versioned_constants;
    let empty_contract = FeatureContract::Empty(CairoVersion::Cairo0);
    let account = FeatureContract::AccountWithoutValidations(account_cairo_version);

Suggestion:

    let account = FeatureContract::AccountWithoutValidations(CairoVersion::Cairo1);

crates/blockifier/src/transaction/transactions_test.rs line 1535 at r1 (raw file):

    let account_tx = declare_tx(
        declare_tx_args! {
            max_fee: MAX_FEE,

Suggestion:

max_fee:0,

crates/blockifier/src/transaction/transactions_test.rs line 1599 at r1 (raw file):

        },
        revert_error: None,
    };

Please remove. You can just compare the actual_fee and the fee_transfer_call_info.

Code quote:

    // Build expected validate call info.
    let expected_validate_call_info = declare_validate_callinfo(
        tx_version,
        account_cairo_version,
        class_hash,
        account.get_class_hash(),
        sender_address,
        account
            .get_class()
            .tracked_resource(&versioned_constants.min_compiler_version_for_sierra_gas),
    );

    let da_gas = starknet_resources.state.to_gas_vector(use_kzg_da);
    let expected_cairo_resources = get_expected_cairo_resources(
        versioned_constants,
        TransactionType::Declare,
        &starknet_resources,
        vec![&expected_validate_call_info],
    );
    let mut expected_actual_resources = TransactionResources {
        starknet_resources,
        computation: ComputationResources {
            vm_resources: expected_cairo_resources,
            ..Default::default()
        },
    };

    add_kzg_da_resources_to_resources_mapping(
        &mut expected_actual_resources.computation.vm_resources,
        &state_changes_for_fee,
        versioned_constants,
        use_kzg_da,
    );

    let expected_total_gas = expected_actual_resources.to_gas_vector(
        versioned_constants,
        use_kzg_da,
        &GasVectorComputationMode::NoL2Gas,
    );

    let expected_execution_info = TransactionExecutionInfo {
        validate_call_info: expected_validate_call_info,
        execute_call_info: None,
        fee_transfer_call_info: None,
        receipt: TransactionReceipt {
            fee: Fee(0),
            da_gas,
            resources: expected_actual_resources,
            gas: expected_total_gas,
        },
        revert_error: None,
    };

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/test_fee_declare_v0 branch from 3647bd4 to 12e2894 Compare October 22, 2024 08:10
Copy link
Contributor Author

@avivg-starkware avivg-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: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @noaov1)


crates/blockifier/src/transaction/transactions_test.rs line 1510 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

I think you can remove it.

Done.


crates/blockifier/src/transaction/transactions_test.rs line 1513 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

And than you can remove the variable use_kzg_da :)

Done.


crates/blockifier/src/transaction/transactions_test.rs line 1515 at r1 (raw file):

    let block_context = &BlockContext::create_for_account_testing_with_kzg(use_kzg_da);
    let versioned_constants = &block_context.versioned_constants;
    let empty_contract = FeatureContract::Empty(CairoVersion::Cairo0);

Done.


crates/blockifier/src/transaction/transactions_test.rs line 1516 at r1 (raw file):

    let versioned_constants = &block_context.versioned_constants;
    let empty_contract = FeatureContract::Empty(CairoVersion::Cairo0);
    let account = FeatureContract::AccountWithoutValidations(account_cairo_version);

Done.


crates/blockifier/src/transaction/transactions_test.rs line 1535 at r1 (raw file):

    let account_tx = declare_tx(
        declare_tx_args! {
            max_fee: MAX_FEE,

Done.


crates/blockifier/src/transaction/transactions_test.rs line 1599 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Please remove. You can just compare the actual_fee and the fee_transfer_call_info.

Done.

Copy link
Collaborator

@noaov1 noaov1 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 @avivg-starkware)


crates/blockifier/src/transaction/transactions_test.rs line 1512 at r2 (raw file):

    let tx_version = TransactionVersion::ZERO;
    let block_context = &BlockContext::create_for_account_testing();
    let versioned_constants = &block_context.versioned_constants;

Can be removed, right?

Code quote:

    let versioned_constants = &block_context.versioned_constants;

crates/blockifier/src/transaction/transactions_test.rs line 1530 at r2 (raw file):

        None,
        ExecutionSummary::default(),
    );

Please remove

Code quote:

    let state_changes_for_fee = declare_expected_state_changes_count(tx_version);
    let starknet_resources = StarknetResources::new(
        0,
        0,
        class_info.code_size(),
        StateResources::new_for_testing(state_changes_for_fee),
        None,
        ExecutionSummary::default(),
    );

crates/blockifier/src/transaction/transactions_test.rs line 1546 at r2 (raw file):

    let actual_execution_info = account_tx.execute(state, block_context, true, true).unwrap();

    // Test execution info result.

Please remove (same below)

Code quote:

    // Test execution info result.

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/test_fee_declare_v0 branch from 12e2894 to 5d6804a Compare October 22, 2024 16:44
Copy link
Contributor Author

@avivg-starkware avivg-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, 3 unresolved discussions (waiting on @noaov1)


crates/blockifier/src/transaction/transactions_test.rs line 1515 at r1 (raw file):

Previously, avivg-starkware wrote…

Done.

Apologies, I reverted, seems like it's necessary for compatibility with the transaction version (otherwise panicks on fn declare_tx (line 1522)

Code snippet:

pub fn declare_tx(declare_tx_args: DeclareTxArgs, class_info: ClassInfo) -> AccountTransaction {
    let tx_hash = declare_tx_args.tx_hash;
    let declare_tx = starknet_api::test_utils::declare::declare_tx(declare_tx_args);

   panicks on this line--->  AccountTransaction::Declare(DeclareTransaction::new(declare_tx, tx_hash, class_info).unwrap())
}

crates/blockifier/src/transaction/transactions_test.rs line 1535 at r1 (raw file):

Previously, avivg-starkware wrote…

Done.

Also apologies, I reverted as well. when using Fee(0), it panicks with min higher than max. I will look into it, I think this might point to something I should fix, WDYT?


crates/blockifier/src/transaction/transactions_test.rs line 1512 at r2 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Can be removed, right?

yes indeed thankyou!


crates/blockifier/src/transaction/transactions_test.rs line 1530 at r2 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Please remove

Done.


crates/blockifier/src/transaction/transactions_test.rs line 1546 at r2 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Please remove (same below)

Done.

Copy link

Artifacts upload triggered. View details here

Copy link
Collaborator

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


crates/blockifier/src/transaction/transactions_test.rs line 1535 at r1 (raw file):

Previously, avivg-starkware wrote…

Also apologies, I reverted as well. when using Fee(0), it panicks with min higher than max. I will look into it, I think this might point to something I should fix, WDYT?

Try setting the charge_fee flag to false.

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/test_fee_declare_v0 branch from 5d6804a to e44d858 Compare October 27, 2024 12:11
Copy link

Artifacts upload triggered. View details here

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/remove_enforce_fee_from_actual_fee branch from 5d61ccb to 711b445 Compare October 27, 2024 12:13
Copy link
Contributor Author

@avivg-starkware avivg-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: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @noaov1)


crates/blockifier/src/transaction/transactions_test.rs line 1535 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Try setting the charge_fee flag to false.

definitely. solved, thankyou

Copy link
Collaborator

@noaov1 noaov1 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 @avivg-starkware)

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/remove_enforce_fee_from_actual_fee branch from 711b445 to a5cc906 Compare October 28, 2024 09:04
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/test_fee_declare_v0 branch from e44d858 to 9a92234 Compare October 28, 2024 09:04
Copy link

Artifacts upload triggered. View details here

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/remove_enforce_fee_from_actual_fee branch from a5cc906 to 8a1bdd4 Compare October 30, 2024 12:22
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/test_fee_declare_v0 branch from 9a92234 to 71cfefc Compare October 30, 2024 12:25
Copy link

Artifacts upload triggered. View details here

@avivg-starkware avivg-starkware changed the base branch from avivg/blockifier/remove_enforce_fee_from_actual_fee to main October 30, 2024 13:28
Copy link

Artifacts upload triggered. View details here

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/test_fee_declare_v0 branch from 71cfefc to 14acef4 Compare October 30, 2024 13:31
Copy link

Artifacts upload triggered. View details here

Copy link
Contributor Author

@avivg-starkware avivg-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 r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @avivg-starkware)

@avivg-starkware avivg-starkware merged commit 5f90026 into main Oct 30, 2024
12 checks passed
@avivg-starkware avivg-starkware deleted the avivg/blockifier/test_fee_declare_v0 branch October 30, 2024 15:28
@github-actions github-actions bot locked and limited conversation to collaborators Nov 2, 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.

3 participants