-
Notifications
You must be signed in to change notification settings - Fork 1
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
Several ergonomic improvements #27
base: main
Are you sure you want to change the base?
Conversation
6ca74bf
to
691b9c4
Compare
Not sure why we would need this and it causes problems ...
Functions that allocate should start with `to_`. `as_` is used for cheap conversions.
To minimise breaking changes, we introduce extension traits that provide most of the old functionality that was defined on the wrapper types. Fixes #13.
9868b59
to
fee1f70
Compare
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.
Nice solution with the extension trait! I never consider that one.
@@ -191,8 +192,8 @@ impl Address { | |||
Address { | |||
network, | |||
addr_type: AddressType::Integrated(payment_id), | |||
public_spend, | |||
public_view, | |||
public_spend: public_spend.compress(), |
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.
❓ As a general question, why should we store compressed points? I would assume that one could wait until serialisation.
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.
Unfortunately, EdwardsPoint
s don't implement Hash
so we would have to find a solution around that. Compressing is not cheap but I guess doing it inside Hash
might be fine?
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.
True, you have to compress before you hash.
On principle, I would delay compressing until it's strictly necessary. The thing about CompressedEdwardsY
is that it's no different to 32 bytes, so it can hold data that doesn't actually map to an EdwardsPoint
(see decompress
).
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.
Yeah I think moving the compressing to a manual implementation of Hash
is probably the better idea.
Thank you. It is good if you want to keep the type hierarchy flat. In our case, both |
Draft until dalek-cryptography/curve25519-dalek#353 is accepted.
Additionally, the changes to private/publickey are controversal and should probably go somewhere else and/or be delayed.