Skip to content

Re-export types from arithmetic module at toplevel#58

Merged
tarcieri merged 1 commit into
masterfrom
reexport-arithmetic-types-from-toplevel
Jun 25, 2020
Merged

Re-export types from arithmetic module at toplevel#58
tarcieri merged 1 commit into
masterfrom
reexport-arithmetic-types-from-toplevel

Conversation

@tarcieri

Copy link
Copy Markdown
Member

This commit removes the arithmetic module from the public API of the k256 and p256 crates, re-exporting the AffinePoint, ProjectivePoint, and Scalar types from the crate's toplevel.

Additionally, it moves the test_vectors module to the toplevel, gated under the test-vectors feature (or cfg(test))).

To make it clear these types are available conditionally based on the arithmetic and test-vectors features, doc_cfg is used to tag them in rustdoc as rendered by https://docs.rs

@tarcieri tarcieri requested review from newpavlov, str4d and tuxxy June 23, 2020 16:19
This commit removes the `arithmetic` module from the public API of the
`k256` and `p256` crates, re-exporting the `AffinePoint`,
`ProjectivePoint`, and `Scalar` types from the crate's toplevel.

Additionally, it moves the `test_vectors` module to the toplevel,
gated under the `test-vectors` feature (or `cfg(test))`).

To make it clear these types are available conditionally based on the
`arithmetic` and `test-vectors` features, `doc_cfg` is used to tag
them in rustdoc as rendered by https://docs.rs
@tarcieri

Copy link
Copy Markdown
Member Author

Rendered rustdoc:

k256

Screen Shot 2020-06-23 at 9 19 48 AM

p256

Screen Shot 2020-06-23 at 9 15 30 AM

@tarcieri tarcieri force-pushed the reexport-arithmetic-types-from-toplevel branch from 5069911 to fe4ef9b Compare June 23, 2020 16:20
@codecov-commenter

codecov-commenter commented Jun 23, 2020

Copy link
Copy Markdown

Codecov Report

Merging #58 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #58   +/-   ##
=======================================
  Coverage   80.66%   80.66%           
=======================================
  Files          12       12           
  Lines        1355     1355           
=======================================
  Hits         1093     1093           
  Misses        262      262           
Impacted Files Coverage Δ
k256/src/arithmetic.rs 84.93% <ø> (ø)
k256/src/arithmetic/field.rs 85.54% <ø> (ø)
k256/src/arithmetic/scalar.rs 60.60% <ø> (ø)
p256/src/arithmetic.rs 82.93% <ø> (ø)
p256/src/arithmetic/field.rs 82.11% <ø> (ø)
p256/src/arithmetic/scalar.rs 75.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 359f16d...fe4ef9b. Read the comment docs.

@newpavlov newpavlov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not familiar enough with code in this repository to properly review changes, but at the first glance it looks good to me.

@nickray

nickray commented Jun 23, 2020

Copy link
Copy Markdown
Member

What is the reasoning for arithmetic being a feature (crate is rather barren without, and the feature doesn't trigger additional dependencies AIUI)? My instinct would be to organize the library around modules field, point/curve, scalar, signature, agree, with randomness the core optional feature.

That said, I do prefer your suggestion over status quo.

A remark that perhaps doesn't quite fit here but seems relevant: For my microcontroller stuff, FieldElement newtypes [u32; 8] instead of [u64; 4], which impacts all the other types (and I deal with it via features that switch the entire type, not traits/generics).

@tarcieri

tarcieri commented Jun 23, 2020

Copy link
Copy Markdown
Member Author

What is the reasoning for arithmetic being a feature (crate is rather barren without, and the feature doesn't trigger additional dependencies AIUI)?

These crates are used in two different ways at the moment:

  1. With the arithmetic feature on (when available) it can function in a self-contained manner
  2. With it off, the PublicKey, CompressedPoint/UncompressedPoint, and weierstrass::Curve types can still be used in conjunction with the ecdsa crate to provide common key and signature types (generic around the underlying weierstrass::Curve) in cases where it's wrapping existing ECDSA implementations in a common trait-based API.

This includes things like:

  • Wrappers for Rust libraries which provide their own arithmetic implementation e.g. secp256k1 the C FFI wrapper for libsecp256k1, ring for NIST curves
  • Hardware devices as in the yubihsm and yubikey-piv crates (a SoloKeys host library could impl the same traits too!)
  • Cloud KMS services

With the types defined in the curve crates, it's possible to use trait objects as a common interface for representing various ECDSA providers. For example, in the Tendermint KMS project I work on:

https://github.com/iqlusioninc/tmkms/blob/develop/src/keyring/ecdsa.rs#L24

...we are able to support a libsecp256k1-based signer, Ledgers, and YubiHSM2s through a common keyring.

Arguably we could remove the arithmetic feature, and in these cases the curve arithmetic would be compiled even when it isn't needed, which isn't the end of the world. However, I imagine as more (e.g. table-based, possibly using tables computed as part of a build step) complex curve arithmetic is added, it will be increasingly nice to be able to skip all of that in the event that all you care about are the curve and "wire" types for interoperability between crates.

My instinct would be to organize the library around modules field, point/curve, scalar, signature, agree, with randomness the core optional feature.

I'd say for the most part it is, but large parts of it are under the arithmetic module. With an expose-arithmetic feature, you could potentially pub use the modules at the toplevel as well, and the arithmetic module would still be invisible to the end user.

For my microcontroller stuff, FieldElement newtypes [u32; 8] instead of [u64; 4], which impacts all the other types (and I deal with it via features that switch the entire type, not traits/generics).

Some semi-related precedent for this is the field implementations in the polyval crate:

https://github.com/RustCrypto/universal-hashes/tree/master/polyval/src/field

Namely there are separate u32 (for microcontrollers) vs u64 implementations of the field (in addition to a PCLMULQDQ-based one which uses XMM registers). Which implementation is used, and what types end up being used by the rest of the crate, are controlled entirely through conditional compilation, as opposed to a trait/generic-based API.

I believe curve25519-dalek works similarly?

@tarcieri tarcieri merged commit 134cf33 into master Jun 25, 2020
@tarcieri tarcieri deleted the reexport-arithmetic-types-from-toplevel branch June 25, 2020 19:50
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.

4 participants