impl (From<u32>, Hash, Ord, Product) for Scalar#52
impl (From<u32>, Hash, Ord, Product) for Scalar#52A-Manning wants to merge 3 commits intozkcrypto:mainfrom
Conversation
A-Manning
force-pushed
the
ash/scalar-traits
branch
from
232d2c9 to
3aa4d87
Compare
A-Manning
force-pushed
the
ash/scalar-traits
branch
from
3aa4d87 to
3e24b43
Compare
A-Manning
changed the title
| impl From<u32> for Scalar { | ||
| fn from(val: u32) -> Scalar { | ||
| Scalar([val as u64, 0, 0, 0]) * R2 | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
As is visible from other changes in the PR, this decreases usability by requiring the caller to specify the source type (e.g. 5u64.into() instead of 5.into()). I suspect this would be more usable if impl From<u64> for Scalar was replaced with impl From<T: Into<u64>> for Scalar (and it would then also work for any u* type).
| impl Ord for Scalar { | ||
| fn cmp(&self, other: &Self) -> core::cmp::Ordering { | ||
| let mut self_bytes = self.to_bytes(); | ||
| let mut other_bytes = other.to_bytes(); | ||
| &self_bytes.reverse(); | ||
| &other_bytes.reverse(); | ||
| self_bytes.cmp(&other_bytes) | ||
| } | ||
| } | ||
|
|
||
| impl PartialOrd for Scalar { | ||
| fn partial_cmp(&self, other: &Self) -> Option<core::cmp::Ordering> { | ||
| Some(self.cmp(other)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Cryptographic structures, in particular finite fields, do not have a total ordering (in the "well acktually" mathematical sense) so I disagree with exposing Ord/PartialOrd when they are in the form of Scalar since they're expected to behave like finite fields. However, obviously we can totally order them when they're represented in serialized form (as this implementation leverages) assuming they're serialized canonically.
If you need Ord for things like a BTreeMap (for example) then the end user should convert it into its serialized form and depend on the total ordering of that instead.
4 participants
No description provided.