Descriptor support for displayaddress#117
Merged
achow101 merged 3 commits intoFeb 15, 2019
Merged
Conversation
Sjors
force-pushed
the
2019/02/displayaddress_descriptor
branch
from
c161d8f to
d29f0ae
Compare
Sjors
force-pushed
the
2019/02/displayaddress_descriptor
branch
from
d29f0ae to
3b6e3bd
Compare
achow101
reviewed
Feb 4, 2019
Sjors
force-pushed
the
2019/02/displayaddress_descriptor
branch
from
3b6e3bd to
e22dc4c
Compare
Member
|
Needs rebase |
Sjors
force-pushed
the
2019/02/displayaddress_descriptor
branch
from
e22dc4c to
dfaf436
Compare
Member
Author
|
Rebased |
achow101
reviewed
Feb 10, 2019
Member
achow101
left a comment
achow101
left a comment
There was a problem hiding this comment.
What happens if you do --sh_wpkh --desc wpkh(...)?
| process_commands(self.dev_args + ['displayaddress', '--path', 'm/44h/1h/0h/0/0']) | ||
| process_commands(self.dev_args + ['displayaddress', '--sh_wpkh', '--path', 'm/49h/1h/0h/0/0']) | ||
| process_commands(self.dev_args + ['displayaddress', '--wpkh', '--path', 'm/84h/1h/0h/0/0']) | ||
| process_commands(self.dev_args + ['displayaddress', '--desc', 'wpkh(m/84h/1h/0h/0/0)']) |
Member
There was a problem hiding this comment.
I don't think that's a valid descriptor.
| if origin_match == None: | ||
| return None | ||
|
|
||
| return cls(origin_fingerprint, origin_path, base_key, path_suffix, testnet, sh_wpkh, wpkh) |
Member
There was a problem hiding this comment.
I got an error which points here: UnboundLocalError: local variable 'path_suffix' referenced before assignment
path_suffix won't be set if the else branch above is taken.
Sjors
force-pushed
the
2019/02/displayaddress_descriptor
branch
5 times, most recently
from
80423aa to
36ac3e0
Compare
Member
Author
|
Rebased. I added a check to prevent |
achow101
reviewed
Feb 14, 2019
| xpub = client.get_pubkey_at_path(descriptor.m_path_base)['xpub'] | ||
| if descriptor.base_key != xpub and descriptor.base_key != xpub_to_pub_hex(xpub): | ||
| return {'error':'Key in descriptor does not match device: ' + desc,'code':BAD_ARGUMENT} | ||
| client.display_address(descriptor.m_path, descriptor.sh_wpkh, descriptor.wpkh) |
Member
There was a problem hiding this comment.
This needs to be a return statement.
Member
Author
There was a problem hiding this comment.
I think we need to add a linter to Travis :-) Though I guess it's hard to tell because Python doesn't enforce return values.
Sjors
force-pushed
the
2019/02/displayaddress_descriptor
branch
from
36ac3e0 to
0c1f9f6
Compare
achow101
reviewed
Feb 14, 2019
Member
achow101
left a comment
achow101
left a comment
There was a problem hiding this comment.
Testing everything else, I think that's the last thing before I merge this
| if path is not None: | ||
| if sh_wpkh == True and wpkh == True: | ||
| return {'error':'Both `--wpkh` and `--sh_wpkh` can not be selected at the same time.','code':BAD_ARGUMENT} | ||
| client.display_address(path, sh_wpkh, wpkh) |
Sjors
force-pushed
the
2019/02/displayaddress_descriptor
branch
from
0c1f9f6 to
a71658c
Compare
Member
|
ACK a71658c |
achow101
added a commit
that referenced
this pull request
Feb 15, 2019
a71658c displayaddress: descriptor support, make --path a named argument (Sjors Provoost) 163f926 Add descriptor class (Sjors Provoost) 6fc1524 HardwareWalletClient: store fingerprint (Sjors Provoost) Pull request description: Adds a `Descriptor` class, which currently handles some of the descriptor functionality. Changes `displayaddress` to allow a descriptor argument. This means `--path` and `--desc` are now named arguments. When using a descriptor `displayaddress` performs additional checks to make sure the fingerprint and xpub match. To enable the fingerprint check, I'm storing the fingerprint on `HardwareWalletClient` instances. Tree-SHA512: f149481203c340b8e5347f42fd2924f1b2095197d6c25f39f94acfb11f430881412cce48c510260db4e773b662e8b2a0a747c413ba7fadfc6eb78a85f1ec4838
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Adds a
Descriptorclass, which currently handles some of the descriptor functionality.Changes
displayaddressto allow a descriptor argument. This means--pathand--descare now named arguments.When using a descriptor
displayaddressperforms additional checks to make sure the fingerprint and xpub match. To enable the fingerprint check, I'm storing the fingerprint onHardwareWalletClientinstances.