Pack proposal struct #363
Pack proposal struct #363
Conversation
…bs/sx-core into pack_proposal_struct
| max_end_timestamp: felt, | ||
| // timestamps contains the following packed into a single felt (each one is 32 bit): | ||
| // snapshot_timestamp, start_timestamp, min_end_timestamp, max_end_timestamp | ||
| timestamps: felt, |
There was a problem hiding this comment.
can we use a TyepAlias here maybe?
There was a problem hiding this comment.
the type here is felt but we can use TypeAlias (see here) to have something like:
using PackedTimestamps = (inner: felt)
and then we could use timestamps: PackedTimestamps
this way the reader understands that it's a "special" felt :)
There was a problem hiding this comment.
something like PackedTimestamps = (packed_timestamps : felt); seems a little unnecessary. Its an internal value that users wont ever need to interact with directly anyway so i think its okay to keep it a felt.
There was a problem hiding this comment.
Your choice :) I guess it's just the Rust way of thinking that got me wanting this. I find it much cleaner to have a type wrapper for this packedTimestamp. If I'm reading the code and I see this felt I will be thinking "what the fuck?". If I read the code and I see it's a special type (that just wraps a felt, but it doesn't matter), I will understand more what it is. But honestly it's a detail :)
There was a problem hiding this comment.
I just tried and we would need to do all sorts of casting to make it work as the lib uses felts. It would make more sense to have a generic type in the FeltUtils lib, a type like 4x32BitPacked . then use timestamps: 4x32BitPacked in the proposal struct.
There was a problem hiding this comment.
Okay tried doing this and I cant get it to compile. I dont think type aliases can be used for return values
There was a problem hiding this comment.
lets just stick to felts for the time being I think
closes #184
Reduces cost to create a proposal by nearly 30% for the vanilla setup