joinmarket-webui / jam

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

feat: quick freeze/unfreeze UTXOs on send page #771

Closed amitx13 closed 2 months ago

amitx13 commented 4 months ago

subtask of #689 This PR fixes #765
Quick freeze/unfreeze UTXOs from selected source Jar. Users will have the ability to select specific UTXOs when performing a direct-send or participating in a coin-join.

theborakompanioni commented 4 months ago

Getting tags[0] is undefined after sending a transaction (after the first tx from a clean npm run regtest:init):

Also, the modal opens immediately when selecting a source jar - is this intentional?

amitx13 commented 3 months ago

Fixing one thing ended up causing a bit of a ruckus elsewhere. So it took a while to sort all of them 😂.

amitx13 commented 3 months ago

@editwentyone Can you please take a while and assist me on what should be the deposit color? In our meet @theborakompanioni told that UTXOs with deposit tags should not have the mixed icon and green color,  So, what should it be instead? Should it be red or something else?

editwentyone commented 3 months ago

take grey like "regular" in the screenshot, its just a deposit, I don't think we need a highlight there

image

https://www.figma.com/design/kfejZJFlwBywvLEnPEmJo1/JoinMarket-UI?node-id=5267-69693&t=UyjfOQsttOYHnAmV-11

amitx13 commented 3 months ago

Please take a look and let me know if there's anything I need to add or fix. 

amitx13 commented 3 months ago

There is still an issue in the tooltip of the selected jar, tho. When the user clicks on the accordion (Sending Options), the scroll bar appears, and due to that, the tooltip gets displaced from its original position. I tried a few things, like forced rendering, but it did not work out as expected.

It can be fixed by increasing the size of the sendForm, but if anyone knows how it can be fixed, please let me know. It will be a great help.🙏

theborakompanioni commented 3 months ago

There is still an issue in the tooltip of the selected jar, tho. When the user clicks on the accordion (Sending Options), the scroll bar appears, and due to that, the tooltip gets displaced from its original position. I tried a few things, like forced rendering, but it did not work out as expected.

Think this is such a minor UI detail, it can be tackled in a follow-up PR!

Please take a look and let me know if there's anything I need to add or fix.

Generally, it works as expected.

Some thoughts regarding the code:

Small issues:

amitx13 commented 3 months ago

Thanks for the feedback. @theborakompanioni really appreciates your efforts. Yeah, I tried using framework components, but in some cases it was not working as expected, so I wrote custom CSS. I will try to take a look one more time if I can somehow make it possible. 

I have a small doubt regarding the small issue (5th point): I cannot click the Jar anymore. The current flow is what Edi wanted (https://github.com/joinmarket-webui/jam/issues/765#issuecomment-2146943136),) so I implemented that. But if that's not what you want, could you please explain what the flow should be?

theborakompanioni commented 3 months ago

Thanks for the feedback. @theborakompanioni really appreciates your efforts. Yeah, I tried using framework components, but in some cases it was not working as expected, so I wrote custom CSS. I will try to take a look one more time if I can somehow make it possible.

I have a small doubt regarding the small issue (5th point): I cannot click the Jar anymore. The current flow is what Edi wanted (https://github.com/joinmarket-webui/jam/issues/765#issuecomment-2146943136),) so I implemented that. But if that's not what you want, could you please explain what the flow should be?

Thanks for the feedback. @theborakompanioni really appreciates your efforts. Yeah, I tried using framework components, but in some cases it was not working as expected, so I wrote custom CSS. I will try to take a look one more time if I can somehow make it possible.

If possible, take a look at other code parts that already use modals/dialogs/etc. At best, it would look the same or quite similar without additional custom styles.

I have a small doubt regarding the small issue (5th point): I cannot click the Jar anymore. The current flow is what Edi wanted (https://github.com/joinmarket-webui/jam/issues/765#issuecomment-2146943136),) so I implemented that. But if that's not what you want, could you please explain what the flow should be?

I guess you should still be able to select a jar by clicking the jar symbol which I think is in line with what @editwentyone suggested. Once it is selected, a second click opens the modal. Do you know what I mean?

amitx13 commented 3 months ago

Okay, now I get it. Thank you for the explanation.

editwentyone commented 3 months ago

please add more padding top and bottom, that us way to close

Bildschirmfoto 2024-06-11 um 10 39 00

amitx13 commented 3 months ago

Reused 2 components ConfirmModal and SelectableJar Used react-table-library for the table as it is used in /wallet Fixed all of the small issue:

Please have a look 🙏 and let me know if there's anything I need to add or fix.

editwentyone commented 3 months ago

we discussed making the list after 5 entries scrollable for the time being.

after that maybe sorting by label and value, but that's out of scope

barrytra commented 3 months ago

I was trying to use this code for displaying UTXO list in FB creation modal as well. However the code breaks saying Cannot read properties of undefined (reading 'tag') Same issue persists for me if I run this branch and try to select UTXOs to send a transaction. Screenshot from 2024-06-26 17-42-57 @amitx13 is this fixed? If not lmk.. we can tackle this together. Or guide me if I am going wrong somewhere.

amitx13 commented 3 months ago

I was trying to use this code for displaying UTXO list in FB creation modal as well. However the code breaks saying Cannot read properties of undefined (reading 'tag') Same issue persists for me if I run this branch and try to select UTXOs to send a transaction. Screenshot from 2024-06-26 17-42-57 @amitx13 is this fixed? If not lmk.. we can tackle this together. Or guide me if I am going wrong somewhere.

Hey @barrytra, Please hold off until tomorrow. I also need to reuse this component for https://github.com/joinmarket-webui/jam/issues/773, and I'm currently making some changes to it. I'll update you once I've resolved the issue.

amitx13 commented 3 months ago

@barrytra Please have a look now. Additionally, I have also reused the component in this PR: https://github.com/joinmarket-webui/jam/pull/788/files look at src/components/Send/index.tsx

barrytra commented 3 months ago

I went through the code and tried using it as a component in FB creation. A suggestion from my side: How about if we export showUtxos component as a div and not a modal as a modal is not required in FB creation. U can append the modal over showUtxos component elsewhere. What do you think about it @amitx13 ?

barrytra commented 3 months ago

I was trying to use this code for displaying UTXO list in FB creation modal as well. However the code breaks saying Cannot read properties of undefined (reading 'tag') Same issue persists for me if I run this branch and try to select UTXOs to send a transaction. Screenshot from 2024-06-26 17-42-57

moreover I am still getting this error on my setup :(

amitx13 commented 3 months ago

I think you can still reuse the UtxoListDisplay and Divider components to make it work in FB creation becz you don't need the alerts and Confirmation modal that come with ShowUtxos and for your tag error, i think it's because you are not reloading the utxos like i have done in handleReload in ShowUtxos . I can help you with the code just push your changes and share your branch

barrytra commented 3 months ago

I think you can still reuse the UtxoListDisplay and Divider components to make it work in FB creation becz you don't need the alerts and Confirmation modal that come with ShowUtxos and for your tag error, i think it's because you are not reloading the utxos like i have done in handleReload in ShowUtxos . I can help you with the code just push your changes and share your branch

Okay!! I get that. I'll use UtxoListDisplay in FB creation. For the error part:While sending funds, It works fine if a Jar does not have any frozen Utxo but this error occurs when there is a frozen one. This is what I observed working with it. You can try creating a FB from a jar and freezing some Utxos, and then sending a txn through that jar

amitx13 commented 3 months ago

I think you can still reuse the UtxoListDisplay and Divider components to make it work in FB creation becz you don't need the alerts and Confirmation modal that come with ShowUtxos and for your tag error, i think it's because you are not reloading the utxos like i have done in handleReload in ShowUtxos . I can help you with the code just push your changes and share your branch

Okay!! I get that. I'll use UtxoListDisplay in FB creation. For the error part:While sending funds, It works fine if a Jar does not have any frozen Utxo but this error occurs when there is a frozen one. This is what I observed working with it. You can try creating a FB from a jar and freezing some Utxos, and then sending a txn through that jar

Hey @barrytra I have re-tested the scenario you described, and it is working fine on my end. Before: Screenshot from 2024-06-27 18-06-23 After: Screenshot from 2024-06-27 18-06-59

It seems like the issue might be due to an outdated build on your side.If you have a moment, could you please try pulling the latest changes from the PR and rebuilding the project? Thank you for your feedback and for taking the time to test the functionality I really appreciate your efforts

barrytra commented 3 months ago

It seems like the issue might be due to an outdated build on your side.If you have a moment, could you please try pulling the latest changes from the PR and rebuilding the project? Thank you for your feedback and for taking the time to test the functionality I really appreciate your efforts

I rebuilt it, but the issue still persists for me.. (IDK why). Let's wait until others review the same. Though thanks a lot for making the components. I'll try using them in FB creation.

barrytra commented 3 months ago

One more insight regarding the error: The code works perfectly fine for all jars except Jar A. Below are the UTXOs present in Jar A: Screenshot from 2024-06-27 20-34-18 What I infer is that last 3 UTXOs are empty and have no tag under them. This might be the reason I am getting the error: Cannot read properties of undefined (reading 'tag') At first place I don't really get why are those 3 empty UTXOs present there. But if this can be possible somehow then I think having this case (in which tag has no value) might solve the issue.

amitx13 commented 3 months ago

Hmm, that makes sense. I'm not sure why those empty UTXOs are there, but it's definitely a good idea to handle this case. Thanks for catching that and solving the error, @barrytra . I really appreciate it. Great work, buddy!

barrytra commented 3 months ago

Great! Now it works perfect for me. Great Job @amitx13 . Let's wait for others to review it as well.

theborakompanioni commented 3 months ago

Quick feedback from looking over the changes. Code looks generally quite good. I think we are almost there! Well done :muscle:

Things that can be done in a follow-up PR:

Other considerations: Jam should discourage spending multiple "reused" UTXOs in different transactions, i.e. UTXOs sitting on the "same" address. They should be frozen/unfrozen together–or at least, show a warning to the user that what is being done is not advisable. This is not yet done in the Jar Details view so it does not necessarily have to be done in this PR. Just a heads-up.

theborakompanioni commented 3 months ago

Addendum: What is the default ordering of the UTXOs? Should it be descending by confirmations?

Maybe we should provide at least the possibility to order by value and confirmations (can also be done in a follow-up PR).. what do you think?

editwentyone commented 3 months ago

default could be by "date" in this case by numbers of confirmation, the older ones at the bottom, newer ones at the top.

being able to sort is awesome in a later PR

amitx13 commented 3 months ago
amitx13 commented 3 months ago

At last, Added the default sorting of UTXOs, with the older ones at the bottom, newer ones at the top. As discussed above,

amitx13 commented 3 months ago

Please have a look @theborakompanioni and let me know if there are any modifications needed in the UI. Looking forward to your responses. And regarding the confirmation tooltip, it can be addressed in a follow-up PR.

amitx13 commented 3 months ago

The tooltip was literally 10 lines of code so i ended up adding here. Behavior: The tooltip will only be visible on hover if the confirmation count exceeds 9999.

theborakompanioni commented 2 months ago

@amitx13 Nice work! Thanks for addressing this so quickly!

Found some more things, but those are minor and can be considered later on:

Since these points can be addressed in follow-up PRs and the current code works great: Approved! :rocket:

amitx13 commented 2 months ago

Hey @theborakompanioni I appreciate your thoughts. Here is what i think

  • reloading takes a long time (especially the /display request) on mainnet, which is not obvious during development (maybe we should introduce an optional artificial delay in dev:start?). So imho, it is not necessary to reload all data again, at least not /display - since it is loaded on initial page load and after a transaction has been made. What do you think?

Yes reloading takes a long time but /display request is essential for updating tags properly becz /display request is the one which appends the tag (status) for UTXOs After a transaction has been made,reloadUtxos is being called, not reloadAll. I think the only thing we can do to reduce time is to call reloadDisplay instead of reloadAll

theborakompanioni commented 2 months ago

Hey @theborakompanioni I appreciate your thoughts. Here is what i think

  • reloading takes a long time (especially the /display request) on mainnet, which is not obvious during development (maybe we should introduce an optional artificial delay in dev:start?). So imho, it is not necessary to reload all data again, at least not /display - since it is loaded on initial page load and after a transaction has been made. What do you think?

Yes reloading takes a long time but /display request is essential for updating tags properly becz /display request is the one which appends the tag (status) for UTXOs After a transaction has been made,reloadUtxos is being called, not reloadAll. I think the only thing we can do to reduce time is to call reloadDisplay instead of reloadAll

Ah, yes, you are right.. only /utxo enpoint is reloaded after a transaction. Would it be possible and feasible to adapt that, i.e. reload /display after wallet UTXOs change but not loading it every time the modal is opened?

amitx13 commented 2 months ago

I have optimized the UTXO display update logic. The /display request is now made only during the initial render and when opening the modal after a transaction, preventing unnecessary reloads each time the modal is opened.

Please take a look and, if everything looks good, merge the changes.

theborakompanioni commented 2 months ago

Some notes, that can be fixed in a follow up PR:

amitx13 commented 2 months ago
  • change-out probably should get a color (yellow?) and not show the mixed icon

Hey are there any resources that give an idea of all the tags available in Jam and their respective icons and colors? since it's not available in Figma Design.

  • Remember resetting the amount field on sweeps when the source jar changes (follow-up PR!)

Hey, I've implemented the logic so that if Jar A is selected and sweep is true, the sweep amount of Jar A is shown in the amount input field, and now if Jar B is selected, the amount field updates to show the sweep amount for Jar B. However, if you prefer resetting it we can do that too.

theborakompanioni commented 2 months ago
  • change-out probably should get a color (yellow?) and not show the mixed icon

Hey are there any resources that give an idea of all the tags available in Jam and their respective icons and colors? since it's not available in Figma Design.

Good point. I don't think there is.

  • Remember resetting the amount field on sweeps when the source jar changes (follow-up PR!)

Hey, I've implemented the logic so that if Jar A is selected and sweep is true, the sweep amount of Jar A is shown in the amount input field, and now if Jar B is selected, the amount field updates to show the sweep amount for Jar B. However, if you prefer resetting it we can do that too.

Imho, resetting the value is preferable, as a user might be surprised otherwise and clicking the sweep button once is a reasonable trade-off. However, that can be done in a follow-up PR, as the changes are now consistent and additional adaptions can be discussed separately.

amitx13 commented 2 months ago

If it is okay for you, I will merge this now and make some small adaptions in a follow-up PR.

Yea sure, feel free to merge the PR.

Thank you for your work and your time @amitx13. I think users will highly appreciate that feature, as it was quite circuitous before.

Thank you! I appreciate your kind words and the opportunity to contribute. Btw, just a heads-up on the bonus task (DirectSend RPC-Api): I reached out to some of my prev-mentors and mates, but none of them were able to identify why the OpenAPI Diff Action is throwing an error. However, I will keep looking into it and try to solve it. Let's see where it goes.