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

feat: expose versioned constants from past versions #2049

Merged

Conversation

dorimedini-starkware
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware commented Jul 8, 2024

This change is Reviewable

@dorimedini-starkware dorimedini-starkware self-assigned this Jul 8, 2024
@dorimedini-starkware dorimedini-starkware force-pushed the dori/expose-versioned-constants-versions branch 2 times, most recently from 0a54c33 to 8617d83 Compare July 8, 2024 06:51
Copy link
Collaborator Author

@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: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @amosStarkware and @Yoni-Starkware)


Cargo.toml line 41 at r2 (raw file):

once_cell = "1.19.0"
papyrus_storage = "0.4.0-dev.4"
paste = "1.0.15"

Used to concatenate identifiers, for example:

paste! { [<VERSIONED_CONSTANTS_ $variant>] }

for $variant = V0_13_0 becomes

VERSIONED_CONSTANTS_V0_13_0

Code quote:

paste = "1.0.15"

Copy link
Collaborator Author

@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: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @amosStarkware, @giladchase, and @Yoni-Starkware)


crates/blockifier/src/versioned_constants.rs line 42 at r3 (raw file):

        paste! {
            $(
                pub(crate) const [<VERSIONED_CONSTANTS_ $variant:upper _JSON>]: &str =

VERSIONED_CONSTANTS_V0_13_0_JSON for example.
If $variant is Latest then we'll get VERSIONED_CONSTANTS_LATEST_JSON

Code quote:

[<VERSIONED_CONSTANTS_ $variant:upper _JSON>]

Copy link
Collaborator Author

@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: 0 of 6 files reviewed, 3 unresolved discussions (waiting on @amosStarkware, @giladchase, and @Yoni-Starkware)


crates/blockifier/src/versioned_constants.rs line 35 at r3 (raw file):

        /// Enum of all the versioned constants versions.
        #[derive(Clone, Debug, EnumCount, EnumIter, Hash, Eq, PartialEq)]
        pub enum StarknetVersionForVersionedConstants {

name suggestions?

Code quote:

StarknetVersionForVersionedConstants

@dorimedini-starkware dorimedini-starkware force-pushed the dori/expose-versioned-constants-versions branch 4 times, most recently from c8dd7ae to f970b61 Compare July 9, 2024 16:43
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.

Can you change the py_block_executor to use the new getter?

Reviewed 2 of 6 files at r2, 2 of 4 files at r5, all commit messages.
Reviewable status: 4 of 6 files reviewed, 4 unresolved discussions (waiting on @amosStarkware, @dorimedini-starkware, and @giladchase)


crates/blockifier/src/versioned_constants.rs line 35 at r3 (raw file):

Previously, dorimedini-starkware wrote…

name suggestions?

VersionedConstantsByStarknetVersion?


crates/blockifier/src/versioned_constants.rs line 119 at r5 (raw file):

    /// To use custom constants, initialize the struct from a file using `try_from`.
    pub fn latest_constants() -> &'static Self {
        &VERSIONED_CONSTANTS_LATEST

Use the new impl

(how can you refer to a name that is not defined?)

Suggestion:

   Self::from(StarknetVersion::Latest)

Copy link
Collaborator Author

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

WDYM?
It calls the latest_constants endpoint, why change it?

Reviewable status: 4 of 6 files reviewed, 4 unresolved discussions (waiting on @amosStarkware, @giladchase, and @Yoni-Starkware)


crates/blockifier/src/versioned_constants.rs line 35 at r3 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

VersionedConstantsByStarknetVersion?

isn't StarknetVersion better...?


crates/blockifier/src/versioned_constants.rs line 119 at r5 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Use the new impl

(how can you refer to a name that is not defined?)

Self::from or Self::get? I'm in favor of the latter

@dorimedini-starkware dorimedini-starkware force-pushed the dori/expose-versioned-constants-versions branch from f970b61 to 5eb43d4 Compare July 18, 2024 13:00
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.

It is a step towards getting the version from Python, but it can be a separate PR.
:lgtm:

Reviewed 1 of 4 files at r5, 1 of 3 files at r6, all commit messages.
Reviewable status: 4 of 6 files reviewed, 3 unresolved discussions (waiting on @amosStarkware, @dorimedini-starkware, and @giladchase)


crates/blockifier/src/versioned_constants.rs line 35 at r3 (raw file):

Previously, dorimedini-starkware wrote…

isn't StarknetVersion better...?

Ok, but it contains more than just the version

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 2 of 3 files at r6.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @amosStarkware, @dorimedini-starkware, and @giladchase)

Copy link
Collaborator Author

@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, 2 unresolved discussions (waiting on @amosStarkware and @giladchase)


crates/blockifier/src/versioned_constants.rs line 35 at r3 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Ok, but it contains more than just the version

WDYM? the enum is just the version, no data

@dorimedini-starkware dorimedini-starkware merged commit 08da831 into main-v0.13.2 Jul 19, 2024
11 checks passed
@dorimedini-starkware dorimedini-starkware deleted the dori/expose-versioned-constants-versions branch July 19, 2024 10:47
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