stellar-hd-wallet #78
Conversation
| "github.com/tyler-smith/go-bip39" | ||
| ) | ||
|
|
||
| var wordsCount uint32 |
There was a problem hiding this comment.
I'm curious why we should support customizable entropy. Seems complicated to use: we don't document the various valid values and users probably won't understand the implications of using a smaller number of words.
There was a problem hiding this comment.
Good point. Switched to 256-bit entropy.
| @@ -0,0 +1,62 @@ | |||
| package derive | |||
There was a problem hiding this comment.
We should probably move this file underneath internal to prevent external imports.
bartekn
force-pushed
the
stellar-hd-wallet
branch
from
f5304c6 to
775ef86
Compare
bartekn
force-pushed
the
stellar-hd-wallet
branch
from
3b4d723 to
723bf59
Compare
|
OK, I think this is ready for review, merge and release (version |
| readString() | ||
| } | ||
|
|
||
| println("WARNING! Store the words above in a save place!") |
bartekn
force-pushed
the
stellar-hd-wallet
branch
from
7b6b8c4 to
3727837
Compare
| Words string | ||
| Passphrase string | ||
| Error string | ||
| Output string |
There was a problem hiding this comment.
it's hard to tell expected output vs input values without reading the test cases. Could use want as a prefix for expected values, which is go convention.
| }, | ||
| } | ||
|
|
||
| for _, test := range tests { |
There was a problem hiding this comment.
should use subtests, t.Run -- test output is much easier to read and work with, mentioned this pattern in previous reviews as well
| ) | ||
|
|
||
| var reader = bufio.NewReader(os.Stdin) | ||
| var out io.Writer = os.Stdout |
There was a problem hiding this comment.
could put these vars on a struct and these functions can hang off that struct. then when testing we can inject the inputs to the struct to be buffer inputs (prefilled) and outputs so testing is easier. while in production we can pass in the standard input and output buffers
These changes would also make this file a more generally usable utility outside of just the stellar-hd-wallet tool and we could move it to /support
There was a problem hiding this comment.
Yes, I think we can do this in a separate PR if there is a need for such struct.
However, this doesn't solve the issue - we would still need to have a global variable (struct object this time) and overwrite it's fields. I think the long term fix would be something like this:
- Create the
Appstruct that will containRunEmethods for each command. - Have the
IOstruct object inAppstruct.
Then we can create App object an overwrite IO in a cleaner way. Let's do it in the next version.
There was a problem hiding this comment.
yes that's right, we should use dependency injection for this IO struct rather than using a global var. ok to leave this for the next PR.
| words := strings.Split(test.Words, " ") | ||
| input := fmt.Sprintf("%d\n%s\n%s\n", len(words), strings.Join(words, "\n"), test.Passphrase) | ||
|
|
||
| // Global variables, AFAIK there is no elegant way to pass it to cobra.Command |
There was a problem hiding this comment.
see my comment below in io.go about using a struct to avoid the global vars. You may need to create a common function that is called by the cobra command, and test that function rather than the cobra command to achieve the dependency injection needed
|
thanks for the changes, LGTM! 👍 |
Implements BIP-39 and BIP-44 for Stellar keys derivation.
TODO:
NewSeedWithErrorCheckingconst EntropyByteswith comment instead of using256.