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.

Sign up for GitHub

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

Jump to bottom

Fee Limit UX Improvements #1110

Merged
merged 9 commits into from
Nov 23, 2022
Merged

Fee Limit UX Improvements #1110

merged 9 commits into from
Nov 23, 2022

Conversation

otech47
Copy link
Contributor

@otech47 otech47 commented Oct 27, 2022 •
edited
Loading

Description

Builds directly on top of the branch in #1101

Addresses issue #1106 with a piecemeal approach. Currently this build is stable and tested but is missing a few of the additional improvements that will be added in a separate PR. That PR will build directly on top of this branch as a base in case we want to merge this one as-is.

  • make the field completely optional, leave it blank by default, and attempt the payment with some dynamic fee limit that doesn't exceed X% of the payment amount (X = 100% for 1-1000 sats, X=5% if >1000 sats)
  • allow users to enter either a fixed sat limit OR a percentage of the payment
  • leave a default value of 100 sat limit but use fee estimation to detect whether the fee will likely exceed 100 sats, and show an indicator alerting the user they may want increase the fee limit
  • allow the user to change their default fee limit in settings
  • show a proper error message when the route fails due to a low fee limit and allow the user to try again with a higher fee

This pull request is categorized as a:

  • New feature
  • Bug fix
  • Code refactor
  • Configuration change
  • Locales update
  • Quality assurance
  • Other

Checklist

  • I’ve run npm run tsc and made sure my code compiles correctly
  • I’ve run npm run lint and made sure my code didn’t contain any problematic patterns
  • I’ve run npm run prettier and made sure my code is formatted correctly
  • I’ve run npm run test and made sure all of the tests pass

Testing

If you modified or added a utility file, did you add new unit tests?

  • No, I’m a fool
  • Yes
  • N/A

I have tested this PR on the following platforms (please specify OS version and phone model/VM):

  • Android
  • iOS

I have tested this PR with the following types of nodes (please specify node version and API version where appropriate):

  • LND
  • c-lightning-REST
  • spark
  • Eclair
  • LndHub

Locales

  • I’ve added new locale text that requires translations
  • I’m aware that new translations should be made on the Zeus Transfix page and not directly to this repo

Third Party Dependencies and Packages

  • Contributors will need to run npm install after this PR is merged in
  • 3rd party dependencies have been modified:
    • verify that package.json and package-lock.json have been properly updated
    • verify that dependencies are installed for both iOS and Android platforms

Other:

  • Changes were made that require an update to the README
  • Changes were made that require an update to onboarding

@otech47
Change default fee from 10 to 100
3c1bbe1
@otech47
Remove unneeded placeholder values
aab6896 
Doesn't seem like this is needed since the
fee estimate is shown in its own field above anyway
and rarely would the user delete the input value
to gain information about the feeEstimate
@otech47
Hoist and refactor sendPayment function call
b5c62a8 
The new function enforces a newly exported SendPaymentReq
type before calling into the TransactionsStore

We can do additional logic checks here if we want our own
fee estimation without cluttering the render function
@otech47
Add new error message for routing failures
22a3243 
Allows the user to go back and try the payment again

for ZeusLN#1106
@otech47
Add fee recommendation when sending a payment
1ab6ede 
Indicates to the user if the fee estimate is higher than the limit set

for ZeusLN#1106
@otech47
Add default fee calculator in FeeUtils
a4f8a47
@otech47
Use default fee if the fee limit is not set
4f15488
@otech47
Use error theme color instead of warning
052efc0
@otech47
Add tests for new util function
3743046 
included tests above and below 1000 and
also an input expected to cause floating point error
to make sure the truncation occurs
@otech47 otech47 marked this pull request as ready for review October 27, 2022 15:21
@kaloudis
Copy link
Contributor

Looking squeaky clean

kaloudis
kaloudis approved these changes Nov 23, 2022
View reviewed changes
Copy link
Contributor

@kaloudis kaloudis left a comment

Choose a reason for hiding this comment

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

LGTM!

@kaloudis kaloudis merged commit 9a72356 into ZeusLN:master Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaloudis kaloudis kaloudis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@otech47 @kaloudis