Skip to content
This repository has been archived by the owner on Aug 12, 2022. It is now read-only.

GetPublicKey (getHDNode) message with "script_type" #79

Merged
merged 4 commits into from
Nov 21, 2018

Conversation

szymonlesisz
Copy link
Contributor

@szymonlesisz szymonlesisz commented Nov 18, 2018

solves: #78
Make sending "script_type" possible from FW 1.7.2/2.0.10 and only in case when extended CoinInfo* object is sent as a parameter of getHDNode method.

*CoinInfo - an object used in mytrezor wallet. It has additional fields (like xpub_magic_segwit_p2sh and name)

Also related to trezor-graveyard/bitcoinjs-trezor#7

@@ -24,6 +25,11 @@ export type MessageResponse<T> = {

export type DefaultMessageResponse = MessageResponse<Object>;

/* eslint-disable no-redeclare */
declare function coinNetwork(coin: string | bitcoin.Network): bitcoin.Network;
declare function coinNetwork(coin: CoinInfo): CoinInfo;
Copy link
Contributor

Choose a reason for hiding this comment

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

ugh this is rather complicated :D I think I simplified all this in the PRs that also use the new bitcoinjs-trezor... but since we don't have this merged, I guess it's OK?

Copy link
Contributor

Choose a reason for hiding this comment

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

the problem is that we got all the three types here just because of backwards compatibility with some old version if mytrezor that doesn't exist anymore. We can very well get rid of all the different input types because we are the only ones using these libraries.... that's what I did in the waiting PRs

but anyway if this seems to be working now,,,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, i described this in comment below and also in bitcoinjs-trezor issue. Coin definition needs to be changed in the whole stack (starts from bitcoinjs-trezor thru trezor.js and ends in mytrezor) to avoid unnecessary conversion from "xpub" to "ypub" back and forth.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to do it as simple as i could, i'm aware it's just a workaround. But we don't want to "unscrew the whole tractor" right now :)

@karelbilek
Copy link
Contributor

I would love to simplify the whole thing more and threw away the three types of arguments, but this seems OK

I already did that in the new bitcoinjs-trezor PRs :/ but that will take a while to merge

@karelbilek
Copy link
Contributor

I haven't tested it that much, but if it doesn't work someone will figure it out rather quickly

also the cloning of the network ... :-( it needs to be done now unfortunately, again we can change the bitcoinjs later but not now

@karelbilek karelbilek merged commit 98db2f3 into master Nov 21, 2018
@karelbilek karelbilek deleted the feature/getpublickey-with-scripttype branch November 21, 2018 07:11
@karelbilek
Copy link
Contributor

in 6.19.0

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