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.

Sign up for GitHub

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

Jump to bottom

UI : Show considered UTXOs before performing transaction #807

Merged
merged 11 commits into from
Aug 20, 2024
Merged

UI : Show considered UTXOs before performing transaction #807

merged 11 commits into from
Aug 20, 2024

Conversation

amitx13
Copy link
Contributor

@amitx13 amitx13 commented Aug 8, 2024

This PR fixes #773

Changes:

  1. User are now able to review their considered UTXOs before performing a sweep
  2. Successful implementation of
    image

@amitx13 amitx13 self-assigned this Aug 8, 2024
@amitx13 amitx13 added this to the SoB milestone Aug 8, 2024
@amitx13 amitx13 changed the title Show considered UTXOs before performing sweep transaction UI : Show considered UTXOs before performing sweep transaction Aug 8, 2024
@amitx13
Copy link
Contributor Author

amitx13 commented Aug 8, 2024

I requested the JM devs to review the OpenAPI Diff (JoinMarket-Org/joinmarket-clientserver#1721) almost two weeks ago, but unfortunately, there has been no response. As a result, I went ahead and implemented the review of the considered UTXOs for the sweep transaction. Please take a look.

@editwentyone
Copy link

strange, I can't review because I can't see this modal with the considered UTXOs anymore

@amitx13
Copy link
Contributor Author

amitx13 commented Aug 15, 2024

strange, I can't review because I can't see this modal with the considered UTXOs anymore

Hey, this PR needs to be rebased with TBK's follow-up. Also, just a heads-up—in the current scenario, it only works for sweep.

@amitx13 amitx13 force-pushed the UI-selected-utxos-before-performing-transaction branch from 8c8198e to 3447800 Compare August 16, 2024 14:36
@amitx13
Copy link
Contributor Author

amitx13 commented Aug 17, 2024

Q. Why introduced disableCheckboxCell and customTheme?

Well, because it wasn't aligning with what @editwentyone wanted, I added a customTheme parameter to give future developers more flexibility and ease of use.

As for disableCheckboxCell, previously, only the input checkbox was hidden, while the cell was still present all the time, which again wasn't aligning with the desired UI. It looked like this:

Screenshot from 2024-08-17 22-36-48
Instead of this:

image

@editwentyone
Copy link

two questions:

  • why do we allow this feature only when sweeping? IMO it should be always on?
  • it should be better to see the beginning and the end of the address, not two times "…" : like » "bc18da1 … 29x1a"

0xSaksham
0xSaksham previously approved these changes Aug 19, 2024
View reviewed changes
@amitx13 amitx13 marked this pull request as draft August 19, 2024 07:35
@amitx13
Copy link
Contributor Author

amitx13 commented Aug 19, 2024

  • why do we allow this feature only when sweeping? IMO it should be always on?

Hey we discussed this earlier in the meet where we came up with this common conclusion that showing the actual used UTXOs is only possible once JoinMarket-Org/joinmarket-clientserver#1721 is merged and released. 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 #1721, the transaction will be based on the selected UTXOs. which is why this feature is only enabled during a sweep.

However, you previously suggested rephrasing selected UTXOs to considered UTXOs so we could potentially display those UTXOs in both direct send and coinjoin as well.

Thanks @editwentyone,

@amitx13 amitx13 changed the title UI : Show considered UTXOs before performing sweep transaction UI : Show considered UTXOs before performing transaction Aug 19, 2024
@amitx13 amitx13 marked this pull request as ready for review August 19, 2024 08:21
@amitx13
Copy link
Contributor Author

amitx13 commented Aug 19, 2024

Done with the changes. @editwentyone and @theborakompanioni , if you have a moment, please have a look.

@amitx13 amitx13 force-pushed the UI-selected-utxos-before-performing-transaction branch from f77fa3a to 58c5e38 Compare August 20, 2024 08:34
@theborakompanioni
Copy link
Collaborator

Done with the changes. @editwentyone and @theborakompanioni , if you have a moment, please have a look.

Nice work.. overall I am unsure of the phrasing ("Considered UTXOs"), but that can be done in a follow-up PR. I have pushed small changes, care to take a look?

@amitx13
Copy link
Contributor Author

amitx13 commented Aug 20, 2024

I was aiming for future compatibility with the updated directsend RPC-API (if it gets merged), but I agree we can go with this approach too. Thanks for the changes! If there's nothing else to add, I think we can merge this PR.

@theborakompanioni theborakompanioni merged commit 96844d9 into devel Aug 20, 2024
@theborakompanioni theborakompanioni deleted the UI-selected-utxos-before-performing-transaction branch August 20, 2024 14:00
0xSaksham added a commit that referenced this pull request Aug 21, 2024
@0xSaksham @amitx13 @theborakompanioni
Updating #812 branch (#819)
48f7095 
* fix: amount input field to properly interpret BTC (#800)

* feat: add SchedulerConfirmationModal component for starting scheduled sweep (#803)

* feat: add SchedulerConfirmationModal component for starting scheduled sweep

* removed dead code

* modal is centered and design changes

* used ConfirmModal and pre-written styles

* refactor: SchedulerConfirmationModal.tsx

minor production error

* build(deps): update dependencies (#813)

 @emotion/react          ^11.11.4  →  ^11.13.0
 @types/react             ^18.3.2  →   ^18.3.3
 formik                    ^2.4.5  →    ^2.4.6
 qrcode                    ^1.5.3  →    ^1.5.4
 react-bootstrap           ^2.9.2  →   ^2.10.4
 react-router-bootstrap   ^0.26.2  →   ^0.26.3
 react-router-dom         ^6.23.1  →   ^6.26.1
 husky            ^8.0.3  →    ^9.1.4
 i18next        ^23.11.4  →  ^23.13.0
 lint-staged     ^14.0.1  →   ^15.2.9
 prettier         ^3.2.5  →    ^3.3.3
 react-i18next   ^14.1.1  →   ^15.0.1

* refactor: align utxo list and modal components (#815)

* refactor: externalize UtxoIcon component and utxoTags function

* refactor: externalize UtxoConfirmations component

* refactor: reuse utxo icons in Jar details and UTXO list

* refactor(ui): simpler checkbox in utxo list

* refactor(send): vertically align balance

* refactor(send): tooltip for shortened addresses

* refactor(ui): externalize UtxoTags component

* ui(send): show considered UTXOs before performing transaction (#807)

---------

Co-authored-by: apX13_ <[email protected]>
Co-authored-by: Thebora Kompanioni <[email protected]>
@MarnixCroes MarnixCroes mentioned this pull request Oct 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theborakompanioni theborakompanioni theborakompanioni approved these changes

@0xSaksham 0xSaksham 0xSaksham left review comments

@editwentyone editwentyone Awaiting requested review from editwentyone

Assignees

@amitx13 amitx13

Labels
None yet
Projects
None yet
Milestone
SoB
Development

Successfully merging this pull request may close these issues.

(UI) Show "Selected UTXOs before performing transaction"
4 participants
@amitx13 @editwentyone @theborakompanioni @0xSaksham