-
Notifications
You must be signed in to change notification settings - Fork 82
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
frontend: label change #2788
base: master
Are you sure you want to change the base?
frontend: label change #2788
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Only one doubt about the first commit comment:
Possible false negative in case the user used another wallet and derived some change address behind the current change gap limit.
I think that in case the user received on a change address uncovered by our gap limit he will miss the whole utxo, not only the change
badge. wdyt?
backend/coins/btc/account.go
Outdated
@@ -873,12 +874,19 @@ func (account *Account) SpendableOutputs() []*SpendableOutput { | |||
panic(err) | |||
} | |||
for outPoint, txOut := range utxos { | |||
isChange := false | |||
for _, subacc := range account.subaccounts { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the same code as
bitbox-wallet-app/backend/coins/btc/account.go
Lines 722 to 726 in 81b3b25
for _, subacc := range account.subaccounts { | |
if subacc.changeAddresses.LookupByScriptHashHex(scriptHashHex) != nil { | |
return true | |
} | |
} |
would be nice to move that to a method on Account, with a unit test, so it can be reused
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it would be better to make the AccountAddress change aware and then use getAddress instead of IsChange
, here is how it could look like:
3b7fa12
I think it makes sense for AccounAdress to know whether it is change or not. Often times we'll want to get some address anyways, and even if we really only care for the IsChange
value the overhead (looking at the receiveAdresses) does not matter at all since a hash map is used for the lookup (O(1)) I think this would make the code easier to understand.
Please lmk what you think @benma
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(added three commits after the initial backend commit, can squash them in case we want to do it like this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please squash
Right, I don't know why I thought he would see the utxo. I'll amend the commit message, thanks. |
23bf35a
to
8da7d2c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
frontend LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
frontend LGTM
backend/coins/btc/account.go
Outdated
@@ -873,12 +874,19 @@ func (account *Account) SpendableOutputs() []*SpendableOutput { | |||
panic(err) | |||
} | |||
for outPoint, txOut := range utxos { | |||
isChange := false | |||
for _, subacc := range account.subaccounts { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please squash
9d97273
to
49b49c7
Compare
@NicolaLS is this ready for BE review or does it need other changes? |
@Beerosagos It's ready, I added the |
40ad717
to
bca4b7c
Compare
2132b6a
to
bfa4c9a
Compare
bfa4c9a
to
034eb2f
Compare
@Beerosagos adressed all change requests and @benma is okay with changing the tests package if I understood correctly: So should be ready for final review/merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK 🙏
please add a changelog entry 🙏 |
07fca71
to
e0f87e9
Compare
Add a boolean field to the response of the utxos enpoint (the getUTXOS handler) that tells us whether the utxo is to a change address of our wallet or not. Also: - Add IsChange method on btc.Account that returns a boolean. True if the provided scriptHashHex is in our change address chain and false if not. - Add a unit test for the IsChange method. - Fix btc account and coin unit test package names.
Label utxos that are to change addresses from our account (subaccounts respectively) as change to give the user more information about the utxo when selecting it in coin control.
Label utxos that are to change addresses from our account (subaccounts
respectively) as change to give the user more information about the utxo
when selecting it in coin control.