feat(quote): make settlement time dynamic #3834
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3834 +/- ##
==========================================
+ Coverage 82.29% 82.32% +0.02%
==========================================
Files 702 703 +1
Lines 26168 26229 +61
Branches 3544 3561 +17
==========================================
+ Hits 21535 21593 +58
- Misses 4567 4570 +3
Partials 66 66
Continue to review full report in Codecov by Sentry.
|
| it('returns default for bank account when no bounds are present', () => { | ||
| const quoteData = _.cloneDeep(mockFiatConnectQuotes[1]) as FiatConnectQuoteSuccess | ||
| quoteData.fiatAccount.BankAccount = { | ||
| ...quoteData.fiatAccount.BankAccount!, |
There was a problem hiding this comment.
| ...quoteData.fiatAccount.BankAccount!, | |
| ...(quoteData.fiatAccount.BankAccount ?? {}), |
nit (it's a test so it doesn't actually matter very much)
There was a problem hiding this comment.
This gives a typescript error, so I'll stick with the ! operator
| const daysToSettlementEstimation = ( | ||
| lowerBound: number, | ||
| upperBound: number | ||
| ): SettlementEstimation => { |
There was a problem hiding this comment.
it might make sense to be more defensive against the cases where lowerBound > upperBound or lowerBound < 0
There was a problem hiding this comment.
currently I think those would produce nonsensical outputs
There was a problem hiding this comment.
I added the checks to the start of the getTimeEstimation function, which should ensure the days/hours to settlement estimation functions receive good inputs
| if (lowerBound === upperBound || lowerBound === 0) { | ||
| return { | ||
| settlementTime: SettlementTime.LESS_THAN_X_DAYS, | ||
| upperBound, | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| settlementTime: SettlementTime.X_TO_Y_DAYS, | ||
| lowerBound, | ||
| upperBound, | ||
| } |
There was a problem hiding this comment.
it may not be worth the effort, but this does look almost identical between the two functions. There's probably a way to make it more DRY
There was a problem hiding this comment.
I'm not that concerned about it though
There was a problem hiding this comment.
Was struggling to cleanly make it more DRY, so am sticking with this for now
| lowerBound > upperBound | ||
| ) { | ||
| // payment method can only be bank or fc mobile money | ||
| return this.getPaymentMethod() === PaymentMethod.Bank |
There was a problem hiding this comment.
This is more of a nit, but I get a little nervous when assumptions are made about what type a value would be in a way that won't be caught by the TS compiler if that value were able to take on more types; e.g., once getPaymentMethod can plausibly return payment types other than Bank and FiatConnectMobileMoney, it's very probable that this line wouldn't get updated to reflect that.
Kind of a pain to deal with since PaymentMethod encompasses payment types that could never be on a FiatConnectQuote, and I'm not sure the best way to do this such that TS will complain if a value was added to the PaymentMethod enum and wasn't explicitly dealt with here. For now maybe add a TODO: to ensure that this gets updated once more payment types are possible?
There was a problem hiding this comment.
one option is to define a default that is not payment method-specific, and default to that if the payment method is not bank or mobile money
There was a problem hiding this comment.
yeah makes sense to me. out of curiosity, i wanted to see if it were possible, if we narrowed the enum type to FC-specific payment types, to write a switch with compile-time guarantees that all cases were covered. turns out it is possible, though the approach is a little bizarre.
There was a problem hiding this comment.
I decided to just write a TODO for now. I think the default approach works too but requires a small design discussion on what the default message should be
| lowerBound > upperBound | ||
| ) { | ||
| // payment method can only be bank or fc mobile money | ||
| return this.getPaymentMethod() === PaymentMethod.Bank |
There was a problem hiding this comment.
one option is to define a default that is not payment method-specific, and default to that if the payment method is not bank or mobile money
Description
Currently, the time estimation for a FiatConnect quote is set to fixed values. This PR makes it so that the time estimation is dynamic and based on data sent from the quote provider.
Test plan
Added unit tests and modified existing unit test
Related issues
Backwards compatibility
Yes.