Implement crypto byte array newtypes in term of a shared type #3684
Conversation
davxy
added
R0-silent
davxy
requested review from
andresilva,
michalkucharczyk,
bkchr,
a team and
dmitry-markin
| } | ||
| } | ||
|
|
||
| impl<const N: usize, T> UncheckedFrom<[u8; N]> for CryptoBytes<N, T> { |
There was a problem hiding this comment.
nit: do we need unchecked_from?
There was a problem hiding this comment.
I think that the original intent of UncheckedFrom was to make explicit the fact that the construction of the object is performed without actually validating the content (e.g. in case of EC based crypto that the value represents a valid point on the used curve).
While this may have sense, should be said that the same types were exposing constructors like CryptoBytes::from_raw([u8; N]) or ByteArray::from_raw([u8; N]) where the docs explicitly states that the check is not performed.
IMHO may be sufficient to document that the constructors from a raw byte array doesn't ensure validity of the bytes when these are then tried to be deserialized into the higher level object.
What could go wrong if you use a bad byte array:I've not removed it yet (as other traits and redundant stuff) to keep this PR as minimal as possible
- Signatures: nothing. The verification simply fails.
- Public keys: if used to verify something. It will fail.
So, in both cases, the failure is postponed to the usage stage instead of the construction phase.
May instead have sense to remove the From<[u8; 32]>? And keep UncheckedFrom only?
There was a problem hiding this comment.
May instead have sense to remove the From<[u8; 32]>? And keep UncheckedFrom only?
I see your point, it makes sense. Being explicit here is better. On the other hand from is tempting as it is shorter.
But still it is a nit, cleanup could be done in some follow-up.
There was a problem hiding this comment.
May instead have sense to remove the
From<[u8; 32]>? And keepUncheckedFromonly?
Hmmm... it could make sense, but I guess maybe only if we remove all of the other easy ways to make a T in an unchecked fashion?
Also, in my refactoring PR I'm using these From impls to convert between raw arrays when passing these types through the FFI boundary between the runtime and the host. So if we remove UncheckedFrom I'll have to add another wrapper strategy type just for passing these using the UncheckedFrom. Not a big deal, but it is extra code that will have to be added.
There was a problem hiding this comment.
I'd also like to add that the wrapped type is (and was) public. So maybe having this "UncheckedFrom" doesn't really add anything really valuable? Mostly syntactic sugar.
As said I leave it untouched for now
| pub struct SignatureTag; | ||
|
|
||
| /// Generic encoded signature. | ||
| pub type SignatureBytes<const N: usize, SubTag> = CryptoBytes<N, (SignatureTag, SubTag)>; |
There was a problem hiding this comment.
nit: it is all mostly wrappers, but maybe you could add some tests? However I am not 100% they are actually necessary. Feel free to ignore.
| /// | ||
| /// The tag `T` is held in a `PhantomData<fn() ->T>`, a trick allowing | ||
| /// `CryptoBytes` to be `Send` and `Sync` regardless of `T` properties | ||
| /// ([ref](https://doc.rust-lang.org/nomicon/phantom-data.html#table-of-phantomdata-patterns)). |
…tech#3684) Introduces `CryptoBytes` type defined as: ```rust pub struct CryptoBytes<const N: usize, Tag = ()>(pub [u8; N], PhantomData<fn() -> Tag>); ``` The type implements a bunch of methods and traits which are typically expected from a byte array newtype (NOTE: some of the methods and trait implementations IMO are a bit redundant, but I decided to maintain them all to not change too much stuff in this PR) It also introduces two (generic) typical consumers of `CryptoBytes`: `PublicBytes` and `SignatureBytes`. ```rust pub struct PublicTag; pub PublicBytes<const N: usize, CryptoTag> = CryptoBytes<N, (PublicTag, CryptoTag)>; pub struct SignatureTag; pub SignatureBytes<const N: usize, CryptoTag> = CryptoBytes<N, (SignatureTag, CryptoTag)>; ``` Both of them use a tag to differentiate the two types at a higher level. Downstream specializations will further specialize using a dedicated crypto tag. For example in ECDSA: ```rust pub struct EcdsaTag; pub type Public = PublicBytes<PUBLIC_KEY_SERIALIZED_SIZE, EcdsaTag>; pub type Signature = PublicBytes<PUBLIC_KEY_SERIALIZED_SIZE, EcdsaTag>; ``` Overall we have a cleaner and most importantly **consistent** code for all the types involved All these details are opaque to the end user which can use `Public` and `Signature` for the cryptos as before
…ature Bytes (paritytech#3806) Another simple refactory to prune some duplicate code Follow up of: paritytech#3684
Introduces
CryptoBytestype defined as:The type implements a bunch of methods and traits which are typically expected from a byte array newtype
(NOTE: some of the methods and trait implementations IMO are a bit redundant, but I decided to maintain them all to not change too much stuff in this PR)
It also introduces two (generic) typical consumers of
CryptoBytes:PublicBytesandSignatureBytes.Both of them use a tag to differentiate the two types at a higher level. Downstream specializations will further specialize using a dedicated crypto tag. For example in ECDSA:
Overall we have a cleaner and most importantly consistent code for all the types involved
All these details are opaque to the end user which can use
PublicandSignaturefor the cryptos as before