Skip to content
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

elliptic-curve: make FromEncodedPoint return a CtOption #782

Merged
merged 1 commit into from
Oct 2, 2021

Conversation

tarcieri
Copy link
Member

@tarcieri tarcieri commented Oct 2, 2021

Internally k256/p256 return a CtOption for this method, then convert to an Option.

It would be helpful when implementing other traits from the group crate, e.g. GroupEncoding, if this instead returned a CtOption.

Internally `k256`/`p256` return a `CtOption` for this method, then
convert to an `Option`.

It would be helpful when implementing other traits from the `group`
crate, e.g. `GroupEncoding`, if this instead returned a `CtOption`.
@tarcieri tarcieri force-pushed the elliptic-curve/from-encoded-point-ctoption branch from 0b57d60 to b5911b5 Compare October 2, 2021 18:08
@tarcieri tarcieri merged commit 23cddf0 into master Oct 2, 2021
@tarcieri tarcieri deleted the elliptic-curve/from-encoded-point-ctoption branch October 2, 2021 18:12
tarcieri added a commit to RustCrypto/signatures that referenced this pull request Oct 2, 2021
tarcieri added a commit to RustCrypto/signatures that referenced this pull request Oct 2, 2021
tarcieri added a commit to RustCrypto/elliptic-curves that referenced this pull request Oct 2, 2021
Corresponding changes for:

RustCrypto/traits#782

Internally these were already returning `CtOption` anyway. It should
also simplify the implementation of `GroupEncoding`.
tarcieri added a commit to RustCrypto/elliptic-curves that referenced this pull request Oct 2, 2021
Corresponding changes for:

RustCrypto/traits#782

Internally these were already returning `CtOption` anyway. It should
also simplify the implementation of `GroupEncoding`.
@tarcieri tarcieri mentioned this pull request Nov 20, 2021
@madninja
Copy link

I believe a corresponding change to ToCompactEncoding to return a CtOption instead of an Option would help with chaining key construction. The complexity in switching between CtOption and Option in the steps in: https://github.com/helium/helium-crypto-rs/pull/20/files#diff-522b3f5cfeb12570fd7f3eb9445fbe8a5151dadd10e4dc51ca79d2de9b51c9deR211-R230 doesn't smell right, does it.

I tried putting together a patch here but got lost in the elliptic_curves crate picking up that patch without newer commits creeping in

@tarcieri
Copy link
Member Author

Yes, sorry, this got missed when group and the other APIs switched to CtOption.

Regarding that code example, there's an impl of From<CtOption<T>> for Option<T>, so you can use Option::from(ct_option) to do the conversion (or ct_option.into() if type inference permits it)

@madninja
Copy link

Yes, sorry, this got missed when group and the other APIs switched to CtOption.

I guess I'll await the change to trickle in since I couldn't get the parts to fit together right trying to make a PR for the two relevant crates.

Regarding that code example, there's an impl of From<CtOption<T>> for Option<T>, so you can use Option::from(ct_option) to do the conversion (or ct_option.into() if type inference permits it)

Ah yes! I missed that, updated. Thanks for the pointer

@tarcieri
Copy link
Member Author

It's a breaking change, so we can't fix it until the next minor version bump

@madninja
Copy link

It's a breaking change, so we can't fix it until the next minor version bump

completely understand

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants