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: utxo list #430

Merged
29 commits merged into from
Aug 2, 2022
Merged

feat: utxo list #430

29 commits merged into from
Aug 2, 2022

Conversation

ghost
Copy link

@ghost ghost commented Jul 27, 2022

Resolves #338.

Still in draft as some cleanup, i18n, etc. is still missing. Should be ready to play around with, though. Let me know what you think. More advanced stuff like filtering the table by tags or so can be done later imo.

⚠️ The diff is only so huge because I installed a library to handle the table.

📸

Screenshot 2022-07-27 at 17 27 52

@ghost ghost added the enhancement New feature or request label Jul 27, 2022
@ghost ghost requested a review from theborakompanioni July 27, 2022 15:29
@ghost ghost self-assigned this Jul 27, 2022
@ghost
Copy link
Author

ghost commented Jul 28, 2022

Btw, since it came up in the ccommunity call: The tags are colored as follows:

  • cj-out: 🟢 Green to indicate success. We want people to have lots of green UTXOs.
  • change-out: 🟡 Yellow to indicate "be careful". Getting change out of a collaborative transaction is something completely normal (unless you sweep). So it's fine to have these UTXOs, but you have to be careful with them.
  • reused: 🔴 Red to indicate danger. You're likely doing something wrong if you have a reused address.

All other tags (new, used, non-cj-change, etc.) won't have any color as they feel somewhat neutral to me. Thoughts on that? cc @dergigi

@ghost ghost force-pushed the utxo-list branch from 13c9933 to 9c21378 Compare July 28, 2022 07:44
@ghost ghost force-pushed the utxo-list branch from 9c21378 to 22f944a Compare July 28, 2022 07:46
package.json Outdated Show resolved Hide resolved
)}
</div>
) : (
<div className={styles.utxoListContainer}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

className also utxoListContainer here? Maybe just a semantic issue.

@theborakompanioni
Copy link
Collaborator

Dark mode issue in the "Account Details" view. This might have been introduced by my while updating bootstrap.

As I am the culprit, I will fix it myself later on. Just noting it down.

@ghost
Copy link
Author

ghost commented Jul 28, 2022

@theborakompanioni Added the confirmations to the table as discussed. ✅ Also added a refresh button that reloads the wallet info and thus refreshes the utxos.

Screenshot 2022-07-28 at 12 18 26

@ghost ghost requested review from theborakompanioni and dergigi July 28, 2022 12:59
const indexOfLockedTag = rawStatus.indexOf('[LOCKED]')

if (indexOfLockedTag !== -1) {
locktime = rawStatus.substring(0, indexOfLockedTag).trim()
Copy link
Collaborator

@theborakompanioni theborakompanioni Jul 28, 2022

Choose a reason for hiding this comment

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

utxo.locktime is also populated for time-locked utxos. Is there a specific reason why locktime is parsed from the status value? Would be nice to remove this special handling of "[LOCKED]". Just asking, because It has not been that reliable in the past.

Copy link
Author

Choose a reason for hiding this comment

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

That is a good point actually. Let me change that. 👍

@theborakompanioni
Copy link
Collaborator

theborakompanioni commented Jul 28, 2022

What follows are some observations while playing with the UI. First of all: Great job. Amazing for a first iteration. It is very informative, functional, visually appealing, and hopefully quite useful for users. I'm eager to hear the feedback from the community.

Observations

  • Currently, it is possible to unfreeze non-expired fidelity bonds.

    • It is prevented in current CurrentWalletAdvanced view. Cannot remember exactly right now what the issue is, but I think this leads to some problems. At best, it is handled in the backend, but till that is the case, we should keep this behaviour imho (was introduced in c68e4c5).
  • Currently, freezing addresses is possible, if a taker or maker operation is running. In JM <=v0.9.6 the operation succeeds, but will lead to problems; in >v0.9.6 (still unreleased) this is prevent server-side. ("Action not allowed" alert is shown ✔️, but no further information on why that is ❌)

  • If the overlay is opened, switched to tab "Jar Details" and subsequently closed. The next time the overlay is opened tab "UTXOs" is activated, but the contents of tab "Jar Details" are still shown. This also applies to the active Jar -> Open Jar#0 -> Switch to Jar#2 (active) -> Close overlay -> Click Jar#0 -> Jar#2 still shown ❌ .


What follows are things that can probably be addressed in follow-up PRs:

  • Can sorting the utxo list by confirmations be added? I think there is a use case for users seeking to "spend my most recent coins" or "spend oldest coins".

Nits

  • Hovering over addresses in "Jar Details" displays a border - is this intended? If so, a left padding is missing. I think it actually looks good.. Addressed in fix: text and border colors after bootstrap update #432

  • "Wallet Info reload" function is great. But it feels like a visual feedback is missing (On reload success -> nothing changes -> user might not know what happened or are "unsatisfied"/).
    Thinking of two possible solutions:

    • showing a ✔️ icon like we do on successful "address copy" (simple and quick)
    • introducing toasts - e.g. "Successfully reloaded wallet information" (time-consuming and probably not needed yet)
      • However, they'd be useful in the future, e.g. displaying infos from websockets (incoming transaction, etc.)
  • Now that the switching of Jars in the overlay looks nice, do you think it should switch places with the "close" button?

    • Thought: This is the only view having the close button on the left hand side. Would be more consistent to have it on the right, imho.
  • If the first account in a jar is empty, the accordion body is visually empty (other show an xpub, but JM treats it specially and does not provide the xpub for the first account)

    • As a user, this would "surpise" me, or at least raise questions. What do you think, should some sort of message be shown?
      "e.g. internal account is empty"
  • Should contents of column "Value" be aligned on the right hand side?

  • Current max. confirmations is 99999999, then the value is visually cut off. We have time until year ~3909 till block 99,999,999 is mined.. so maybe this can be fixed later.

Questions

  • Have you considered showing the sum of all selected utxos? That (in combination with "freeze all non-selected"; see below) is one of the most useful features I can think of. Use case is e.g. to sweep an approximate amount to cold storage or spend the funds otherwise as payment. As a user, I do not want to sum it up manually. What do you think?

  • Do you think "freeze all non-selected coins" is a common use case in general?

  • Should the same visual representation of tags as in the "UTXOs" tab be used in the "Jar Details" tab?

  • Have you seen or tried SortToggleType.AlternateWithReset? Would it be beneficial?

  • Can advanced mode be removed? Maybe the flag useAdvancedWalletMode can be kept, but maybe the actual "CurrentWalletAdvanced" view (and "DisplayAccount" respectively) is not needed anymore?

    • Moving all used sub-views into jar_details directory (e.g. DisplayBranch.tsx)

@dergigi
Copy link
Contributor

dergigi commented Aug 1, 2022

Nice! Great job, that looks really awesome already. Love the confirmation indicator.

All other tags (new, used, non-cj-change, etc.) won't have any color as they feel somewhat neutral to me. Thoughts on that?

I like the green/yellow/red distinction, and I think having no colors on the other tags is fine. Let's keep it like that and see how it is perceived by users.


Only thing I found during testing is that there can be a label/text overlap in the "Jar Details" view, but I guess that's unrelated to this PR. Just bringing it up here so we don't forget.

Screenshot 2022-08-01 at 09 03 48


Jam is a web UI for JoinMarket with focus on user-friendliness. It aims to provide sensible defaults and be easy to use for beginners while still having the features advanced users expect.

We're getting there! 😊 🚀

@ghost
Copy link
Author

ghost commented Aug 2, 2022

Awesome, thanks for all the detailed feedback! 🙌 🙏 It's great to know that everything's been thoroughly tested. 👌

I think I addressed everything that needs immediate attention. Please have another look. @theborakompanioni.

Below my comments on the things I didn't address yet.

"Wallet Info reload" function is great. But it feels like a visual feedback is missing

There's some feedback in terms of a loading spinner being shown and hidden again after loading. I personally don't think much more feedback is required (cf. a reload button in a browser which also only has two states usually: loading and not loading.)

If the first account in a jar is empty, the accordion body is visually empty

Jar details needs a redesign and rewrite imo. Haven't touched it beyond some obvious issues.

Current max. confirmations is 99999999

Let's push that fix to an upcoming version. 😄

Have you considered showing the sum of all selected utxos?

Good idea, let's track that as an improvement: #440.

Do you think "freeze all non-selected coins" is a common use case in general?

No I think that'd be confusing to be honest. 😄 Is that a common pattern elsewhere -- performing an operation on non-selected items?

Should the same visual representation of tags as in the "UTXOs" tab be used in the "Jar Details" tab?

Yes. As mentioned, the Jar Details tab needs a redesign.

Can advanced mode be removed? Maybe the flag useAdvancedWalletMode can be kept, but maybe the actual "CurrentWalletAdvanced" view (and "DisplayAccount" respectively) is not needed anymore?

Yes I think we're ready to finally remove it now. I tracked #441.

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.

Added two suggestions, one is a tiny visual "improvement", the other fixes a display bug in dark-mode. (Another one was introduced by me and is fixed in #442)

Please fix and/or merge on your own will. Approved ✔️
This feature rocks 🪨 🪨 🪨

@ghost ghost merged commit 61a3956 into master Aug 2, 2022
@ghost ghost deleted the utxo-list branch August 2, 2022 13:24
@ghost ghost mentioned this pull request Aug 3, 2022
@editwentyone
Copy link

https://www.figma.com/file/kfejZJFlwBywvLEnPEmJo1/JoinMarket-UI?node-id=5071%3A87607

image

Bildschirmaufnahme.2022-08-12.um.14.07.26.mov

@editwentyone
Copy link

There's some feedback in terms of a loading spinner being shown and hidden again after loading. I personally don't think much more feedback is required (cf. a reload button in a browser which also only has two states usually: loading and not loading.)

I think its better to have a green Checkmark replacing the spinning loader / refresh icon for 2 seconds as an indicator.
your example with a reload for a browser window has a visual feedback on success: the whole window goes blank and rebuilds when you do a browser refresh. because we don't do that, a little success indicator (also possible as a toast) in our case would be helpful

@dergigi dergigi mentioned this pull request Aug 24, 2022
3 tasks
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jar drill down view
3 participants