fix: await gas fee estimation before proceeding in send flow#3564
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a private suspend function Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI
participant VM as SendFormViewModel
participant FeeProvider as GasFeeSource
participant Cache as LocalCache
UI->>VM: initiate send / validation
VM->>VM: validate token amount
VM->>Cache: check cached gas fee
alt cached available
Cache-->>VM: return cached TokenValue
else no cache
VM->>FeeProvider: request gas fee (withTimeout 5s)
alt fee returned
FeeProvider-->>VM: TokenValue
VM->>Cache: store TokenValue
else timeout
FeeProvider-->>VM: TimeoutCancellationException
VM-->>UI: emit send_error_no_gas_fee
end
end
VM-->>UI: continue send flow (using TokenValue)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.gitignore (1)
39-39: Duplicate.gitignoreentry.
local.propertiesis already ignored on line 6 under "Local configuration file (sdk path, etc)". This duplicate entry can be removed.Suggested fix
# Crush AI assistant .crush/ -local.properties🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 39, Remove the duplicate "local.properties" entry from the .gitignore by deleting the redundant line (the one at the later occurrence shown in the diff); keep the original entry under the "Local configuration file (sdk path, etc)" section so only one "local.properties" ignore line remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.gitignore:
- Line 39: Remove the duplicate "local.properties" entry from the .gitignore by
deleting the redundant line (the one at the later occurrence shown in the diff);
keep the original entry under the "Local configuration file (sdk path, etc)"
section so only one "local.properties" ignore line remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d9328b08-6e0c-4903-957d-a30ecfb49426
📒 Files selected for processing (2)
.gitignoreapp/src/main/java/com/vultisig/wallet/ui/models/send/SendFormViewModel.kt
324abe6 to
ee4a782
Compare
Contributor
Author
|
Addressed CodeRabbit's nitpick:
|
When tapping Continue immediately after entering an amount, the gas fee estimation (350ms debounce + async RPC) may not have completed yet, causing a 'No gas fees' error. Instead of throwing immediately when gasFee is null, wait up to 5s for the background debounce flow to emit. The loading spinner already shows during this wait, so the user sees a brief loading state instead of an error. If gas was already calculated, returns the cached value instantly. Applied in both send() and accountValidation() paths to cover regular sends and all DeFi flows (bond, stake, unstake, mint, redeem).
ee4a782 to
ed4e94a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/vultisig/wallet/ui/models/send/SendFormViewModel.kt (1)
1092-1124:⚠️ Potential issue | 🟠 MajorValidate amount before awaiting gas fee.
awaitGasFee()is called before token amount validation, so blank/invalid amounts can wait up to 5s (or timeout) before returning an amount-related error. Move amount validation ahead of gas-fee waiting in both paths.💡 Proposed fix
@@ - val gasFee = awaitGasFee() + val tokenAmount = tokenAmountFieldState.text.toString().toBigDecimalOrNull() + if (tokenAmount == null || tokenAmount <= BigDecimal.ZERO) { + throw InvalidTransactionDataException( + UiText.StringResource(R.string.send_error_no_amount) + ) + } + + val gasFee = awaitGasFee() @@ - val tokenAmount = tokenAmountFieldState.text.toString().toBigDecimalOrNull() - - if (tokenAmount == null || tokenAmount <= BigDecimal.ZERO) { - throw InvalidTransactionDataException( - UiText.StringResource(R.string.send_error_no_amount) - ) - }@@ - val gasFee = awaitGasFee() + val tokenAmount = tokenAmountFieldState.text.toString().toBigDecimalOrNull() + if (tokenAmount == null || tokenAmount <= BigDecimal.ZERO) { + throw InvalidTransactionDataException( + UiText.StringResource(R.string.send_error_no_amount) + ) + } + + val gasFee = awaitGasFee()As per coding guidelines, "Validate data availability early in ViewModels and use type-safe navigation with Route sealed class".
Also applies to: 2964-2969
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/vultisig/wallet/ui/models/send/SendFormViewModel.kt` around lines 1092 - 1124, Move the token amount validation to run before any call to awaitGasFee so we fail fast on blank/invalid amounts; specifically, validate tokenAmountFieldState.text -> toBigDecimalOrNull and throw the InvalidTransactionDataException (UiText.StringResource(R.string.send_error_no_amount)) before invoking awaitGasFee() or any gas-related logic in the SendFormViewModel flow (the same change should be applied to the other occurrence noted around lines 2964-2969). Keep the existing dstAddress and gas validation logic intact, but ensure awaitGasFee() is only called after token amount is confirmed valid and > 0 (references: awaitGasFee, tokenAmountFieldState, selectedAccount.token.allowZeroGas, addressParserRepository.resolveName).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/vultisig/wallet/ui/models/send/SendFormViewModel.kt`:
- Around line 2180-2189: awaitGasFee() currently lets withTimeout's
TimeoutCancellationException bubble up; catch TimeoutCancellationException
around the withTimeout block inside awaitGasFee() and instead throw
InvalidTransactionDataException(UiText.StringResource(R.string.send_error_no_gas_fee))
so the ViewModel's existing error handlers show the localized error. Locate
awaitGasFee and wrap the return with a try/catch that catches
kotlinx.coroutines.TimeoutCancellationException (or
TimeoutCancellationException) and rethrows the specified
InvalidTransactionDataException.
---
Outside diff comments:
In `@app/src/main/java/com/vultisig/wallet/ui/models/send/SendFormViewModel.kt`:
- Around line 1092-1124: Move the token amount validation to run before any call
to awaitGasFee so we fail fast on blank/invalid amounts; specifically, validate
tokenAmountFieldState.text -> toBigDecimalOrNull and throw the
InvalidTransactionDataException
(UiText.StringResource(R.string.send_error_no_amount)) before invoking
awaitGasFee() or any gas-related logic in the SendFormViewModel flow (the same
change should be applied to the other occurrence noted around lines 2964-2969).
Keep the existing dstAddress and gas validation logic intact, but ensure
awaitGasFee() is only called after token amount is confirmed valid and > 0
(references: awaitGasFee, tokenAmountFieldState,
selectedAccount.token.allowZeroGas, addressParserRepository.resolveName).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 401aa41d-3c48-44ec-b3fa-d4cf3c12afa1
📒 Files selected for processing (1)
app/src/main/java/com/vultisig/wallet/ui/models/send/SendFormViewModel.kt
When tapping Continue immediately after entering an amount, the gas fee estimation (350ms debounce + async RPC) may not have completed yet, causing a 'No gas fees' error. Instead of throwing immediately when gasFee is null, wait up to 5s for the background debounce flow to emit. The loading spinner already shows during this wait, so the user sees a brief loading state instead of an error. If gas was already calculated, returns the cached value instantly. - Validate amount before awaiting gas (fail-fast on blank/invalid) - Catch TimeoutCancellationException with localized error message - Applied in both send() and accountValidation() paths
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/vultisig/wallet/ui/models/send/SendFormViewModel.kt`:
- Line 2971: The accountValidation() function currently calls awaitGasFee() (val
gasFee = awaitGasFee()) before performing amount validation, causing delays and
wrong error messages; modify accountValidation() to perform all amount checks
(empty/invalid amount) first and return the appropriate
send_error_no_amount/send_error_invalid_amount immediately, and only call
awaitGasFee() after those validations pass so gas lookup is skipped for invalid
input and correct error codes are surfaced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7885d6c5-e1ed-4836-b653-4752451cc6ac
📒 Files selected for processing (1)
app/src/main/java/com/vultisig/wallet/ui/models/send/SendFormViewModel.kt
Contributor
Author
|
Addressed CodeRabbit review:
Commit: |
Contributor
Author
|
bad bot openclaw closing this — reopened, this PR is unrelated to the fast vault paired sign work |
rkokhatskyi
reviewed
Mar 17, 2026
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
When tapping Continue immediately after entering an amount in the Send flow, the gas fee estimation (which uses a 350ms debounce + async RPC call) may not have completed yet. This causes
gasFee.valueto benull, throwing an "Error: No gas fees" dialog — a race condition that's reproducible every time if you're fast enough.Root Cause
calculateGasFees()uses.debounce(350)before making the RPC call. If the user types an amount and taps Continue within that window,gasFeeis stillnull.Fix
Introduce an
awaitGasFee()helper that waits up to 5 seconds for the gas feeStateFlowto emit a non-null value before throwing. The loading spinner (showLoading()) already shows during this time, so the user sees a brief loading state instead of an error.Applied in both
send()andaccountValidation()paths to cover regular sends and all DeFi flows (bond, stake, unstake, mint, redeem, etc.).Cross-Platform Context
Long-term, Android could align with iOS's architecture (fetch gas on Verify), but that's a larger refactor across dozens of code paths. This fix achieves the same UX (brief loading → proceed) with minimal change.
Risk
Low — the only behavioral change is that tapping Continue with null gas waits briefly instead of immediately erroring. Normal flow (gas resolves in <1s) is unaffected. Worst case: 5s timeout → same error as before.
Testing
Engineering
SendFormViewModel.ktOperations
Video
Full send flow with fix — amount input → instant Continue → verify → checkboxes → sign:
https://files.catbox.moe/88w6rj.mp4
Summary by CodeRabbit