-
Notifications
You must be signed in to change notification settings - Fork 339
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
Minor improvements to Account package #1224
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1224 +/- ##
==========================================
- Coverage 92.06% 92.02% -0.04%
==========================================
Files 49 49
Lines 1399 1392 -7
==========================================
- Hits 1288 1281 -7
Misses 111 111
Continue to review 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.
LGTM!
} else { | ||
assert(MIN_TRANSACTION_VERSION <= tx_version, Errors::INVALID_TX_VERSION); | ||
} | ||
assert(is_tx_version_valid(), Errors::INVALID_TX_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.
👍🏼
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.
I left some non-blocking comments regarding the changelog. Otherwise, LGTM!
// Check tx version | ||
let tx_info = get_tx_info().unbox(); | ||
let tx_version: u256 = tx_info.version.into(); | ||
// Check if tx is a query | ||
if (tx_version >= QUERY_OFFSET) { | ||
assert( | ||
QUERY_OFFSET + MIN_TRANSACTION_VERSION <= tx_version, Errors::INVALID_TX_VERSION | ||
); | ||
} else { | ||
assert(MIN_TRANSACTION_VERSION <= tx_version, Errors::INVALID_TX_VERSION); | ||
} | ||
assert(is_tx_version_valid(), Errors::INVALID_TX_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.
Very nice!
pub fn execute_calls(mut calls: Span<Call>) -> Array<Span<felt252>> { | ||
pub fn execute_calls(calls: Span<Call>) -> Array<Span<felt252>> { |
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 we list this under Changed
(not breaking) in the changelog?
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.
If so, same with __execute__
/// If the transaction is a simulation (version >= `QUERY_OFFSET`), it must be | ||
/// greater than or equal to `QUERY_OFFSET` + `MIN_TRANSACTION_VERSION` to be considered valid. | ||
/// Otherwise, it must be greater than or equal to `MIN_TRANSACTION_VERSION`. | ||
pub fn is_tx_version_valid() -> bool { | ||
let tx_info = starknet::get_tx_info().unbox(); | ||
let tx_version = tx_info.version.into(); | ||
if tx_version >= QUERY_OFFSET { | ||
QUERY_OFFSET + MIN_TRANSACTION_VERSION <= tx_version | ||
} else { |
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.
I'd add this to the changelog as well since it's pub
PR Checklist