zap-iOS
zap-iOS copied to clipboard
feature: set feelimit percentage in settings
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):
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.
Codecov Report
Merging #315 into master will increase coverage by
3.71%. The diff coverage is61.71%.
@@ 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 dataPowered by Codecov. Last update efa92e2...eac918e. Read the comment docs.
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"?
@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. 😀
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.
Added unit tests for the SendViewModel