zap-iOS icon indicating copy to clipboard operation
zap-iOS copied to clipboard

feature: set feelimit percentage in settings

Open chitowncrispy opened this issue 6 years ago • 5 comments

Description

The user is now able to set a fee limit percentage for any on-chain payments that they make or invoices that they fulfill. When the user goes to make an on-chain payment which has a fee above their threshold they are present a warning and are given the options to cancel or proceed with payment. When the user fulfills an invoice they are also presented a warning and are given the options to cancel or proceed but there is some additional precaution taken. On the LND API, we are now specifying a fee limit for the invoice fulfillment which guarantees that the fee will not be greater than a certain percentage.

Most of the logic for this feature is in the SendViewController.swift and SendViewModel.swift. The logic for determining the fee limit to eventually set on the API call can be found in the determineSendStatus function. A new series of callbacks and delegation takes place in order to process payments within this MVC pair.

Motivation and Context

This is to address issue #278.

How Has This Been Tested?

On Testnet I have done a series of payments on-chain as well as fulfilled different invoices.

On-chain I have sent small payments that have triggered the fee limit alert and they have processed fine. I have also tested that we can cancel the payment as if we were unwilling to pay the large fees and that also worked fine. Finally, I have sent payments large enough to not trigger the fee limit alert and they have also processed fine.

On the lightning network, I have tested that we can cancel the invoice fulfillment as if we were unwilling to pay the large fees and that worked fine. I also fulfilled an invoice for 100 sats which is right at our threshold amount and I observed that the fee limit set for that payment was 100%. The actual fee ended up much lower but I observed that the API call was capped. I also fulfilled small invoices for just over 100 sats which triggered the alert and those were sent without a fee limit as we have outlined in the logic. Finally, I fulfilled invoices that were large enough to not trigger a fee alert and I observed that the fee limit was sent correctly to what the user had specified in the settings.

I was also able to change the fee limit selected in the settings and see that persisted between different app launches.

Screenshots (if appropriate):

Screen Shot 2019-11-11 at 3 01 44 PM Screen Shot 2019-11-11 at 3 01 18 PM Screen Shot 2019-11-11 at 3 02 14 PM Screen Shot 2019-11-11 at 3 02 52 PM

Types of changes

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [x] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • [x] My code follows the code style of this project.
  • [ ] My change requires a change to the documentation.
  • [ ] I have updated the documentation accordingly.
  • [x] I have read the CONTRIBUTING document.
  • [x] I have added tests to cover my changes.
  • [x] All new and existing tests passed.

chitowncrispy avatar Nov 11 '19 03:11 chitowncrispy

Codecov Report

Merging #315 into master will increase coverage by 3.71%. The diff coverage is 61.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #315      +/-   ##
==========================================
+ Coverage   10.71%   14.42%   +3.71%     
==========================================
  Files         247      250       +3     
  Lines        9803    10237     +434     
==========================================
+ Hits         1050     1477     +427     
- Misses       8753     8760       +7
Impacted Files Coverage Δ
...ngs/Items/LightningRequestExpirySettingsItem.swift 0% <0%> (ø) :arrow_up:
...xtensions/Localizable/ExpiryTime+Localizable.swift 0% <0%> (ø) :arrow_up:
...brary/Scenes/Settings/SettingsViewController.swift 0% <0%> (ø) :arrow_up:
...y/Scenes/ModalDetail/Send/SendViewController.swift 0% <0%> (ø) :arrow_up:
Lightning/Services/TransactionService.swift 18.75% <0%> (ø) :arrow_up:
Library/Extensions/UIAlertController.swift 0% <0%> (ø) :arrow_up:
Library/Generated/strings.swift 14.89% <0%> (+7.91%) :arrow_up:
...izable/PaymentFeeLimitPercentage+Localizable.swift 0% <0%> (ø)
...s/Items/LightningPaymentFeeLimitSettingsItem.swift 0% <0%> (ø)
Library/Scenes/Settings/Settings.swift 75.34% <100%> (+75.34%) :arrow_up:
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update efa92e2...eac918e. Read the comment docs.

codecov-io avatar Nov 11 '19 03:11 codecov-io

Apple recommends not using yes or no for button titles.

Give alert buttons succinct, logical titles. The best button titles consist of one or two words that describe the result of selecting the button. As with all button titles, use title-style capitalization and no ending punctuation. To the extent possible, use verbs and verb phrases that relate directly to the alert title and message—for example, View All, Reply, or Ignore. Use OK for simple acceptance. Avoid using Yes and No.

(https://developer.apple.com/design/human-interface-guidelines/ios/views/alerts/)

maybe we can change them to "cancel" and "send" or "pay"?

ottosuess avatar Nov 12 '19 11:11 ottosuess

@JimmyMow what do you think about displaying the warning for on-chain payments?

With lightning payment it makes sense because you don't know the actual fee before sending and the fee grows in relation to the amount you send. With on-chain payments you see the actual fee before you send and it is only partially related to the amount you send.

Idk if a % based limit makes sense for on-chain. But i also don't see reasons against it. 😀

ottosuess avatar Nov 12 '19 11:11 ottosuess

Talked to @JimmyMow about whether to show warnings for on-chain transactions or not.

The reason why we need the warning is to protect users from getting tricked into overpaying fees for lightning payments.

That's why we came to the conclusion that it just complicates UX if we display the warning for on-chain as well.

ottosuess avatar Nov 14 '19 10:11 ottosuess

Added unit tests for the SendViewModel

chitowncrispy avatar Nov 15 '19 22:11 chitowncrispy