-
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
test(blockifier): function to build calldata for recursive call_contract calls #1449
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1449 +/- ##
===========================================
+ Coverage 40.10% 67.33% +27.22%
===========================================
Files 26 103 +77
Lines 1895 13723 +11828
Branches 1895 13723 +11828
===========================================
+ Hits 760 9240 +8480
- Misses 1100 4082 +2982
- Partials 35 401 +366 ☔ 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.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @TzahiTaub and @yoavGrs)
crates/blockifier/src/test_utils.rs
line 103 at r1 (raw file):
Self::CairoVersion(version) } }
I would delete this - it is not intuitive. conversion from CairoVersion to the new enum should be explicit.
The other way around can be implemented if you want
Code quote:
impl From<CairoVersion> for CompilerBasedVersion {
fn from(version: CairoVersion) -> Self {
Self::CairoVersion(version)
}
}
crates/blockifier/src/test_utils.rs
line 112 at r1 (raw file):
} } // Storage keys.
newline
Suggestion:
}
// Storage keys.
crates/blockifier/src/execution/syscalls/syscall_tests/call_contract.rs
line 113 at r1 (raw file):
) { let outer_contract = FeatureContract::TestContract(outer_version); let inner_contract = FeatureContract::TestContract(inner_version);
consider making [inner|outer]_version
params of type CompilerBasedVersion
.
will be easy to parametrize a third variant later
Suggestion:
#[values(
CompilerBasedVersion::CairoVersion(CairoVersion::Cairo0),
CompilerBasedVersion::CairoVersion(CairoVersion::Cairo1)
]
outer_version: CairoVersion,
#[values(
CompilerBasedVersion::CairoVersion(CairoVersion::Cairo0),
CompilerBasedVersion::CairoVersion(CairoVersion::Cairo1)
]
inner_version: CairoVersion,
) {
let outer_contract = outer_version.get_test_contract();
let inner_contract = inner_version.get_test_contract();
7160c7a
to
d903234
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: 1 of 3 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware and @yoavGrs)
crates/blockifier/src/test_utils.rs
line 103 at r1 (raw file):
Previously, dorimedini-starkware wrote…
I would delete this - it is not intuitive. conversion from CairoVersion to the new enum should be explicit.
The other way around can be implemented if you want
It's sort of backward compatability + convinience thing. The OldCairo1 should be explicit in tests, and anything that was Ciaro1 until now should run as a new Cairo1 contract (i.e., sierra gas mode). The other way around is not the thing I'm interested in.
I just hate writing CompilerBasedVersion::CairoVersion(CairoVersion::Cairo1)
crates/blockifier/src/test_utils.rs
line 112 at r1 (raw file):
Previously, dorimedini-starkware wrote…
newline
Done.
crates/blockifier/src/test_utils.rs
line 110 at r2 (raw file):
/// Returns the tracked resource for a contract execution with the current version, assuming no /// calls were made to other contracts prior to this execution.
Help with rewriting this will be appreciated.
Code quote:
/// Returns the tracked resource for a contract execution with the current version, assuming no
/// calls were made to other contracts prior to this execution.
crates/blockifier/src/execution/syscalls/syscall_tests/call_contract.rs
line 113 at r1 (raw file):
Previously, dorimedini-starkware wrote…
consider making
[inner|outer]_version
params of typeCompilerBasedVersion
.
will be easy to parametrize a third variant later
Changed the test a little and added the other cases. How does it look?
c4a60ac
to
77410e1
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 r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @TzahiTaub and @yoavGrs)
crates/blockifier/src/test_utils.rs
line 110 at r2 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
Help with rewriting this will be appreciated.
hmmm... maybe
/// Returns the context-free tracked resource of this contract (does not take caller contract into account).
?
77410e1
to
98ec680
Compare
Artifacts upload triggered. View details here |
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: 1 of 3 files reviewed, all discussions resolved (waiting on @dorimedini-starkware and @yoavGrs)
crates/blockifier/src/test_utils.rs
line 110 at r2 (raw file):
Previously, dorimedini-starkware wrote…
hmmm... maybe
/// Returns the context-free tracked resource of this contract (does not take caller contract into account).
?
Bravo!
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 r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yoavGrs)
No description provided.