Feat/classic hd derivation path #487
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Date: 2022-02-23 07:38:20-08:00 Signed-off-by: meows <[email protected]>
Date: 2022-07-06 07:42:26-07:00 Signed-off-by: meows <[email protected]>
…ults A setter SetCoinTypeConfiguration is defined to configure global vars and defaults given some coin type. The default is configured for Ethereum mainnet via an init() function call in the hd.go file. Configuration for testnet and Ethereum Classic paths are supported via flags.go which uses this setter when the flag --usb is used in conjunction with a supported chain flag, eg. --ropsten (any testnet) or --classic. Date: 2022-07-06 10:28:46-07:00 Signed-off-by: meows <[email protected]>
|
Great work @meowsbits. I want to give it a test with Ledger before approving it. |
This is intended to help avoid merge conflict issues in the future. Unfortunately I have to modify the TestHDPathParsing test function, adding the SetCoinTypeConfiguration call to (re-)establish the default values for derivation paths. This is because - the configurations are defined as globals, - and we redefine them for the core-geth implementation Date: 2022-07-06 14:20:13-07:00 Signed-off-by: meows <[email protected]>
Date: 2022-07-06 14:21:58-07:00 Signed-off-by: meows <[email protected]>
|
@meowsbits just tested it with Ledger and it works great. I am wondering * if it makes sense to use the ETHClassic Slip (0x3d) for its testnets (Mordor & Kotti). That way, UX will not be confusing. I wanted to try using the Ledger's ETC app (accounts) on Mordor and it wasn't working. I am totally fine merging this PR as it is, as it is inline with the spec.
Nice work. |
|
What about a flag that would override (or "define with -force"?) the value?
|
Nice solution. I vote towards |
This flag can be used in conjunction with --usb to define any path id for the global accounts package configuration for HD wallets. Date: 2022-07-13 10:59:15-07:00 Signed-off-by: meows <[email protected]>
Before this patch, the HD path was configured even if the flag was set to a falsey value. This fixes it so that if --usb is used to turn itself off, then the path configuration cases are skipped. Date: 2022-07-13 11:02:52-07:00 Signed-off-by: meows <[email protected]>
|
Last two commits add |
|
Works great. Let's merge it @meowsbits. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR builds from and replaces #452 (thanks @ziogaschr) which provided the pattern for configurable HD derivation path support. This branch has been rebased on master since then.
When
--usbis used in conjunction with--classicor any testnet value (eg.--sepolia), theaccountspackage defaults will be configured accordingly for the HD derivation path logic and defaults.Comments are left using Ethereum Classic values as examples.