Make getDerivedHDAddress return address, not key #218
Conversation
|
So I think we should leave the old one unchanged (due to not wanting breaking changes that other API's depend on) and duplicate that function where you made those changes and add a new suffix, e.g. getDerivedHDAddressAsP2PKH. I think the reason for this confusion is lack of specificity re naming conventions, and referencing from sources like Learn me a bitcoin wherein the dgub 112 char key is mentioned as an address and not a p2pkh legacy address.
|
chromatic
force-pushed
the
getderivedhdaddress-returns-address
branch
from
6dddb2b
to
bdffe31
Compare
|
That makes sense and I'm content to rework (or let someone else rework) this PR for that approach. If that's the approach you prefer, I do think clarifying the documentation of both functions is valuable. |
|
Yeah if you wouldn't mind reworking those functions while leaving getDerivedHDAddress the same, it would be much appreciated. |
chromatic
force-pushed
the
getderivedhdaddress-returns-address
branch
from
bdffe31
to
b403f69
Compare
|
I've rewritten my commit to go in that direction. This has tests but not documentation in |
This derives a key from a given public or private key and returns the derived key in P2PKH form.
chromatic
force-pushed
the
getderivedhdaddress-returns-address
branch
from
b403f69
to
e84242c
Compare
|
Looks good. Will do a formal review later this afternoon. |
| * @param account The account that the derived address would belong to. | ||
| * @param ischange Boolean value representing either a change or receiving address. | ||
| * @param addressindex The index of the receiving/change address per account. | ||
| * @param outp2pkh The derived address in P2PSH form. |
| res = getDerivedHDAddressAsP2PKH(masterkey_main_ext, 0, true, 0, extout); | ||
| u_assert_int_eq(res, true); | ||
| u_assert_str_eq(extout, "D91jVi3CVGhRmyt83fhMdL4UJWtDuiTZET"); | ||
| res = getDerivedHDAddressAsP2PKH(masterkey_main_ext, 0, false, 0, extout); |
There was a problem hiding this comment.
nit: L144-148 and L156-161 are duplicate test cases
|
This looks good. I agree with the other comments and on updating |
There was a problem hiding this comment.
Good change, logical - thanks for that input @chromatic . Appreciated.
Addresses GH #217.
I proved the first test correct with a third-party derivation library; looking for feedback before I go further.