Skip to content

Conversation

@ovitrif
Copy link
Collaborator

@ovitrif ovitrif commented Sep 11, 2025

Closes: #309
Closes: #310

This PR refactors the custom numeric keyboard and its associated view for amount display.

Description

  • replace Keyboard component with new NumberPad written from scratch
  • refactor currency types, formatting methods and related components
  • remove redundant logic for handling node not running in receive amount screen
  • fix(e2e): use single text view for amount + placeholder
  • fix(receive): dont request liquidity for 0 LN channels
  • fix(receive): disable continue button if cjit amount is below min

Next steps:

  • Replace all AmountInput usages with the new NumberPad (ie. replace OS keyboard with custom one)
  • Replace NumberPadSimple used to for pin input with NumberPad(type = NumberPadType.SIMPLE)

Preview

Modern - Send Classic - Transfer
modernSend.mp4
classicTransfer.mp4

QA Notes

  • added unit tests for 45+ scenarios in AmountInputViewModelTest

Tests

  • numberpad - modern + fiat:
    • enter sats amount of 5 digits
    • switch to fiat > tap a key → no input changes (check both small and large text) ✓
    • delete all → placeholder = 0.00
    • enter 0., tap delete twice → entire input value deleted ✓
    • when input is empty tap . → input value = 0.
  • numberpad - classic:
  • try adding more than 8 decimals → should not ✓
  • enter 0., tap delete twice → expect entire value deleted
  • when input is empty tap . → expect 0. as value
  • fix for NumberPad issues in fiat mode #309:
    1. Decimal Point Input Blocking (double-tapping "." blocks input) → unit testes
    2. Decimal Digit Limitation (unlimited digits after decimal in fiat) → unit testes
  • fix for NumberPad issues in classic BTC denomination #310:
    1. Classic BTC numberpad shows "000" instead of "." button
    2. Broken Currency conversion calculation when switching units
  • regression: numpad in each flow:
    • SendAmountScreen: send > amount
    • ReceiveAmountScreen: on wallet wo. LN >receive > tap receive on LN switch
    • EditInvoiceScreen: receive > edit
    • SpendingAmountScreen: transfer to spending
  • regression: edit invoice:
    1. receive > edit > enter amount > show qr → edit → amount persists ✓
    2. receive > edit > enter amount > close sheet → reopen → amount resets ✓
  • regression: pin input::
    • settings > security > pin > sanity check pin input

@ovitrif ovitrif self-assigned this Sep 11, 2025
github-advanced-security[bot]

This comment was marked as outdated.

@ovitrif ovitrif marked this pull request as ready for review September 12, 2025 19:37
# Conflicts:
#	app/src/main/java/to/bitkit/ui/screens/wallets/send/SendAmountScreen.kt
#	app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt
@ovitrif ovitrif requested a review from jvsena42 September 13, 2025 15:24
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice!

Comment on lines +181 to 188
fun getCurrentRate(currency: String): FxRate {
val rates = _currencyState.value.rates
val rate = rates.firstOrNull { it.quote == currency }

fun getCurrentRate(currency: String): FxRate? {
return _currencyState.value.rates.firstOrNull { it.quote == currency }
return checkNotNull(rate) {
"Rate not found for currency: $currency in: ${rates.joinToString { it.quote }}"
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This could cause a crash if not inside a method with exception handling, but I saw that it is handled in all called places
  2. Could also be private, but I see it has been used in tests

So, it is OK for me to keep like this

Copy link
Collaborator Author

@ovitrif ovitrif Sep 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could cause a crash if not inside a method with exception handling, but I saw that it is handled in all called places

TY 👍🏻
Would make sense to make it private or expose only for testing; then it should be ok for it to throw.

Copy link
Member

@jvsena42 jvsena42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice refactor!
Tested the same flows + starting the app offline to check for crashes in a Mi A2

optional for future: could add a long press action to the NumberPadDeleteButton button

@jvsena42 jvsena42 merged commit d42a876 into master Sep 15, 2025
7 checks passed
@jvsena42 jvsena42 deleted the fix/custom-numpad branch September 15, 2025 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NumberPad issues in classic BTC denomination NumberPad issues in fiat mode

3 participants