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 erc20 sierra contract #2668

Open
wants to merge 1 commit into
base: aviv/refactor_py_state_reader
Choose a base branch
from

Conversation

AvivYossef-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

AvivYossef-starkware commented Dec 15, 2024

Copy link
Contributor Author

@AvivYossef-starkware AvivYossef-starkware left a comment

Choose a reason for hiding this comment

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

The contact is taken from here

Reviewable status: 0 of 2 files reviewed, all discussions resolved

@AvivYossef-starkware AvivYossef-starkware marked this pull request as ready for review December 15, 2024 12:30
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/refactor_py_state_reader branch from d5d55fb to 57b45d3 Compare December 15, 2024 12:34
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/add_erc20_sierra_contract branch from b6de543 to a38fb1d Compare December 15, 2024 12:34
Copy link
Contributor

@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.

Reviewed 1 of 2 files at r1, all commit messages.
Reviewable status: 1 of 2 files reviewed, 4 unresolved discussions (waiting on @AvivYossef-starkware and @dorimedini-starkware)


crates/blockifier/src/test_utils/contracts.rs line 294 at r1 (raw file):

        assert_ne!(self.cairo_version(), CairoVersion::Cairo0);
        // TODO (Meshi 01/01/2025): add a spacial case for ERC20 when ERC20 sierra is supported.
        assert!(!matches!(self, &Self::ERC20(CairoVersion::Cairo1(_))));

I am not sure about the comment phrasing but, I would like to have the fact that this is not the ERC20 compiled Sierra documented.

Suggestion:

 // This is not the compiled Sierra file of the existing ERC20 contract, 
 // but a file that was taken from the compiler repo of another ERC20 contract. 
 if matches!(self, &Self::ERC20(CairoVersion::Cairo1(_))) {
     return ERC20_SIERRA_CONTRACT_PATH;
}

crates/blockifier/src/test_utils/contracts.rs line 83 at r1 (raw file):

                                          erc20_contract_without_some_syscalls_compiled.json";
const ERC20_CAIRO1_CONTRACT_SOURCE_PATH: &str = "./ERC20/ERC20_Cairo1/ERC20.cairo";
const ERC20_CONTRACT_NAME: &str = "erc20_contract";

Suggestion:

const ERC20_SIERRA_CONTRACT_PATH: &str = "./ERC20/ERC20_Cairo1/erc20.sierra.json";

crates/blockifier/src/test_utils/contracts.rs line 253 at r1 (raw file):

    }

    fn get_non_erc20_base_name(&self) -> &str {

Change the name if it also returns the ERC20 contract

Code quote:

get_non_erc20_base_name

crates/blockifier/src/test_utils/contracts.rs line 264 at r1 (raw file):

            Self::CairoStepsTestContract => CAIRO_STEPS_TEST_CONTRACT_NAME,
            Self::SierraExecutionInfoV1Contract(_) => EXECUTION_INFO_V1_CONTRACT_NAME,
            Self::ERC20(_) => ERC20_CONTRACT_NAME,

Suggestion:

 Self::ERC20(_) => unreachable!(),

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/refactor_py_state_reader branch from 57b45d3 to 29d2e1e Compare December 15, 2024 14:05
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/add_erc20_sierra_contract branch from a38fb1d to df0d546 Compare December 15, 2024 14:05
Copy link
Contributor Author

@AvivYossef-starkware AvivYossef-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: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware and @meship-starkware)


crates/blockifier/src/test_utils/contracts.rs line 253 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Change the name if it also returns the ERC20 contract

Done.


crates/blockifier/src/test_utils/contracts.rs line 294 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

I am not sure about the comment phrasing but, I would like to have the fact that this is not the ERC20 compiled Sierra documented.

Done.

Copy link
Contributor

@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.

Reviewed all commit messages.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware and @dorimedini-starkware)


crates/blockifier/feature_contracts/cairo1/sierra/erc20_contract.sierra.json line 49 at r2 (raw file):

        "0x2ee1e2b1b89f8c495f200e4956278a4d47395fe262f27b52e5865c9524c08c3",
        "0x3288d594b9a45d15bb2fcb7903f06cdb06b27f0ba88186ec4cfaa98307cb972",
        "0x11",

Move this file to crates/blockifier/ERC20/ERC20_Cairo1

Copy link
Contributor

@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.

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware and @dorimedini-starkware)

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/refactor_py_state_reader branch from 29d2e1e to 3a292c7 Compare December 15, 2024 15:28
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 all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware)

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/add_erc20_sierra_contract branch from c857512 to 69c3d44 Compare December 15, 2024 19:04
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/refactor_py_state_reader branch from 56b7c70 to aab30c9 Compare December 15, 2024 19:34
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/add_erc20_sierra_contract branch from 69c3d44 to c17bcea Compare December 15, 2024 19:35
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/refactor_py_state_reader branch 2 times, most recently from c832388 to e66c37b Compare December 15, 2024 20:07
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/add_erc20_sierra_contract branch from c17bcea to e83ae75 Compare December 15, 2024 20:07
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/add_erc20_sierra_contract branch from e83ae75 to f45b6e0 Compare December 15, 2024 20:18
Copy link
Contributor Author

@AvivYossef-starkware AvivYossef-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: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @meship-starkware)


crates/blockifier/feature_contracts/cairo1/sierra/erc20_contract.sierra.json line 49 at r2 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Move this file to crates/blockifier/ERC20/ERC20_Cairo1

ops

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants