Closed soyuka closed 6 years ago
@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)
@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! :grin:
@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 :man_facepalming:.
Second commit is a cherry-pick from https://github.com/datrs/hypercore/pull/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?
Feed::with_storage
=> if the storage doesn't have any public key, generate one and store it?
Yeah, sounds reasonable! :D
I changed the status to wip :p. I'll add more tests to the FeedBuilder if you agree with the implementation!
@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! :heart:
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 :heart:! 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 :).
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!
I think that the implementation is good now:
PartialKeypair
struct to simplify the storage read function. 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. secret_key
call on the builder if none is given.@soyuka looks like we introduced a conflict with the new builder introduced in #17, would you mind rebasing and pushing again?
Done!
🙋 feature
I've implemented two new functions:
read_keypair
andwrite_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
generate and store
?)Context
https://github.com/datrs/hypercore/issues/16
Semver Changes
patch