Add set_ap_to_actual_fee hint #2
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Merged
odesenfans
suggested changes
Feb 5, 2024
| let actual_fee = execution_helper | ||
| .execution_helper | ||
| .borrow() | ||
| .tx_execution_info |
There was a problem hiding this comment.
Could you add a fn tx_execution_info(&self) -> Result<&TransactionExecutionInfo, ExecutionHelperError> method to ExecutionHelper?
There was a problem hiding this comment.
Hmmm, that doesn't really make sense to me because it's an Option<TransactionExecutionInfo> which expresses that the TEI may or may not be present (depending on context, specifically whether we are between start_tx() and end_tx(), I believe).
Also, there currently isn't any impl ExecutionHelper, all the "helpers" exist in impl ExecutionHelperWrapper. Something like this might fit in there, but there are a lot of fields you could potentially do this for, which would be a lot of code bloat. I still don't think a Result makes sense, either.
There was a problem hiding this comment.
This is the model used for the DeprecatedOsSyscallHandlerWrapper. I agree that it's a lot of bloat but all that RC + RefCell unwrapping should remain an implementation detail IMO.
src/hints/tests.rs
Outdated
| use crate::hints::*; | ||
|
|
||
| #[fixture] | ||
| fn block_context() -> BlockContext { |
There was a problem hiding this comment.
ExecutionHelperWrapper::new() takes a block_context. Otherwise it's irrelevant
There was a problem hiding this comment.
Makes sense. Could you put this fixture in some common test code to avoid duplication then?
odesenfans
force-pushed
the
msl/hints
branch
2 times, most recently
from
96c52e4 to
7652c6b
Compare
* first iteration of is_n_ge_two hint * refactor is_n_ge_two hint. remove unused dependencies. start test * set vm memory in test * remove unused macros * fix test and remove debugging prints * add EOF line * add test cases * use rstest cases. refactor hint code to avoid using match. rename hint tests module. * apply formatter * add case for n > two * remove End of file line * run fmt+nightly command * run fmt+nightly command
odesenfans
force-pushed
the
notlesh/set_ap_to_actual_fee_hint
branch
from
a2a29c7 to
33bc4f7
Compare
notlesh
added a commit
that referenced
this pull request
Mar 22, 2024
* wip * wip, some progress * WIP: Declare unimplemented hints * Fix unused warnings * Hint: SET_SYSCALL_PTR (#4) * Add is_n_ge_two hint (#3) * first iteration of is_n_ge_two hint * refactor is_n_ge_two hint. remove unused dependencies. start test * set vm memory in test * remove unused macros * fix test and remove debugging prints * add EOF line * add test cases * use rstest cases. refactor hint code to avoid using match. rename hint tests module. * apply formatter * add case for n > two * remove End of file line * run fmt+nightly command * run fmt+nightly command * add hint in hint map and use ids map (#6) * add hint in hint map and use ids map * remove unused macro. makes hint constant public * Add is_on_curve hint and basic test (#1) * Add is_on_curve hint and basic test * Improve test * Add TODO * Update Cargo.lock * Attempt at better ids_data * Set fp and add another segment * Clean up * fmt * Fix clippy lints * fmt * fmt * Add IS_ON_CURVE to hints map * Use constants for ONE and ZERO * fmt * Clean up secp_p comments * Use vars for var names * Remove duplicate mod tests * Add set_ap_to_actual_fee hint (#2) Co-authored-by: Olivier Desenfans <[email protected]> Co-authored-by: Herman Obst Demaestri <[email protected]> * Add txn/call hints (#5) Co-authored-by: Olivier Desenfans <[email protected]> * Add preimage hints (#8) Co-authored-by: Olivier Desenfans <[email protected]> * Feature: read/write Cairo structs safely (#9) Problem: Cairo structs are accessed in some hints. The Python VM has abstractions to access fields by name that the Rust VM cannot implement. This leads to accessing these structs by hardcoding the offset of each field. Solution: The new `CairoType` trait allows to read/write an entire struct in one go. The derive implementation reads/writes fields one by one according to their place in the struct definition. For cases where reading/writing the whole struct is not appropriate, the new `FieldOffsetGetters` macro implements a `get_<field>_offset()` method for each field of the struct to avoid hardcoding values. * fix tests * one more hint * fix contracts in StarknetOsInput * few more hints * ignore block test * Add a couple missing hints * calculate tx_hash * fix TRANSACTIONS_LEN * Maybe-fix gen_signature_arg hint * Don't put signature in ap * remove unnecesary unwraps * fix post merge problems * remove outdated tests * is_reverted hint * wip * Pull in relevant hashing code from starknet_on_rust * Convert equivalent contract structs * wip * test with cairo1 * cairo1 test setup, breaks at hint trying to load v1 class * wip * wip * cairo1 class loading + execution without cairo1 hint processor * fix tests * fix clippy * first round of comments * fix typo * remove unnecessary code --------- Co-authored-by: Olivier Desenfans <[email protected]> Co-authored-by: Herman Obst Demaestri <[email protected]> Co-authored-by: Stephen Shelton <[email protected]>
odesenfans
added a commit
that referenced
this pull request
Mar 23, 2024
* wip * wip, some progress * WIP: Declare unimplemented hints * Fix unused warnings * Hint: SET_SYSCALL_PTR (#4) * Add is_n_ge_two hint (#3) * first iteration of is_n_ge_two hint * refactor is_n_ge_two hint. remove unused dependencies. start test * set vm memory in test * remove unused macros * fix test and remove debugging prints * add EOF line * add test cases * use rstest cases. refactor hint code to avoid using match. rename hint tests module. * apply formatter * add case for n > two * remove End of file line * run fmt+nightly command * run fmt+nightly command * add hint in hint map and use ids map (#6) * add hint in hint map and use ids map * remove unused macro. makes hint constant public * Add is_on_curve hint and basic test (#1) * Add is_on_curve hint and basic test * Improve test * Add TODO * Update Cargo.lock * Attempt at better ids_data * Set fp and add another segment * Clean up * fmt * Fix clippy lints * fmt * fmt * Add IS_ON_CURVE to hints map * Use constants for ONE and ZERO * fmt * Clean up secp_p comments * Use vars for var names * Remove duplicate mod tests * Add set_ap_to_actual_fee hint (#2) Co-authored-by: Olivier Desenfans <[email protected]> Co-authored-by: Herman Obst Demaestri <[email protected]> * Add txn/call hints (#5) Co-authored-by: Olivier Desenfans <[email protected]> * Add preimage hints (#8) Co-authored-by: Olivier Desenfans <[email protected]> * Feature: read/write Cairo structs safely (#9) Problem: Cairo structs are accessed in some hints. The Python VM has abstractions to access fields by name that the Rust VM cannot implement. This leads to accessing these structs by hardcoding the offset of each field. Solution: The new `CairoType` trait allows to read/write an entire struct in one go. The derive implementation reads/writes fields one by one according to their place in the struct definition. For cases where reading/writing the whole struct is not appropriate, the new `FieldOffsetGetters` macro implements a `get_<field>_offset()` method for each field of the struct to avoid hardcoding values. * fix tests * one more hint * fix contracts in StarknetOsInput * few more hints * ignore block test * Add a couple missing hints * calculate tx_hash * fix TRANSACTIONS_LEN * Maybe-fix gen_signature_arg hint * Don't put signature in ap * remove unnecesary unwraps * fix post merge problems * remove outdated tests * is_reverted hint * wip * Pull in relevant hashing code from starknet_on_rust * Convert equivalent contract structs * wip * test with cairo1 * cairo1 test setup, breaks at hint trying to load v1 class * wip * wip * cairo1 class loading + execution without cairo1 hint processor * fix tests * fix clippy * first round of comments * wip * cairo1 syscall handler * fix imports * fix fmt, clippy * fix clippy * fix fmt --------- Co-authored-by: Olivier Desenfans <[email protected]> Co-authored-by: Herman Obst Demaestri <[email protected]> Co-authored-by: Stephen Shelton <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds
set_ap_to_actual_feehint