-
Notifications
You must be signed in to change notification settings - Fork 91
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
Require to manually set an entropy source #240
Require to manually set an entropy source #240
Conversation
cc3173e
to
016bd63
Compare
20f4106
to
e93a801
Compare
src/builder.rs
Outdated
/// stored at the given location. | ||
/// | ||
/// If `config` is provided, it will override the default config. | ||
pub fn from_entropy_seed_path(seed_path: String, config: Option<Config>) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why don't we take EntropySourceConfig as input instead of 3 different functions:
- from_entropy_seed_path
- from_entropy_seed_bytes
- from_mnemonic
i thought that will simplify the api, are there any nuances ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, two reasons mainly:
- for one I'd like to keep
EntropySourceConfig
and the other*Config
objects internal in order to not require a whole other construction step until the user gets to a useableNode
. Currently, it's:
let builder = Builder::new();
let node = builder.build();
It will be:
let mnemonic = generate_entropy_mnemonic();
let builder = Builder::from_mnemonic(mnemonic, None, None);
let node = builder.build();
I don't like it to be:
let mnemonic = generate_entropy_mnemonic();
let entropy = EntropySourceConfig::from_mnemonic(mnemonic, None, None);
let builder = Builder::from_entropy(entropy);
let node = builder.build();
EntropySourceConfig
can't be exposed in bindings as-is, for example since UniFFI doesn't support fixed-length arrays. So we're doing preliminary checks on the length before actually 'accepting' the set values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I however now renamed the constructors to make them a bit shorter and easier to use:
Builder::from_seed_path
Builder::from_seed_bytes
Builder::from_mnemonic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer this, but i will leave it up to you.
EntropySourceConfig::from_mnemonic(mnemonic, None, None);
let builder = Builder::new(entropy);
Mainly because entropy source is just one of the required args we are introducing,
if in future we end up adding 1 more required arg, it will become cumbersome to support all combinations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this won't solve the issue your describing, as we'd still need to provide a Builder::from_config(config, entropy)
method or give Config
optionally to Builder::new
. So it just requires the user to (understand and) construct yet another opaque object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it does solve, unless there is some misunderstanding. Since
Builder::new(config, entropy)
is still better than
Builder::from_seed_path
Builder::from_seed_bytes
Builder::from_mnemonic
and in future if we add new required attribute, it will still be
Builder::new(config, entropy, new_attribute)
instead of multiple combinations of the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know your thoughts on this, lets close this.
If you are not convinced, feel free to move forward with current approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm considering to keep the current constructors but drop the Option<Config>
and rather offer a set_config
/with_config
setter for it. The only issue is that it suddenly introduces divergent behavior based on the ordering, i.e.,
Builder::from_mnemonic(mnemonic, None)
.set_config(config)
.set_network(network)
wouldn't necessary be the same as
Builder::from_mnemonic(mnemonic, None)
.set_network(network)
.set_config(config)
Do you have any opinion on that? In contrast, the current approach is uglier/clunkier but can't lead to unexpected behavior. And, for the reasons stated above, I'd really like to avoid to introduce a builder-for-the-builder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, divergent behavior based on the ordering would be very confusing.
e93a801
to
c21642b
Compare
Previously, we would default to auto-generating a `keys_seed` file in the storage path. While this was convenient, especially in tests, it's confusing for a lot of users and promotes questionable security practices, i.e., storing key material on disk. Here, we therefore require the user to manually specify an entropy source when constructing a `Builder`. Users migrating from previous versions can simply migrate by supplying the path to their `keys_seed` file via `Builder::from_entropy_seed_file`.
.. to make them a bit shorter/easier to use.
c21642b
to
e480ef7
Compare
Rebased to resolve minor conflict. |
We might still want to look into this eventually, but since we couldn't really conclude on the best way forward here, I'm closing this for now. |
Previously, we would default to auto-generating a
keys_seed
file in the storage path. While this was convenient, especially in tests, it's confusing for a lot of users and promotes questionable security practices, i.e., storing key material on disk.Here, we therefore require the user to manually specify an entropy source when constructing a
Builder
. Users migrating from previous versions can simply migrate by supplying the path to theirkeys_seed
file viaBuilder::from_entropy_seed_file
.