Skip to content

relative path key derivation#1

Open
alonmuroch wants to merge 1 commit intowealdtech:masterfrom
alonmuroch:feature/relative_derivation
Open

relative path key derivation#1
alonmuroch wants to merge 1 commit intowealdtech:masterfrom
alonmuroch:feature/relative_derivation

Conversation

@alonmuroch
Copy link

Hi Jim,
I've made a small change to enable relative key derivation.
The motivation behind it is being able to derive keys without needing the seed, for example:
Say I have the key key: m/12381/3600 I can now derive from it my validator keys relatively to key and not the seed.

@codecov-commenter
Copy link

codecov-commenter commented Jun 21, 2020

Codecov Report

Merging #1 into master will decrease coverage by 3.86%.
The diff coverage is 73.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #1      +/-   ##
==========================================
- Coverage   83.17%   79.31%   -3.87%     
==========================================
  Files           2        2              
  Lines         107      116       +9     
==========================================
+ Hits           89       92       +3     
- Misses          9       12       +3     
- Partials        9       12       +3     
Impacted Files Coverage Δ
crypto.go 76.23% <73.52%> (-4.20%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0a4894...726f409. Read the comment docs.

@wealdtech
Copy link
Collaborator

Thank you for this pr. A couple of items:

  • the names of the tests seem to be inverted: TestPrivateKeyForRelativePath() tests PrivateKeyFromSeedAndPath and vice versa
  • some of the codepaths in PrivateKeyForRelativePath() aren't tested; please could tests be added to cover these cases?

@alonmuroch
Copy link
Author

Hi,
I've double checked and the test names seem to be correct. Am I missing something?
I think what is not tested was the private methods to test path validity, I could easily add tests but the test file package name is different (with _test suffix). You mind if I change the package name by removing the suffix?

@wealdtech
Copy link
Collaborator

Sorry, didn't read the test properly. The TestPrivateKeyForRelativePath() test calls PrivateKeyFromSeedAndPath() and then calls PrivateKeyForRelativePath(). Would it be possible to change this test so that it starts with a master key rather than deriving it, so it's a proper unit test for the PrivateKeyForRelativePath() function?

Regarding coverage: if you run go test --coverprofile=coverage.out && go tool cover --html=coverage.out -o coverage.html and check out the generated coverage.html file you'll see there are a few error conditions in PrivateKeyForRelativePath() that are not checked.

(The private functions seem to be covered okay; in general, testing private functions is done with a separate file using the same package as the functions to be tested, and in a file x_internal_test.go)

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.

3 participants