Configurable Bip44 CoinType & HdPath for SDK users

[yangyanqing]

• Targeted PR against correct branch (see CONTRIBUTING.md)

close: #4144

• Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
• Wrote tests
• Updated relevant documentation (docs/)
• Added a relevant changelog entry: clog add [section] [stanza] [message]
• rereviewed Files changed in the github PR explorer

---

For Admin Use:

• Added appropriate labels to PR (ex. wip, ready-for-review, docs)
• Reviewers Assigned
• Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

[codecov]

Codecov Report

Merging #4300 into master will decrease coverage by 0.02%.
The diff coverage is 52.63%.

@@            Coverage Diff             @@
##           master    #4300      +/-   ##
==========================================
- Coverage   59.12%   59.09%   -0.03%     
==========================================
  Files         217      217              
  Lines       14605    14618      +13     
==========================================
+ Hits         8635     8639       +4     
- Misses       5332     5341       +9     
  Partials      638      638

[codecov]

Codecov Report

Merging #4300 into master will decrease coverage by 0.03%.
The diff coverage is 47.36%.

@@            Coverage Diff             @@
##           master    #4300      +/-   ##
==========================================
- Coverage   54.58%   54.54%   -0.04%     
==========================================
  Files         248      248              
  Lines       15967    15980      +13     
==========================================
+ Hits         8716     8717       +1     
- Misses       6617     6629      +12     
  Partials      634      634

[alessio]

Any thoughts on this @jleni?

[alexanderbez]

Generally, I believe it is advisable to support varying BIP32 HD paths and not have it hard-coded in a SDK. Left some general feedback.

[jleni]

Any thoughts on this @jleni?

This will break Ledger app compatibility. Ledger enforces BIP44 registrations and apps are signed and restricted to paths that start with 44'/118'. Atoms are registered at 118: https://github.com/satoshilabs/slips/blob/master/slip-0044.md

If derivation paths do not start with 44'/118' any cryptography operation will be rejected by the secure element. A new app will need to be published for a different coinType

[yangyanqing]

This will break Ledger app compatibility. Ledger enforces BIP44 registrations and apps are signed and restricted to paths that start with 44'/118'. Atoms are registered at 118: https://github.com/satoshilabs/slips/blob/master/slip-0044.md

Any project based on cosmos-sdk should publish their own ledger app, if they want to singed by ledger.

If derivation paths do not start with 44'/118' any cryptography operation will be rejected by the secure element. A new app will need to be published for a different coinType

Yes

@jleni

[jackzampolin]

What is blocking merge here?

[alexanderbez]

ACK

[alexanderbez]

Sorry for the delay on this. This get an ACK from me. It needs to have merge conflicts resolved (sorry @yangyanqing).

[jleni]

This will break Ledger app compatibility. Ledger enforces BIP44 registrations and apps are signed and restricted to paths that start with 44'/118'. Atoms are registered at 118: https://github.com/satoshilabs/slips/blob/master/slip-0044.md

Any project based on cosmos-sdk should publish their own ledger app, if they want to singed by ledger.

If derivation paths do not start with 44'/118' any cryptography operation will be rejected by the secure element. A new app will need to be published for a different coinType

Yes

A few comments:
1- I think these restrictions/assumptions should be included in the docs or at least the changelog.
Ledger support in the SDK will only work with 44'/118'/.. and the Cosmos App (CLA=0x55). This is because Ledger requires a SLIP-0044 registrating and cryptographically restricts apps to certain paths.

2- Also should this change trigger an issue in gaia in case we need updates there?
Assuming custom bip44 paths are the way forward, what is the purpose of a default value in the SDK if every project is supposed to specify their own hrp/bip44 path? Maybe gaia should behave as any SDK user and set 118/cosmos via the API?

3- About HSM integration, should part of this functionality move to gaia? If people plan to develop their own apps for each specific case, this will definitely require some rework. On the other hand,

These points are probably part of a longer discussion and not a reason to block the PR at all, but I think it would be something to take into account in future work.

[jackzampolin]

Interesting question, do we even need paths other than 44'/118'/*'/*/*? Wouldn't it be nice for users to be able to use the same Cosmos app all throughout the Cosmos?

Ignoring ☝️, I do think we need the docs updates in this PR to point users at the correct way to enable ledger integration for their apps if we are removing it.

[yangyanqing]

All conflicts were resolved. @alexanderbez @jackzampolin @jleni

BTW: Your new avatar is so cool ! 👍 @alexanderbez

[jleni]

Any news about some new tests that check this new functionality?

[alexanderbez]

Thanks @yangyanqing :)

[jackzampolin]

Do we have a tracking issue for the docs update here?

[alessio]

I'm afraid no - @yangyanqing feel like writing some little docs about this change?

[yangyanqing]

Which doc should be add to for this change ? @jackzampolin @alessio

[alessio]

Let's start with godoc comments:

https://github.com/cosmos/cosmos-sdk/pull/4300/files#diff-239369c90dd98b7585b77a44eb24a9a5R128

coint type is no longer static, to start with.

I'd love to see some documentation written (somehow, somewhere) with the users of this change (namely developers) in mind.

[yangyanqing]

Add comments in #4474 .