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
SNIP 12 Utilities #935
SNIP 12 Utilities #935
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.
Excellent start, Eric! I know we're still waiting on things to become finalized, but I left some initial feedback
Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>
Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>
Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>
Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>
Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>
… into feat/snip-12
|
||
#[starknet::interface] | ||
trait INonces<TState> { | ||
fn nonces(self: @TState, owner: ContractAddress) -> 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.
i think felt252 might be just fine. but I wonder if you had any thoughts on using u256
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.
felt252 should be cheaper than u256 even in the future if we have a smaller felt. As Ariel confirmed, I don't think fel252 will be removed, so for this case, it suits us better IMO.
// selector!( | ||
// "\"StarknetDomain\"( | ||
// \"name\":\"shortstring\", | ||
// \"version\":\"shortstring\", | ||
// \"chainId\":\"shortstring\", | ||
// \"revision\":\"shortstring\" | ||
// )" | ||
// ); |
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 wonder if we should just use the macro directly
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.
More expensive step(gas)-wise for a constant thing.
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 a few tiny suggestions; otherwise, I think this is good to go (and fix changelog conflicts)! Nice work :)
Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com>
Fixes #409
PR Checklist