-
Notifications
You must be signed in to change notification settings - Fork 25
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
fix(cairo_native): remove native FC folder #2464
Conversation
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 4 of 7 files at r1.
Reviewable status: 4 of 7 files reviewed, all discussions resolved (waiting on @dorimedini-starkware and @Yoni-Starkware)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2464 +/- ##
===========================================
+ Coverage 40.10% 71.60% +31.49%
===========================================
Files 26 98 +72
Lines 1895 13611 +11716
Branches 1895 13611 +11716
===========================================
+ Hits 760 9746 +8986
- Misses 1100 3435 +2335
- Partials 35 430 +395 ☔ View full report in Codecov by Sentry. |
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: 4 of 7 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)
crates/blockifier/src/test_utils/contracts.rs
line 118 at r1 (raw file):
| Self::TestContract(version) | Self::ERC20(version) | Self::LegacyTestContract(version) => *version,
This has a problematic Meaning now. Up until now, if you had a version, you had all versions but the legacy contract only has Cairo1, and Native does not have Cairo0
Code quote:
Self::LegacyTestContract(version) =
crates/blockifier/src/test_utils/contracts.rs
line 160 at r1 (raw file):
| Self::TestContract(v) | Self::ERC20(v) | Self::LegacyTestContract(v) => *v = version,
This is risky. We need to find a way to disallow the creation of Cairo0 LeagayContract. Which might be problematic for the all contacts method see below.
Code quote:
| Self::LegacyTestContract(v) => *v = version,
crates/blockifier/src/test_utils/contracts.rs
line 446 at r1 (raw file):
let mut other_contract = contract; other_contract.set_cairo_version(contract.cairo_version().other()); vec![other_contract].into_iter()
I can go the direct way here, check for the legacy contract, and remove the has only casm and native, but I prefer the generalization here. This function assumes that there is no special treatment for Native CairoVersion.
Also, the fact that the default is Cairo0 makes us create a LegacyTestContract(Cairo0), which means nothing.
Code quote:
} else if contract.has_only_casm_and_native() {
let mut other_contract = contract;
other_contract.set_cairo_version(contract.cairo_version().other());
vec![other_contract].into_iter()
3e035be
to
faa8438
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: 4 of 7 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)
crates/blockifier/tests/feature_contracts_compatibility_test.rs
line 245 at r2 (raw file):
} #[cfg(feature = "cairo_native")] CairoVersion::Native => panic!("This test does not support native CairoVersion."),
This won't work if we decide in the future to add native compiled files, but as we do not have a future plan to do so I think panic is a good solution.
Code quote:
CairoVersion::Native => panic!("This test does not support native CairoVersion."),
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 5 of 7 files at r1, all commit messages.
Reviewable status: 5 of 7 files reviewed, 5 unresolved discussions (waiting on @dorimedini-starkware and @meship-starkware)
crates/blockifier/src/test_utils/contracts.rs
line 450 at r2 (raw file):
} }) }
Let's make it simple. Remove all functions that are used only here, and implement all_contracts
using a single match
expression explicitly.
Remove the .other()
function as well
Code quote:
pub fn all_contracts() -> impl Iterator<Item = Self> {
// EnumIter iterates over all variants with Default::default() as the cairo
// version.
Self::iter().flat_map(|contract| {
if contract.has_all_versions() {
let mut other_contract = contract;
other_contract.set_cairo_version(contract.cairo_version().other());
vec![contract, other_contract].into_iter()
} else if contract.has_only_casm_and_native() {
let mut other_contract = contract;
other_contract.set_cairo_version(contract.cairo_version().other());
vec![other_contract].into_iter()
} else {
vec![contract].into_iter()
}
})
}
faa8438
to
99ddcb0
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 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)
crates/blockifier/src/test_utils/contracts.rs
line 118 at r1 (raw file):
Previously, meship-starkware (Meshi Peled) wrote…
This has a problematic Meaning now. Up until now, if you had a version, you had all versions but the legacy contract only has Cairo1, and Native does not have Cairo0
why does the legacy contract now have two cairo1 variants? why not only cairo1 non-native?
IIRC the legacy contract must be compiled with a legacy compiler --> too old for a native compilation; no?
IIUC we can't run native on old cairo1 contracts...
if I'm right, you can simply revert this added version
data in the legacy variant and all is well again.
IF we will have a contract that doesn't have cairo0 but has two cairo1 versions, then you're right... we'll need the following refactor:
enum RunnableCairo1Version {
Casm,
Native,
}
enum RunnableVersion {
Cairo0,
Cairo1(RunnableCairo1Version),
}
enum FeatureContract {
SecurityTests,
TestContract(RunnableVersion),
OnlyCairo1Contract(RunnableCairo1Version),
... etc
}
crates/blockifier/src/test_utils/contracts.rs
line 160 at r1 (raw file):
Previously, meship-starkware (Meshi Peled) wrote…
This is risky. We need to find a way to disallow the creation of Cairo0 LeagayContract. Which might be problematic for the all contacts method see below.
see above: I think we should disallow creation of both cairo0 and native, and then there's no problem
crates/blockifier/src/test_utils/contracts.rs
line 446 at r1 (raw file):
Previously, meship-starkware (Meshi Peled) wrote…
I can go the direct way here, check for the legacy contract, and remove the has only casm and native, but I prefer the generalization here. This function assumes that there is no special treatment for Native CairoVersion.
Also, the fact that the default is Cairo0 makes us create a LegacyTestContract(Cairo0), which means nothing.
see above (no native legacy): i think this can be simplified
contract.supported_versions().into_iter().map(|version| {
let mut versioned_contract = contract.clone();
versioned_contract.set_version(version);
versioned_contract
})
crates/blockifier/src/test_utils/contracts.rs
line 450 at r2 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
Let's make it simple. Remove all functions that are used only here, and implement
all_contracts
using a singlematch
expression explicitly.
Remove the.other()
function as well
I suggest implementing a supported_versions()
method and iterating over it.
crates/blockifier/tests/feature_contracts_compatibility_test.rs
line 155 at r3 (raw file):
// TODO (Meshi 01/01/2025) Remove this once native folder is deleted. #[cfg(feature = "cairo_native")] CompilationArtifacts::Cairo1Native { .. } => {
do you even need this variant now that you aren't compiling sierra->native?
Code quote:
CompilationArtifacts::Cairo1Native
crates/blockifier/tests/feature_contracts_compatibility_test.rs
line 293 at r3 (raw file):
#[cfg(feature = "cairo_native")] CairoVersion::Native => panic!("This test does not support native CairoVersion."), }
cleaner, IMO (no let mut x: T;
lines)
Suggestion:
let compiled_paths_on_filesystem = match cairo_version {
CairoVersion::Cairo0 => {
verify_and_get_files(cairo_version)
.into_iter()
.map(|metadata| metadata.compiled_path().to_string())
.collect()
}
CairoVersion::Cairo1 => {
let (fetched_compiled_paths_on_filesystem, sierra_paths_on_filesystem) =
verify_and_get_files(cairo_version)
.into_iter()
.map(|metadata| (metadata.compiled_path(), metadata.sierra_path()))
.collect();
let mut sierra_paths_from_enum: Vec<String> = FeatureContract::all_feature_contracts()
.filter(|contract| contract.cairo_version() == cairo_version)
.map(|contract| contract.get_sierra_path())
.collect();
sierra_paths_from_enum.sort();
sierra_paths_on_filesystem.sort();
assert_eq!(sierra_paths_from_enum, sierra_paths_on_filesystem);
fetched_compiled_paths_on_filesystem
}
#[cfg(feature = "cairo_native")]
CairoVersion::Native => panic!("This test does not support native CairoVersion."),
}
crates/blockifier/src/test_utils/contracts.rs
line 302 at r3 (raw file):
#[cfg(feature = "cairo_native")] CairoVersion::Native => "sierra", },
Suggestion:
"feature_contracts/cairo{}/{}{}.json",
match cairo_version {
CairoVersion::Cairo0 => "0/compiled",
CairoVersion::Cairo1 => "1/compiled",
#[cfg(feature = "cairo_native")]
CairoVersion::Native => "1/sierra",
},
99ddcb0
to
79cbf1d
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 1 of 3 files at r3.
Reviewable status: 6 of 8 files reviewed, 7 unresolved discussions (waiting on @dorimedini-starkware and @meship-starkware)
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 8 files reviewed, 7 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)
crates/blockifier/src/test_utils/contracts.rs
line 118 at r1 (raw file):
Previously, dorimedini-starkware wrote…
why does the legacy contract now have two cairo1 variants? why not only cairo1 non-native?
IIRC the legacy contract must be compiled with a legacy compiler --> too old for a native compilation; no?
IIUC we can't run native on old cairo1 contracts...
if I'm right, you can simply revert this addedversion
data in the legacy variant and all is well again.
IF we will have a contract that doesn't have cairo0 but has two cairo1 versions, then you're right... we'll need the following refactor:enum RunnableCairo1Version { Casm, Native, } enum RunnableVersion { Cairo0, Cairo1(RunnableCairo1Version), } enum FeatureContract { SecurityTests, TestContract(RunnableVersion), OnlyCairo1Contract(RunnableCairo1Version), ... etc }
NM created a new contract that is the same as the legacy contract and called it test_entry_point_v1. We decided it was unnecessary, and now every Cairo1 contract also has a Native version. You are right about the compiler, though. So we can either have two legacy Sierras, or we can get back the native entry point V1 contract. I think adding the NM contract is the better answer. WDYT @Yoni-Starkware?
crates/blockifier/tests/feature_contracts_compatibility_test.rs
line 155 at r3 (raw file):
Previously, dorimedini-starkware wrote…
do you even need this variant now that you aren't compiling sierra->native?
The short answer is no, but this would mean that natives would not be treated differently in compile. This is fine for now, but if we would like to use the contracts compile method outside of the feature contracts test, it might be problematic.
crates/blockifier/tests/feature_contracts_compatibility_test.rs
line 293 at r3 (raw file):
Previously, dorimedini-starkware wrote…
cleaner, IMO (no
let mut x: T;
lines)
Done.
crates/blockifier/src/test_utils/contracts.rs
line 302 at r3 (raw file):
#[cfg(feature = "cairo_native")] CairoVersion::Native => "sierra", },
Done.
62d9f14
to
a57e810
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 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)
crates/blockifier/src/test_utils/contracts.rs
line 118 at r1 (raw file):
I think adding the NM contract is the better answer
you mean bringing back the test_entry_point_v1 contract? sounds simplest to me too
crates/blockifier/tests/feature_contracts_compatibility_test.rs
line 155 at r3 (raw file):
Previously, meship-starkware (Meshi Peled) wrote…
The short answer is no, but this would mean that natives would not be treated differently in compile. This is fine for now, but if we would like to use the contracts compile method outside of the feature contracts test, it might be problematic.
As long as native compilation is done during runtime - I don't see a reason to implement any useless code in the feature contracts testing framework.
if we want to start precompiling, we'll refactor... but that can wait
crates/blockifier/src/test_utils/contracts.rs
line 22 at r3 (raw file):
#[cfg(feature = "cairo_native")] use crate::execution::native::contract_class::NativeCompiledClassV1; #[cfg(feature = "cairo_native")]
do you still need this gate...? you deleted the relevant import
Code quote:
#[cfg(feature = "cairo_native")]
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, 5 unresolved discussions (waiting on @Yoni-Starkware)
crates/blockifier/src/test_utils/contracts.rs
line 118 at r1 (raw file):
Previously, dorimedini-starkware wrote…
I think adding the NM contract is the better answer
you mean bringing back the test_entry_point_v1 contract? sounds simplest to me too
In this case, either the test_entry_point_v1 contract would have both Ciro1 and Native versions, and then it would be just like the legacy contract, or it would only have native, and then the feature contract test would also have to check native. I prefer the first case. I think it easier to assume that for all Native contracts, we have a casm one, especially when Native also needs the casm file
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, 5 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)
crates/blockifier/src/test_utils/contracts.rs
line 118 at r1 (raw file):
or it would only have native, and then the feature contract test would also have to check native
wdym "the feature contract test would also have to check native"?
you mean the recompile test?
can't the recompile test treat cairo1 casm and native the same way? it is not responsible for generating native artifacts; it just needs to create sierra + casm for all cairo1 sources...
I was thinking the recompilation logic would just treat both the legacy contract and the NM contract as "cairo1" for purpose of recompilation
a57e810
to
a3dbc9f
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 38 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)
crates/blockifier/src/test_utils/contracts.rs
line 22 at r3 (raw file):
Previously, dorimedini-starkware wrote…
do you still need this gate...? you deleted the relevant import
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 1 of 3 files at r5.
Reviewable status: 0 of 38 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware)
7b28f4a
to
1cd3784
Compare
a3dbc9f
to
69a8e31
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 38 of 38 files at r7, 16 of 16 files at r8, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)
crates/blockifier/src/test_utils/contracts.rs
line 450 at r2 (raw file):
Previously, dorimedini-starkware wrote…
I suggest implementing a
supported_versions()
method and iterating over it.
concrete suggestion in another comment
crates/blockifier/src/test_utils/contracts.rs
line 138 at r8 (raw file):
Self::SierraExecutionInfoV1Contract(rv) => { assert!(matches!(version, CairoVersion::Cairo1(_))); *rv = version.get_runnable_version()
... and then you won't need the get_runnable_version
method on the CairoVersion enum
Suggestion:
match version {
CairoVersion::Cairo0 => panic!("{self:?} has not Cairo0 variant."),
CairoVersion::Cairo1(runnable) => *rv = runnable,
}
crates/blockifier/src/test_utils/contracts.rs
line 424 at r7 (raw file):
} pub fn all_contracts() -> impl Iterator<Item = Self> {
EDIT: I didn't see the support_cairo_version
before this comment, but my suggestion
- is still simpler IMO
- supports native variants
- doesn't rely on the developer to remember to update the function... as it is currently written, if i add another contract (with cairo0, for example),
support_cairo_version
won't return it in the vector, and the compiler won't complain. only the feature contracts test will fail, right?
how about:
pub fn all_contract_versions(&self) -> impl Iterator<Item = Self>
which returns 1, 2 or 3 contracts, depending on which versions are supported?
this will be a much simpler match
:
match self {
TestContract(_) | ..all contracts with all versions supported.. => {
// create 2 or 3 variants, depending on native feature active or not
}
Legacy | ... all contracts with only one version.. => vec![self].into(),
SpecialNativeContract => RunnableCairo1::iter().map(|v| ..create cairo1 variant..)
}
that way, in the all_contracts
iterator can simply do
Self::iter().flat_map(|contract| contract.all_contract_versions())
Code quote:
pub fn all_contracts() -> impl Iterator<Item = Self>
crates/blockifier/src/test_utils.rs
line 100 at r8 (raw file):
} pub fn get_runnable_version(&self) -> RunnableCairoVersion {
ah...
let's differentiate between the "cairo1 runtime" and the "general cairo runtime".
why do you need to convert a CairoVersion to a RunnableCairoVersion at all?
only the Cairo1 variant should have this conversion...
I would prefer a "runs_casm" boolean method, instead of abusing the runnable enum (which should be for cairo1 only)
Code quote:
pub fn get_runnable_version(&self) -> RunnableCairoVersion
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 38 files at r7, 2 of 16 files at r8.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)
crates/blockifier/feature_contracts/cairo1/test_contract_execution_info_v1.cairo
line 1 at r8 (raw file):
#[starknet::contract]
This contract is unnecessary if we use the legacy contract and ignore the Sierra version constraint on the native compilation for the V1 execution info test. Still, I think I'd prefer to keep it for now and add a PR that will remove this contract later so we can Start adding more tests for native.
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 16 files at r8, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware)
1cd3784
to
50f6e95
Compare
50f6e95
to
f8fbee1
Compare
69a8e31
to
60b4d1d
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: 25 of 40 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)
crates/blockifier/src/test_utils.rs
line 100 at r8 (raw file):
Previously, dorimedini-starkware wrote…
ah...
let's differentiate between the "cairo1 runtime" and the "general cairo runtime".
why do you need to convert a CairoVersion to a RunnableCairoVersion at all?
only the Cairo1 variant should have this conversion...
I would prefer a "runs_casm" boolean method, instead of abusing the runnable enum (which should be for cairo1 only)
Done.
crates/blockifier/src/test_utils/contracts.rs
line 450 at r2 (raw file):
Previously, dorimedini-starkware wrote…
concrete suggestion in another comment
Done.
crates/blockifier/src/test_utils/contracts.rs
line 424 at r7 (raw file):
Previously, dorimedini-starkware wrote…
EDIT: I didn't see the
support_cairo_version
before this comment, but my suggestion
- is still simpler IMO
- supports native variants
- doesn't rely on the developer to remember to update the function... as it is currently written, if i add another contract (with cairo0, for example),
support_cairo_version
won't return it in the vector, and the compiler won't complain. only the feature contracts test will fail, right?how about:
pub fn all_contract_versions(&self) -> impl Iterator<Item = Self>
which returns 1, 2 or 3 contracts, depending on which versions are supported?
this will be a much simplermatch
:match self { TestContract(_) | ..all contracts with all versions supported.. => { // create 2 or 3 variants, depending on native feature active or not } Legacy | ... all contracts with only one version.. => vec![self].into(), SpecialNativeContract => RunnableCairo1::iter().map(|v| ..create cairo1 variant..) }
that way, in the
all_contracts
iterator can simply doSelf::iter().flat_map(|contract| contract.all_contract_versions())
I don't understand why we need the native contract as well, but I agree that a compilation error when adding a contract is better than failing a test. LMK, if you feel it's too complicated @Yoni-Starkware @dorimedini-starkware
Done.
crates/blockifier/src/test_utils/contracts.rs
line 138 at r8 (raw file):
Previously, dorimedini-starkware wrote…
... and then you won't need the
get_runnable_version
method on the CairoVersion enum
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 15 of 15 files at r9, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)
crates/blockifier/src/test_utils/contracts.rs
line 424 at r7 (raw file):
Previously, meship-starkware (Meshi Peled) wrote…
I don't understand why we need the native contract as well, but I agree that a compilation error when adding a contract is better than failing a test. LMK, if you feel it's too complicated @Yoni-Starkware @dorimedini-starkware
Done.
I think now is better :)
crates/blockifier/src/test_utils/contracts.rs
line 438 at r9 (raw file):
ret_val.push(native_contract); } ret_val
how about this? you won't need the assert, and this function can now be called even when self
isn't Cairo0
Suggestion:
#[cfg(not(feature = "cairo_native"))]
let versions = vec![
CairoVersion::Cairo0,
CairoVersion::Cairo1(RunnableCairo1::Casm)
];
#[cfg(feature = "cairo_native")]
let versions = vec![
CairoVersion::Cairo0,
CairoVersion::Cairo1(RunnableCairo1::Casm),
CairoVersion::Cairo1(RunnableCairo1::Native),
];
versions.iter().map(|v| {
let versioned_contract = *self;
versioned_contract.set_cairo_version(v);
versioned_contract
})
crates/blockifier/src/test_utils/contracts.rs
line 452 at r9 (raw file):
ret_val.push(native_contract); } ret_val
see above: conditionally define a versions
vector by feature, and then map.
that way you won't need the assert.
consider, also, doing this (only compute the versions
vector in the match
, and share the map
logic):
let versions = match self {
...
};
versions.iter().map(|v| {
let versioned_contract = *self;
versioned_contract.set_cairo_version(v);
versioned_contract
})
Code quote:
// This would only work if the default runnable cairo1 is Casm.
assert!(self.cairo_version() == CairoVersion::Cairo1(RunnableCairo1::Casm));
let mut ret_val = vec![*self];
#[cfg(feature = "cairo_native")]
{
let mut native_contract = *self;
native_contract
.set_cairo_version(CairoVersion::Cairo1(RunnableCairo1::Native));
ret_val.push(native_contract);
}
ret_val
f8fbee1
to
cd34971
Compare
60b4d1d
to
01e3d80
Compare
cd34971
to
53605f5
Compare
01e3d80
to
9c9c78c
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: 39 of 40 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)
crates/blockifier/src/test_utils/contracts.rs
line 438 at r9 (raw file):
Previously, dorimedini-starkware wrote…
how about this? you won't need the assert, and this function can now be called even when
self
isn't Cairo0
Done.
crates/blockifier/src/test_utils/contracts.rs
line 452 at r9 (raw file):
Previously, dorimedini-starkware wrote…
see above: conditionally define a
versions
vector by feature, and then map.
that way you won't need the assert.
consider, also, doing this (only compute theversions
vector in thematch
, and share themap
logic):let versions = match self { ... }; versions.iter().map(|v| { let versioned_contract = *self; versioned_contract.set_cairo_version(v); versioned_contract })
I think that the match should return the final vector. I made a helper function that shares the map logic. I can have the match return the version list, but then I would have to separate the only cairo1 from the only cairo0. If you think that is better, then I'll do it.
9c9c78c
to
24398b4
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 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yoni-Starkware)
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 4 of 38 files at r7, 1 of 16 files at r8, 5 of 15 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @meship-starkware)
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 @meship-starkware)
No description provided.