Skip to content
This repository has been archived by the owner on Nov 30, 2021. It is now read-only.

crypto: fix Bip44 derivation path #577

Merged
merged 17 commits into from
Oct 16, 2020
Merged

Conversation

araskachoi
Copy link
Contributor

Closes: #XXX

Description


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@araskachoi araskachoi changed the title change derivationpath to geth's const change Bip44 derivation path Oct 14, 2020
@araskachoi araskachoi changed the title change Bip44 derivation path edit Bip44 derivation path Oct 14, 2020
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

Thanks! Minor comments. Also pending a bugfix changelog entry 👍

types/config.go Outdated Show resolved Hide resolved
types/config.go Outdated Show resolved Hide resolved
crypto/hd/algorithm_test.go Outdated Show resolved Hide resolved

mnemonic := "picnic rent average infant boat squirrel federal assault mercy purity very motor fossil wheel verify upset box fresh horse vivid copy predict square regret"

bz, err := DeriveSecp256k1(mnemonic, keys.DefaultBIP39Passphrase, "m/44'/60'/0'/0/0")
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a test case that compares the addresses generated with and without the m/ prefix to be not equal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added the test

araskachoi and others added 5 commits October 15, 2020 08:51
Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
@araskachoi
Copy link
Contributor Author

araskachoi commented Oct 15, 2020

Thanks! Minor comments. Also pending a bugfix changelog entry 👍

@fedekunze I think its g2g! If you want to give a last look and appove!

@fedekunze fedekunze changed the title edit Bip44 derivation path crypto: fix Bip44 derivation path Oct 16, 2020
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

ACK

@fedekunze fedekunze merged commit 0870a27 into development Oct 16, 2020
@fedekunze fedekunze deleted the araska/fixbip44path branch October 16, 2020 13:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants