Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@gilescope
Copy link
Contributor

@gilescope gilescope commented Sep 3, 2021

app_crypto_pair::from_seed(_: &Seed)

to

app_crypto_pair::from_seed(_: Seed)

from_seed shouldn't be used so this doesn't really matter, but I think it's nicer if the function takes ownership of the seed.
(This PR looks even nicer once zebra lands as you can hand the ownership over to it)

(There's already a from_seed_slice if people really want to retain ownership)

@gilescope gilescope added A0-please_review Pull request needs code review. B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Sep 3, 2021
@bkchr
Copy link
Member

bkchr commented Sep 4, 2021

If from_seed doesn't require ownership of the seed, it shouldn't take it.

@bkchr bkchr closed this Sep 4, 2021
@gilescope
Copy link
Contributor Author

Doesn't it feel more secure that it takes ownership of the seed rather than the seed continuing to float about memory until such time as it gets dropped? We could ensure at that point that the seed's memory was zeroed once we'd made use of it. (And if people did want to keep the seed we'd force them to clone it - this feels safer by design to me.)

@bkchr
Copy link
Member

bkchr commented Sep 5, 2021

(And if people did want to keep the seed we'd force them to clone it - this feels safer by design to me.)

Why does this feels safer that the user needs to clone it? In the current design the user could ensure that the type is wrapped in a Zeroize wrapper. If the user would pass it, it would still not be automatically zeroed.

@louismerlin louismerlin removed the D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. label Oct 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants