search

joinmarket-webui / jam

Your sats. Your privacy. Your profit.
https://jamapp.org
MIT License
236 stars 48 forks source link

773 UI show selected utxos before performing transaction #788

Open amitx13 opened 1 week ago

amitx13 commented 1 week ago

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
theborakompanioni commented 1 day ago

As far as I am aware, showing the actual used UTXOs is only possible once https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/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 commented 1 day ago

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

theborakompanioni commented 1 day ago

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;

:+1:

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 https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/1713#discussion_r1632318596 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 commented 1 day ago

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