backend/btc/address: be explicit about chainIndex/addressIndex #3325
Conversation
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
benma
force-pushed
the
address-refactor
branch
2 times, most recently
from
5d93dbb to
f42c977
Compare
bznein
reviewed
May 6, 2025
| @@ -47,22 +47,28 @@ type AccountAddress struct { | |||
| } | |||
|
|
|||
| // NewAccountAddress creates a new account address. | |||
| // The chainIndex is 0 for receive and 1 for change. | |||
There was a problem hiding this comment.
It's not clear what "chainIndex" is here; it's not one of the argument (though I assume you refer to the one accessed via derivation.SimpleChainIndex()? ) maybe we can make it clearer
There was a problem hiding this comment.
Ah it's a leftover comment from before I introduced the Derivation struct. Removed.
The relative keypath always has two elements, the first element indicating change and the second indicating the address index. A relative keypath of arbitrary length is harder to handle. E.g. it is not compatible with output descriptors with multipath indices (`xpub/<0;1>/*`), where also two elements are required to derive an address (the multipath index selecting between `<X;Y>` and the address index). The unit tests change because the GetAddress test function used an empty relative path instead of a chainIndex and addressIndex, which now was forced to change with the stricter types. I considered keeping `ChainIndex uint32` instead of `Change bool`, because in multipaths, one could have more than two, e.g. `xpub/<X;Y;Z>/*`, but I prefer the stricter type and it's not likely we will ever need more than two (receive & change).
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.
The relative keypath always has two elements, the first element indicating change and the second indicating the address index.
A relative keypath of arbitrary length is harder to handle. E.g. it is not compatible with output descriptors with multipath indices (
xpub/<0;1>/*), where also two elements are required to derive an address (the multipath index selecting between<X;Y>and the address index).The unit tests change because the GetAddress test function used an empty relative path instead of a chainIndex and addressIndex, which now was forced to change with the stricter types.
I considered keeping
ChainIndex uint32instead ofChange bool,because in multipaths, one could have more than two,
e.g.
xpub/<X;Y;Z>/*, but I prefer the stricter type and it's notlikely we will ever need more than two (receive & change).