Make Uint64, Uint128, Int64, Int128 usable as keys
#86
Conversation
src/int_key.rs
Outdated
|
|
||
| #[inline] | ||
| fn to_cw_bytes(&self) -> Self::Buf { | ||
| self.$tt().to_cw_bytes() |
There was a problem hiding this comment.
Can we implement this with
$tt::from(self)i.e. using the From implementation to get the primitive type. Feels more stable than this somewhat strange getter.
There was a problem hiding this comment.
This conversion doesn't seem to be present for signed types, probably because the conversion isn't trivial
There was a problem hiding this comment.
Ah, no, I mean the first one to primitive type. So this in total:
$tt::from(self).to_cw_bytes()
But it seems like exporters like there is no From for Uint128 -> uint128 and similar. This would be nice to have for sure.
There was a problem hiding this comment.
Yeah, I did try that. I wanted a quick macro that can cover all the integer types from cosmwasm_std, but I can change it around if you feel it'd be easier for maintenance/understanding
There was a problem hiding this comment.
I am wondering about a different approach which is more elegant, also looking at 256-bit and 512-bit values for which no primitive type exists.
What to_cw_bytes/from_cw_bytes does for signed integers is simply flip the first bit of the BE representation (most significant bit). So instead of going through the primitive type we can just:
- For unsigned integers use
self.to_be_bytes() - For signed integers use
let bytes = self.to_be_bytes()and the most significant bit ofbytes[0]
There was a problem hiding this comment.
You may be right! I'll look at it.
I'd still rather merge this PR without 256 and 512 (seems it would require breaking API, see above). I'll do a follow-up when I'm sure the next release will be breaking anyway.
There was a problem hiding this comment.
Yeah, I agree with the split. I just think those iXY() getter functions to get the primary type are ugly and inconsistent and should not be built upon. However, this should not be a blocker
There was a problem hiding this comment.
Yeah, I think I like your solution better too. I'll give it a try.
This doesn't break the API in a very serious way.
I think if we want to do the same for 256-bit and 512-bit values, we'll need to break API by adding larger variants to this enum. I guess we might as well (in another PR) since we're likely to release cw-storage-plus 3.0.0.