-
Notifications
You must be signed in to change notification settings - Fork 107
feat: expose versioned constants from past versions #2049
feat: expose versioned constants from past versions #2049
Conversation
0a54c33
to
8617d83
Compare
There was a problem hiding this 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"
8617d83
to
0c24692
Compare
There was a problem hiding this 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>]
There was a problem hiding this 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
c8dd7ae
to
f970b61
Compare
There was a problem hiding this 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)
There was a problem hiding this 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
Signed-off-by: Dori Medini <[email protected]>
f970b61
to
5eb43d4
Compare
There was a problem hiding this 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.
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
There was a problem hiding this 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)
There was a problem hiding this 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
This change is