Skip to content
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

Allow user-provided ratchet seed and inumber #91

Merged
merged 6 commits into from
Dec 2, 2022

Conversation

appcypher
Copy link
Member

@appcypher appcypher commented Nov 24, 2022

Summary

In some cases, we want to be able to deterministically create a directory with seeds generated at userland, for example, from private keys. There is a hacky solution to this by passing a carefully designed rng, but that is likely to break in future versions.

This PR adds the API for accepting user-specified ratchet seed and inumber that solves the problem outlined above.

This PR implements the following features

  • Support user-specified ratchet seed and inumber
  • Expose corresponding wasm/js API
  • Bump version

Test plan (required)

  • Testing

    rs-wnfs test --all

Closing issues

Fixes #86

@appcypher appcypher requested a review from a team as a code owner November 24, 2022 11:59
@codecov
Copy link

codecov bot commented Nov 24, 2022

Codecov Report

Merging #91 (4f346ec) into main (9d3b5c2) will increase coverage by 1.35%.
The diff coverage is 81.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #91      +/-   ##
==========================================
+ Coverage   67.89%   69.25%   +1.35%     
==========================================
  Files          26       27       +1     
  Lines        2003     2010       +7     
  Branches      482      470      -12     
==========================================
+ Hits         1360     1392      +32     
+ Misses        252      239      -13     
+ Partials      391      379      -12     
Impacted Files Coverage Δ
wnfs/examples/private.rs 0.00% <0.00%> (ø)
wnfs/src/common/utils.rs 70.00% <ø> (ø)
wnfs/src/private/forest.rs 59.15% <ø> (ø)
wnfs/src/private/hamt/node.rs 64.45% <ø> (+0.38%) ⬆️
wnfs/src/private/privateref.rs 68.96% <68.96%> (ø)
wnfs/src/private/node.rs 77.90% <93.33%> (+8.99%) ⬆️
wnfs/src/private/directory.rs 77.34% <100.00%> (+1.92%) ⬆️
wnfs/src/private/file.rs 80.67% <100.00%> (+1.50%) ⬆️
wnfs/src/private/previous.rs 72.98% <100.00%> (+0.57%) ⬆️
wnfs/src/public/directory.rs 71.17% <0.00%> (-0.21%) ⬇️
... and 4 more

Copy link
Member

@matheus23 matheus23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Should we also add a matching function PrivateRef::from_seed(parent_bare_name: Namefilter, ratchet_seed: HashOutput, inumber: HashOutput) -> PrivateRef?

- Remove Result from `PrivateNodeHeader::get_private_ref`
@matheus23
Copy link
Member

What's the reasoning on using with_* naming?
I actually find it slightly confusing and think from_* makes more sense, because the parameters to the from_seed and from_ratchet_key functions deterministically define the PrivateRef. Every property in the PrivateRef depends on the arguments in these functions.
with seems to indicate there's 'something else' to it.

That said, I may just not know the normal rust conventions yet! 😅

@appcypher
Copy link
Member Author

I don't mind either really but generally with_* seems to be preferred if you constructing an object out of other thing(s), and from if you are going to implement the From<T> trait. In this case that means implementing a From<(Namefilter, HashOutput, ...)> which doesn't feel nice. So I went with with_*. But I agree with you from_* sounds better.

Maybe this is sth @zeeshanlakhani can help out with.

@matheus23
Copy link
Member

Ah and @appcypher can you add a unit test that creates a PrivateDirectory::with_seed, stores it in a PrivateForest, then loads it again via a PrivateRef::with_seed with the same parameters? 🙏

Copy link
Member

@matheus23 matheus23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

@appcypher appcypher merged commit f6cadeb into main Dec 2, 2022
@appcypher appcypher deleted the appcypher/expose-seed branch December 2, 2022 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

exposing a constructor for PrivateRef
2 participants