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(Send): Fee breakdown table #606

Merged
merged 31 commits into from
Aug 28, 2023
Merged

feat(Send): Fee breakdown table #606

merged 31 commits into from
Aug 28, 2023

Conversation

httpiga
Copy link
Contributor

@httpiga httpiga commented Feb 4, 2023

Resolves #632.

Hello people! 🧡
I'm trying to implement the fee breakdown table in the send page, as designed in the Figma project.
UI seems ready (see screenshot below), but befor implementing the logic I need to better understand how fees work in Jam 😄

  • Where can I find the "Absolute fee limit" and "Relative fee limit" values?
  • What do you mean by "Absolute/Relative fee limit, chosen randomly"? The limit should be fixed, do you mean that the fee is randomly selected below the absolute/relative limit?
  • What value do we want to show in the personal absolute/relative fee limit, if the user didn't set any?
  • How to calculate the estimated total fees? The Figma design shows a single value, what about calculating the minimum and maximum possible fees for the transaction, and displaying the range?

image

@dergigi
Copy link
Contributor

dergigi commented Feb 6, 2023

What do you mean by "Absolute/Relative fee limit, chosen randomly"? The limit should be fixed, do you mean that the fee is randomly selected below the absolute/relative limit?

Yes, this is poorly worded. The limit is fixed and the fee is chosen randomly below the limit.


I'll let @theborakompanioni answer the rest. I assume you've read https://jamdocs.org/market/fees/ and the linked note on fees?

@theborakompanioni
Copy link
Collaborator

Hello people! orange_heart I'm trying to implement the fee breakdown table in the send page, as designed in the Figma project. UI seems ready (see screenshot below), but befor implementing the logic I need to better understand how fees work in Jam smile

Hey @httpiga, sorry for the late reply.

* Where can I find the "Absolute fee limit" and "Relative fee limit" values?

These can be loaded with function useLoadFeeConfigValues from hooks/Fees.ts . See usage in Send, FeeConfigModal, etc.

* What do you mean by "Absolute/Relative fee limit, **chosen randomly**"? The **limit** should be fixed, do you mean that the fee is randomly selected below the absolute/relative limit?

Already addressed by @dergigi above. Agree, wording is not clear and should be improved.

* What value do we want to show in the personal absolute/relative fee limit, if the user didn't set any?

These values must be set in order for collaborative transactions to work. JM does not provide default values and will use x in the standard config. The standalone image of Jam will populate these settings with random values.

Currently, Jam will only prompt a user to set them manually, after an attempt to invoke a collaborative transaction fails ("Config variables 'max_cj_fee_rel' and 'max_cj_fee_abs' must be set in your joinmarket.cfg in order to send collaborative transactions. Consider adding them to your config manually."). This can also be improved, as the fee settings are now loaded immediately and the fee values should already be present.

* How to calculate the estimated total fees? The Figma design shows a single value, what about calculating the minimum and maximum possible fees for the transaction, and displaying the range?

e.g. See the code in PaymentConfirmModal for the current estimate, which is shown before a user attempts a transaction. Code can probably be externalized to be used in multiple components. Note, that it only estimates the "maker fees" (paid to other JM makers participating in the transaction), not the miner fee for the transaction, as this is not possible for two reasons:

  • the amount of inputs and outputs is not know beforehand, so it is not possible to know the transaction size, hence the transaction fees cannot be estimated
  • it is also impossible if the user has a "block target" as mining fee target (read: "Include my TX in the next n blocks", not a sats/vbyte value), as Jam cannot know information about the current mempool conditions. See feat: display fee settings on Send page #532 (comment) for more info on these values.

Hope I could give a better insight. Tricky topic. Please do not hesitate to ask further questions if something is not clear.

I'm not sure about the correctness of using useEffect inside the useFeeConfigValues hook,
in particular regarding the effectiveness of the cleanup function (including `abortCtrl.abort()`)
@httpiga
Copy link
Contributor Author

httpiga commented Feb 19, 2023

Sorry for the late answer, guys - thank you so much for your replies, you really helped me clarify the JM fee logic 🙏🏻
My last commits are about wrapping functions in hooks and getting & displaying the fee config values, here's the current status: (ignore the wrong alignment in the estimated fees row)
image

Regarding my recent work, I am unsure about eventual side effects and the actual effectiveness of the return function in the useEffect in useFeeConfigValues, I'd appreciate a comment on that - just to avoid to break something hahahah

I'd like to reflect about the current UI of this component and I plan to share a proposal with you all in the following days 🧡

@httpiga
Copy link
Contributor Author

httpiga commented Feb 26, 2023

Hello there! I tried simplifying this section and making it more clear to the end user, especially about the logic behind the fee mechanism.
I'm going to share a proof of concept: I put effort into the contents, so please ignore completely the UI, I'll refine it if and when this proposal will be approved. Also, the phrases act like placeholders, we can improve them later.

Base case

image

Case "amount lower than absolute and relative max fee" -> max fee = amount

image

Case "absolute fee = max estimated fee"

image

Case "relative fee = max estimated fee"

image

Tooltip (text and formatting to be improved)

Screenshot 2023-02-26 at 21 12 42

What do you think about this?

  • Edit: imagine a "change here" link to be included in the fee limit between parenthesis

@httpiga
Copy link
Contributor Author

httpiga commented May 27, 2023

Hey guys, here's my last proposal to improve the clarity of this component:
Screenshot 2023-05-27 at 19 09 05
Screenshot 2023-05-27 at 19 12 07

  • Accordion title changed to "How much is the maximum collaborators fee?" - we already described how much effect the collaborators number has in the description of the collaborators number select. Here we're talking about fees.
  • Specify that the max collaborators fee does not include the mining fee.
  • Added link to the settings page to change fee limits.
  • Improved explanation of why we can't estimate mining fees.
  • Improved UI.
    I was tempted to add something like "We strongly suggest to check the current mempool conditions and adapt your fee limits accordingly in the settings page" to the mining fees info popup, but it seemed already too long.

In general, I tried simplifying it and keeping only what (I think) brings value to the end user. What do you think?

@editwentyone
Copy link

editwentyone commented May 29, 2023

thank you for the hard work! your time is very appreciated

quick feedback from my side:

what do you think?

Bildschirmfoto 2023-05-29 um 14 20 46

@httpiga
Copy link
Contributor Author

httpiga commented Jun 5, 2023

Thank you all guys for your feedback!
@editwentyone I fixed the visibility in dark mode
Screenshot 2023-06-05 at 19 30 19

regarding the table UI, I see what you mean, what do you think about this visualization?

Counterparties multiplied by...        10
----------------------------------------------
Absolute fee limit (123sats)           1230sats
----------------------------------------------
OR relative fee limit (0.02%)          4567sats
===============================================
Max collaborators fee                  4567sats
----------------------------------------------
Plus mining fee (i)                    5sats/vB

The critical point that I see here is that, to preserve clarity for the user, we should keep the fee estimate calculation explicit, in the form of collaborators*(absolute_limit OR relative_limit) + mining_fee (like in the above schema) or (collaborators*absolute_limit OR collaborators*relative_limit) + mining_fee (like in my screenshot). I'd keep mining fee separated from our estimate to make it clear that we can only estimate collaborators fee and we don't include mining fee estimation (that's why I wouldn't include mining fee alongside the absolute/relative fee rows in the UI).
So that we're saying: "here's what you'll spend at maximum for collaborators, so that you can change your fee limits accordingly. This number doesn't include mining fees and they are separate from this estimate."

@httpiga
Copy link
Contributor Author

httpiga commented Jun 5, 2023

Additionally, while developing this feature and playing around with amounts and fee limits, I noticed that fee limits impacts differently depending on the amounts you're using: for large amounts, one may wants to keep relative fee as low as possible, so that it will be lower than the absolute limit. On the other hands, for small amounts one may wants to keep absolute fee limit as low as possible, so that it will be lower than the relative limit. This is obvious from a mathematical point of view, but (for now) isn't so explicit in Jam's UI.

For example, I may have set, let's say, a max fee of 200sats or 0.1% per collaborator, thinking that 0.1% is a small amount. I won't be aware that in most cases 0.1% is way higher than 200sats, and so I risk to always pay more fees than I wanted to.

I think that this feature, if well implemented, could bring a lot of value to the users when it comes to correctly adjust their fee limits depending on their volumes.

Copy link
Contributor

@MaxHillebrand MaxHillebrand left a comment

Choose a reason for hiding this comment

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

that hover tooltip is in light mode, and I'd prefer to avoid paying for the user's medical bill from the resulting bleeding eyes...

Dark mode should be consistently dark, maybe a different shade of black for the tooltip box?

@httpiga
Copy link
Contributor Author

httpiga commented Jun 12, 2023

@MaxHillebrand 😂 Done!

Screenshot 2023-06-12 at 19 35 21

@httpiga
Copy link
Contributor Author

httpiga commented Jul 4, 2023

@editwentyone I'm pinging you just in case you missed my last comment, let me know what you think about it!

@editwentyone
Copy link

editwentyone commented Jul 5, 2023

@httpiga i like the idea to tell the user based on their amaount what fee option to use and calculacte / predict it as precise as possible.

any ideas how that could look like? or is this a task for me ;)

@httpiga
Copy link
Contributor Author

httpiga commented Jul 19, 2023

@editwentyone I thought that the user would estimate by themself what fee option to use, but suggesting the lower one based on the amount sounds better! It shouldn't be hard at this point but I'd address it on a separate PR.

In order to close this one, what I wish from you is feedback about my proposal about the table UI in response to your previous comment

@editwentyone
Copy link

editwentyone commented Jul 20, 2023

@httpiga so, after finally looking at and thinking about the fee table, we can make it way easier but more precise to communicate what costs are coming up, I will have a call with tbk and propose something asap.

right now neither my initial nor yours makes a lot of sense (sorry to realize that so late) without modifying it further. hope its ok for you to wait a bit and then tackle it again.

@httpiga
Copy link
Contributor Author

httpiga commented Jul 20, 2023

I'm totally fine with it, thank you for taking care of this!

@editwentyone
Copy link

editwentyone commented Jul 22, 2023

hey,

so I came up with something different, hope it is clearer and if not, we really need to consider to remove it all together. looking forward to your feedback. also I did more than just the fee breakdown, you will see in the video.

▶️ Prototype in Figma

  • new fee breakdown as a preview for the user
  • new send button behavior to get rid of the confirmation dialog
  • new summary page layout while joining
fees-button-summary_rf28.mp4

@theborakompanioni
Copy link
Collaborator

theborakompanioni commented Jul 25, 2023

hey,

so I came up with something different, hope it is clearer and if not, we really need to consider to remove it all together. looking forward to your feedback. also I did more than just the fee breakdown, you will see in the video.

* new fee breakdown as a preview for the user

* new send button behavior to get rid of the confirmation dialog

* new summary page layout while joining

I like everything except the button behaviour.. I am not the one to actually suggest UI stuff, but it seems a little bit more complex (especially on mobile - where there is no "hovering") and is a bit confusing (for me). The current approach seems simpler.. 🤔

What do you think @httpiga ?

(However, I like the new fee summary better than the current "table"!)

@theborakompanioni
Copy link
Collaborator

theborakompanioni commented Aug 23, 2023

I can apply some suggestions myself, if you want. May I push some of these changes?

Sure, feel free!

Rough outline of the changes in the latest commits:

  • "Fee Estimation" changed to "Collaborator fees"
  • Bolder font style for "This does not include regular mining fees."
  • Open Fee Settings Modal
  • De-emphasize non-effective fee
  • make it work properly for "sweeps"

@theborakompanioni
Copy link
Collaborator

@httpiga Ready for review? Let me know if you are okay with the latest adaptions 🙏

@theborakompanioni
Copy link
Collaborator

theborakompanioni commented Aug 25, 2023

Note: It does not play too nicley with missing max_cj_fee_abs or max_cj_fee_rel settings. But let's fix that in a follow-up PR.
There should probably be a special warning (with the sending form disabled) if one of them is missing.

And since the fee values are now loaded in the component, we can finally refactor/remove the enhanceTakerErrorMessageIfNecessary function. 🎉

@httpiga httpiga marked this pull request as ready for review August 25, 2023 10:07
@theborakompanioni theborakompanioni self-requested a review August 25, 2023 11:15
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.

tACK. Handling missing fee config values needs to be done in a follow-up PR, other than that, I feel this is ready to be merged 🚀

@theborakompanioni theborakompanioni added enhancement New feature or request UI/UX Issue related to cosmetics, design, or user experience labels Aug 25, 2023
@httpiga httpiga merged commit 7a6b920 into joinmarket-webui:master Aug 28, 2023
@theborakompanioni
Copy link
Collaborator

Great work @httpiga . Thanks for your endurance and for sticking to it, even when requirements changed during development 🧡

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request UI/UX Issue related to cosmetics, design, or user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fee Adjustment Modal - some finetuning
6 participants