Skip to content
This repository has been archived by the owner on Apr 25, 2024. It is now read-only.

fix(wallet): adds utxo selection to manual spend on wallet interface #223

Merged

Conversation

humanumbrella
Copy link
Contributor

Description

Allow UTXO selection on manual spend in Wallet interface.

Does this PR introduce a breaking change?

  • Yes
  • No

Does this PR fix an open issue?

  • Yes
  • No

Fixes #54

Follow-up to #221 - adds in the necessary logic to handle utxo selection across multiple addresses

@bucko13
Copy link
Contributor

bucko13 commented Jun 9, 2021

Not sure how we want to handle since this isn't really blocking, but I found a small bug where the address level checkbox is deselected when you navigate back to edit a transaction

To reproduce:

  1. author a transaction. in my example I have one that's a partial spend from address and a full spend
  2. Go to send
  3. Go back to edit
  4. Notice the select boxes for the address are no longer selected

What could be blocking though is that I selected only 1/2 of the UTXOs from an address however when going back it looks like both are selected. If this is the case and can be confirmed, I'd say this is blocking since we don't want someone accidentally spending all the UTXOs when they only meant to spend part.

Screen Shot 2021-06-09 at 11 53 53 AM

Copy link
Contributor

@bucko13 bucko13 left a comment

Choose a reason for hiding this comment

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

Couple of small comments on the code as is, but I think we should address the issue when you go back to edit after clicking to sign

package.json Show resolved Hide resolved
src/components/ScriptExplorer/UTXOSet.jsx Outdated Show resolved Hide resolved
setInputs(inputsToSpend);
let totalInputsToSpend = inputsToSpend;

// The following is only relevant on the wallet interface
Copy link
Contributor

Choose a reason for hiding this comment

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

🤦

Copy link
Contributor

Choose a reason for hiding this comment

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

Even worse that we have to do this multiple times in the same method

if (numLocalInputsToSpend === 0) {
setSpendCheckbox(false);
} else if (
numLocalInputsToSpend >= 1 &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think you need the >=1 here

Suggested change
numLocalInputsToSpend >= 1 &&
numLocalInputsToSpend &&

@waldenraines
Copy link
Contributor

waldenraines commented Jun 9, 2021

Not sure how we want to handle since this isn't really blocking, but I found a small bug.

To reproduce:

  1. author a transaction. in my example I have one that's a partial spend from address and a full spend
  2. Go to send
  3. Go back to edit
  4. Notice the select boxes for the address are no longer selected

What could be blocking though is that I selected only 1/2 of the UTXOs from an address however when going back it looks like both are selected. If this is the case and can be confirmed, I'd say this is blocking since we don't want someone accidentally spending all the UTXOs when they only meant to spend part.

This is also the case if you navigate away from the send tab (by going to the addresses or receive tab, for example). When you come back you end up in the same state as shown in the screenshot.

@humanumbrella
Copy link
Contributor Author

I updated things to make sure that it at least shows that the address is now selected (full checkbox, not indeterminate or missing) when you navigate away and/or click edit transaction.

It will require more effort given the current design to actually save which inputs were selected. It would be easier with a larger refactor of how the application is structured.

@humanumbrella humanumbrella requested a review from bucko13 July 13, 2021 21:04
@bucko13
Copy link
Contributor

bucko13 commented Jul 27, 2021

Don't know if this needs to be a blocker, but this was a weird little bug. If there's only one UTXO in an address, selecting it in the utxo selector doesn't work:

utxo-select.mp4

@bucko13
Copy link
Contributor

bucko13 commented Jul 27, 2021

Tested happy path and the reset of the utxos if going back to edit and it all seems to work other than the bug identified above. If that is easily fixable, then it'd be good if we can get that in (maybe just disabling that view if only one utxo as a quick fix? 🤷‍♂️ ). Otherwise it LGTM!

@humanumbrella humanumbrella requested review from bucko13 and waldenraines and removed request for waldenraines and bucko13 July 27, 2021 22:06
Copy link
Contributor

@bucko13 bucko13 left a comment

Choose a reason for hiding this comment

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

Confirmed it works! 👏 🎉 :shipit:

@waldenraines waldenraines merged commit ef4ab05 into unchained-capital:master Jul 28, 2021
@humanumbrella humanumbrella deleted the adds-wallet-coin-selection branch November 21, 2021 02:29
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.

Spend specific UTXO rather than all at once
3 participants