-
Notifications
You must be signed in to change notification settings - Fork 2
feat: Storable #149
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
feat: Storable #149
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #149 +/- ##
=======================================
Coverage ? 91.50%
=======================================
Files ? 45
Lines ? 2449
Branches ? 0
=======================================
Hits ? 2241
Misses ? 208
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
remollemo
left a comment
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.
@remollemo reviewed 1 file and made 7 comments.
Reviewable status: 1 of 3 files reviewed, 7 unresolved discussions (waiting on @RoeeGross).
packages/utils/src/storage/tests/test_utils.cairo line 43 at r1 (raw file):
let packed = Storable160::encode(val); let unpacked: i128 = Storable160::decode(packed); assert(val == unpacked, 'i128 mismatch');
why not add also here +/-max values?
packages/utils/src/storage/tests/test_utils.cairo line 48 at r1 (raw file):
#[test] fn test_eth_address_storable() { let val: EthAddress = 123.try_into().unwrap();
please use a full value for this.
e.g.
0xDb6368868e71eE8D4637954ed51afaEe34c711D3 // just a random one.
packages/utils/src/storage/utils.cairo line 75 at r1 (raw file):
/// decode(encode(v)) == v /// - If the type implements `Default` or `Zero`, it is preferred that: /// encode(default or zero) == (0, 0)
why isn't it a must that it'd be so?
Code quote:
/// - If the type implements `Default` or `Zero`, it is preferred that:
/// encode(default or zero) == (0, 0)packages/utils/src/storage/utils.cairo line 76 at r1 (raw file):
/// - If the type implements `Default` or `Zero`, it is preferred that: /// encode(default or zero) == (0, 0) pub trait Storable160<V> {
I think Castablexxx is a better name pattern than Storablexxx
as we are not talking about storage, but about into-ability (Intoable would be ok to, but it's confusing as it's too much not a word).
i can live with Storable, nontheless
Code quote:
Storable1packages/utils/src/storage/utils.cairo line 77 at r1 (raw file):
/// encode(default or zero) == (0, 0) pub trait Storable160<V> { /// Convert a value into its 160-bit storage representation `(low, high)`.
why you add the word storage here? woudn't
the wording w/o that word be more accurate?
Suggestion:
/// Convert a value into its 160-bit representation `(low, high)`.packages/utils/src/storage/utils.cairo line 84 at r1 (raw file):
} pub impl PrimitiveStorable160<T, +Into<T, u128>, +TryInto<u128, T>, +Drop<T>> of Storable160<T> {
is this a sub generic for all the u128 castable?
a small comment would be helpful
Code quote:
pub impl PrimitiveStorable160<T, +Into<T, u128>, +TryInto<u128, T>, +Drop<T>> of Storable160<T> {packages/utils/src/storage/utils.cairo line 102 at r1 (raw file):
impl I8Offset of SignedIntegerOffset<i8> { fn offset() -> felt252 { 128 // 2^7
2 ^ 7 = 5
these comments are confusing, as in cairo ^ is xor, please use ** for exp
Suggestion:
2**7
remollemo
left a comment
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.
@remollemo reviewed 2 files and all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @RoeeGross).
RoeeGross
left a comment
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.
@RoeeGross resolved 6 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RoeeGross).
44e3465 to
c0fe019
Compare
RoeeGross
left a comment
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.
@RoeeGross resolved 1 discussion.
Reviewable status: 1 of 3 files reviewed, all discussions resolved (waiting on @remollemo).
RoeeGross
left a comment
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.
@RoeeGross reviewed 3 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @RoeeGross).
remollemo
left a comment
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.
@remollemo reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved.
Note
Adds compact storage casting utilities and tests.
Castable160(tuple(u128, u32)) andCastable64(u64) traits withencode/decodeEthAddress(160-bit)Written by Cursor Bugbot for commit c0fe019. This will update automatically on new commits. Configure here.
This change is