Storage: implement keypair read/write #18

soyuka · 2018-08-17T08:38:17Z

🙋 feature

I've implemented two new functions: read_keypair and write_keypair in the Storage implementation.

As I'm fairly new to rust I have a few unsure things (see comments) and am open to corrections!

Notable change: I've remove the unimplemented open_key method as I guessed that you wanted to do what I did :).

Checklist

valid implementation
use the new methods in the feed (add a function to generate and store?)

Context

#16

Semver Changes

patch

soyuka · 2018-08-17T08:39:09Z

src/storage/mod.rs

+  use super::*;
+  use crypto::generate_keypair;
+  use sha2::Sha512;
+  use ed25519_dalek::{Keypair};


This seemed right to avoid an unused import, let me know if there's a better way!

soyuka · 2018-08-17T08:41:44Z

src/storage/mod.rs

+    let message: &[u8] = "Hello world".as_bytes();
+    let sig: Signature = keypair.sign::<Sha512>(&message);
+
+    assert!(readen_keypair.verify::<Sha512>(&message, &sig),


I thought that it'd be a good idea to verify with a signature from the original keypair. What's weird is that rust tells me that this is a bool although a test is done with is_ok in the ed25519 library.

I also thought about comparing the two array of bytes ([u8; KEYPAIR_LENGTH]) but I haven't found any elegant way of doing it!

yoshuawuyts · 2018-08-17T13:58:18Z

@soyuka would you mind running rustfmt? (We removed it for CI a while back, but now that it's stable we should probably re-enable, haha)

yoshuawuyts · 2018-08-17T14:06:44Z

@soyuka hmmm, bit of a bigger thing here - but did you see that a couple of weeks ago we moved from storing a single keypair to storing the public / private key separately.

Could we perhaps duplicate this code so we have 1 for the public key, and 1 for secret key. In the Node.js version of Dat, these are then stored on disk as .dat/content.key and .dat/metadata.key.

Really liking the code so far though; looks like this is definitely the right direction! 😁

soyuka · 2018-08-17T16:32:38Z

@soyuka hmmm, bit of a bigger thing here - but did you see that a couple of weeks ago we moved from storing a single keypair to storing the public / private key separately.

OMG sorry haha, this is the commit that lead me to read the code and find that todo 🤦‍♂️.

Second commit is a cherry-pick from #19 :)

I guess that we have to figure out of to use this feature inside the feed?

Feed::with_storage => if the storage doesn't have any public key, generate one and store it?

yoshuawuyts · 2018-08-17T16:55:12Z

Feed::with_storage => if the storage doesn't have any public key, generate one and store it?

Yeah, sounds reasonable! :D

soyuka · 2018-08-17T18:53:21Z

src/feed_builder.rs

-      storage,
-      public_key,
-      secret_key: None,
+  pub fn new(public_key: Option<PublicKey>, mut storage: Storage<T>) -> Self {


I'm really not sure if we'd want the logic here or directly in the Feed. It also feels weird to have an Option as first argument but I wanted to avoid a BC break.

What is a BC break?

Backward compatibility break, but I then realized that I had change the public_key signature so my code is breaking the backward compatibility promise. Anyway I think that it's a bit early into the project to care about such things 😅.

soyuka · 2018-08-20T09:39:20Z

I changed the status to wip :p. I'll add more tests to the FeedBuilder if you agree with the implementation!

yoshuawuyts · 2018-08-21T09:23:11Z

src/feed_builder.rs

+          public_key,
+          secret_key: None,
+        },
+        Err(_) => {


Hmm, do you think there's a way we can move this out to FeedBuilder? Maybe it's just me, but I wouldn't expect the keys to be generated inside storage.rs. I feel keeping the initialization logic strictly inside FeedBuilder might make it the easiest to reason about, even if it adds a bit code. Does that... make sense?

I'm not sure what the right approach is instead. Perhaps an intermediate struct that can be "half-open" and tries to read the key, and then has a method to indicate whether or not it has a public key set on it? e.g.

initialize instance

check if it has a key on it

if it doesn't have a key, set a key

finalize initialization

all operations are now available

I really don't know haha, I think it needs a little bit of experimentation. Or maybe I'm wrong here and it like doesn't make any sense at all? I'm really keen to hear your opinion on this because I'm very unsure, haha.

yoshuawuyts · 2018-08-21T09:25:03Z

@soyuka also I wanted to mention I super appreciate all the work you're putting into this! This patch is shaping up to be really good; thanks for doing all this! ❤️

soyuka · 2018-08-21T10:08:59Z

I've moved out the code from the builder and it indeed looks cleaner.

To go further, I'd like to propose the following signature for the FeedBuilder:

  /// Create a new instance.
  #[inline]
  pub fn new(storage: Storage<T>, public_key: PublicKey, secret_key: Option<SecretKey>) -> Self {
    Self {
      storage,
      public_key,
      secret_key
    }
  }

I don't really understand the need for a secret_key method if we can make it optional from the start.

also I wanted to mention I super appreciate all the work you're putting into this! This patch is shaping up to be really good; thanks for doing all this!

Kind words ❤️! I'm really enjoying this especially that I'm learning rust and reading/writing code is my way of doing it! Thanks to you for giving me the motivation/opportunity through this project :).

yoshuawuyts · 2018-08-23T12:36:42Z

I don't really understand the need for a secret_key method if we can make it optional from the start.

I was thinking that we could specialize our struct based on which arguments are passed in. E.g. all optional arguments are called as methods on the builder. It's something I'd prefer to have for the resulting API because I think it's really clean.

That said I'm cool with changing the API in the meantime. If this can help us get key storage faster,
then I'm all for it!

soyuka · 2018-08-23T14:28:41Z

I think that the implementation is good now:

I've introduced a PartialKeypair struct to simplify the storage read function.
I didn't change the FeedBuilder signature, no real benefits
Feed::with_storage is now attempting to read keys from the given storage, if none is present it'll generate and write them to the storage.
The secret key is optional in the storage and it'll skip the secret_key call on the builder if none is given.

yoshuawuyts · 2018-08-24T10:40:58Z

@soyuka looks like we introduced a conflict with the new builder introduced in #17, would you mind rebasing and pushing again?

yoshuawuyts

This is really really good! Thanks so much! :D

Signed-off-by: abluchet <[email protected]>

soyuka · 2018-08-24T10:44:46Z

Done!

yoshuawuyts

Yay!

soyuka commented Aug 17, 2018

View reviewed changes

soyuka force-pushed the implement-keypair-storage branch 2 times, most recently from fa73d29 to 816a2e7 Compare August 17, 2018 16:28

soyuka force-pushed the implement-keypair-storage branch 2 times, most recently from 2ef39dc to 1ae4332 Compare August 17, 2018 18:51

soyuka commented Aug 17, 2018

View reviewed changes

soyuka changed the title ~~🙋 feature - Storage: implement keypair read/write~~ WIP: Storage: implement keypair read/write Aug 20, 2018

yoshuawuyts reviewed Aug 21, 2018

View reviewed changes

soyuka force-pushed the implement-keypair-storage branch 3 times, most recently from 713145f to 81e8cb1 Compare August 21, 2018 10:02

soyuka force-pushed the implement-keypair-storage branch from 81e8cb1 to 2935cb1 Compare August 23, 2018 13:59

soyuka changed the title ~~WIP: Storage: implement keypair read/write~~ Storage: implement keypair read/write Aug 23, 2018

soyuka force-pushed the implement-keypair-storage branch 2 times, most recently from d6847bc to 4ac34a6 Compare August 23, 2018 14:21

yoshuawuyts approved these changes Aug 24, 2018

View reviewed changes

abluchet and others added 2 commits August 24, 2018 12:43

Storage: implement keypair read/write

4e3f222

Signed-off-by: abluchet <[email protected]>

PublicKey generated if not present in the storage

adc5b1f

Signed-off-by: abluchet <[email protected]>

soyuka force-pushed the implement-keypair-storage branch from 4ac34a6 to adc5b1f Compare August 24, 2018 10:44

yoshuawuyts approved these changes Aug 25, 2018

View reviewed changes

yoshuawuyts merged commit 7d6bde0 into datrs:master Aug 25, 2018

soyuka deleted the implement-keypair-storage branch August 26, 2018 16:51

soyuka mentioned this pull request Aug 27, 2018

Seal KeyPair type #7

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Storage: implement keypair read/write #18

Storage: implement keypair read/write #18

soyuka commented Aug 17, 2018 •

edited

Loading

soyuka Aug 17, 2018

soyuka Aug 17, 2018

yoshuawuyts commented Aug 17, 2018

yoshuawuyts commented Aug 17, 2018

soyuka commented Aug 17, 2018

yoshuawuyts commented Aug 17, 2018

soyuka Aug 17, 2018

yoshuawuyts Aug 21, 2018

soyuka Aug 21, 2018

soyuka commented Aug 20, 2018

yoshuawuyts Aug 21, 2018

yoshuawuyts commented Aug 21, 2018

soyuka commented Aug 21, 2018 •

edited by yoshuawuyts

Loading

yoshuawuyts commented Aug 23, 2018

soyuka commented Aug 23, 2018

yoshuawuyts commented Aug 24, 2018

yoshuawuyts left a comment

soyuka commented Aug 24, 2018

yoshuawuyts left a comment

Storage: implement keypair read/write #18

Storage: implement keypair read/write #18

Conversation

soyuka commented Aug 17, 2018 • edited Loading

Checklist

Context

Semver Changes

soyuka Aug 17, 2018

Choose a reason for hiding this comment

soyuka Aug 17, 2018

Choose a reason for hiding this comment

yoshuawuyts commented Aug 17, 2018

yoshuawuyts commented Aug 17, 2018

soyuka commented Aug 17, 2018

yoshuawuyts commented Aug 17, 2018

soyuka Aug 17, 2018

Choose a reason for hiding this comment

yoshuawuyts Aug 21, 2018

Choose a reason for hiding this comment

soyuka Aug 21, 2018

Choose a reason for hiding this comment

soyuka commented Aug 20, 2018

yoshuawuyts Aug 21, 2018

Choose a reason for hiding this comment

yoshuawuyts commented Aug 21, 2018

soyuka commented Aug 21, 2018 • edited by yoshuawuyts Loading

yoshuawuyts commented Aug 23, 2018

soyuka commented Aug 23, 2018

yoshuawuyts commented Aug 24, 2018

yoshuawuyts left a comment

Choose a reason for hiding this comment

soyuka commented Aug 24, 2018

yoshuawuyts left a comment

Choose a reason for hiding this comment

soyuka commented Aug 17, 2018 •

edited

Loading

soyuka commented Aug 21, 2018 •

edited by yoshuawuyts

Loading