-
Notifications
You must be signed in to change notification settings - Fork 69
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
refactor: use Nullable<u256> in Stack #47
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.
Looks good! Just some minor comments 🚀
const STACK_OVERFLOW: felt252 = 'Kakarot: StackOverflow'; | ||
const STACK_UNDERFLOW: felt252 = 'Kakarot: StackUnderflow'; |
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.
Nice to create errors like these!
// This should be in the corelib | ||
trait NullableTraitExt<T> { | ||
fn new(value: T) -> Nullable<T>; | ||
} | ||
|
||
impl DestructStack of Destruct<Stack> { | ||
fn destruct(self: Stack) nopanic { | ||
self.items.squash(); | ||
impl NullableTraitExtImpl of NullableTraitExt<u256> { | ||
fn new(value: u256) -> Nullable<u256> { | ||
let nullable = nullable_from_box(BoxTrait::new(value)); | ||
nullable |
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.
Would it make sense to add a TODO
here to remove it once available in corelib?
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.
Yeah indeed!
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, do we have an idea of the overhead introduced by the Box
? in any case, I'd rather go cleaner first, then optimize
Very little, I compared the previous implementation with this one by running the tests with minimal gas values for both and it was roughly equal. |
Pull Request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: #45
What is the new behavior?
Nullable<u256>
instead of using helper functions storing(low,high)
limbsDoes this introduce a breaking change?
Other information
Stack tests available_gas values have been reduced to a value close to the minimum required - it will allow us to detect eventual efficiency decreases