Skip to content
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

773 UI Show Considered UTXOs before performing transaction #788

Closed

Conversation

amitx13
Copy link
Contributor

@amitx13 amitx13 commented Jun 27, 2024

This pull request builds upon the changes introduced in #771 765-ui-implement-utxo-selection-modal. Please merge #771 before reviewing this PR.

This PR fixes #773

Changes:

  1. User are now able to review their selected UTXOs
  2. Successful implementation of
    image

@amitx13 amitx13 self-assigned this Jun 27, 2024
@amitx13 amitx13 linked an issue Jun 27, 2024 that may be closed by this pull request
editwentyone
editwentyone previously approved these changes Jul 1, 2024
0xSaksham
0xSaksham previously approved these changes Jul 8, 2024
Copy link
Contributor

@0xSaksham 0xSaksham left a comment

Choose a reason for hiding this comment

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

Works for me!

@theborakompanioni
Copy link
Collaborator

theborakompanioni commented Jul 8, 2024

As far as I am aware, showing the actual used UTXOs is only possible once JoinMarket-Org/joinmarket-clientserver#1713 is merged and release, right?

I am not sure this is–in its current form–more confusing to the user.

Edit: For non-sweep transactions.

@amitx13
Copy link
Contributor Author

amitx13 commented Jul 8, 2024

Yes, you are right. Showing the actual used UTXOs is only possible once JoinMarket-Org/joinmarket-clientserver#1713 is merged and released. I totally agree with your opinion because, as you mentioned earlier, the UTXOs selected in the quick freeze/unfreeze are not actually selected for the transaction; only the JAR(mixdepth) is selected. After the successful merge of #1713, the transaction will be based on the selected UTXOs. So, showing the selected UTXOs after the successful merge of #1713 will make more sense.
So @theborakompanioni If you have a moment, could you please review #1713 and check why the build keeps failing? That's the only thing I'm stuck at, and sorry for being so annoying—I know you're busy. And thank you for looking after all off us all the time

amitx13 added 5 commits July 8, 2024 19:11
- Fixed sweep amount
- Refactored code (naming)
- Removed unnecessary warnings
- Added minor improvements
- Fixed a few bugs
@theborakompanioni
Copy link
Collaborator

Yes, you are right. Showing the actual used UTXOs is only possible once JoinMarket-Org/joinmarket-clientserver#1713 is merged and released. I totally agree with your opinion because, as you mentioned earlier, the UTXOs selected in the quick freeze/unfreeze are not actually selected for the transaction;

👍

So we can still include this component and display the UTXOs to the user if it is a sweep transaction!

So, showing the selected UTXOs after the successful merge of #1713 will make more sense.

.. for non-collaborative transactions. Right?

So @theborakompanioni If you have a moment, could you please review #1713 and check why the build keeps failing? That's the only thing I'm stuck at, and sorry for being so annoying—I know you're busy. And thank you for looking after all off us all the time

You are not annoying–far from it. Always happy when someone contributes and donates his time.
As of why the build keeps failing, I think @kristapsk comment JoinMarket-Org/joinmarket-clientserver#1713 (comment) hints at it -> the OpenAPI description (docs/api/wallet-rpc.yaml) must reflect your changes. Also, the PR needs some tests to verify the new behaviour.

@amitx13
Copy link
Contributor Author

amitx13 commented Jul 8, 2024

So we can still include this component and display the UTXOs to the user if it is a sweep transaction!

yes we can do that

.. for non-collaborative transactions. Right?

yes

You are not annoying–far from it. Always happy when someone contributes and donates his time. As of why the build keeps failing, I think @kristapsk comment JoinMarket-Org/joinmarket-clientserver#1713 (comment) hints at it -> the OpenAPI description (docs/api/wallet-rpc.yaml) must reflect your changes. Also, the PR needs some tests to verify the new behaviour.

I have included those changes in wallet-rpc.yaml You can take a look at some of my previous commits of #1713 but it was not working as expected so i ended up removing it

Co-authored-by: Thebora Kompanioni <[email protected]>
@theborakompanioni theborakompanioni changed the base branch from master to devel July 8, 2024 14:43
@amitx13
Copy link
Contributor Author

amitx13 commented Jul 16, 2024

Current behaviour: UTXOs are only available for review in case of sweep transction
Future Implementation: After the successful merge of JoinMarket-Org/joinmarket-clientserver#1713 add the UTXOs review modal for non-collaborative transactions.

@editwentyone
Copy link

one nitpick: label it from "Selected UTXOs" to "Considered UTXOs" in the Modal, rest is ok from copy side

@amitx13 amitx13 changed the title 773 UI show selected utxos before performing transaction 773 UI Show Considered UTXOs before performing transaction Jul 16, 2024
@amitx13 amitx13 dismissed stale reviews from 0xSaksham and editwentyone via f5f228a July 21, 2024 12:33
@amitx13 amitx13 closed this Aug 8, 2024
@amitx13 amitx13 deleted the 773-ui-show-selected-utxos-before-performing-transaction branch August 8, 2024 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(UI) Show "Selected UTXOs before performing transaction"
4 participants