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: basic fee settings #522

Merged
merged 50 commits into from
Oct 4, 2022
Merged

feat: basic fee settings #522

merged 50 commits into from
Oct 4, 2022

Conversation

theborakompanioni
Copy link
Collaborator

@theborakompanioni theborakompanioni commented Sep 27, 2022

Partly addresses #518.

This PR is a first attempt to let users adapt some (not all) fee related settings:
Namely:

Please help with the fee settings description if you can. Most of it is taken from JM config file comments with just minor adaptions. Any improvement is highly appreciated.

Questions on arbitrarily chosen min/max values (mainly to prevent unintended inputs):

  • Min value of tx_fees_factor: Current implementation has min. of 10%. Should values below be allowed? e.g. zero means, that no fee randomization is done. Zero is also handled fine by JM.
  • Max value of max_cj_fee_rel: Currently max of 50%. Increase/decrease?
  • Max value of tx_fees: Currently max of 100 sats/vbyte. Increase/decrease? No upper limit? There is a config value absurd_fee_per_kb which translates to 350 sats/vbyte by default.. maybe take this settings value as an upper limit?

📸

Tests

  • setting tx fee rate manually to 1.12 sats/vbyte with 12% rate variance:
2022-09-26 03:01:58,174 [INFO]  Using this manually set tx feerate (randomized for privacy): 1190 sat/vkB (1.1 sat/vB).
  • setting tx fee rate manually to 42 blocks:
2022-09-26 03:41:52,464 [INFO]  Using bitcoin network feerate for 42 block confirmation target (randomized for privacy): 10000 sat/vkB (10.0 sat/vB)
  • setting max_cj_fee_abs to 249 sats) (below the default order with abs fee 250sats) and max_cj_fee_rel to 0.0002% to trigger an error when attempting a collaborative tx with 123456789 sats:
2022-09-28 10:47:45,191 [INFO]  INFO:Received offers from joinmarket pit
2022-09-28 10:47:45,192 [WARNING]  ERROR not enough liquidity in the orderbook n=1 suitable-counterparties=0 amount=27300 totalorders=0
2022-09-28 10:47:45,193 [INFO]  Taker not continuing after receipt of orderbook
2022-09-28 10:47:45,193 [INFO]  Coinjoin did not complete successfully.
  • after increasing max_cj_fee_abs to 251sats: Success!

@theborakompanioni theborakompanioni added enhancement New feature or request UI/UX Issue related to cosmetics, design, or user experience labels Sep 27, 2022
@theborakompanioni theborakompanioni self-assigned this Sep 27, 2022
@theborakompanioni theborakompanioni marked this pull request as ready for review September 28, 2022 11:44
@dergigi
Copy link
Contributor

dergigi commented Sep 28, 2022

Some questions that came up during my tests:

  • Why can't I set it to 1 sat/vByte?
  • Why link the sats/vByte fee to the randomized percentage? When set to 20% I have to set the fee to 1.2, which is very weird. It would be much more intuitive to just set it to 1 and the maximum of 1.2 is implicit in the 20% setting. Set the minimum, percentage defines the maximum.

Validation issues:

  • 1.2 works, but 1,2 doesn't. We should make sure that inputs are i18n agnostic
  • I can't set min abs fee to 0 but I can set it to0.00000000001 which is actually 0

JoinMarket issues:

Both limits given in this section, relative and absolute, must be exceeded in order to not consider a certain offer.

I trust that this is true, but it is very weird behavior, isn't it? I would assume that if I set limits, breaking either limit would be a deal-breaker. Example: I don't ever want to pay more than a million sats, and I don't ever want to pay more than 10% in fees. If the order is large enough, a small percentage might be way more than a million sats, which makes one of the two rules I set obsolete.

Just thinking out loud here, not sure if I'm the weird one or if the way that JoinMarket works is the weird one.

Edit: I think I answered this myself: the absolute fee is per maker, the relative fee is in total. That makes sense, and with that the weird condition makes sense too. You don't want to overpay a single maker (hence the absolute limit) and you don't want to overpay in total (hence the relative limit).

Edit 2: But the conditional in the way you phrased it still doesn't make sense, because it would override one of the limits. I don't want to overpay, period! Hence it should be:

If an offer exceeds either of these limits it will be declined.

At least in my head that makes sense. Maybe I'm still stupid.

Copy link
Contributor

@dergigi dergigi left a comment

Choose a reason for hiding this comment

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

Some i18n inconsistencies in terms of number styles.

src/i18n/locales/en/translation.json Outdated Show resolved Hide resolved
src/i18n/locales/en/translation.json Outdated Show resolved Hide resolved
src/i18n/locales/en/translation.json Outdated Show resolved Hide resolved
src/i18n/locales/en/translation.json Outdated Show resolved Hide resolved
src/i18n/locales/en/translation.json Outdated Show resolved Hide resolved
src/i18n/locales/en/translation.json Outdated Show resolved Hide resolved
src/i18n/locales/en/translation.json Outdated Show resolved Hide resolved
This is special case in that it will be translated to
1.001 sats/vbyte afterwards. This might be surprising to users
but it was decided that this is better than telling them that
the min is 1.001 from a UX point of view.
@theborakompanioni
Copy link
Collaborator Author

I'll have another go at improving the wording a bit once the other issues are fixed.

@dergigi Should be ready for a re-review now. Please adapt the phrases to whatever you think fits best.

I guess 1 sats/vbyte can be transformed to 1001 sats/kilo-vbyte as the one and only special case. It will surprise user, though, but your are right that it feels better than not allowing it.

I'd be for that. To me it would be more surprising that I can't set it to 1 sat/vB.

Adapted! Thx! Still testing though (and waiting for feedback on JoinMarket-Org/joinmarket-clientserver#1360 (comment)) - but seems good so far.

I have never seen any form behave like that - is this really an issue?

Every single calculator app will parse whatever comma-style you use as a comma, no matter if you press . or ,. It's not a critical issue, just annoying. At least we want to be consistent, I tried to input what was suggested as an example (1,2) and it failed, because it should've been 1.2.

Sorry.. you know.. "don't trust, verify" and all that.. Gnome Calculator certainly does not do it. I have not addressed this as of yet, as this is the standard behaviour of html input elements with type=number.

@dergigi
Copy link
Contributor

dergigi commented Sep 29, 2022

Sorry.. you know.. "don't trust, verify" and all that.. Gnome Calculator certainly does not do it. I have not addressed this as of yet, as this is the standard behaviour of html input elements with type=number.

Notation and thus formatting (and thus input behavior) depends on i18n, see this and this and this.

Screenshot 2022-09-29 at 14-54-57 Decimal separator - Wikipedia

These local differences are, of course, why stuff like Intl.NumberFormat exists.

Support for all these i18n details in terms of input for HTML forms is still abysmal and will depend on your browser, see the table included in this post and this open issue at bugzilla.

Firefox respects the language setting of the page. Chrome imposes the decimal separator of the OS or the browser? Edge imposes the decimal point, regardless of the language settings of page or browser or OS. What a mess

Screenshot 2022-09-29 at 14 59 29

By the way, the suggestion in this post is to always explicitly set a locale such as lang="en-150" for forms, allowing supported browsers to pick up on these differences.


In any case, while I think this stuff is interesting, it is truly unimportant. We definitely have bigger fish to fry 😅

@theborakompanioni
Copy link
Collaborator Author

In any case, while I think this stuff is interesting, it is truly unimportant. We definitely have bigger fish to fry sweat_smile

Hey.. thanks for taking the time to explain. I tried setting lang="fr" and... indeed it worked ✔️ !
Albeit just for inserting data (,). After a reload, it will revert to using a dot (.).

Edit: Just works in Firefox, had no luck with Chrome.

image

I'll push the change for this form, and after merging this, will update all other forms to include the lang attribute!

@dergigi
Copy link
Contributor

dergigi commented Sep 29, 2022

I realize that JoinMarket uses the same parameter for "block target" and "sats/vByte", but it feels weird that one overrides the other in the interface.

Example: if I set the block target to 3 blocks, switch to sats/vByte, it will be set to 3 sats/vByte. If I change that now to 10 sats/vByte and switch back to the block target setting, the block target is now set to 10 blocks.

All that being said, I'm tempted to merge this soon and make these kinds of improvements in follow-up PRs.

@dergigi
Copy link
Contributor

dergigi commented Sep 29, 2022

Note to self: we should mention the defaults of all values in the interface.

Copy link
Contributor

@dergigi dergigi left a comment

Choose a reason for hiding this comment

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

Looks good to me ✅

Great job @theborakompanioni 👏

src/components/settings/FeeConfigModal.tsx Outdated Show resolved Hide resolved
src/components/settings/FeeConfigModal.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@dergigi dergigi left a comment

Choose a reason for hiding this comment

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

Final changes look good to me too. I'll go ahead and merge this now.

@dergigi dergigi merged commit 54dd396 into master Oct 4, 2022
@dergigi dergigi deleted the fees branch October 4, 2022 09:05
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.

3 participants