Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add segwit xpub to ku #384

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jusasiiv
Copy link
Contributor

This adds the option to get ku output also for Bitcoin BIP49 p2sh segwit and BIP84 segwit xPubs

Can be considered if the bipxx_as_string codes would be better in HierarchialKey

@@ -43,7 +43,7 @@ def get_entropy():
return entropy


def create_output(item, key, output_key_set, subkey_path=None):
def create_output(item, key, output_key_set, args, subkey_path=None):
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of making it an option, can you simply look to see if the network supports bip49 and/or bip84, and always print it if so?

This will break a lot of tests because it will add output. Go ahead and eyeball the diffs, and if they look okay, I can quickly repair them after I commit your fix (using this REPAIR_TESTS=1 py.test tests trick – it will rewrite the broken tests. If you can't figure out what I'm talking about, I'm happy to repair the tests, since it takes just a minute).

Copy link
Owner

@richardkiss richardkiss left a comment

Choose a reason for hiding this comment

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

(See inline comments)

…ided is a bip node. Remove args from cmd use
@jusasiiv
Copy link
Contributor Author

Hi,

This will break a lot of tests because it will add output. Go ahead and eyeball the diffs, and if they look okay, I can quickly repair them after I commit your fix

The diff is pretty messy, but I think it looks fine. If you could repair the tests that would be great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants