fix(fiat-onramp): banxa onramp flow erroneous failure popup and return to komodo button#2608
fix(fiat-onramp): banxa onramp flow erroneous failure popup and return to komodo button#2608
Conversation
included coderabbitai suggestions in main release PR
Banxa "Return to Komodo" button attempts to set `window.parent.location`, which is not allowed in sandboxed iframes, and is not desired behaviour. In doing so, it would require the user to log in again after the redirect
WalkthroughThis update introduces significant refactoring and enhancements to the fiat onramp flow, including both backend Dart logic and frontend HTML/JavaScript components. The fiat widget HTML is modularized, with improved sandboxing and message handling. Dart code sees changes to enums, state management, error handling, and type safety, especially around fiat order status and webview integration. New mechanisms for secure webview settings and flexible dialog display modes are added, supporting dialog, fullscreen, and external tab/browser. Several method signatures are updated for clarity, type safety, and consistency, and error handling is strengthened throughout the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant FiatWidget (iframe)
participant WebViewDialog
User->>App: Initiates fiat onramp
App->>FiatWidget: Loads iframe with base64-decoded URL
FiatWidget->>App: Sends message (order status, navigation, etc.)
App->>WebViewDialog: Opens webview in dialog/fullscreen/new tab (mode-dependent)
WebViewDialog->>FiatWidget: Displays provider UI
FiatWidget->>App: Forwards status/events via postMessage
App->>User: Updates UI based on status (success, failure, pending)
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Visit the preview URL for this PR (updated for commit efc6cea): https://walletrc--pull-2608-merge-8ugezf4h.web.app (expires Fri, 16 May 2025 11:47:24 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f66a4ff03faa546f12f0ae5a841bd9eff2714dcc |
Use FiatOrderStatus enum, add the missing status, and normalise the remaining strings given that the input string is lowercased
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes issues in the Banxa onramp flow where an erroneous failure popup was displayed and the "Return to Komodo" button did not work. It opens the Banxa page in a new tab, refactors URL parameter retrieval and message event handling, and updates documentation/comments to improve maintainability and clarity.
- Updated iframe sandbox configuration with fallback messaging.
- Refactored message handling with new _komodo* functions.
- Improved documentation and error handling flow.
Files not reviewed (14)
- lib/bloc/fiat/banxa_fiat_provider.dart: Language not supported
- lib/bloc/fiat/base_fiat_provider.dart: Language not supported
- lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart: Language not supported
- lib/bloc/fiat/fiat_onramp_form/fiat_form_state.dart: Language not supported
- lib/bloc/fiat/fiat_order_status.dart: Language not supported
- lib/bloc/fiat/fiat_repository.dart: Language not supported
- lib/bloc/fiat/models/fiat_buy_order_error.dart: Language not supported
- lib/bloc/fiat/models/fiat_buy_order_info.dart: Language not supported
- lib/bloc/fiat/models/fiat_price_info.dart: Language not supported
- lib/bloc/fiat/payment_status_type.dart: Language not supported
- lib/bloc/fiat/ramp/ramp_fiat_provider.dart: Language not supported
- lib/views/fiat/fiat_form.dart: Language not supported
- lib/views/fiat/fiat_provider_web_view_settings.dart: Language not supported
- lib/views/fiat/webview_dialog.dart: Language not supported
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (2)
lib/bloc/fiat/fiat_order_status.dart (1)
28-33:⚠️ Potential issueInclude
pendingPaymentinisSubmittingcheck
pendingPaymentrepresents an intermediate “work-in-progress” state that still requires user action, yet it is not covered byisSubmitting. Consumers that rely onisSubmitting(e.g. disabling the Buy button or showing spinners) will therefore treatpendingPaymentlike an idle state and may let the user re-submit the form or close the dialog prematurely.- bool get isSubmitting => - this == FiatOrderStatus.inProgress || this == FiatOrderStatus.submitted; + bool get isSubmitting => + this == FiatOrderStatus.inProgress || + this == FiatOrderStatus.submitted || + this == FiatOrderStatus.pendingPayment;lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart (1)
302-317:⚠️ Potential issueSwitch statement is missing
break;→ illegal fall-through & wrong logic
Dart does not allow implicit fall-through betweencaselabels. This block will not compile and, even if allowed, would overwriteupdatedStatus(e.g. a widgetClose event becomes inProgress).- switch (eventType) { - case PaymentStatusType.widgetCloseRequestConfirmed: - case PaymentStatusType.widgetClose: - case PaymentStatusType.widgetCloseRequest: - updatedStatus = FiatOrderStatus.windowCloseRequested; - case PaymentStatusType.purchaseCreated: - updatedStatus = FiatOrderStatus.inProgress; - case PaymentStatusType.paymentStatus: - final status = data['status'] as String? ?? 'declined'; - updatedStatus = FiatOrderStatus.fromString(status); - ... - break; - } + switch (eventType) { + case PaymentStatusType.widgetCloseRequestConfirmed: + case PaymentStatusType.widgetClose: + case PaymentStatusType.widgetCloseRequest: + updatedStatus = FiatOrderStatus.windowCloseRequested; + break; + case PaymentStatusType.purchaseCreated: + updatedStatus = FiatOrderStatus.inProgress; + break; + case PaymentStatusType.paymentStatus: + final status = data['status'] as String? ?? 'declined'; + updatedStatus = FiatOrderStatus.fromString(status); + break; + default: + break; + }
🧹 Nitpick comments (6)
lib/bloc/fiat/fiat_order_status.dart (1)
40-60: Normalisation could be more robust
status.toLowerCase()is a good start, but many providers sometimes return trimmed / spaced variants (e.g. “Pending Payment”).extraVerificationin the docs is spelt with a capital “V”; the lower-case mapping works, but a quickreplaceAll(RegExp(r'\s'), '')would make the parser future-proof against white-space.- final normalized = status.toLowerCase(); + final normalized = status.toLowerCase().replaceAll(RegExp(r'\s+'), '');lib/bloc/fiat/fiat_onramp_form/fiat_form_state.dart (1)
122-161:copyWithsignature:ValueGetterpattern can’t set fields to a non-null defaultUsing lazy
ValueGetteris clever for heavy computations, but:
- Call-sites that want to null-out
selectedAssetAddressmust wrap() => null, which is unintuitive.- The new pattern is inconsistent with other simple fields (
checkoutUrl,orderId) that still accept nullable values directly.Consider overloading for clarity or reverting to nullable parameters and documenting expensive cases separately.
lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart (1)
157-165: Force-unwrap ofselectedAssetAddressrisks runtime crash
state.selectedAssetAddress!is force-unwrapped four times. AlthoughgetFormIssue()currently prevents_onSubmitFormwhen it’snull, future edits might bypass that guard. Consider a safer access pattern:- final newOrder = await _fiatRepository.buyCoin( - accountReference: state.selectedAssetAddress!.address, + final address = state.selectedAssetAddress?.address; + if (address == null) { + _log.severe('submit called without a wallet address'); + return; + } + final newOrder = await _fiatRepository.buyCoin( + accountReference: address,assets/web_pages/fiat_widget.html (1)
40-67: Unused parameter & double query – minor cleanup
_komodoSetIframeUrlFromParamsaccepts aparamsargument but immediately ignores it and re-reads the query string. This is harmless but confusing.-function _komodoSetIframeUrlFromParams(params) { - const urlParam = _komodoGetUrlParameter('fiatUrl'); +function _komodoSetIframeUrlFromParams() { + const urlParam = _komodoGetUrlParameter('fiatUrl');Also, registering the same handler via both
window.onmessage = …andaddEventListeneris redundant. Retain only theaddEventListenerform unless you specifically need legacy support.lib/views/fiat/webview_dialog.dart (1)
60-64:launchURLStringsuccess not awaited or reported
launchURLStringreturns a boolean indicating success. Swallowing that result hides failures (e.g., popup blocked). Suggest logging or surfacing an error:- await launchURLString(url, inSeparateTab: true); + final launched = await launchURLString(url, inSeparateTab: true); + if (!launched) { + debugPrint('Failed to open $url in external browser'); + }lib/bloc/fiat/ramp/ramp_fiat_provider.dart (1)
301-303: UI control parameter additionThe
hideExitButton: 'true'parameter addresses the "Return to Komodo" button functionality issue mentioned in the PR objectives. This forces the Ramp UI to hide its exit button, likely because the flow now opens in a new tab where the browser's back/close functionality can be used instead.The commented-out
variantparameter seems to be preparation for potentially changing how the UI is displayed (hosted, desktop, mobile, etc.). Consider adding a comment explaining why this is commented out and under what conditions it should be uncommented.'hideExitButton': 'true', - // 'variant': 'hosted', // desktop, mobile, auto, hosted-mobile + // 'variant': 'hosted', // Options: desktop, mobile, auto, hosted-mobile + // Note: Uncomment when we migrate to a newer Banxa API version that supports this parameter
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
assets/web_pages/fiat_widget.html(1 hunks)lib/bloc/fiat/banxa_fiat_provider.dart(1 hunks)lib/bloc/fiat/base_fiat_provider.dart(2 hunks)lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart(17 hunks)lib/bloc/fiat/fiat_onramp_form/fiat_form_state.dart(7 hunks)lib/bloc/fiat/fiat_order_status.dart(3 hunks)lib/bloc/fiat/fiat_repository.dart(1 hunks)lib/bloc/fiat/models/fiat_buy_order_error.dart(1 hunks)lib/bloc/fiat/models/fiat_buy_order_info.dart(3 hunks)lib/bloc/fiat/models/fiat_price_info.dart(2 hunks)lib/bloc/fiat/payment_status_type.dart(2 hunks)lib/bloc/fiat/ramp/ramp_fiat_provider.dart(8 hunks)lib/views/fiat/fiat_form.dart(4 hunks)lib/views/fiat/fiat_provider_web_view_settings.dart(1 hunks)lib/views/fiat/webview_dialog.dart(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Desktop (linux)
- GitHub Check: Test web-app-macos
- GitHub Check: Build Desktop (windows)
- GitHub Check: Build Mobile (Android)
- GitHub Check: build_and_preview
- GitHub Check: Build Desktop (macos)
- GitHub Check: Test web-app-linux-profile
- GitHub Check: validate_code_guidelines
- GitHub Check: unit_tests
- GitHub Check: Build Mobile (iOS)
🔇 Additional comments (22)
lib/views/fiat/fiat_provider_web_view_settings.dart (4)
5-10: Well-structured trusted domain configurationThe explicit list of trusted domains is a good security practice, allowing fine-grained control over which external domains can be loaded in the WebView. The regex patterns accommodate the necessary variations while maintaining security boundaries.
18-20: Good default parameter implementationUsing a default parameter for
trustedDomainFilterswith the predefined constants makes the method flexible while providing secure defaults. This approach allows for easy customization when needed without compromising security by default.
24-38: Comprehensive sandbox configuration with clear documentationThe sandbox configuration is well-documented with explanations for each permission. The deliberate exclusion of
ALLOW_TOP_NAVIGATIONenhances security by preventing parent navigation hijacking, which is an important security consideration for payment flows.
39-61: Strong security implementation with content blockingThe implementation of content blockers with a default deny-all policy followed by explicit allowances for trusted domains follows security best practices. This defense-in-depth approach ensures that only explicitly trusted content can be loaded.
lib/bloc/fiat/models/fiat_buy_order_error.dart (1)
21-24: Good addition of specific error type for parsing failuresAdding a dedicated constructor for parsing errors improves error handling specificity and provides a standardized way to handle parsing failures. This will help with debugging and appropriate error messaging in the UI.
lib/bloc/fiat/banxa_fiat_provider.dart (1)
27-27: Improved status parsing with centralized logicReplacing the custom parsing logic with a call to
FiatOrderStatus.fromStringcentralizes the status parsing responsibility and ensures consistent handling across the application. This should help resolve issues with erroneous failure popups mentioned in the PR objectives.lib/bloc/fiat/payment_status_type.dart (3)
2-10: Consistent string literal styleThe change to use single quotes for string literals improves consistency with Dart style guidelines. This kind of consistency makes the code more maintainable.
14-15: Improved field declaration positioningRepositioning the field declaration improves code readability and follows standard Dart style guidelines.
24-24: Enhanced error handling with const FormatExceptionUsing a
constFormatException is more efficient as it doesn't create a new object each time the exception is thrown. This is a small but valuable optimization.lib/bloc/fiat/models/fiat_price_info.dart (1)
24-30: Repositioned static zero instanceThe static
zeroinstance was moved after thefromJsonconstructor for better code organization. The functionality remains unchanged.lib/bloc/fiat/base_fiat_provider.dart (1)
71-73: Improved URI path handlingThe code now properly handles endpoints with leading slashes, avoiding potential double-slash issues when constructing the final API URL.
lib/bloc/fiat/models/fiat_buy_order_info.dart (2)
24-40: Renamed and improved constructor semanticsThe constructor was renamed from
info()tofromCheckoutUrl(String url)to better reflect its purpose, and it now properly initializes thecheckoutUrlwith the provided URL.The name now clearly indicates the constructor's behavior, improving code readability.
42-58: Renamed and repurposed empty constructorThe constructor was renamed from
fromCheckoutUrl(String url)toempty()which better reflects its purpose of creating an empty instance with default values and an empty checkout URL.lib/views/fiat/fiat_form.dart (1)
208-217: Guard against empty or malformed checkout URLs
stateSnapshot.checkoutUrlis passed straight toWebViewDialog.show.
If the Bloc ever emitssubmittedbefore the URL is resolved (e.g. slow network), an empty string will yield a blank web-view or, on Linux, an external browser error.if (status == FiatOrderStatus.submitted) { - await WebViewDialog.show( + if (stateSnapshot.checkoutUrl.isEmpty) { + Logger('FiatForm').warning('Checkout URL missing for submitted order'); + return; + } + await WebViewDialog.show(lib/bloc/fiat/ramp/ramp_fiat_provider.dart (8)
36-37: Type safety improvementChanging the parameter type from
ICurrencyto the more specificCryptoCurrencyimproves type safety by ensuring that only crypto currencies can be used with this method. This aligns with the method implementation which requires crypto-specific properties.
42-43: Type safety improvementRefining the parameter type from the generic
ICurrencyto the concreteCryptoCurrencyensures proper type safety in the API request, preventing runtime errors that could occur if an incompatible currency type was passed.
54-56: Helpful comment additionThe added comment clarifies the expected format for the
fiatValueparameter, making it clear why the conversion to double is necessary. This improves code maintainability.
64-65: Type safety improvementSame pattern as other methods - replacing
ICurrencywithCryptoCurrencyimproves type safety and makes the API contract more explicit.
149-152: Defensive runtime type checkingThe added type check with a clear error message ensures that if the method is called with an incorrect currency type, it will fail fast with a descriptive error instead of failing later with a potentially cryptic error.
219-224: Improved fee validation logicThe refactored method extracts the fee calculation for better readability and adds defensive validation to ensure the fee ratio is valid (less than 1). This prevents potential division by zero or negative values that could result from invalid fee configurations.
243-246: Consistent type checkingThis type check maintains consistency with the pattern established in other methods, ensuring that the API is used correctly with appropriate parameter types.
286-289: Consistent type checkingSame pattern applied to the
buyCoinmethod, maintaining consistency across the API surface and preventing type-related errors.
Assumption: redirect occurs in new tab or popup webview dialog, so the desired behaviour is to return to the KW tab rather than navigating back to a home page that requires the user to sign in again
- increases the number of coins available for onramp, mostly from the banxa supported coins list - revert regression in the `getAbbr` function, although a better approach would be a good idea when migrating to the SDK
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/bloc/fiat/models/fiat_buy_order_info.dart (1)
60-97: Robust error handling for JSON parsing.The
fromJsonfactory now explicitly checks for the presence of nesteddataandorderkeys, returning an error state with a descriptive message when the expected structure is missing. This defensive approach prevents null reference exceptions during JSON deserialization.
🧹 Nitpick comments (4)
assets/web_pages/fiat_widget.html (4)
5-6: Add a charset meta tag for proper encoding.Without an explicit
<meta charset>declaration, some browsers may misinterpret the page’s character encoding, leading to garbled text.Apply this diff:
<title>Fiat OnRamp</title> - <meta name="viewport" content="width=device-width, initial-scale=1.0"> + <meta charset="UTF-8"> + <meta name="viewport" content="width=device-width, initial-scale=1.0">
28-30: Include a descriptivetitleattribute on the iframe for accessibility.Adding a
titlehelps screen readers understand the purpose of the embedded content.Example diff:
- <iframe id="fiat-onramp-iframe" - sandbox="allow-forms allow-scripts allow-same-origin allow-top-navigation allow-top-navigation-by-user-action allow-popups" - src=""> + <iframe + id="fiat-onramp-iframe" + title="Fiat On-Ramp Widget" + sandbox="allow-forms allow-scripts allow-same-origin allow-top-navigation allow-top-navigation-by-user-action allow-popups" + src="">
38-41: UseDOMContentLoadedinstead ofloadfor faster initialization.The
loadevent waits for all resources (images, iframes) to finish loading, delaying your iframe setup. Switching toDOMContentLoadedensures the script runs as soon as the DOM is ready.- window.addEventListener('load', function () { - _komodoSetIframeUrlFromParams(); - }); + window.addEventListener('DOMContentLoaded', () => { + _komodoSetIframeUrlFromParams(); + });
113-116: Remove or scope the debugconsole.logto avoid leaking sensitive data.Logging raw messages may expose PII or internal transaction details in the browser console. Consider removing it or gating it behind a debug flag.
- // flutter_inappwebview - console.log(messageString); + // flutter_inappwebview + // TODO: remove or wrap in debug check before logging + // console.debug(messageString);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
assets/web_pages/checkout_status_redirect.html(1 hunks)assets/web_pages/fiat_widget.html(1 hunks)lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart(23 hunks)lib/bloc/fiat/fiat_onramp_form/fiat_form_state.dart(7 hunks)lib/bloc/fiat/fiat_order_status.dart(3 hunks)lib/bloc/fiat/fiat_repository.dart(2 hunks)lib/bloc/fiat/models/fiat_buy_order_info.dart(4 hunks)lib/bloc/fiat/models/i_currency.dart(1 hunks)lib/shared/widgets/coin_item/coin_item.dart(1 hunks)lib/views/fiat/fiat_asset_icon.dart(1 hunks)lib/views/fiat/fiat_currency_list_tile.dart(1 hunks)lib/views/fiat/fiat_form.dart(3 hunks)lib/views/fiat/fiat_inputs.dart(1 hunks)lib/views/fiat/fiat_payment_methods_grid.dart(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- lib/views/fiat/fiat_currency_list_tile.dart
- lib/shared/widgets/coin_item/coin_item.dart
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/bloc/fiat/fiat_repository.dart
- lib/views/fiat/fiat_form.dart
- lib/bloc/fiat/fiat_onramp_form/fiat_form_state.dart
🧰 Additional context used
🧠 Learnings (1)
lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart (3)
Learnt from: takenagain
PR: KomodoPlatform/komodo-wallet#2608
File: lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart:2-3
Timestamp: 2025-05-01T21:00:36.970Z
Learning: It's acceptable to use unconditional `dart:io` imports in the Komodo wallet codebase when the usage is guarded by `!kIsWeb` conditional checks that prevent the platform-specific code from being executed in web environments.
Learnt from: takenagain
PR: KomodoPlatform/komodo-wallet#2608
File: lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart:208-225
Timestamp: 2025-05-01T20:56:36.180Z
Learning: The `kIsWasm` constant is available in Flutter version 3.22+ (released May 2024) and is used to detect if an application is running on the WebAssembly backend. When writing platform-specific code, use both `kIsWeb` and `kIsWasm` checks to properly handle both JavaScript and WebAssembly compilation targets.
Learnt from: takenagain
PR: KomodoPlatform/komodo-wallet#2608
File: lib/views/fiat/webview_dialog.dart:50-55
Timestamp: 2025-05-01T21:00:56.962Z
Learning: `kIsWasm` is a valid constant in Flutter 3.22+ (released May 2024) that detects if an application is running on WebAssembly. It complements `kIsWeb` for more precise platform detection.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Desktop (linux)
- GitHub Check: Build Mobile (Android)
- GitHub Check: Build Desktop (windows)
- GitHub Check: Test web-app-macos
- GitHub Check: build_and_preview
- GitHub Check: Build Mobile (iOS)
- GitHub Check: validate_code_guidelines
- GitHub Check: Build Desktop (macos)
- GitHub Check: Test web-app-linux-profile
- GitHub Check: unit_tests
🔇 Additional comments (28)
lib/views/fiat/fiat_inputs.dart (1)
138-139: Appropriate use of getAbbr() method for consistent currency abbreviation handling.The switch from using
widget.initialFiat.symboltowidget.initialFiat.getAbbr()ensures consistent currency abbreviation handling, which helps resolve the coin abbreviation parsing regression mentioned in the PR objectives.lib/views/fiat/fiat_payment_methods_grid.dart (1)
46-47: Good change for consistent currency abbreviation handling.Replacing direct symbol access with the
getAbbr()method ensures consistent display of currency abbreviations in the "no options to purchase" message, aligning with the coin abbreviation handling improvements in this PR.lib/views/fiat/fiat_asset_icon.dart (3)
27-27: Proper use of getAbbr() method for fiat currency icon.Using
currency.getAbbr()instead of direct symbol access provides consistent abbreviation handling for fiat currency icons.
30-32: Good documentation with clear TODO comment.The TODO comment clearly explains the inconsistency in icon layout handling between
CoinItemand other icon widgets, which will be valuable for future standardization efforts.
35-35: Consistent use of getAbbr() for SDK asset retrieval.Using
currency.getAbbr()for SDK asset retrieval ensures that the correct abbreviation is used when looking up assets, which is critical for proper coin recognition.lib/bloc/fiat/models/i_currency.dart (1)
84-104: Strong implementation for handling special case currency abbreviations.The enhanced
getAbbr()method properly addresses the inconsistency between provider-returned symbols and configuration storage. The detailed TODO comment explains the issue well, and the implementation handles known exception cases appropriately.This change is central to fixing the coin abbreviation/ticker parsing regression mentioned in the PR objectives, ensuring more coins are correctly available in the selection dropdown.
assets/web_pages/checkout_status_redirect.html (3)
12-17: Code formatting improvements look good.The parameter extraction logic has been reformatted with consistent indentation and variable naming patterns.
26-37: Improved handling of window communication.The revised code properly handles the parent window communication, including the addition of a fallback for when status is null. The code now also focuses the opener window after sending the message, which improves the user experience.
39-41: Clearer window closing strategy.The added comments explain the rationale behind using
window.close()for both Ramp and Banxa checkout pages. This aligns with the PR objectives of improving the Banxa onramp flow and fixing dialog/popup z-order issues.lib/bloc/fiat/models/fiat_buy_order_info.dart (2)
24-40: Constructor naming improvements enhance code clarity.The renamed constructors better reflect their purpose:
fromCheckoutUrlfor creating an instance with a provided URL, andemptyfor creating an empty instance.
42-58: Renamed constructor provides semantic clarity.Renaming this constructor to
empty()clearly communicates its purpose of creating an empty instance with default values.lib/bloc/fiat/fiat_order_status.dart (5)
2-3: Added logging for better diagnostics.The import of the logging package allows for better error reporting when handling unknown status values.
5-6: Improved status naming for better semantic clarity.Renaming
pendingtoinitialbetter describes the state where the user has not yet started the payment process.
12-14: Added new status for better flow representation.The new
pendingPaymentstatus correctly differentiates between the initial state and a state where user action is required to complete payment.
28-33: Updated isSubmitting getter to include new status.The getter now correctly includes
pendingPaymentas a submitting state, ensuring consistent behavior across the application.
38-72: Enhanced status parsing with improved error handling.The
fromStringmethod now:
- Performs case-insensitive parsing
- Handles additional Banxa status types
- Defaults to
inProgressfor unknown statuses instead of failingThis directly addresses the PR objective of fixing the "erroneous failure popup" by not marking unknown statuses as failed.
lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart (12)
2-3: Platform import usage is properly guarded.Following the learnings in this project, the
dart:ioimport is acceptable as its usage is properly guarded with!kIsWebchecks to prevent issues in web environments.
24-25: Added explicit import for WebViewDialogMode.The explicit import for
WebViewDialogModefrom the webview_dialog.dart file improves code clarity by making the dependency more obvious.
94-112: Improved null safety with lazy evaluation.The code now uses lazy evaluation for setting
selectedAssetAddressandselectedCoinPubkeys, which better aligns with Flutter's patterns for state management and prevents potential null reference issues.
135-136: Updated order status to use renamed enum value.The code now uses
FiatOrderStatus.initialinstead ofpending, consistent with the enum changes.
172-187: Enhanced checkout URL handling.The code now:
- More explicitly casts the checkout URL
- Adds clearer comments about the HTML wrapper page requirements
- Uses a dedicated method to determine the appropriate WebView mode
This aligns with the PR objective of opening the Banxa payment page in a new browser tab to avoid dialog/popup issues.
208-225: Well-designed WebView mode selection logic.The new
_determineWebViewMode()method:
- Properly handles all platform variations (Web, Wasm, Linux)
- Takes into account specific provider requirements
- Provides clear comments explaining the implementation choices
The use of both
kIsWebandkIsWasmchecks is appropriate given the project's use of Flutter 3.22+.
405-406: Updated order status reset.The code now resets the order status to
initialinstead ofpendingwhen the WebView is closed, consistent with the enum changes.
420-422: Consistent use of lazy evaluation pattern.The asset address update now uses the lazy evaluation pattern for the
selectedAssetAddressproperty, consistent with other similar state updates in the file.
434-435: Improved error handling with provider-specific errors.The error parsing now captures provider-specific error messages in the
providerErrorfield, which allows for more detailed error reporting to users.
476-477: Proper nulling of provider errors on form refresh.The code now clears provider errors when refreshing the form, preventing stale error messages from persisting.
496-498: Consistent initial state setting.The code now sets the order status to
initialand clears any provider errors when initializing the form, providing a clean slate for new form submissions.
578-580: Consistent default state.The default payment methods state now properly initializes with the renamed
initialstatus and clears any provider errors.
takenagain
force-pushed
the
fix/banxa-payment-status
branch
from
ced4fa8 to
15d8257
Compare
takenagain
force-pushed
the
fix/banxa-payment-status
branch
from
15d8257 to
7e0f0f6
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart (1)
303-318:⚠️ Potential issueMissing
breakstatements cause unintended fall-through in theswitchEach
casere-assignsupdatedStatus, but the absence ofbreaklets execution fall through to subsequent cases, overwriting the intended value and masking states such aswindowCloseRequested.- case PaymentStatusType.widgetCloseRequestConfirmed: - case PaymentStatusType.widgetClose: - case PaymentStatusType.widgetCloseRequest: - updatedStatus = FiatOrderStatus.windowCloseRequested; + case PaymentStatusType.widgetCloseRequestConfirmed: + case PaymentStatusType.widgetClose: + case PaymentStatusType.widgetCloseRequest: + updatedStatus = FiatOrderStatus.windowCloseRequested; + break; case PaymentStatusType.purchaseCreated: updatedStatus = FiatOrderStatus.inProgress; + break; case PaymentStatusType.paymentStatus: final status = data['status'] as String? ?? 'declined'; updatedStatus = FiatOrderStatus.fromString(status); + break;Add
break;after each logical block (or restructure into separateswitch/ifchains) to prevent logic errors.lib/bloc/fiat/ramp/ramp_fiat_provider.dart (1)
202-210: 💡 Verification agent🧩 Analysis chain
Possible undefined properties
minPurchaseAmount/maxPurchaseAmountonHostAssetsConfig
coins.minPurchaseAmountandcoins.maxPurchaseAmountare accessed here, butcoinsis aHostAssetsConfiginstance (not an individual asset). If the class doesn’t expose aggregate min/max fields, this will be a compile-time error.Please verify that
HostAssetsConfigindeed defines these getters or change the code to derive the limits from the selected asset.
🏁 Script executed:
#!/bin/bash # Check if HostAssetsConfig declares the expected properties rg -n --context 2 $'class HostAssetsConfig' rg -n $'minPurchaseAmount' | headLength of output: 1384
Fix missing
minPurchaseAmount/maxPurchaseAmounton HostAssetsConfigHostAssetsConfig (lib/bloc/fiat/ramp/models/host_assets_config.dart) only declares:
class HostAssetsConfig { final List<RampAssetInfo> assets; // … }It does not expose
minPurchaseAmountormaxPurchaseAmount, so these lines in ramp_fiat_provider.dart will fail to compile:
- lib/bloc/fiat/ramp/ramp_fiat_provider.dart:202-210
Please address this by either:
Adding getters to HostAssetsConfig, for example:
class HostAssetsConfig { final List<RampAssetInfo> assets; // … Decimal get minPurchaseAmount => /* aggregate or default value */; Decimal get maxPurchaseAmount => /* aggregate or default value */; }Or changing the fallback in ramp_fiat_provider.dart to use a value available on the selected asset (e.g.,
asset.defaultMinPurchaseAmountor another valid source):--- lib/bloc/fiat/ramp/ramp_fiat_provider.dart @@ -202,7 +202,7 @@ 'min': (asset.hasValidMinPurchaseAmount() ? asset.minPurchaseAmount - : coins.minPurchaseAmount) + : asset.minPurchaseAmount) // or other valid fallback .toString(), 'max': (asset.hasValidMaxPurchaseAmount() ? asset.maxPurchaseAmount
♻️ Duplicate comments (1)
lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart (1)
210-213: Platform check looks good – no web build issuesThe familiar
!kIsWeb && !kIsWasm && Platform.isLinuxpattern safely short-circuits thePlatformcall for web builds, so the unconditionaldart:ioimport won’t break compilation.
No action needed.
🧹 Nitpick comments (1)
lib/bloc/fiat/ramp/models/ramp_asset_info.dart (1)
76-79:hasValidMinPurchaseAmounttreats only-1as “no-limit”; clarify edge-casesThe helper currently returns
truefor0even though the docs say “Null or -1 indicates no minimum limit.”
If0should also mean “no limit”, the comparison needs to be changed to> Decimal.zero.
Otherwise, consider amending the comment to avoid confusion.- return minPurchaseAmount! > Decimal.fromInt(-1); + // Treat 0 as an explicit minimum, -1 as sentinel "no limit". + return minPurchaseAmount! > Decimal.fromInt(-1);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
assets/web_pages/fiat_widget.html(1 hunks)lib/app_config/app_config.dart(1 hunks)lib/bloc/fiat/banxa_fiat_provider.dart(5 hunks)lib/bloc/fiat/base_fiat_provider.dart(3 hunks)lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart(25 hunks)lib/bloc/fiat/fiat_onramp_form/fiat_form_state.dart(7 hunks)lib/bloc/fiat/models/i_currency.dart(5 hunks)lib/bloc/fiat/ramp/models/ramp_asset_info.dart(1 hunks)lib/bloc/fiat/ramp/ramp_fiat_provider.dart(9 hunks)lib/views/fiat/fiat_form.dart(4 hunks)lib/views/fiat/fiat_inputs.dart(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- lib/app_config/app_config.dart
🚧 Files skipped from review as they are similar to previous changes (6)
- lib/bloc/fiat/base_fiat_provider.dart
- lib/views/fiat/fiat_inputs.dart
- lib/bloc/fiat/banxa_fiat_provider.dart
- lib/views/fiat/fiat_form.dart
- lib/bloc/fiat/models/i_currency.dart
- assets/web_pages/fiat_widget.html
🧰 Additional context used
🧠 Learnings (1)
lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart (3)
Learnt from: takenagain
PR: KomodoPlatform/komodo-wallet#2608
File: lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart:2-3
Timestamp: 2025-05-01T21:00:36.970Z
Learning: It's acceptable to use unconditional `dart:io` imports in the Komodo wallet codebase when the usage is guarded by `!kIsWeb` conditional checks that prevent the platform-specific code from being executed in web environments.
Learnt from: takenagain
PR: KomodoPlatform/komodo-wallet#2608
File: lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart:208-225
Timestamp: 2025-05-01T20:56:36.180Z
Learning: The `kIsWasm` constant is available in Flutter version 3.22+ (released May 2024) and is used to detect if an application is running on the WebAssembly backend. When writing platform-specific code, use both `kIsWeb` and `kIsWasm` checks to properly handle both JavaScript and WebAssembly compilation targets.
Learnt from: takenagain
PR: KomodoPlatform/komodo-wallet#2608
File: lib/views/fiat/webview_dialog.dart:50-55
Timestamp: 2025-05-01T21:00:56.962Z
Learning: `kIsWasm` is a valid constant in Flutter 3.22+ (released May 2024) that detects if an application is running on WebAssembly. It complements `kIsWeb` for more precise platform detection.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Desktop (windows)
- GitHub Check: Test web-app-macos
- GitHub Check: Build Mobile (Android)
- GitHub Check: Build Mobile (iOS)
🔇 Additional comments (1)
lib/bloc/fiat/fiat_onramp_form/fiat_form_state.dart (1)
30-47: State initialisation & lazy setters are well-designedSwitching to factory constructors for default currencies, adding
webViewMode/providerError, and usingValueGetterfor lazy updates improves clarity and avoids unnecessary rebuilds. Nice work!
takenagain
requested review from
AndrewDelaney,
CharlVS,
ShantanuSharma9873 and
smk762
smk762
left a comment
smk762
left a comment
There was a problem hiding this comment.
- "Return to Komodo Wallet" button functions as expected ✔️
- No erroneous error message about failed payment ✔️
- Order fulfilled and coins received successfully 🎉
Fixes the issues mentioned in #2570 where the Banxa flow incorrectly triggered a payment failed popup and failed to do anything when the button was pressed. The "Komodo" text appears to be linked to the partner name configured on the Banxa dashboard.
We are pinned to Banxa API v1.3. Further adjustments would be required to migrate to v2 (v2.1 being the latest). Ramp also has newer API versions that we could upgrade to (v2 and v3).
Changes
Banxa page now opens in a new tab rather than a webview dialog to circumvent dialog/popup z-order issues.
Fixed a coin abbreviation/ticker parsing regression, so there are now more coins available in the coin selection dropdown (mostly from Banxa)
The provider error response is now shown if order creation fails when clicking "Buy Now". Incorrect address formats and minimum coin amounts are likely reasons for this, and the additional information could help with debugging/support.
Improved documentation/explanation comments, refactoring, and implementation of coderabbitai recommendations around error handling from main PR
Disabled the following coins due to address format errors, mostly on Banxa's side (done in app_config.dart)
Disabled coins for fiat onramp
Banxa:
Ramp:
Demo environment limitations
Due to limitations with Banxa demo environment, testing with real fiat transfers is required to confirm that the payment failure popup is not shown when payment succeeds.
Both Ramp and Banxa stop or fail before completing the mock transaction (no final success status).
Banxa & Ramp limitations
coinsconfig file).Banxa demo testing information *Updated*
Banxa demo testing information can be used to test local
debugbuilds up until the payment processing step. Note that Banxa returns a payment failed status at the end, so the payment failure popup is expected in the localdebugbuilds with the demo environment.Ramp demo testing information
Note: The demo environment does not release funds currently, so the transaction status will remain in the "Processing transaction" state, and the transaction will not succeed.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor