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

test(concurrency): add ERC20 cairo 1 contract for testing #1875

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

meship-starkware
Copy link
Contributor

@meship-starkware meship-starkware commented May 7, 2024

This change is Reviewable

@codecov-commenter
Copy link

codecov-commenter commented May 7, 2024

Codecov Report

Attention: Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.

Project coverage is 78.66%. Comparing base (b898b1c) to head (d85da65).
Report is 1 commits behind head on main.

Files Patch % Lines
crates/blockifier/src/test_utils/contracts.rs 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1875      +/-   ##
==========================================
+ Coverage   78.60%   78.66%   +0.05%     
==========================================
  Files          62       62              
  Lines        8892     8917      +25     
  Branches     8892     8917      +25     
==========================================
+ Hits         6990     7015      +25     
- Misses       1455     1457       +2     
+ Partials      447      445       -2     

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

@meship-starkware meship-starkware force-pushed the meship/blockifier/add_fee_utils_to_concurrecy branch 2 times, most recently from 73712d0 to 270d343 Compare May 7, 2024 08:22
@meship-starkware meship-starkware force-pushed the meshi/add_cairo1_erc20_to_tests branch from 073230e to d6757cf Compare May 7, 2024 08:28
Copy link
Contributor Author

@meship-starkware meship-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 6 files reviewed, 1 unresolved discussion (waiting on @avi-starkware, @noaov1, and @Yoni-Starkware)


crates/blockifier/src/concurrency/fee_utils_test.rs line 25 at r2 (raw file):

        version: TransactionVersion::THREE
    });
    let chain_info = &block_context.chain_info;

It might be a good idea to create a function out of this code, which would make it easier if another test wants to use cairo1 erc20 later.

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

Where did you find this contract?

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @avi-starkware and @noaov1)

@meship-starkware meship-starkware force-pushed the meship/blockifier/add_fee_utils_to_concurrecy branch 2 times, most recently from 44c89e8 to 6539532 Compare May 8, 2024 10:05
Copy link
Contributor Author

@meship-starkware meship-starkware left a comment

Choose a reason for hiding this comment

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

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

Oh :)
so please rename the file to erc20.casm.json

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @avi-starkware and @noaov1)

Copy link
Collaborator

@Yoni-Starkware Yoni-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.
Reviewable status: 1 of 6 files reviewed, 3 unresolved discussions (waiting on @avi-starkware, @meship-starkware, and @noaov1)


crates/blockifier/src/concurrency/fee_utils_test.rs line 36 at r2 (raw file):

    let mut concurrency_call_info = create_fee_transfer_call_data(state, &account_tx, true);
    let call_info = create_fee_transfer_call_data(state, &account_tx, false);

You can remove test_state_cairo1

Suggestion:

    let state_reader = test_state_reader(chain_info, BALANCE, &[(account, 1)], erc20_version)

    let sequencer_balance = 100;
    let sequencer_address = block_context.block_info.sequencer_address;
    fund_account(chain_info, sequencer_address, sequencer_balance, &mut state_reader);

    let state = CachedState::from(state_reader);
    let mut concurrency_call_info = create_fee_transfer_call_data(state, &account_tx, true);
    let call_info = create_fee_transfer_call_data(state, &account_tx, false);

crates/blockifier/src/test_utils/initial_test_state.rs line 45 at r2 (raw file):

    initial_balances: u128,
    contract_instances: &[(FeatureContract, u16)],
    erc20_contract_version: CairoVersion,

erc20_version/erc20_cairo_version

Suggestion:

erc20_version

@meship-starkware meship-starkware force-pushed the meshi/add_cairo1_erc20_to_tests branch 2 times, most recently from c09f2b3 to 61bde95 Compare May 9, 2024 06:12
@meship-starkware meship-starkware force-pushed the meship/blockifier/add_fee_utils_to_concurrecy branch 2 times, most recently from 5b81e9f to 9ae663f Compare May 9, 2024 06:21
Base automatically changed from meship/blockifier/add_fee_utils_to_concurrecy to main May 9, 2024 06:23
@meship-starkware meship-starkware force-pushed the meshi/add_cairo1_erc20_to_tests branch from 61bde95 to af0285c Compare May 9, 2024 06:25
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.

Can you please add the cairo code itself?

Reviewed 3 of 6 files at r1, 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @avi-starkware and @meship-starkware)


crates/blockifier/ERC20_without_some_syscalls/ERC20/erc20_contract_without_some_syscalls.casm.json line 0 at r3 (raw file):
What is this file?


crates/blockifier/src/test_utils/contracts.rs line 53 at r3 (raw file):

// ERC20 contract is in a unique location.
const ERC20_CONTRACT_PATH: &str =
    "./ERC20_without_some_syscalls/ERC20/erc20_contract_without_some_syscalls_compiled.json";

Suggestion:

const ERC20_CAIRO0_CONTRACT_PATH: &str =
    "./ERC20_without_some_syscalls/ERC20/erc20_contract_without_some_syscalls_compiled.json";

crates/blockifier/src/test_utils/initial_test_state.rs line 8 at r3 (raw file):

use strum::IntoEnumIterator;

use super::CairoVersion;

Please avoid using super

Code quote:

use super::CairoVersion;

Copy link
Contributor Author

@meship-starkware meship-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 @avi-starkware and @noaov1)


crates/blockifier/ERC20_without_some_syscalls/ERC20/erc20_contract_without_some_syscalls.casm.json line at r3 (raw file):

Previously, noaov1 (Noa Oved) wrote…

What is this file?

it's the same as the erc20. I forgot to delete it.

@meship-starkware meship-starkware force-pushed the meshi/add_cairo1_erc20_to_tests branch from af0285c to dac2250 Compare May 16, 2024 08:49
Copy link
Contributor Author

@meship-starkware meship-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: 4 of 8 files reviewed, 3 unresolved discussions (waiting on @avi-starkware and @noaov1)


crates/blockifier/src/test_utils/contracts.rs line 53 at r3 (raw file):

// ERC20 contract is in a unique location.
const ERC20_CONTRACT_PATH: &str =
    "./ERC20_without_some_syscalls/ERC20/erc20_contract_without_some_syscalls_compiled.json";

Done.


crates/blockifier/src/test_utils/initial_test_state.rs line 8 at r3 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Please avoid using super

Done.


crates/blockifier/ERC20_without_some_syscalls/ERC20/erc20_contract_without_some_syscalls.casm.json line at r3 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

it's the same as the erc20. I forgot to delete it.

Done.

Copy link
Collaborator

@Yoni-Starkware Yoni-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 4 files at r3, all commit messages.
Reviewable status: 4 of 8 files reviewed, 4 unresolved discussions (waiting on @avi-starkware, @meship-starkware, and @noaov1)


crates/blockifier/src/concurrency/fee_utils_test.rs line 18 at r4 (raw file):

#[rstest]
pub fn test_fix_call_info(

Seems irrelevant for this PR. Did you rebase?

Code quote:

ub fn test_fix_call_info(

Copy link
Collaborator

@Yoni-Starkware Yoni-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: 4 of 8 files reviewed, 5 unresolved discussions (waiting on @avi-starkware, @meship-starkware, and @noaov1)


crates/blockifier/src/test_utils/contracts.rs line 54 at r4 (raw file):

const ERC20_CAIRO0_CONTRACT_PATH: &str =
    "./ERC20_without_some_syscalls/ERC20/erc20_contract_without_some_syscalls_compiled.json";
const ERC20_CAIRO1_CONTRACT_PATH: &str = "./ERC20_without_some_syscalls/ERC20/erc20.casm.json";

Please fix the file locations accordingly

Suggestion:

const ERC20_CAIRO0_CONTRACT_PATH: &str =
    "./ERC20/ERC20_cairo0/erc20_contract_without_some_syscalls_compiled.json";
const ERC20_CAIRO1_CONTRACT_PATH: &str = "./ERC20/ERC20_cairo1/erc20.casm.json";

@meship-starkware
Copy link
Contributor Author

crates/blockifier/src/concurrency/fee_utils_test.rs line 18 at r4 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Seems irrelevant for this PR. Did you rebase?

I rebased now, but these changes are for this test to use the cairo1 erc20. LMK if you still think there are still irrelevant changes.

Copy link
Contributor Author

@meship-starkware meship-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 16 files reviewed, 6 unresolved discussions (waiting on @avi-starkware, @noaov1, and @Yoni-Starkware)


crates/blockifier/src/concurrency/fee_utils_test.rs line 18 at r4 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

I rebased now, but these changes are for this test to use the cairo1 erc20. LMK if you still think there are still irrelevant changes.

I understood the message and will change it.
Done.


crates/blockifier/src/test_utils/contracts.rs line 54 at r4 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Please fix the file locations accordingly

Done.


crates/blockifier/src/test_utils/initial_test_state.rs line 87 at r5 (raw file):

    CachedState::from(state_reader)
}

I am not sure this is the solution we want here, but it was the easiest way to compensate for the fact that Rust does not have default parameters. I can add to all the places that use test_state Cairo version 0 instead.

Copy link
Collaborator

@Yoni-Starkware Yoni-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 4 files at r4, 14 of 15 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @avi-starkware and @noaov1)


crates/blockifier/src/test_utils/initial_test_state.rs line 87 at r5 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

I am not sure this is the solution we want here, but it was the easiest way to compensate for the fact that Rust does not have default parameters. I can add to all the places that use test_state Cairo version 0 instead.

That's okay. Maybe choose a better name in a different PR (test_state_with_erc20_v0 to the old func)
Can you please change @avi-starkware transfers test to use the new erc20? (also in a different PR)

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

@meship-starkware meship-starkware merged commit 35f5081 into main Jun 20, 2024
9 checks passed
gswirski pushed a commit to reilabs/blockifier that referenced this pull request Jun 26, 2024
)

* feat: enable global flag for TransactionOptions

* set global flag on all options

* remove short from args
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