search

tyler-smith / go-bip39

The BIP39 library for Go.
MIT License
555 stars 196 forks source link

merge rivine fork #13

Closed GlenDC closed 6 years ago

GlenDC commented 6 years ago

In this PR you'll find all useful changes that we made in the Rivine fork, applied to your go-bip39 library as it is in master.

As discussed in https://github.com/tyler-smith/go-bip39/issues/7#issuecomment-404312615.

GlenDC commented 6 years ago

Once this PR has been merged, I'll delete our fork and use your library directly for the Rivine blockchain project.

Furthermore I would also be more than happy to help maintain this library where needed, given that we rely on it for a couple of projects, we would help ourselves as well, by helping out as a maintainer. If you agree, feel free to give me the required rights to help you do so.

tyler-smith commented 6 years ago

Merged ce3d5bb, 59d8316 , and 4028094.

4028094 is fine but has conflicts with b995a0b.

b995a0b I don't understand. It appears to be providing a function (EntropyFromMnemonic) which is exactly like the already existent MnemonicToByteArray function. Is there a reason for both? Do you think one naming convention is better than the other or do they do something different that I'm missing?

GlenDC commented 6 years ago

1f6c1d8 provides the EntropyFromMnemonic function, not b995a0b. Unless I misunderstand something in https://github.com/tyler-smith/go-bip39/pull/13#issuecomment-405175238, I think you made some mistakes in the commit ID referencing.

Also as far as I understand, MnemonicToByteArray creates a new seed from a mnemonic, while EntropyFromMnemonic (the one I add in 1f6c1d8) recovers the old seed used, which was used to generate the given mnemonic.

You'll see the difference in effect immediately if you replace https://github.com/tyler-smith/go-bip39/pull/13/commits/1f6c1d80783848164ee587482d4e62ff86b53999#diff-8308e211b6db3deb5bcefc410e02f7f9R437 with outEntropy, err := MnemonicToByteArray(mnemonic). You'll notice the unit test will fail, as you generated a new seed (entropy), rather than recovering the old (original) one.

Do you think you couldl live with just having MnemonicToByteArray until we can get a V2 API going?

Nope, I need this now, as I need to be able to recover a seed from a mnemonic, otherwise I'll have to keep using our fork.

tyler-smith commented 6 years ago

1f6c1d8 provides the EntropyFromMnemonic function, not b995a0b. Unless I misunderstand something in #13 (comment), I think you made some mistakes in the commit ID referencing.

You're correct. I copy/pasted the wrong hash.

I see what this is doing, and it's very inline with where I want to go. Eventually I want to remove MnemonicToByteArray and have the interface just be conversions between entropy and mnemonics and the seed generation function. In v2, this function may be bip39.Unmarshal("abandon ...") ([]byte, error) along a functionbip39.Marshal(entropy []bytes) error

Thanks for the addition.