-
Notifications
You must be signed in to change notification settings - Fork 1
SpendingAmountScreen state handling improvement #264
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.
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
Conversation
app/src/main/java/to/bitkit/ui/screens/transfer/SpendingAmountScreen.kt
Dismissed
Show dismissed
Hide dismissed
app/src/main/java/to/bitkit/ui/screens/transfer/SpendingAmountScreen.kt
Dismissed
Show dismissed
Hide dismissed
app/src/main/java/to/bitkit/ui/screens/transfer/SpendingAmountScreen.kt
Dismissed
Show dismissed
Hide dismissed
|
I'll do the TransferViewModel clean up and Unit tests in other branch to don't obfuscate the fixes from this one |
ovitrif
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice work!
Found only an issue which needs to be addressed IMHO, related to the max amount fee calculation
| val totalFeeFromAvailableAmount = lightningRepo.calculateTotalFee(amountSats = onChainBalance) | ||
| .getOrDefault(DEFAULT_TX_FEE.toULong()) | ||
|
|
||
| // Get the max available balance discounting onChain fee | ||
| val availableAmount = onChainBalance - totalFeeFromAvailableAmount |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We always get NodeException.InsufficientFunds exception when trying to calculateTotalFee for the entire onChainBalance.
That's the reason I added walletRepo.getMaxSendAmount(), which is not perfect either (I got sometimes failure for the onChainBalance - 1000sat buffer solution), but still much less error-prone:
bitkit-android/app/src/main/java/to/bitkit/repositories/WalletRepo.kt
Lines 318 to 336 in 46d613a
| suspend fun getMaxSendAmount(): ULong = withContext(bgDispatcher) { | |
| val totalOnchainSats = balanceState.value.totalOnchainSats | |
| if (totalOnchainSats == 0uL) { | |
| return@withContext 0uL | |
| } | |
| try { | |
| val minFeeBuffer = 1000uL | |
| val amountSats = (totalOnchainSats - minFeeBuffer).coerceAtLeast(0uL) | |
| val fee = lightningRepo.calculateTotalFee(amountSats).getOrThrow() | |
| val maxSendable = (totalOnchainSats - fee).coerceAtLeast(0uL) | |
| return@withContext maxSendable | |
| } catch (_: Throwable) { | |
| Logger.debug("Could not calculate max send amount, using as fallback 90% of total", context = TAG) | |
| val fallbackMax = (totalOnchainSats.toDouble() * 0.9).toULong() | |
| return@withContext fallbackMax | |
| } | |
| } |
Much recommended to use the getMaxSendAmount() here as well, that way we won't ALWAYS end up with the DEFAULT_TX_FEE.
PS. I informed @coreyphillips about this issue but it's not the highest priority thus looking into it will be delayed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better, though I still get error: Invalid request format when selecting the max amount
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, because Invalid request format comes from bitkit-core's http call to Blocktank staging.
ovitrif
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice refactor, the changed code works good!
Added an remark for improvement, feel free to address outside of this PR though.
Tested
- Normal Success flow
- Max amount
- 25%
- Error value higher than max amount
| if (sats > _spendingUiState.value.maxAllowedToSend) { | ||
| setTransferEffect( | ||
| TransferEffect.ToastError( | ||
| title = context.getString(R.string.lightning__spending_amount__error_max__title), | ||
| description = context.getString( | ||
| R.string.lightning__spending_amount__error_max__description | ||
| ).replace("{amount}", _spendingUiState.value.maxAllowedToSend.toString()), | ||
| ) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the RN app, the amount will not update if it would exceed the max limit.
I think we can easily do the same here with this code right after the effect:
_spendingUiState.update { it.copy(overrideSats = it.satsAmount) }
return
Related to #229
Description
Preview
transfer_tested.mp4
QA Notes
Tested