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

[BREAK API] ECDSA.Recoverable to ECDSAWithKeyRecovery & ECDSA.NonRecoverable to ECDSA #34

Merged
merged 3 commits into from May 12, 2023

Conversation

Sajjon
Copy link
Owner

@Sajjon Sajjon commented Mar 28, 2023

This PR changes two two namespaces:
K1.ECDSA.Recoverable and K1.ECDSA.NonRecoverable into K1.ECDSAWithKeyRecovery and just K1.ECDSA respectively.

The rationale is:

  • This flattens the otherwise nested structure ECDSA (which was the namespace for Recoverable and NonRecoverable which was kind of asymmetric w.r.t. K1.Schnorr.
  • Assumption: Most user are probably not in need of ECDSA signatures that do offer recoverability of public key, therefor being able to use the much shorter namespace K1.ECDSA instead of K1.ECDSA.NonRecoverable is preferred.

Drawback:
Both ECDSA variants have two structs with options used for signing and validation, named both before this PR and after this PR (i.e. unchanged) K1.ECDSA.SigningOptions and K1.ECDSA.ValidationOptions, and it before this PR it was hiearcally correct that these structs was defined in the shared namespace ECDSA. If this PR were to be merged it is less elegant that it looks like these structs belong to the non recoverable ECDSA variant, since it is used by both. However this is mostly a detail.

I'm torn between what the best API... Unfortunately swift-crypto has not solved this particular problem of two variant for the same signature scheme, so I can't look for answer there (which I've otherwise done a lot when designing the API of this library).

Summary of API:

Given:

let hashed = Data(SHA256.hash(data: Data("Hey Bob!".utf8)))

ECDSA

let privateKey: K1.ECDSA.PrivateKey = .init()
let publicKey: K1.ECDSA.PublicKey = privateKey.publicKey
let signature = try privateKey.signature(for: hashed)
publicKey.isValidSignature(signature, hashed: hashed) // true

ECDSA With Recovery

let privateKey: K1.ECDSAWithKeyRecovery.PrivateKey = .init()
let publicKey: K1.ECDSAWithKeyRecovery.PublicKey = privateKey.publicKey
let signature = try privateKey.signature(for: hashed)
publicKey.isValidSignature(signature, hashed: hashed) // true

Schnorr (unchanged)

let privateKey: K1.Schnorr.PrivateKey = .init()
let publicKey: K1.Schnorr.PublicKey = privateKey.publicKey
let signature = try privateKey.signature(for: hashed)
publicKey.isValidSignature(signature, hashed: hashed) // true

…and 'K1.ECDSA.NonRecoverable' to just 'K1.ECDSA', under the assumption that most user will need to use ECDSA signatures that offers recoverability of publickey.
@Sajjon Sajjon added BREAKS API Breaking API change help wanted Extra attention is needed labels Mar 28, 2023
@Sajjon Sajjon changed the title ECDSA.Recoverable -> ECDSAWithKeyRecovery and ECDSA.NonRecoverable -> ECDSA [BREAK API] ECDSA.Recoverable to ECDSAWithKeyRecovery & ECDSA.NonRecoverable to ECDSA Mar 31, 2023
@Sajjon Sajjon merged commit 9e6c271 into main May 12, 2023
2 checks passed
@Sajjon Sajjon deleted the api_break_rename_ECDSA_namesspaces branch May 12, 2023 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BREAKS API Breaking API change help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant