Skip to content
This repository has been archived by the owner on Feb 26, 2020. It is now read-only.

API change: Helper functions to combine/merge keys #2

Open
kianenigma opened this issue Feb 12, 2019 · 9 comments
Open

API change: Helper functions to combine/merge keys #2

kianenigma opened this issue Feb 12, 2019 · 9 comments
Labels
wontfix This will not be worked on

Comments

@kianenigma
Copy link
Collaborator

kianenigma commented Feb 12, 2019

Currently, one of the API functions returns a byte array which is the concatenation of the private and public key respectively.

pub fn keypair_from_seed(seed: &[u8]) -> [u8; KEYPAIR_LENGTH] { ... }

It is the user's duty to properly separate them as documented in the Rust code.

While there also exists another function which does almost the same but only returns the secret/private portion of the key.

pub fn secret_from_seed(seed: &[u8]) -> [u8; SECRET_KEY_LENGTH] { ... }

Consequently, the sign function is currently accepting the public and private key separately.

pub fn sign(public: &[u8], private: &[u8], message: &[u8]) -> [u8; SIGNATURE_LENGTH] { ... }

Based on the above, I see at least two improvements that could be made:

  • is the secret_from_seed even needed?
  • would a sign function that accepts the keypair instead be more handy?
  • a set of functions can be exposed to merge/separate keys to facilitate the progress. e.g. private_from_pair(keypair) etc.
    • Though, I believe adding them could be a downgrade of performance as they will 1) pass data back and forth between JS and wasm eventhough its a simple array slice. 2) slightly increase the size of the wasm blob.
@burdges
Copy link

burdges commented Feb 13, 2019

As a rule, the keypair is the secret key type and the actual private key type should never be used. It's okay to use the private key type for serialization though, as you can derive the keypair from the secret key.

@kianenigma
Copy link
Collaborator Author

Based on the latest discussion with @jacogr who will be first to use this library I was informed that having the public/private portions separated is actually more convenient for him. This will still be open to change but I think it is fine as in the context of wasm exported function, none of the rust-defined data structures such as Private and Public exist anymore. Essentially, all of them will be seen as UInt8Array() from the perspective of Javascript.

@kianenigma kianenigma added the wontfix This will not be worked on label Feb 13, 2019
@burdges
Copy link

burdges commented Feb 13, 2019

It's fine to separate them for user facing js things that do not require high performance, just provide a sign function that calls .to_public() or .expand_to_public(). Anytime you want high performance then your code should use Keypair as the secret key everywhere because the .to_public() call is almost as slow as signing.

In practice, almost all methods in this library use only a Keypair because doing so simplifies the code, so you'll need to call to_keypair().whatever(). Ideally, I might've renamed Keypair to SecretKey and SecretKey to PreSecretKey or whatever, but sounds like that ship sailed.

@jacogr
Copy link
Contributor

jacogr commented Feb 13, 2019

That rename would have helped ;) Never too late, we still need updates, so rather earlier than later.

But yes, the public portion is separated out since that is used (via ss58) to represent the address.

EDIT: With a rename (i.e. pair is actually secretKey), it makes more sense to pass it in as a whole

@burdges
Copy link

burdges commented Feb 13, 2019

I'm torn still because really all performant dlog crypto libraries should have keypair types that provide this optimization. It's maybe worse to hide the keypair form a person who is looking for it?

Also, there are two serializations for a keypair with maybe the best being to serialize only keypair.secret and call .to_keypair() when you get it back. Algorithms almost always require a pair but infrequent serialization wants just the secret.

If we renamed, then what name should the existing secret key be then? PureSecretKey or TruePureSecretKey maybe?

@jacogr
Copy link
Contributor

jacogr commented Feb 13, 2019

Ok, now I get your dilemma... Not a cryptographer, so it took me a while to get on the same page. I can only comment on a external use perspective where (in "simpler" libraries, e.g. sodium), you only have the secretKey to pass into the sign process. And publicKey to verify those. So generally, those are the important external bits.

But then you start looking at derivation and friends, and well, clueless.

@burdges
Copy link

burdges commented Feb 13, 2019

In the end, this is a performance optimization, but it's often best to stick with the usual terminology.

I actually did one rename here already btw:

  • SecretKey -> MiniSeretKey
  • ExpandedSecretKey -> SecretKey

I felt justified in that rename because it's makes our soft BIP32 analog in derive.rs far more intuitive and no sane terminology or standards exists in that area.

Anyways just because I do one thing in schnorrkel does not mean that schnorrkel-js needs to do the same thing. If the js stuff is not used for anything high performance then you can call .to_public() every time. It's not that slow. :)

@kianenigma
Copy link
Collaborator Author

kianenigma commented Feb 14, 2019

Based on the discussion I can propose the following to be A) more aligned with how the original schnorrkel is working B) performant C) easier to use for JS

  • You get your secretKey via
// output vector will be 64 bytes; The real private key 
pub fn secret_from_seed(seed: &[u8]) -> Vec<u8> {...}

I am not sure if we still need the keypair_from_seed? @jacogr @burdges

  • The get the public key to be used as address:
// output vector will be 32 bytes; The real public key 
pub fn expand_to_public(secret: &[u8]) -> Vec<u8> {...}
  • to sign something:
pub fn sign(secret: &[u8], message: &[u8]) -> Vec<u8> { ... }
  • Verifying will stay the same.
pub fn verify(signature: &[u8], message: &[u8], pubkey: &[u8]) -> bool {

I think this is fulfilling in the sense that:

  • From the JS's perspective, the secret key is the keypair
  • From my perspective, I will be mostly using Keypair und the hood.

I actually do agree that this looks better. Much better.
@jacogr I will implement this in a new branch. Once you're ready it will be merged.

@kianenigma
Copy link
Collaborator Author

The above API is implemented but not yet merged published in #4.

@kianenigma kianenigma added waiting-for-merge ✋ and removed wontfix This will not be worked on labels Feb 15, 2019
@kianenigma kianenigma added wontfix This will not be worked on and removed waiting-for-merge ✋ labels Mar 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
wontfix This will not be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants