feature: set feelimit percentage in settings #315
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
|
||
| import Foundation | ||
|
|
||
| public enum PaymentFeeLimitPercentage: Int, Codable, CaseIterable { |
There was a problem hiding this comment.
is this used in SwiftLnd or can it be moved to Library?
There was a problem hiding this comment.
Good catch. I moved it to Library
|
Apple recommends not using yes or no for button titles.
(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. 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. |
# Conflicts: # Library/Scenes/ModalDetail/Send/SendViewModel.swift
…pending upon the type of fee limit error
|
Added unit tests for the SendViewModel |
| import Foundation | ||
|
|
||
| public enum PaymentFeeLimitPercentage: Int, Codable, CaseIterable { | ||
| case zero = 0 |
There was a problem hiding this comment.
if i understand this correctly zero actually means "none"?
in the concrete implementation of lnd the value 0 means that there is no fee limit. but the enum should abstract concrete implementation details away. imo calling it none would make it a bit easier to understand.
| super.init() | ||
|
|
||
| Settings.shared.lightningRequestExpiry | ||
| .observeNext { [isSelectedOption] currenteExpiryTime in |
| @@ -45,4 +45,20 @@ extension UIAlertController { | |||
|
|
|||
| return alertController | |||
| } | |||
|
|
|||
| static func feeLimitAlertController(message: String, sendAction: @escaping () -> Void) -> UIAlertController { | |||
There was a problem hiding this comment.
I don't like my UIAlertController extension any more 😅
Imo it would make sense to remove it and put the Alerts in the ViewControllers where they are actually used.
(as long as both Alerts are only used in one place).
Otherwise the code just gets separated without much benefit. just makes it harder to reason about it.
There was a problem hiding this comment.
My other PR also has an alert added to this controller. I don't mind where they live but I would rather open an issue to refactor the alerts out of this extension and address it after the PRs are merged rather than refactor both PRs.
| case .zero: | ||
| return L10n.PaymentFeeLimitPercentage.none | ||
| default: | ||
| return "\(self.rawValue)%" |
There was a problem hiding this comment.
We should use the NumberFormatter() from the SendViewController here. Maybe put it in its own extension file.
There are many different ways to format the percent sign (https://en.wikipedia.org/wiki/Percent_sign#Correct_style) 😅
| let subtitleText = Observable<String?>(nil) | ||
| let isSubtitleTextWarning = Observable(false) | ||
| let sendStatus = Subject<Int?, SendError>() |
There was a problem hiding this comment.
So far i've tried avoiding use of ReactiveKit and stuck to Bond for several reasons:
- I wan't to limit the use of 3rd party libraries for security reasons and replace bond & reactive kit by the new native
combineframework in a few years. - I want the app to be approachable to every iOS dev so i don't want to many custom libraries that add additional hurdles for getting into the code.
- it's just too different to regular mvc / mvvc stuff and not very readable (for me). I once built a reactive app, went on holidays, and when i came back i couldn't understand anything i did and had to start from scratch. 😅
i'm fine merging this like it is. but in general i believe that reactive code has more drawbacks than benefits (at least in the specific case of zap iOS).
There was a problem hiding this comment.
I completely understand your points but I think there are a few things worth considering.
- The
Bondframework is built on top ofReactiveKitwhich inherently means we are usingReactiveKitwhereverBondis used. - The
combineframework seems eerily similar to aReactiveframework. I feel like going allReactivewould be the best path forward if the goal is to refactor everything out to use that in the future. - I didn't use
Bondin particular here because the way we seem to be usingBondis with anObservablewhich doesn't support the error state. We also don't have a direct UI item that changes based upon the result of this "function."
I feel like in order to make it easiest on contributors going forward we either define a clearer code architecture/styling guide detailing how to do "delegation/Reactive" programming or we begin the refactor now to pull out ReactiveKit as well as Bond. It's just hard to mandate against its use if we have it pulled in as a framework IMO.
| if feePercent > Decimal(Settings.shared.lightningPaymentFeeLimit.value.rawValue) { | ||
| self.sendStatus.receive(event: .failed(.feePercentageGreaterThanUserLimit(FeeInfo(feePercentage: feePercent, userFeeLimitPercentage: Settings.shared.lightningPaymentFeeLimit.value.rawValue, sendFeeLimitPercentage: 100)))) | ||
| } else { | ||
| self.sendStatus.receive(Settings.shared.lightningPaymentFeeLimit.value.rawValue) |
There was a problem hiding this comment.
you had a version with delegation before, right? to be honest i would prefer that. it's just more common and easier to understand.
There was a problem hiding this comment.
I didn't have a version with delegation. I wouldn't mind merging this as is and then we can have a discussion/define a clearer code styling/architecture guide and then refactor this later to however you see most fit.
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.swiftandSendViewModel.swift. The logic for determining the fee limit to eventually set on the API call can be found in thedetermineSendStatusfunction. 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
Checklist: