-
Notifications
You must be signed in to change notification settings - Fork 211
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
Modified AsRef impl for U-type macros #196
Conversation
- Potential bug with current implementation that led to unused API - Replaced with useful internal getter for &[u64] types Fixes: paritytech#195
It looks like @fubuloubu signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
Co-Authored-By: Wei Tang <accounts@that.world>
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.
Hmm, I don't know the reason why it was implemented the way it was (cc @debris, paritytech/primitives#25), but this feels more consistent with fixed-hash. OTOH, it's probably a breaking change...
Or maybe we can keep both implementations, actually ( |
Co-Authored-By: Andronik Ordian <write@reusable.software>
Also seems like U-types are missing |
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 it is good to remove the old AsRef implementation - as far as I can remember I did the same some time ago for the fixed_hash
crate and type.
However, while the AsRef<[u8]>
makes total sense for the hash types I am not sure about the Uint
types here since they have endianess that is not communicated through this API.
Maybe it would be better to add digits_be
and digits_le
instead.
Hmm, I was trying to leverage the For my own knowledge, why does integer types have the concept of endian-ness whereas bytes types do not? |
That's because integer types do have the property of total order. |
Just noticed that macro seems to assume Little Endian. Is this correct? parity-common/uint/src/uint.rs Lines 420 to 432 in 5b65cd2
|
Yes, this is intended for U-types.
use ethereum_types::U256;
use bitvec::prelude::*;
fn main() {
let n = U256::from(0b0101);
let bits = n.0.as_bitslice::<LittleEndian>();
println!("{}{}{}{}", bits[3] as u8, bits[2] as u8, bits[1] as u8, bits[0] as u8);
} |
My use case is trying to genericize |
You mean represent them at runtime? - if yes, then there are certain crates already available - shameless pug: https://crates.io/crates/apint If no: Then that's what U-types are already doing for you. |
So, is the lack of specification of endian-ness the blocker of this PR? If so, could we just assume it is LE (as it currently is) for implementing the |
From a software technical point of view I would not recommend doing so since the On the other handside I am not the one in charge for parity-common, so I do not want to be the show stopper. |
Is there a better type I could use for the conversion that's in |
@fubuloubu if you want to convert between little-endian U-types and big-endian H-types, take a look at this macro in ethereum-types: parity-common/ethereum-types/src/hash.rs Lines 35 to 56 in ec57f89
If you want to use a little-endian bit iterator on U-types, you can create a trait trait AsBitsliceLittleEndian {
fn as_bitslice_le(&self) -> &BitSlice<LittleEndian, u64>;
} and implemented it for U-types e.g. via macro. Does that make sense? |
@ordian wouldn't it be simpler to implement I can implement this trait in |
But that means you can create |
Yes, I want to iterate over the bits of a key from MSB to LSB in order to determine the ordering of hashes in a sparse merkle tree for that given key. |
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.
On the one hand, I agree with @Robbepop that AsRef
doesn't communicate clearly the word order for uint-types, otoh, one can already access the underlying array with .0
, and with a bit of documentation added to impl AsRef
it doesn't make things any worse (we could also add an example with bitvec to uint/examples
). @sorpaas WDYT?
What would be the downside of having explicit methods that do what you want
Note we use |
The methods would not be available as a trait, meaning it would not be possible to leverage them in a generic fashion. |
The underlying array is marked as |
Co-Authored-By: Andronik Ordian <write@reusable.software>
You could simply expose them as traits: pub trait UintEndianEx {
fn to_be_digits(&self) -> impl Iterator<u64>
fn to_le_digits(&self) -> impl Iterator<u64>
fn to_be_bytes(&self) -> impl Iterator<u8>
fn to_le_bytes(&self) -> impl Iterator<u8>
} And then implement them for all |
I'm still a bit new to Rust, but you would have to |
@fubuloubu what do you think is the best way forward here? |
I agree with your assessment that having Another PR could add a better API for accessing it in LE or BE slices, although many external libraries (such as |
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 uint
is not properly engineered for handling LE/BE. I also agree with @ordian that at a minimum we should be explicit about out assumptions and I think this PR is better than what we have. Merging.
Fixes: #195