-
Notifications
You must be signed in to change notification settings - Fork 27
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
feat(starknet_api): add sierra version to class info #2313
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
b8a13b1
to
1caa5af
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.
Should check if Python passes with the new commit before the merge
Reviewable status: 0 of 13 files reviewed, all discussions resolved (waiting on @dorimedini-starkware, @TzahiTaub, and @Yoni-Starkware)
Benchmark movements: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2313 +/- ##
===========================================
+ Coverage 40.10% 77.40% +37.30%
===========================================
Files 26 389 +363
Lines 1895 41288 +39393
Branches 1895 41288 +39393
===========================================
+ Hits 760 31961 +31201
- Misses 1100 7062 +5962
- Partials 35 2265 +2230 ☔ View full report in Codecov by Sentry. |
1caa5af
to
4836cf1
Compare
Benchmark movements: |
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.
@AvivYossef-starkware note that you need to change python's declare tx as well
Reviewed 4 of 13 files at r1.
Reviewable status: 3 of 13 files reviewed, all discussions resolved (waiting on @dorimedini-starkware and @TzahiTaub)
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 11 of 13 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @AvivYossef-starkware and @TzahiTaub)
a discussion (no related file):
- please add reviewers from relevant teams (papyrus + gateway)
- please open py side PR to check for breaking changes
crates/blockifier/src/transaction/test_utils.rs
line 353 at r2 (raw file):
let (sierra_program_length, sierra_version) = match contract_class { ContractClass::V0(_) => (0, SierraVersion::zero()), ContractClass::V1(_) => (100, SierraVersion::new(2, 8, 4)),
move these magic numbers to the struct impl.
that way we can later make it smarter (by checking the latest available compiler version)
Suggestion:
SierraVersion::latest()
crates/papyrus_rpc/src/v0_8/api/mod.rs
line 471 at r2 (raw file):
internal_server_error(format!("Missing deprecated class definition of {class_hash}.")) }) .and_then(|contract_class| {
what's this? why not map
?
Code quote:
and_then
crates/papyrus_rpc/src/v0_8/api/mod.rs
line 475 at r2 (raw file):
let abi_len = contract_class.abi.len(); let sierra_program = (&contract_class.sierra_program).try_into().map_err(internal_server_error)?;
use extract_from_program
explicitly
Code quote:
(&contract_class.sierra_program).try_into()
crates/starknet_api/src/contract_class.rs
line 67 at r2 (raw file):
impl Default for SierraVersion { fn default() -> Self { Self::new(1, 0, 0)
no, use latest()
Code quote:
Self::new(1, 0, 0)
crates/starknet_api/src/contract_class.rs
line 82 at r2 (raw file):
} impl TryFrom<&Vec<Felt>> for SierraVersion {
From
and TryFrom
consume the input by convension; implement an extract_from_program
method instead
Code quote:
impl TryFrom<&Vec<Felt>> for SierraVersion {
crates/starknet_api/src/contract_class.rs
line 94 at r2 (raw file):
// Closure to map a Felt error to a StarknetApiError. let map_felt_to_api_error = |(felt, index): (Felt, usize)| {
Suggestion:
|felt: Felt, index: usize| {
crates/starknet_api/src/contract_class.rs
line 109 at r2 (raw file):
sierra_program[1].try_into().map_err(map_felt_to_api_error((sierra_program[1], 1)))?; let patch = sierra_program[2].try_into().map_err(map_felt_to_api_error((sierra_program[2], 2)))?;
does this work? (non-blocking)
if it does, please delete map_felt_to_api_error and just inline the logic
Suggestion:
let (major, minor, patch): [u64; 3] = [0, 1, 2]
.iter()
.map(|i| {
sierra_program[i].try_into().map_err(map_felt_to_api_error((sierra_program[0], 0)))
})
.collect::<Result<_, _>>()
crates/papyrus_execution/src/lib.rs
line 827 at r2 (raw file):
DEPRECATED_CONTRACT_SIERRA_SIZE, abi_length, SierraVersion::zero(),
same Q
Code quote:
SierraVersion::zero(),
crates/papyrus_execution/src/lib.rs
line 854 at r2 (raw file):
DEPRECATED_CONTRACT_SIERRA_SIZE, abi_length, SierraVersion::zero(),
same Q
Code quote:
SierraVersion::zero(),
crates/starknet_gateway/src/compilation.rs
line 45 at r2 (raw file):
let sierra_version: SierraVersion = (&rpc_contract_class.sierra_program) .try_into()
use explicit conversion, see above
Code quote:
.try_into()
crates/blockifier_reexecution/src/state_reader/reexecution_state_reader.rs
line 30 at r2 (raw file):
sierra.sierra_program[2].try_into().unwrap(), ); let sierra_version: SierraVersion = SierraVersion::new(major, minor, path);
move this logic to the struct impl
Suggestion:
let sierra_version = SierraVersion::extract_from_program(sierra.sierra_program);
crates/blockifier_reexecution/src/state_reader/reexecution_state_reader.rs
line 41 at r2 (raw file):
StarknetContractClass::Legacy(legacy) => { let abi_length = legacy.abi.clone().expect("legendary contract should have abi").len();
lolololol
Code quote:
legendary
crates/blockifier_reexecution/src/state_reader/reexecution_state_reader.rs
line 46 at r2 (raw file):
DEPRECATED_CONTRACT_SIERRA_SIZE, abi_length, SierraVersion::zero(),
Legacy contract is cairo0?
Code quote:
SierraVersion::zero(),
crates/papyrus_execution/src/test_utils.rs
line 316 at r2 (raw file):
0, false, SierraVersion::default(),
what is the default?
I think it should be equal to latest()
(see above)
Code quote:
SierraVersion::default()
22fa4c5
to
afdb57c
Compare
Benchmark movements: |
afdb57c
to
4057743
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: 6 of 14 files reviewed, 15 unresolved discussions (waiting on @dorimedini-starkware, @TzahiTaub, and @Yoni-Starkware)
crates/blockifier/src/transaction/test_utils.rs
line 353 at r2 (raw file):
Previously, dorimedini-starkware wrote…
move these magic numbers to the struct impl.
that way we can later make it smarter (by checking the latest available compiler version)
How should I maintain this function?
crates/blockifier_reexecution/src/state_reader/reexecution_state_reader.rs
line 30 at r2 (raw file):
Previously, dorimedini-starkware wrote…
move this logic to the struct impl
Done, see below about the function name
crates/blockifier_reexecution/src/state_reader/reexecution_state_reader.rs
line 41 at r2 (raw file):
Previously, dorimedini-starkware wrote…
lolololol
:)
crates/blockifier_reexecution/src/state_reader/reexecution_state_reader.rs
line 46 at r2 (raw file):
Previously, dorimedini-starkware wrote…
Legacy contract is cairo0?
yes
crates/papyrus_execution/src/lib.rs
line 827 at r2 (raw file):
Previously, dorimedini-starkware wrote…
same Q
Yes
crates/papyrus_execution/src/lib.rs
line 854 at r2 (raw file):
Previously, dorimedini-starkware wrote…
same Q
Yes
V0 , V1 its cairo 0
crates/papyrus_execution/src/test_utils.rs
line 316 at r2 (raw file):
Previously, dorimedini-starkware wrote…
what is the default?
I think it should be equal tolatest()
(see above)
Done ( but see above )
crates/papyrus_rpc/src/v0_8/api/mod.rs
line 471 at r2 (raw file):
Previously, dorimedini-starkware wrote…
what's this? why not
map
?
It's similar to map, but If you use map
instead, the try_into
call would result in a type mismatch because map
does not expect the closure to return a Result.
crates/papyrus_rpc/src/v0_8/api/mod.rs
line 475 at r2 (raw file):
Previously, dorimedini-starkware wrote…
use
extract_from_program
explicitly
Why?
Would it be clearer if we used try from and specified the type?
crates/starknet_api/src/contract_class.rs
line 67 at r2 (raw file):
Previously, dorimedini-starkware wrote…
no, use
latest()
Done.
crates/starknet_api/src/contract_class.rs
line 82 at r2 (raw file):
Previously, dorimedini-starkware wrote…
From
andTryFrom
consume the input by convension; implement anextract_from_program
method instead
but I implemented it as a reference, so it does not consume the actual input. Just clone the first three fields.
crates/starknet_api/src/contract_class.rs
line 109 at r2 (raw file):
Previously, dorimedini-starkware wrote…
does this work? (non-blocking)
if it does, please delete map_felt_to_api_error and just inline the logic
refactored fit to something rustier
crates/starknet_gateway/src/compilation.rs
line 45 at r2 (raw file):
Previously, dorimedini-starkware wrote…
use explicit conversion, see above
see above
crates/starknet_api/src/contract_class.rs
line 94 at r2 (raw file):
// Closure to map a Felt error to a StarknetApiError. let map_felt_to_api_error = |(felt, index): (Felt, usize)| {
Done.
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 7 of 8 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @AvivYossef-starkware and @TzahiTaub)
crates/blockifier/src/transaction/test_utils.rs
line 353 at r2 (raw file):
Previously, AvivYossef-starkware wrote…
How should I maintain this function?
we need to discuss it, but it's out of scope ATM.
We can use existing infrastructure to read the current version of the compiler crates in the root cargo.toml, in write a test that latest()
returns this version.
crates/papyrus_rpc/src/v0_8/api/mod.rs
line 475 at r2 (raw file):
Previously, AvivYossef-starkware wrote…
Why?
Would it be clearer if we used try from and specified the type?
no, try_from is usually consuming. you are not converting (consuming), you are computing a value from another value
crates/starknet_api/src/contract_class.rs
line 82 at r2 (raw file):
Previously, AvivYossef-starkware wrote…
but I implemented it as a reference, so it does not consume the actual input. Just clone the first three fields.
yes it compiles it's just not convension. from/into are conversions (consuming), not computations
crates/starknet_gateway/src/compilation.rs
line 45 at r2 (raw file):
Previously, AvivYossef-starkware wrote…
see above
still relevant
crates/blockifier_reexecution/src/state_reader/reexecution_state_reader.rs
line 26 at r3 (raw file):
let sierra_length = sierra.sierra_program.len(); let sierra_version = SierraVersion::try_from(&sierra.sierra_program)?;
try_from is consuming by convension, iiuc.
don't implement TryFrom, implement an explicit method (discussed more in below comments)
Code quote:
SierraVersion::try_from(&sierra.sierra_program)?
crates/starknet_api/src/contract_class.rs
line 90 at r4 (raw file):
/// The first 3 felts are the major, minor and patch version. /// The rest of the felts are ignored. /// Generic since there are multiple types of felts.
what?? we should only have a single felt type in the repo
what breaks if you only accept the starknet_types_core felt?
Code quote:
Generic since there are multiple types of felts.
fd5e200
to
bb77d07
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: 6 of 14 files reviewed, 6 unresolved discussions (waiting on @dorimedini-starkware and @TzahiTaub)
crates/blockifier/src/transaction/test_utils.rs
line 353 at r2 (raw file):
Previously, dorimedini-starkware wrote…
we need to discuss it, but it's out of scope ATM.
We can use existing infrastructure to read the current version of the compiler crates in the root cargo.toml, in write a test thatlatest()
returns this version.
Done
crates/blockifier_reexecution/src/state_reader/reexecution_state_reader.rs
line 26 at r3 (raw file):
Previously, dorimedini-starkware wrote…
try_from is consuming by convension, iiuc.
don't implement TryFrom, implement an explicit method (discussed more in below comments)
Done.
crates/papyrus_rpc/src/v0_8/api/mod.rs
line 475 at r2 (raw file):
Previously, dorimedini-starkware wrote…
no, try_from is usually consuming. you are not converting (consuming), you are computing a value from another value
got it
crates/starknet_api/src/contract_class.rs
line 82 at r2 (raw file):
Previously, dorimedini-starkware wrote…
yes it compiles it's just not convension. from/into are conversions (consuming), not computations
Done.
crates/starknet_api/src/contract_class.rs
line 90 at r4 (raw file):
Previously, dorimedini-starkware wrote…
what?? we should only have a single felt type in the repo
what breaks if you only accept the starknet_types_core felt?
It's not from our repository. In the re-execution, we use a different crate to compile the Sierra. That crate has a different type of felt.
sequencer/crates/blockifier_reexecution/src/state_reader/reexecution_state_reader.rs
Line 22 in 4057743
StarknetContractClass::Sierra(sierra) => { |
crates/starknet_gateway/src/compilation.rs
line 45 at r2 (raw file):
Previously, dorimedini-starkware wrote…
still relevant
Done.
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 8 of 8 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alonh5, @AvivYossef-starkware, @DvirYo-starkware, and @TzahiTaub)
crates/starknet_api/src/contract_class.rs
line 65 at r5 (raw file):
// TODO(Aviv): Replace hardcoded version with logic to fetch the latest version from the actual // source.
I think it's ok to hard-code this
just need to write a test that the value is consistent with the toml
please rephrase the TODO
Code quote:
// TODO(Aviv): Replace hardcoded version with logic to fetch the latest version from the actual
// source.
bb77d07
to
b9b6aee
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: 13 of 14 files reviewed, 2 unresolved discussions (waiting on @alonh5, @dorimedini-starkware, @DvirYo-starkware, and @TzahiTaub)
crates/starknet_api/src/contract_class.rs
line 65 at r5 (raw file):
Previously, dorimedini-starkware wrote…
I think it's ok to hard-code this
just need to write a test that the value is consistent with the toml
please rephrase the TODO
Done.
Benchmark movements: |
41e66fb
to
8062001
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: 12 of 15 files reviewed, 2 unresolved discussions (waiting on @alonh5, @dorimedini-starkware, @DvirYo-starkware, and @TzahiTaub)
crates/starknet_gateway/src/compilation.rs
line 46 at r6 (raw file):
Previously, alonh5 (Alon Haramati) wrote…
I see this is happening after the compilation. Is it possible that this will fail if the compilation succeeds? Isn't the sierra version used in compilation?
If it isn't possible then you can return an unexpected error, but if it is possible then we should add a new error to the spec and here (I talked to Ariel about it)
The Sierra version refers to the first three felts in the Sierra program. It may fail (from the Rust compiler's perspective) when attempting to cast the felt into a u64. However, as long as we maintain the convention that the first three positions in the program indicate the version, it should never fail.
Benchmark movements: |
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 3 of 3 files at r9, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alonh5, @AvivYossef-starkware, @DvirYo-starkware, and @TzahiTaub)
crates/starknet_gateway_types/src/errors.rs
line 44 at r8 (raw file):
comment above says
Can you also create a new error for this in the gateway?
does this mean adding a new error in the spec? or a new variant to the GatewaySpecError enum?
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, 3 unresolved discussions (waiting on @alonh5, @AvivYossef-starkware, @DvirYo-starkware, and @TzahiTaub)
crates/starknet_api/src/contract_class.rs
line 54 at r9 (raw file):
pub struct SierraVersion(Version); impl SierraVersion {
Next PRs: replace this with your 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.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @AvivYossef-starkware, @dorimedini-starkware, @DvirYo-starkware, and @TzahiTaub)
crates/starknet_gateway/src/compilation.rs
line 46 at r6 (raw file):
Previously, AvivYossef-starkware wrote…
The Sierra version refers to the first three felts in the Sierra program. It may fail (from the Rust compiler's perspective) when attempting to cast the felt into a u64. However, as long as we maintain the convention that the first three positions in the program indicate the version, it should never fail.
What I'm asking is whether it's possible to pass compilation and then fail the Sierra version extraction.
crates/starknet_gateway_types/src/errors.rs
line 44 at r8 (raw file):
Previously, dorimedini-starkware wrote…
comment above says
Can you also create a new error for this in the gateway?
does this mean adding a new error in the spec? or a new variant to the GatewaySpecError enum?
I meant my reply to that comment. I moved the discussion there.
8062001
to
6b48bff
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: all files reviewed, 3 unresolved discussions (waiting on @alonh5, @dorimedini-starkware, @DvirYo-starkware, and @TzahiTaub)
crates/starknet_gateway/src/compilation.rs
line 46 at r6 (raw file):
Previously, alonh5 (Alon Haramati) wrote…
What I'm asking is whether it's possible to pass compilation and then fail the Sierra version extraction.
No, unless there is a bug in the compiler.
Benchmark movements: |
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, 3 unresolved discussions (waiting on @alonh5, @dorimedini-starkware, @DvirYo-starkware, and @TzahiTaub)
crates/starknet_gateway/src/compilation.rs
line 46 at r6 (raw file):
Previously, AvivYossef-starkware wrote…
No, unless there is a bug in the compiler.
Or, the compiler will stop using the first three places of the sierra program to encode the sierra 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 1 of 8 files at r3, 1 of 8 files at r5, 1 of 3 files at r9.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alonh5, @AvivYossef-starkware, and @TzahiTaub)
crates/papyrus_execution/src/lib.rs
line 838 at r9 (raw file):
DEPRECATED_CONTRACT_SIERRA_SIZE, abi_length, SierraVersion::zero(),
Not the main thing I review here, but consider using constants instead of functions.
SierraVersion::ZERO
(I prefer SierraVersion::DEPRECATED_CLASS_VERSION
)
We do the same for felts (see Felt::ZERO
)
If you decide to keep the function, you can make it constant
crates/papyrus_execution/src/test_utils.rs
line 316 at r9 (raw file):
0, false, SierraVersion::latest(),
Again consider SierraVersion::LATEST
crates/papyrus_rpc/src/v0_8/api/mod.rs
line 471 at r9 (raw file):
internal_server_error(format!("Missing deprecated class definition of {class_hash}.")) }) .and_then(|contract_class| {
Consider creating the final result outside of the and_than
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 @AvivYossef-starkware and @TzahiTaub)
crates/starknet_gateway/src/compilation.rs
line 46 at r6 (raw file):
Previously, AvivYossef-starkware wrote…
Or, the compiler will stop using the first three places of the sierra program to encode the sierra version
In that case, you can just return the existing unexpected error, and you can remove this new variant.
6b48bff
to
1bbe7f1
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: 13 of 15 files reviewed, 2 unresolved discussions (waiting on @alonh5, @dorimedini-starkware, and @TzahiTaub)
crates/starknet_gateway/src/compilation.rs
line 46 at r6 (raw file):
Previously, alonh5 (Alon Haramati) wrote…
In that case, you can just return the existing unexpected error, and you can remove this new variant.
Done.
1bbe7f1
to
aa5b760
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.
Reviewed 2 of 2 files at r10, 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alonh5, @AvivYossef-starkware, and @TzahiTaub)
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 all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alonh5 and @TzahiTaub)
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 1 of 2 files at r10.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)
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: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)
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: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)
No description provided.