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

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

Merged
merged 19 commits into from
Jul 22, 2024

Conversation

amitx13
Copy link
Contributor

@amitx13 amitx13 commented Jun 2, 2024

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.

@amitx13 amitx13 linked an issue Jun 2, 2024 that may be closed by this pull request
@theborakompanioni
Copy link
Collaborator

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
Copy link
Contributor Author

amitx13 commented Jun 8, 2024

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

@amitx13
Copy link
Contributor Author

amitx13 commented Jun 8, 2024

@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
Copy link

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
Copy link
Contributor Author

amitx13 commented Jun 9, 2024

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

@amitx13
Copy link
Contributor Author

amitx13 commented Jun 9, 2024

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
Copy link
Collaborator

theborakompanioni commented Jun 10, 2024

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:

  • nit: Try to follow the naming style and be consistent (e.g. mixing camelCase and snake_case)
    e.g. setshowingUTXOS -> setShowingUtxos in code (camelCase), or showUtxos.Select_UTXOs in translation files (snake_case )
  • Try to use existing components instead of creating new ones
    • And if you must create new components, try make them reusable (e.g. let the caller provide onclick handler, etc.)
  • Try to use the frameworks components instead of custom css

Small issues:

  • If you deselect all UTXOs, you can cancel the alert and still confirm the action - freezing all UTXOs, which I assume, should be avoided?
  • The modal buttons on mobile are displaced (resize browser window, see below)
  • The UTXO list is not readable on small screens (resize browser window, see below)
  • Labels with wrong color (see below), e.g. "reused" and "non-cj-change" background color is green, when it should be red/neutral
  • Cannot click the Jar anymore to select (need to click the toggle button - which is small, hence difficult, on mobile)
  • There is no loading indicator, when the freeze/unfreeze action is performed - users might have many UTXOs (one request per UTXO) or access the app via Tor (high latency) - which all slows down the process (try throttling your network speed to "Slow 3G" in your browser tools)

@amitx13
Copy link
Contributor Author

amitx13 commented Jun 10, 2024

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
Copy link
Collaborator

theborakompanioni commented Jun 10, 2024

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
Copy link
Contributor Author

amitx13 commented Jun 10, 2024

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

@editwentyone
Copy link

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

Bildschirmfoto 2024-06-11 um 10 39 00

Copy link

@editwentyone editwentyone left a comment

Choose a reason for hiding this comment

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

please add some padding at top / bottom

@amitx13
Copy link
Contributor Author

amitx13 commented Jun 13, 2024

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:

  • confirm button disabled on warning
  • No more displaced buttons
  • Easy to read on small screen
  • Corrected the Label colors (deposit - neutral , non-cj-change & reused - red , frozen - blue , joined & cj-out - green)
  • Clickable Jar
  • Loading indecator during Freeze/Unfreeze (I think it would be good if we can show some message like Please wait, your request is being processed. but for now, gone with the loading, as asked, what do you think?)

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

@amitx13 amitx13 requested a review from editwentyone June 13, 2024 20:27
@editwentyone
Copy link

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

@amitx13 amitx13 self-assigned this Jun 25, 2024
@barrytra
Copy link
Contributor

barrytra commented Jun 26, 2024

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
Copy link
Contributor Author

amitx13 commented Jun 26, 2024

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 #773, and I'm currently making some changes to it. I'll update you once I've resolved the issue.

@amitx13
Copy link
Contributor Author

amitx13 commented Jun 27, 2024

@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
Copy link
Contributor

barrytra commented Jun 27, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

amitx13 commented Jun 27, 2024

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
Copy link
Contributor

barrytra commented Jun 27, 2024

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
Copy link
Contributor Author

amitx13 commented Jun 27, 2024

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

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

amitx13 commented Jul 14, 2024

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
Copy link
Collaborator

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
Copy link
Contributor Author

amitx13 commented Jul 16, 2024

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.

editwentyone
editwentyone previously approved these changes Jul 16, 2024
Copy link

@editwentyone editwentyone left a comment

Choose a reason for hiding this comment

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

tACK

@amitx13 amitx13 dismissed stale reviews from editwentyone and theborakompanioni via e819d81 July 21, 2024 12:33
@theborakompanioni
Copy link
Collaborator

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

  • change-out probably should get a color (yellow?) and not show the mixed icon
  • Currently not really understand the function of isColorChange of Balance component - can you verify it does what you want? Only effect I see now is that it is not formatted and has some variant of the background highlight color. Maybe we can have both - color and format?

  • The address should probably be monospace font.

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

@amitx13
Copy link
Contributor Author

amitx13 commented Jul 22, 2024

  • 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
Copy link
Collaborator

  • 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.

Copy link
Collaborator

@theborakompanioni theborakompanioni left a comment

Choose a reason for hiding this comment

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

Amazing 🚀

If it is okay for you, I will merge this now and make some small adaptions in a follow-up PR. Thank you for your work and your time @amitx13. I think users will highly appreciate that feature, as it was quite circuitous before.

@amitx13
Copy link
Contributor Author

amitx13 commented Jul 22, 2024

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.

@theborakompanioni theborakompanioni merged commit 5c8f81e into devel Jul 22, 2024
@theborakompanioni theborakompanioni deleted the 765-ui-implement-utxo-selection-modal branch July 22, 2024 10:08
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) Implement UTXO Selection Modal
4 participants