feat(fiat-onramp): hd wallet address selection support #2570
Conversation
- remove unused functions - remove duplicate imports - ignore unused key element parameter - remove deprecated analysis_options
validators were checking against the old variable field
- add asset, pubkeys and address to fiat form state - add address selected event to fiat form bloc - use logging package in fiat form bloc - improve repository return type hints (don't use abstract type absolutely everywhere)
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request applies numerous refactoring changes across the codebase. In several files, explicit library names and unused imports have been removed. Method signatures, variable types, and event names have been updated to use more specific types (e.g., converting from generic interfaces or double to Decimal) and to improve clarity. The fiat form functionality has been restructured with new events, state properties, and integration changes utilizing the KomodoDefiSdk. Additionally, legacy methods, redundant test code, and obsolete lint rules have been removed to streamline the code. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as FiatForm UI
participant Bloc as FiatFormBloc
participant SDK as KomodoDefiSdk
participant Repo as FiatRepository
UI->>Bloc: Dispatch FiatFormStarted / FiatFormRefreshed
Bloc->>SDK: Initialize SDK & fetch asset/pubkey info (e.g. in _onCoinSelected)
SDK-->>Bloc: Return asset details
Bloc->>UI: Emit updated state (with selectedAsset, updated amounts)
UI->>Bloc: Dispatch FiatFormSubmitted (trigger order submission)
Bloc->>Repo: Process submission and fetch order status
Repo-->>Bloc: Return order status update
Bloc->>UI: Emit final state with order status
Suggested reviewers
Poem
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:
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 (
|
- bloc events and states were not in line with bloc naming conventions - double was still being used in places instead of decimal both changes were made with new&old refactoring efforts in mind
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
seeing as the inputs are not required, the action should not fail if they are empty (as is the case in non-dev feature branches it seems)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (12)
test_units/tests/utils/get_usd_balance_test.dart (1)
1-1: Consider adding migration timeline to the TODO commentThe TODO comment indicates these tests should be revisited or migrated to the SDK. Consider adding a more specific timeline or ticket reference to ensure this technical debt is properly tracked.
test_units/main.dart (2)
59-62: Consider adding JIRA/GitHub issue reference to TODOsThe TODO comment mentions re-enabling or migrating tests to the SDK, but lacks specific details about when or how this should be done. Consider adding a ticket reference to ensure this work isn't forgotten.
group('Utils:', () { - // TODO: re-enable or migrate to the SDK + // TODO(TICKET-123): re-enable or migrate to the SDK after HD wallet implementation is complete // testUsdBalanceFormatter();
69-74: Similar TODO lacks specificitySimilar to the previous comment, this TODO would benefit from additional context such as a ticket reference or timeline.
group('Helpers: ', () { testMaxMinRational(); testCalculateBuyAmount(); - // TODO: re-enable or migrate to the SDK + // TODO(TICKET-123): re-enable or migrate to the SDK after HD wallet implementation is complete // testGetTotal24Change();app_theme/lib/app_theme.dart (1)
1-1: Consider specifying the library name for better organization.Changing from a named library (
library app_theme;) to an unnamed library (library;) might affect code organization and clarity. Named libraries can help with identification and prevent naming conflicts. Unless there's a specific reason to remove the name, consider maintaining the explicit library name.-library; +library app_theme;lib/bloc/bridge_form/bridge_validator.dart (2)
395-395: Updated balance check to use SDK method.The balance property access has been replaced with a method call that takes the SDK as an argument, which aligns with the refactoring to use the SDK for data access.
Consider using a small epsilon or dedicated method for checking if the balance is close to zero rather than exact floating-point comparison, which can be prone to precision issues:
-if (parentSell.balance(sdk) == 0.00) return false; +if (parentSell.balance(sdk) <= 0.001) return false; // or use a dedicated isZeroBalance method
408-410: Updated balance check for parent buy coin.Similar to the parent sell coin, the balance check now uses the SDK method instead of a direct property access.
As with the previous balance check, consider using a small epsilon value instead of exact equality for floating-point comparison:
-if (parentBuy.balance(sdk) == 0.00) return false; +if (parentBuy.balance(sdk) <= 0.001) return false; // or use a dedicated isZeroBalance methodlib/bloc/fiat/models/fiat_transaction_limit.dart (1)
14-16: Ensure JSON fields are consistently strings to avoid casting issues.
If the backend sometimes returns numeric types formin,max, orweekly, this cast (as String?) might fail. Consider additional checks or conversions for non-string values if the server’s response isn’t guaranteed to be a string.lib/views/fiat/fiat_inputs.dart (3)
78-93: Efficient text synchronization
This logic properly updates the text field if the new amount differs from the current one. Consider locale-based decimal handling if users enter commas.
98-104: Runtime casting
CastingICurrencytoFiatCurrencyorCryptoCurrencycould cause runtime errors ifnewValueis mismatched. Prefer typed sources or typed dropdowns to avoid casts.
185-185: Widget naming mismatch
FiatCurrencyItemis used with aCryptoCurrency, which can be confusing. Consider renaming or introducing a more generalized widget for coin items.lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart (1)
122-128: _getAmountInputWithBounds
UsingDecimal.parsefor limit boundaries is correct, but consider usingtryParseto avoid potential exceptions if data is malformed.- minAmount = Decimal.parse(firstLimit.min.toString()); + final parsedMin = Decimal.tryParse(firstLimit.min.toString()); - maxAmount = Decimal.parse(firstLimit.max.toString()); + final parsedMax = Decimal.tryParse(firstLimit.max.toString());lib/bloc/fiat/fiat_onramp_form/fiat_form_event.dart (1)
66-67: Simplified form submission event.The
FiatFormSubmittedevent is concise and clearly named. Consider adding a constructor withconstmodifier for consistency with other events.-final class FiatFormSubmitted extends FiatFormEvent {} +final class FiatFormSubmitted extends FiatFormEvent { + const FiatFormSubmitted(); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
app_theme/lib/app_theme.dart(1 hunks)lib/bloc/analytics/analytics_repo.dart(1 hunks)lib/bloc/bridge_form/bridge_validator.dart(4 hunks)lib/bloc/fiat/banxa_fiat_provider.dart(2 hunks)lib/bloc/fiat/base_fiat_provider.dart(1 hunks)lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart(15 hunks)lib/bloc/fiat/fiat_onramp_form/fiat_form_event.dart(2 hunks)lib/bloc/fiat/fiat_onramp_form/fiat_form_state.dart(5 hunks)lib/bloc/fiat/fiat_repository.dart(2 hunks)lib/bloc/fiat/models/fiat_transaction_limit.dart(3 hunks)lib/bloc/fiat/models/i_currency.dart(3 hunks)lib/bloc/fiat/ramp/ramp_fiat_provider.dart(3 hunks)lib/bloc/taker_form/taker_validator.dart(4 hunks)lib/bloc/withdraw_form/withdraw_form_bloc.dart(0 hunks)lib/model/forms/fiat/fiat_amount_input.dart(2 hunks)lib/views/dex/simple/form/tables/orders_table/grouped_list_view.dart(0 hunks)lib/views/fiat/fiat_form.dart(5 hunks)lib/views/fiat/fiat_inputs.dart(6 hunks)lib/views/fiat/fiat_page.dart(3 hunks)lib/views/fiat/fiat_payment_method_group.dart(1 hunks)lib/views/fiat/fiat_payment_methods_grid.dart(1 hunks)lib/views/nfts/nft_tabs/nft_tab.dart(1 hunks)lib/views/settings/widgets/general_settings/app_version_number.dart(2 hunks)lib/views/settings/widgets/security_settings/password_update_page.dart(2 hunks)lib/views/settings/widgets/security_settings/seed_settings/seed_show.dart(2 hunks)lib/views/wallet/coin_details/coin_details_info/charts/animated_portfolio_charts.dart(0 hunks)lib/views/wallet/coin_details/coin_details_info/coin_addresses.dart(1 hunks)lib/views/wallet/coin_details/withdraw_form/widgets/fill_form/fields/custom_fee/custom_fee_field_evm.dart(0 hunks)lib/views/wallet/coin_details/withdraw_form/widgets/fill_form/fields/fields.dart(0 hunks)lib/views/wallet/wallet_page/wallet_main/wallet_manager_search_field.dart(0 hunks)lib/views/wallets_manager/widgets/wallet_creation.dart(1 hunks)packages/komodo_persistence_layer/analysis_options.yaml(0 hunks)test_units/main.dart(2 hunks)test_units/tests/helpers/total_24_change_test.dart(1 hunks)test_units/tests/utils/get_usd_balance_test.dart(1 hunks)test_units/tests/utils/test_util.dart(0 hunks)
💤 Files with no reviewable changes (8)
- lib/views/dex/simple/form/tables/orders_table/grouped_list_view.dart
- test_units/tests/utils/test_util.dart
- packages/komodo_persistence_layer/analysis_options.yaml
- lib/bloc/withdraw_form/withdraw_form_bloc.dart
- lib/views/wallet/coin_details/withdraw_form/widgets/fill_form/fields/fields.dart
- lib/views/wallet/coin_details/coin_details_info/charts/animated_portfolio_charts.dart
- lib/views/wallet/wallet_page/wallet_main/wallet_manager_search_field.dart
- lib/views/wallet/coin_details/withdraw_form/widgets/fill_form/fields/custom_fee/custom_fee_field_evm.dart
🔇 Additional comments (134)
test_units/tests/utils/get_usd_balance_test.dart (1)
12-51: Document the reason for commenting out all test casesWhile the TODO comment at the top gives some context, it would be helpful to add a more detailed explanation about why these tests are being commented out rather than deleted or fixed. If these tests validate important functionality, commenting them out could hide potential regressions.
Are these tests being replaced by equivalent tests in the SDK? If so, consider either:
- Adding a reference to where the new tests are located
- Deleting these tests entirely to avoid confusion
- Moving these tests to the SDK repository if that's more appropriate
test_units/main.dart (1)
32-89:❓ Verification inconclusive
Consider adding test coverage verification
With multiple test functions commented out, it's important to ensure that test coverage hasn't been significantly reduced, especially for critical functionality related to balance and change calculations.
🏁 Script executed:
#!/bin/bash # Check which files might be affected by removing these tests echo "Finding potential affected files:" rg -t dart "getFormattedUsdBalance|getTotal24Change" --files-with-matches # Check if there are other tests that might provide coverage for these functionalities echo "Checking for other tests that might cover similar functionality:" rg -t dart "usdBalance|24Change|UsdFormatter" --files-with-matchesLength of output: 940
Ensure Sufficient Test Coverage for Balance & Change Calculations
It appears that while tests like
testUsdBalanceFormatter()andtestGetTotal24Change()have been commented out intest_units/main.dart, related functionalities are still being exercised by tests in files such as:
test_units/tests/utils/get_usd_balance_test.darttest_units/tests/helpers/total_24_change_test.dartAdditionally, similar keywords are present in several production files (for example, in
lib/views/wallet/common/wallet_helper.dart), which suggests that these functionalities remain active in the current coverage suite.Action Items:
- Verify that the tests in the aforementioned files adequately cover the balance calculations and 24‑hour change functionalities.
- If the migration to the SDK (or other changes) is intended, please make sure that the new testing approach maintains or exceeds the current coverage.
- Update the documentation or add comments as needed to clarify the test coverage strategy for these critical features.
lib/views/settings/widgets/general_settings/app_version_number.dart (2)
39-40: LGTM: Appropriate lint warning suppression for unused parameter.The added comment properly suppresses the lint warning for an unused constructor parameter in a private class. This is a common pattern in Flutter widget classes where the key parameter is defined by the parent class but not directly used in the child class.
92-93: LGTM: Appropriate lint warning suppression for unused parameter.This lint suppression is consistent with the approach used for other StatelessWidget classes in the file. It specifically targets the unused parameter warning rather than suppressing broader issues.
lib/views/nfts/nft_tabs/nft_tab.dart (1)
122-123: LGTM: Improved lint suppression specificity.The change from
unused_elementtounused_element_parametermakes the lint suppression more precise, which is a good practice. This specificity clearly communicates that only the unused parameter is being suppressed, not the entire element.lib/views/settings/widgets/security_settings/seed_settings/seed_show.dart (2)
81-82: LGTM: Consistent lint warning suppression.This lint suppression follows the same pattern as in other files, specifically targeting the unused parameter rather than the entire element or class.
404-405: LGTM: Consistent lint warning suppression.The lint suppression is appropriately applied to the unused key parameter in the _SelectableSeedWord constructor, following the project's pattern for handling unused parameters in StatelessWidget subclasses.
lib/views/fiat/fiat_payment_method_group.dart (1)
63-63: LGTM: Improved event naming for better clarity.Renaming from
PaymentMethodSelectedtoFiatFormPaymentMethodSelectedimproves code clarity by explicitly indicating which component the event belongs to. This change aligns with best practices for event naming in a complex application.lib/views/fiat/fiat_payment_methods_grid.dart (1)
46-47:❓ Verification inconclusive
Property reference updated to use selectedAsset instead of selectedCoin.
This change aligns with the broader refactoring effort to use "asset" terminology instead of "coin" throughout the codebase. The non-null assertion operator (
!) is used here, so verify thatstate.selectedAsset.valueis always non-null when this code path is executed.
🏁 Script executed:
#!/bin/bash # Check if there are any null checks for selectedAsset before this code path # Look for null checks or conditional rendering related to selectedAsset rg -A 2 -B 2 "selectedAsset\s*[?]\s*[.]\s*value" --glob "*.dart" # Look for other files that have been updated from selectedCoin to selectedAsset rg -l "selectedCoin\s*[.]\s*value" --glob "*.dart"Length of output: 119
Please verify that
state.selectedAsset.valuecan never be null at runtime.The renaming update from
selectedCointoselectedAssetis consistent with the refactoring effort. However, since our initial searches produced no evidence of explicit null checks or conditional validations forstate.selectedAssetbefore its usage (i.e. using the non-null operator!), please manually confirm that the value is indeed always initialized (or safely guarded) by the time this code is reached.
- Confirm that any parent widget or business logic guarantees
state.selectedAssetis non-null.- Check for conditional rendering or initialization logic elsewhere in the codebase to avoid potential runtime exceptions.
To help with this verification, you might run:
🏁 Script executed:
#!/bin/bash # Search for any conditional logic related to state.selectedAsset ensuring it is non-null rg -n -C3 "if\s+\(.*state\.selectedAsset[^\)]*!=\s*null" --glob "*.dart" # Locate assignments or initializations for state.selectedAsset in the repository rg -n -C3 "state\.selectedAsset\s*=" --glob "*.dart"Length of output: 251
Ensure non-null guarantee for
state.selectedAssetbefore usageThe update to replace
selectedCoinwithselectedAssetin the snippet below fits the broader refactoring. However, automated searches did not reveal any explicit null checks or initialization patterns forstate.selectedAssetin the surrounding code. Please manually verify that this property is always initialized (and never null) prior to its usage with the non-null assertion (!).
- Verify that a parent widget or business logic initializes
state.selectedAssetto a valid value.- Ensure that any fallback or conditional rendering logic properly prevents execution of this code when
state.selectedAssetis null.state.selectedAsset.value!.symbol, state.selectedFiat.value!.symbol,lib/bloc/analytics/analytics_repo.dart (1)
54-54: Code simplification for parameter sanitization.The type check has been removed, and all values are now directly converted to strings using
toString(). This is a good simplification since all objects in Dart have atoString()method, and the map parameters are already declared asMap<String, Object>.lib/bloc/bridge_form/bridge_validator.dart (2)
2-3: Added imports for Get_it and Komodo SDK.These new imports support the refactoring of the balance checks to use the SDK for retrieving coin balances.
375-377: Added SDK instance for balance retrieval.The KomodoDefiSdk is now retrieved from the GetIt service locator to support balance checking.
lib/views/wallets_manager/widgets/wallet_creation.dart (1)
16-21: Updated constructor parameter syntax to use super.key.This change utilizes Dart's newer and more concise constructor parameter syntax. The functionality remains the same, but the code is now more elegant and aligned with modern Flutter best practices.
lib/views/settings/widgets/security_settings/password_update_page.dart (2)
104-105: Added linting suppression for unused parameter.The comment
// ignore: unused_element_parameterwas added to suppress warnings for the unusedonSuccessparameter in the private_FormViewconstructor. This improves code quality by acknowledging that the parameter is intentionally passed but not used directly in the class implementation.
370-371: Added linting suppression for unused parameter.Similar to the previous change, this comment suppresses warnings for the unused
backparameter in the_SuccessViewconstructor. This is a good practice when parameters are required for API consistency but not directly used in the implementation.lib/views/fiat/fiat_page.dart (4)
4-4: Added KomodoDefiSdk import for SDK integration.The import for the KomodoDefiSdk package has been added, which is used later to retrieve the SDK instance from the repository provider.
54-54: Added SDK retrieval from repository provider.Added a new variable to get the KomodoDefiSdk instance from context, which is part of the refactoring to use the SDK for fiat operations instead of directly using the coins repository.
56-59: Updated FiatFormBloc instantiation to use SDK.This change replaces the
coinsRepositoryparameter with the newsdkparameter in the FiatFormBloc constructor, and immediately triggers theFiatFormStarted()event. This is part of the migration to use the KomodoDefiSdk for fiat operations.
177-178: Improved linting suppression specificity.Changed the comment from
// ignore: unused_elementto// ignore: unused_element_parameterto more precisely indicate that only the unused parameter should be ignored, not the entire element.lib/bloc/taker_form/taker_validator.dart (2)
2-3: Added GetIt and KomodoDefiSdk imports.Added imports for the GetIt service locator and the KomodoDefiSdk, which are used to retrieve the SDK instance and access coin balances.
253-255: Updated balance retrieval to use SDK.Modified how coin balances are accessed by using a method call with the SDK instance (
balance(sdk)) instead of directly accessing a property. This change ensures balance retrieval is done through the SDK, which may provide more up-to-date information.-if (parentSell.balance == 0.00) return false; +if (parentSell.balance(sdk) == 0.00) return false; -if (parentBuy.balance == 0.00) return false; +if (parentBuy.balance(sdk) == 0.00) return false;The added comment also helps explain the purpose of the SDK variable.
Also applies to: 273-273, 286-286
lib/bloc/fiat/base_fiat_provider.dart (1)
19-19: Improved type specificity in currency list return types.Changed the return types of
getFiatList()andgetCoinList()methods from genericFuture<List<ICurrency>>to more specificFuture<List<FiatCurrency>>andFuture<List<CryptoCurrency>>respectively. This improves type safety and makes the API more explicit about what kind of currencies each method returns.-Future<List<ICurrency>> getFiatList(); +Future<List<FiatCurrency>> getFiatList(); -Future<List<ICurrency>> getCoinList(); +Future<List<CryptoCurrency>> getCoinList();Also applies to: 21-21
lib/bloc/fiat/ramp/ramp_fiat_provider.dart (3)
90-90: More specific return type improves type safetyThe return type is now more specific (
Future<List<FiatCurrency>>instead ofFuture<List<ICurrency>>), which improves type safety and eliminates the need for explicit casting by consumers of this API.
105-105: More specific return type improves type safetyThe return type is now more specific (
Future<List<CryptoCurrency>>instead ofFuture<List<ICurrency>>), which improves type safety and eliminates the need for explicit casting by consumers of this API.
121-121: Updated casting to match new return typeThe cast has been properly updated to align with the more specific return type of the method.
lib/bloc/fiat/models/i_currency.dart (4)
1-3: Appropriate imports for SDK integrationThe addition of these imports enables integration with the KomodoDefiSdk and support for assets conversion.
40-47: Added factory constructor for Asset conversionThis factory method enables converting SDK Assets to CryptoCurrency objects, providing a convenient bridge between the SDK and application models.
71-73: Added toAsset conversion methodThis method complements the
fromAssetfactory and allows for bidirectional conversion between CryptoCurrency and SDK Asset types.
58-63:✅ Verification successful
Simplified getAbbr method with potential implications
The method has been simplified to just return the symbol, with the previous implementation commented out.
Make sure that other parts of the codebase don't rely on the previous format (which included the chain type). Since there's a TODO comment, consider creating a tracking issue to determine if the previous implementation is still necessary anywhere in the codebase.
🏁 Script executed:
#!/bin/bash # Search for uses of getAbbr() to check if any code relies on the previous format echo "Searching for uses of getAbbr() in the codebase..." rg "getAbbr\(\)" -A 3 -B 3Length of output: 3089
Simplified getAbbr() Change – Verified No Downstream Dependency on Chain Type
- The search across the codebase (in files like
fiat_select_button.dartandfiat_repository.dart) shows that all callers ofgetAbbr()are using it solely to retrieve the currency symbol.- There’s no evidence that any part of the code expects the old, concatenated format (with the chain type appended).
- It’s still recommended to create a tracking issue to revisit whether the extended format might be required in any future edge cases.
lib/views/wallet/coin_details/coin_details_info/coin_addresses.dart (4)
34-34: Added late-initialized bloc referenceGood practice to store the bloc as a class variable for proper lifecycle management.
37-41: Properly initialized bloc in didChangeDependenciesUsing
didChangeDependenciesto initialize the bloc reference is the correct approach since it ensures the widget is properly attached to the widget tree before accessing the context.
44-48: Improved bloc disposalUsing the stored bloc reference in the dispose method ensures proper resource cleanup and avoids potential context-related issues during disposal.
54-133: Simplified widget structureThe widget structure has been improved by removing unnecessary nesting and directly using BlocBuilder, making the code more readable and maintainable.
lib/bloc/fiat/fiat_repository.dart (2)
78-88: Enhanced getFiatList with specific return typeThe method now returns a more specific type (
Future<List<FiatCurrency>>instead ofFuture<List<ICurrency>>) and includes improved sorting logic for the returned currencies.
106-113: Enhanced getCoinList with specific return typeThe method now returns a more specific type (
Future<List<CryptoCurrency>>instead ofFuture<List<ICurrency>>), aligning with similar changes in the provider classes.lib/bloc/fiat/models/fiat_transaction_limit.dart (2)
2-2: Switching to Decimal for financial calculations looks solid.
This approach helps avoid floating-point inaccuracies when dealing with money.
30-32: Using Decimal for min, max, and weekly is appropriate.
Retaining these as Decimal fields aligns well with precise monetary calculations.lib/bloc/fiat/banxa_fiat_provider.dart (2)
160-160: Refining return type to Future<List> is a good move.
This improves type safety and clarity when working with a list of fiat currencies.
174-178: Shift from generic ICurrency to CryptoCurrency is commendable.
DeclaringList<CryptoCurrency>enforces correct usage of crypto-specific properties.lib/bloc/fiat/fiat_onramp_form/fiat_form_state.dart (28)
5-8: Enhanced documentation for the form state is helpful.
These doc comments clearly describe the class’s responsibilities.
12-12: Introducing selectedAsset constructor parameter aligns with new asset handling.
This ensures the state can track the chosen crypto currency accurately.
23-24: New properties for asset address and pubkeys expand the form’s capabilities.
Capturing these fields in state is essential for onramp logic.
27-27: Adding an explicit doc comment for the initial state clarifies defaults.
This is beneficial for maintainers.
32-32: Defaulting selectedAsset to BTC is a practical baseline.
It provides a sensible starting point for the form.
36-36: Setting selectedAssetAddress to null in initial state is reasonable.
This ensures no address is assumed without user action.
45-46: Using FiatMode.onramp and null selectedCoinPubkeys by default is coherent with new features.
It keeps the state aligned with typical onramp usage.
48-48: Clarifying the purpose of selectedFiat.
The doc comment ties the fiat field to the selected crypto asset.
52-52: Declaring final CurrencyInput selectedAsset emphasizes immutability.
This ensures consistent usage and enforces correct updates via copyWith.
54-54: Annotation clarifies the fiatAmount usage relative to selectedAsset.
Keeping these doc comments updated fosters better readability.
57-57: Doc comment conveys the meaning of selectedPaymentMethod in the new flow.
This helps maintainers understand its role in final checkout.
60-64: Documenting selectedAssetAddress and selectedCoinPubkeys clarifies on-chain details.
Tracking addresses and pubkeys in state is crucial for automated post-checkout flows.
98-101: Switch to Decimal for minFiatAmount and maxFiatAmount ensures better financial precision.
It aligns with the updated FiatTransactionLimit usage.
102-103: No functional changes at lines 102 and 103.
Skipping further comment as they appear to be blank or spacing adjustments.
105-107: Adding isLoadingCurrencies and isLoading improves UI handling logic.
Explicit distinctions between loading states are beneficial for clarity.
109-112: Requiring selectedAssetAddress != null for canSubmit is a logical safeguard.
Users can’t proceed without specifying an address.
119-120: copyWith additions for selectedAsset allow dynamic updates while preserving immutability.
Consistent with the rest of the newly introduced fields.
122-122: Incorporating selectedAssetAddress into copyWith fosters robust address updates.
This is essential for multi-stage onramp flows.
127-128: Including fiatList and coinList in copyWith manages currency refresh scenarios.
Allows seamless updates when new lists are fetched.
131-131: selectedCoinPubkeys in copyWith covers advanced wallet integrations.
It ensures the state can reflect updated pubkeys.
135-135: Retaining fallback to this.selectedAsset in copyWith is a standard, safe approach.
Preserves existing data when no new asset is provided.
138-138: Same pattern for selectedAssetAddress ensures consistency.
Essential for partial updates without overwriting existing addresses unintentionally.
148-148: selectedCoinPubkeys property also follows the same fallback pattern.
This is aligned with the immutability and partial updates principle.
155-155: Adding selectedAsset to the list of Formz inputs clarifies validation.
This helps unify the form’s validation process.
159-159: Including coinList in props ensures re-rendering when coinList changes.
This is vital for state equality checks in Equatable.
162-162: Adding selectedAsset to props likewise ensures accurate state comparisons.
Prevents unintended stale data if asset changes.
164-164: selectedAssetAddress is now part of props, improving reactivity.
This ensures UI updates if the address changes.
174-174: Remember to include selectedCoinPubkeys in props to track pubkey changes.
Good addition for parity with other properties.lib/views/fiat/fiat_form.dart (11)
37-44: Initializing the form state and refreshing on init is consistent.
This ensures the UI is up-to-date once the user arrives.
47-153: Overhauled build method with BlocListeners & BlocConsumers is well-structured.
These changes consolidate event handling and UI rendering neatly, though keep an eye on potential rebuild performance if the widget tree grows.
155-156: Refactored _completeOrder to dispatch FiatFormSubmitted clearly.
This follows the established pattern of letting the bloc handle logic.
158-168: Forcing refresh on fiat/coin/amount changes ensures real-time updates.
These calls toFiatFormRefreshed(forceRefresh: true)are consistent with dynamic form logic.
170-171: _setActiveTab triggers mode updates in the bloc.
Clean separation of concerns for UI tab selection and state transitions.
179-182: Dispatching FiatFormWalletAuthenticated/FiatFormAccountCleared on login state change is correct.
It aligns with the reactivity when user logs in or out.
193-193: _openCheckoutPage bridging web vs. mobile flows is well-structured.
Providing a consistent experience across platforms can reduce CORS and provider mismatch issues.
210-212: Capturing console messages from WebView is a helpful debugging approach.
Forwarding them to the bloc for status handling is neat.
227-228: Starting order status watch upon submission is a natural next step.
This sets the stage for listening to real-time updates.
237-237: Invoking _showPaymentStatusDialog for non-pending statuses clarifies the flow.
This ensures the user sees immediate feedback on success/failure states.
241-241: _showPaymentStatusDialog details each payment status thoroughly.
Offering distinct user messages based on order status is user-friendly.lib/model/forms/fiat/fiat_amount_input.dart (4)
1-1: Consolidate numeric precision with Decimal
No major concerns with this import; it’s necessary for precise financial calculations.
26-27: Appropriate transition from double? to Decimal?
UsingDecimalfor currency amounts is more precise and avoids floating-point errors.
29-29: Value parsing improved
Switching from double parsing toDecimalhelps avoid precision issues for monetary calculations.
21-23:❓ Verification inconclusive
Verify usage with new constructor signature
Dropping the defaultminValue=0might allow negative amounts if not explicitly set. Please confirm that no calling code relied on the old default.
🏁 Script executed:
#!/bin/bash # Description: Searching for all calls to FiatAmountInput.pure or .dirty to see if code relies on default minValue=0 rg -A 3 "FiatAmountInput\.(pure|dirty)\("Length of output: 1216
Action: Verify Constructor Call Sites and Default Value Handling
We've identified that the pure constructor is called without an explicit minValue in at least one location (in
lib/bloc/fiat/fiat_onramp_form/fiat_form_state.dartviaconst FiatAmountInput.pure()). This change from a defaultminValue = 0to potentiallynullmight let negative amounts slip through unless handled further downstream.
- Review all instances where
FiatAmountInput.pureis used (especially infiat_form_state.dart) to ensure that a missingminValuedoes not inadvertently allow negative amounts.- Confirm that either the consumer code or internal validations guard against negative input when
minValueis not provided explicitly.Please verify that no calling code relies on the previous default of
minValue = 0.lib/views/fiat/fiat_inputs.dart (5)
3-6: New imports for bridging with KomodoDefiSdk
No apparent issues. These references appear necessary for the updated logic.
19-34: Additional constructor parameters
ProvidingselectedAsset,selectedAssetAddress,selectedAssetPubkeys, andonSourceAddressChangedincreases clarity. Please verify backward compatibility or confirm any required references are updated.
36-45: Decimals for amounts
Switching from doubles to decimals is beneficial for accurate financial calculations.
47-48: More robust typed callbacks
Changing callback signatures to acceptFiatCurrency/CryptoCurrencypromotes safer typing versus a genericICurrency.
198-212: SourceAddressField integration
The newRowandSourceAddressFieldlogic is clear and provides an intuitive way to manage addresses. Good addition.lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart (28)
6-12: Imports for Decimal, SDK, and logging
These imports are appropriate for the new form logic.
29-31: Injecting KomodoDefiSdk
Using_sdkto delegate pubkey retrieval centralizes functionality.
33-50: Event registrations
All relevant events are set up, aligning with the bloc pattern for form management.Also applies to: 55-55
62-62: Logger usage
Centralizing logs under_logis helpful for debugging.
64-69: _onStarted: Auto-trigger fetch
Automatically dispatchingFiatFormCurrenciesFetchedon start is convenient. Confirm it won’t trigger duplicate fetch cycles in repeated scenarios.
71-80: _onFiatSelected
Establishes the chosen fiat in state. Implementation is straightforward.
82-107: _onCoinSelected
Retrieves and sets pubkeys for the newly selected coin. Error handling is well done.
109-116: _onAmountUpdated
Debounced updates help reduce unnecessary triggers. Good approach.
139-140: _onSelectPaymentMethod
Updates the payment method retrieval to reflect user choice. No issues found.
156-157: _onSubmitForm
Integrates the form submission workflow correctly.
161-162: Form validation logging
Warns about invalid states. Straightforward usage.
171-178: Order creation
Properly passing decimal-based parameters to the repository. Implementation looks sound.
181-182: Order creation error
Logs warnings and handles fallback.
199-201: Setting final states
Assigns checkout URL and order ID upon success. No concerns.
204-204: Error logging
Using_log.shoutfor high-severity errors is reasonable.
222-223: Check for empty or invalid decimal
Falls back to default payment methods if user input is invalid. Good usability measure.
237-238: _updateAssetPubkeys
Refreshes coin addresses seamlessly when needed.
254-256: onError
Graceful fallback in case payment method retrieval fails.
299-299: Load currency list error
Error is clearly logged.
304-305: _onWalletAuthenticated
Pubkeys are reloaded post-authentication, ensuring updated state.
309-317: Fetching pubkeys
Implementation is consistent. UsingfirstOrNullavoids index errors.
320-321: Error logs
Sets address to null and logs the error if pubkeys retrieval fails.
325-330: _onClearAccountInformation
Resets and clears the form state.
332-346: _updateAssetPubkeys
Similar logic to_onCoinSelected; ensures consistency in updating addresses.
349-350: _onPaymentStatusMessage
Guarded JSON parsing for ramp messages. Implementation is sufficient.
405-432: _onWatchOrderStatus
Monitors Banxa order status stream. Implementation is aligned with the overall architecture.
461-462: Null address scenario
Correctly flags unsupported coins or absent wallet.
476-486: _onCoinAddressSelected
UpdatesselectedAssetAddressupon user selection. Straightforward.lib/bloc/fiat/fiat_onramp_form/fiat_form_event.dart (19)
3-3: Good addition of comprehensive documentation.Adding clear documentation for the base class helps with code understandability and maintainability.
11-14: Well-defined event for initializing the form.The
FiatFormStartedevent is properly documented and follows the consistent naming convention. This is a good practice for handling the initialization of the form.
16-17: Improved naming consistency with the FiatForm prefix.Renaming to
FiatFormOnRampPaymentStatusMessageReceivedaligns with the naming convention and provides better clarity on the event's purpose and scope.
26-28: Good renaming for clarity and consistency.Changing from
SelectedFiatCurrencyChangedtoFiatFormFiatSelectedfollows the naming convention and makes the purpose clearer.
30-30: Type clarification improves code quality.Using the more specific
FiatCurrencytype instead of a generic type improves type safety and documentation.
36-38: Consistent naming convention applied.Renaming from
SelectedCoinChangedtoFiatFormCoinSelectedmaintains consistency with other events and clearly communicates its purpose.
40-40: Type improvement for better clarity.Using the more specific
CryptoCurrencytype instead of a generic type enhances code clarity and type safety.
46-48: Clear naming and documentation for amount updates.The renaming to
FiatFormAmountUpdatedand the added documentation clearly indicate the event's purpose.
56-58: Consistent naming for payment method selection.The renaming to
FiatFormPaymentMethodSelectedfollows the established pattern and improves readability.
69-72: Good documentation and naming for mode updates.The
FiatFormModeUpdatedevent is well-documented and follows the naming convention.
73-75: Helpful named constructor for mode updates.The named constructor
fromTabIndexis a good approach for converting UI-specific data to the domain model.
83-86: Clear purpose for payment status clearing.The
FiatFormPaymentStatusClearedevent is well-documented and follows the naming convention.
88-91: Good authentication event for wallet operations.The
FiatFormWalletAuthenticatedevent provides a clear signal for authentication completion.
93-96: Well-defined event for clearing account data.The
FiatFormAccountClearedevent follows the naming convention and has a clear purpose.
98-102: Good design for refresh functionality with force option.The
FiatFormRefreshedevent includes aforceRefreshparameter with a default value, providing flexibility in refreshing behavior.
104-105: Helpful documentation for the forceRefresh parameter.The documentation clarifies the purpose of the forceRefresh flag, which is good for maintainability.
111-114: Well-defined event for currency fetching.The
FiatFormCurrenciesFetchedevent is properly documented and follows the naming convention.
116-119: Good implementation for order status watching.The
FiatFormOrderStatusWatchStartedevent provides a clear purpose for initiating order status monitoring.
121-129: Key implementation for HD wallet address selection support.The
FiatFormCoinAddressSelectedevent is critical for the HD wallet address selection feature mentioned in the PR objectives. The use ofPubkeyInfotype provides strong typing for addresses.
Flutter compilation errors were hidden in the generate-assets action, which would cause the workflow to fail without printing error logs
- use Decimal.tryParse in payment provider min/max value parsing - add locale-aware decimal parsing - remove 'as Type' casting in UI layer - add todo comment
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (15)
.github/actions/generate-assets/action.yml (1)
65-67: Fix trailing whitespaceThere are trailing spaces on lines 65, 66, and 80 that should be removed.
-# Run flutter build once to download coin icons and config files. -# This step is expected to "fail", since flutter build has to run again +# Run flutter build once to download coin icons and config files. +# This step is expected to "fail", since flutter build has to run again # after the assets are downloaded to register them in AssetManifest.bin ... -set -e # Restore exit on error behavior +set -e # Restore exit on error behaviorAlso applies to: 80-80
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 65-65: trailing spaces
(trailing-spaces)
[error] 66-66: trailing spaces
(trailing-spaces)
lib/shared/utils/formatters.dart (1)
311-313: Consider centralizing repetitive formatting logic.The logic mirrors the approach at lines 183–185. If this pattern occurs frequently, factoring it out into a shared helper might reduce duplication and simplify changes.
lib/bloc/fiat/ramp/models/host_assets_config.dart (1)
1-42: Thorough model definition – consider JSON validation & doc comments.
- The constructor and factory method handle essential fields, but missing or malformed JSON values could cause parse exceptions. Consider adding graceful fallback or validation.
- If this class is public API, adding doc comments on each field and constructor parameter can improve clarity for consumers.
Would you like to incorporate default/fallback handling for missing JSON keys or values?
lib/bloc/fiat/ramp/models/ramp_asset_info.dart (1)
52-74: Comprehensive JSON parsing, but missing toJson method.The
fromJsonmethod handles all fields properly, but there's no correspondingtoJsonmethod which would be needed for full serialization support.Consider adding a
toJsonmethod for consistency with other model classes:+ Map<String, dynamic> toJson() { + return { + 'name': name, + 'symbol': symbol, + 'decimals': decimals, + 'price': price, + 'minPurchaseAmount': minPurchaseAmount?.toString(), + 'maxPurchaseAmount': maxPurchaseAmount?.toString(), + 'address': address, + 'chain': chain, + 'currencyCode': currencyCode, + 'enabled': enabled, + 'hidden': hidden, + 'logoUrl': logoUrl, + 'minPurchaseCryptoAmount': minPurchaseCryptoAmount, + 'networkFee': networkFee.toString(), + 'type': type, + }; + }lib/views/fiat/fiat_form.dart (5)
47-48: Follow through on the TODO for checkout URL reuse.
Caching the generated checkout URL in theFiatFormBloccould prevent generating unnecessary orders on repeated submissions when user inputs are unchanged.Would you like help implementing a mechanism to store and reuse the checkout URL when the relevant form data is unchanged?
71-104: Reassess thelistenWhencondition.
You’re only listening for changes tofiatOrderStatus. If you need to react to other state changes, consider adjusting or removing thelistenWhenfilter.
117-137: Prevent forced casts by validating the input types.
The statements such asstate.selectedFiat.value! as FiatCurrencypresume the type is guaranteed. An unexpected runtime type may cause runtime exceptions. Consider using safe type checks or a fallback.- initialFiat: state.selectedFiat.value! as FiatCurrency, + initialFiat: state.selectedFiat.value is FiatCurrency + ? state.selectedFiat.value as FiatCurrency + : const FiatCurrency('USD', 'United States Dollar'),
158-168: Consider batching refresh triggers.
CallingFiatFormRefreshed(forceRefresh: true)on every single change can be expensive, especially if each triggers network calls. You might batch or debounce these calls for better performance.
193-213: Handle potential connectivity or error states for WebView.
Consider providing user feedback ifcheckoutUrlfails to load or if network issues occur.lib/bloc/fiat/models/fiat_buy_order_info.dart (2)
73-73: Ensure robust error handling withDecimal.parse.
Ifdata['fiat_amount']is non-numeric (e.g., an unexpected string), this could throw aFormatException. Consider a fallback or try-catch.- fiatAmount: Decimal.parse(data['fiat_amount']?.toString() ?? '0'), + fiatAmount: _parseDecimalOrZero(data['fiat_amount']),
129-129: SerializingDecimalconsistently.
UsingtoString()is a common approach for JSON output, though consider formatting if specific decimal precision is required by the consumer.lib/bloc/fiat/fiat_onramp_form/fiat_form_state.dart (1)
109-115:canSubmitgating ensures minimal checks.
Currently, it checksselectedAssetAddress != null. Consider adding validations for user’s coin selection and payment method if needed.lib/model/forms/fiat/fiat_amount_input.dart (1)
55-89: Robust multi-locale parsing implementation
TheparseLocaleAwareDecimalcovers several distinct decimal formats (standard, US/UK, and European). While thorough, note that some locales might use other separators (e.g. spaces). If you expect further locale variations, consider extending or centralizing these parsing rules in a dedicated utility.-// Third attempt: European format (1.234,56) +// Third attempt: European format (1.234,56) or other edge caseslib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart (2)
82-103: Good error handling when retrieving pubkeys
Catching exceptions and logging a warning before settingselectedAssetAddressto null helps avoid crashes. Consider surfacing this error to the UI, so the user knows that pubkey retrieval failed.
405-432: Order status watch concurrency
Usingrestartable()within_onWatchOrderStatushelps prevent multiple overlapping watchers. Confirm that you intend to allow or disallow restarts if the user initiates additional watch events for the same order.Would you like a shell script to verify concurrency usage across these event handlers in the entire codebase?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
.github/actions/generate-assets/action.yml(1 hunks)lib/app_config/app_config.dart(1 hunks)lib/bloc/coins_bloc/coins_bloc.dart(4 hunks)lib/bloc/fiat/banxa_fiat_provider.dart(8 hunks)lib/bloc/fiat/base_fiat_provider.dart(4 hunks)lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart(14 hunks)lib/bloc/fiat/fiat_onramp_form/fiat_form_event.dart(2 hunks)lib/bloc/fiat/fiat_onramp_form/fiat_form_state.dart(5 hunks)lib/bloc/fiat/fiat_repository.dart(7 hunks)lib/bloc/fiat/models/fiat_buy_order_info.dart(7 hunks)lib/bloc/fiat/models/fiat_payment_method.dart(7 hunks)lib/bloc/fiat/models/fiat_price_info.dart(3 hunks)lib/bloc/fiat/models/fiat_transaction_fee.dart(3 hunks)lib/bloc/fiat/ramp/models/host_assets_config.dart(1 hunks)lib/bloc/fiat/ramp/models/models.dart(1 hunks)lib/bloc/fiat/ramp/models/onramp_purchase_quotation/onramp_purchase_quotation.dart(1 hunks)lib/bloc/fiat/ramp/models/onramp_purchase_quotation/ramp_payment_method_name.dart(1 hunks)lib/bloc/fiat/ramp/models/onramp_purchase_quotation/ramp_quote_result.dart(1 hunks)lib/bloc/fiat/ramp/models/onramp_purchase_quotation/ramp_quote_result_for_payment_method.dart(1 hunks)lib/bloc/fiat/ramp/models/ramp_asset_info.dart(1 hunks)lib/bloc/fiat/ramp/ramp_fiat_provider.dart(8 hunks)lib/model/forms/fiat/fiat_amount_input.dart(3 hunks)lib/shared/utils/formatters.dart(3 hunks)lib/shared/utils/utils.dart(2 hunks)lib/views/fiat/fiat_form.dart(6 hunks)lib/views/fiat/fiat_inputs.dart(6 hunks)lib/views/fiat/fiat_payment_method_card.dart(3 hunks)test_units/tests/helpers/total_24_change_test.dart(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- lib/bloc/fiat/ramp/models/models.dart
- lib/bloc/fiat/ramp/models/onramp_purchase_quotation/onramp_purchase_quotation.dart
🚧 Files skipped from review as they are similar to previous changes (4)
- test_units/tests/helpers/total_24_change_test.dart
- lib/bloc/fiat/banxa_fiat_provider.dart
- lib/bloc/fiat/ramp/ramp_fiat_provider.dart
- lib/bloc/fiat/fiat_repository.dart
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/actions/generate-assets/action.yml
[error] 65-65: trailing spaces
(trailing-spaces)
[error] 66-66: trailing spaces
(trailing-spaces)
[error] 80-80: trailing spaces
(trailing-spaces)
🪛 GitHub Actions: takenagain is running unit tests on PR 🚀
lib/views/fiat/fiat_inputs.dart
[error] 208-208: Error: No named parameter with the name 'showBalanceIndicator'.
[error] 208-208: Error: Compilation failed.
🪛 GitHub Actions: takenagain is validating code guidelines 🚀
lib/views/fiat/fiat_inputs.dart
[error] 208-208: Error: No named parameter with the name 'showBalanceIndicator'.
🪛 GitHub Actions: takenagain is deploying a preview build to Firebase Hosting 🚀
lib/views/fiat/fiat_inputs.dart
[error] 208-208: No named parameter with the name 'showBalanceIndicator'.
[error] 1-1: Compilation failed. Flutter build exited with status 1.
🪛 GitHub Actions: takenagain is running UI tests on PR 🚀
lib/views/fiat/fiat_inputs.dart
[error] 208-208: Error: No named parameter with the name 'showBalanceIndicator'.
[error] 208-208: Error: Compilation failed.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Desktop (windows)
- GitHub Check: Build Mobile (iOS)
🔇 Additional comments (80)
.github/actions/generate-assets/action.yml (2)
53-63: Improved Trello parameter handling logicThe new conditional logic ensures that Trello parameters are only added to the build command when all required inputs are available. This is a more robust approach that prevents partial configuration issues.
68-80: Enhanced build process with better output handling and error reportingThe changes improve the build process by:
- Capturing and displaying output from
flutter pub get- Making the build command output visible (rather than redirecting to
/dev/null)- Properly capturing and handling the exit status
These changes will make debugging failures much easier.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 80-80: trailing spaces
(trailing-spaces)
lib/app_config/app_config.dart (1)
11-11: LGTM: Added scaling constant for high-precision calculationsThe new constant
scaleOnInfinitePrecisionwith a value of 20 is appropriate for handling cryptocurrencies like ETH that have high decimal precision (18 decimal places). This will help improve accuracy in financial calculations throughout the app.lib/bloc/fiat/models/fiat_transaction_fee.dart (1)
1-1: Improved precision by replacing double with Decimal for monetary valuesConverting from
doubletoDecimalfor theamountfield is an excellent change for financial calculations. Floating-point types likedoublecan introduce precision errors that are unacceptable when dealing with currency amounts.The implementation includes proper JSON serialization and deserialization for the
Decimaltype.Also applies to: 31-33, 35-35, 39-39
lib/bloc/fiat/ramp/models/onramp_purchase_quotation/ramp_payment_method_name.dart (1)
1-19: LGTM: Well-structured payment method enum with proper string conversionThe new
RampPaymentMethodNameenum is well-designed with:
- Clear value definitions for various payment methods
- A proper string value association for each method
- A robust
fromStringmethod with appropriate error handling for unknown valuesThis approach provides type safety when working with payment methods throughout the application.
lib/bloc/fiat/ramp/models/onramp_purchase_quotation/ramp_quote_result.dart (1)
1-32: Well-structured model implementationThis class is well-designed with:
- Proper immutability through final fields
- Clear factory method for JSON parsing
- Helper method for accessing payment methods
- Strong typing for financial data
The code follows good Dart practices and the structure aligns well with the apparent domain model.
lib/shared/utils/utils.dart (3)
12-12: Good addition of app_config importAdding the app configuration import allows for referencing global configuration values rather than hardcoded constants, improving maintainability.
70-72: Improved precision handling with configurable scaleReplacing the hardcoded value
24with the configurablescaleOnInfinitePrecisionvariable enhances flexibility and consistency across the application.
74-76: Consistent use of configurable scaleThis change ensures that the same precision scale is used consistently throughout the function, improving reliability in financial calculations.
lib/bloc/fiat/ramp/models/onramp_purchase_quotation/ramp_quote_result_for_payment_method.dart (1)
1-32: Well-designed financial model with appropriate precisionThis class makes good use of the Decimal type for financial values, which is crucial for accuracy in monetary calculations. The implementation includes:
- Appropriate immutability with final fields
- Clear factory method for JSON parsing
- Proper null handling for optional fields
Using Decimal instead of double for monetary values prevents floating-point precision issues that could lead to incorrect financial calculations.
lib/views/fiat/fiat_payment_method_card.dart (2)
31-32: Improved type safety with DecimalThe change from nullable double to Decimal improves type safety and precision. The comparison with
Decimal.zerois more explicit and safer than checking for null.
88-88: Enhanced numeric precision for percentage displayUsing
Decimal.fromInt(100)for multiplication ensures the calculation remains within the Decimal type system, maintaining precision throughout the entire calculation chain before converting to string.lib/shared/utils/formatters.dart (2)
8-8: Import for config usage – looks good.No issues found with this import statement for
app_config.dart.
183-183: Check numeric edge cases when formatting amounts.Using
toDecimal(scaleOnInfinitePrecision: scaleOnInfinitePrecision)plustoStringAsFixed(8)can cause rounding or truncation. Ensure large or very smallRationalvalues behave as expected.Do you want to run tests with boundary values to confirm the correct behavior?
lib/bloc/fiat/base_fiat_provider.dart (7)
5-5: Logger import – no immediate concerns.This import is appropriate to facilitate structured logging.
47-47: Static logger initialization – good addition.No immediate issues. This allows structured error and warning messages for the class.
51-51:@protectedusage is suitable here.This annotation cleanly indicates internal usage. No concerns.
100-101: Potential risk of logging sensitive data.Returning
Future.error(json.decode(response.body))after logging a warning might leak sensitive info if the response payload includes user data. Confirm no PII is present in the response.
103-104: Verbose logging of error & stack trace.Double-check that logging stack traces never reveals sensitive details. Otherwise, this approach is standard for diagnosing network issues.
20-20:✅ Verification successful
Changed return type to more specialized list.
Replacing
Future<List<ICurrency>>withFuture<List<FiatCurrency>>clarifies the expected data. Verify all callers have been updated accordingly.
🏁 Script executed:
#!/bin/bash # Description: Search for references to getFiatList to ensure correct usage updates rg -A 3 'getFiatList'Length of output: 1715
Verified return type update for getFiatList across all consumers.
- The change from
Future<List<ICurrency>>toFuture<List<FiatCurrency>>inlib/bloc/fiat/base_fiat_provider.darthas been consistently applied.- References in
lib/bloc/fiat/fiat_repository.dart,lib/bloc/fiat/ramp/ramp_fiat_provider.dart, andlib/bloc/fiat/banxa_fiat_provider.dartnow correctly reflect the specialized return type.- All callers, including those in
lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart, have been updated accordingly.
22-22:✅ Verification successful
Changed return type to more specialized list.
Similar to
getFiatList, confirm that references togetCoinListhandle the newCryptoCurrencytype properly.
🏁 Script executed:
#!/bin/bash # Description: Search for references to getCoinList to ensure correct usage updates rg -A 3 'getCoinList'Length of output: 5876
Changed return type to a more specialized list.
After verifying across the codebase, all found references to
getCoinList—including those in test cases and provider implementations—appear to correctly handle the newCryptoCurrencytype (similar to howgetFiatListis used). No further issues were detected, so please ensure that dependent modules maintain this behavior during ongoing development.lib/bloc/fiat/models/fiat_price_info.dart (5)
1-1: Good move importing Decimal for financial calculations.Adding the Decimal package is a significant improvement for handling financial values with the precision they require.
13-19: Great use of a static zero constant.Using a static
zeroconstant with properDecimal.zeroinitialization provides a reusable empty state instance, which improves code consistency and reduces object creation.
23-28: Proper defensive parsing of JSON values.The conversions from JSON now properly handle potential null values and use
Decimal.parsefor accurate financial representation instead of floating-point values.
32-36: Improved type safety for financial fields.The transition from
doubletoDecimalfor monetary values will prevent floating-point precision issues that are especially problematic in financial applications.
57-61: Proper serialization of Decimal values.Converting Decimal values to strings before adding them to JSON is the correct approach. This ensures precise values are preserved during serialization.
lib/bloc/fiat/models/fiat_payment_method.dart (6)
2-2: Appropriate import for financial calculations.Adding the Decimal package is consistent with the pattern established in other financial classes.
20-29: Good refactoring to use a static field instead of constructor.Replacing the
.none()constructor with a static field improves code clarity and performance by avoiding repeated object instantiation.
54-55: Proper parsing of relative percent as Decimal.Using
Decimal.parsefor the relative percent ensures consistency with the precision improvements throughout the codebase.
66-66: Improved type safety for relative percent.Changing from
doubletoDecimalis consistent with the application's approach to handling financial values.
102-102: Correct serialization of Decimal values.The
toString()method ensures proper representation of the Decimal value in JSON format.
122-153: Well-structured default payment methods with Decimal values.The default payment methods now properly use
FiatPriceInfo.zeroand explicitDecimalvalues, maintaining the improved precision throughout the codebase.lib/bloc/fiat/ramp/models/ramp_asset_info.dart (2)
3-19: Well-defined model class with appropriate fields.The
RampAssetInfoclass has a comprehensive set of fields to represent an asset, with proper typing includingDecimalfor financial values.
38-50: Good validation methods for purchase amounts.The
hasValidMinPurchaseAmountandhasValidMaxPurchaseAmountmethods provide clear validation logic with proper handling of the special case where -1 indicates no limit.lib/bloc/coins_bloc/coins_bloc.dart (5)
7-10: Appropriate imports for additional utilities.The added imports provide access to necessary utility functions and extensions that support the modified implementation.
151-157: Good addition of coin state synchronization.This additional block ensures that coin states are properly synchronized after balance updates, improving the reliability of the displayed coin information.
541-541: More efficient asset retrieval directly from SDK.Retrieving activated assets directly from the SDK with
getActivatedAssets()is more efficient than going through the repository.
544-546: Improved coin lookup using firstWhereOrNull.The use of
firstWhereOrNullwith a direct predicate is cleaner and more efficient than previous approaches.
563-567: Good handling of API-enabled but GUI-disabled coins.This loop ensures that any coins enabled on the API side but not on the GUI side are properly enabled, maintaining synchronization between the two.
lib/views/fiat/fiat_form.dart (7)
37-44: Ensure clean-up or subscription management if needed.
Although you removeddispose(), confirm that there are no ongoing streams or listeners that need unsubscribing to prevent any potential memory leaks.Are there any active listeners or streams that might remain subscribed after this widget is disposed?
61-70: Check for potential race conditions with coin bootstrap.
You correctly handle theisLoginCoinBootstrapFinishedflag. However, confirm there aren’t edge cases where slow fetching ofwalletCoinsmight conflict with subsequent events.
155-156: Method rename aligns well with private usage.
RenamingcompleteOrderto_completeOrderclarifies scoping. The dispatch toFiatFormSubmittedappears correct.
170-171: Tab index update logic looks solid.
UsingFiatFormModeUpdated.fromTabIndexis a clear pattern, ensuring the correct form mode is derived from the selected tab.
179-181: Double-check handling for in-progress orders on logout.
When the user logs out mid-transaction, confirm that partial orders are canceled or saved appropriately.Could you verify if partial orders remain open or if they’re cleaned up automatically on the server?
224-238: Ensure only one dialog or snackbar at a time.
Multiple state updates (e.g., fromsubmittedtofailed) might trigger repeated dialogs or snackbars. Confirm that transitions happen in sequence without stacking feedback.
241-278: Dialog-based status feedback is straightforward.
Your approach to using an AlertDialog is user-friendly. If you need more customization or concurrent flows later, consider a more dynamic overlay or dedicated status screen.lib/bloc/fiat/models/fiat_buy_order_info.dart (4)
24-32: Good transition toDecimal.zerofor precision.
This helps avoid floating-point inaccuracies and ensures consistent monetary calculations.
42-50: Constructors handle default values consistently.
InitializingfiatAmountwithDecimal.zeroinfromCheckoutUrlis a sensible default.
92-92: Accurate representation of fiatAmount withDecimal.
Switching fromdoubletoDecimalensures higher precision in financial operations.
149-149: Copy method signature aligns withDecimalusage.
This ensures consistency across the entire class.lib/bloc/fiat/fiat_onramp_form/fiat_form_state.dart (8)
5-8: Informative doc comments.
The improved documentation clarifies the purpose ofFiatFormState.
27-46: Initial state defaults are well-chosen.
Setting BTC as a default asset and USD as the default fiat currency is fine. Confirm that these defaults align with UX expectations.
79-84: Typed fiatList and coinList enhance clarity.
Switching fromIterable<ICurrency>toIterable<FiatCurrency>/Iterable<CryptoCurrency>eliminates ambiguity in the codebase.
97-101: Decimal-based min/max amounts.
UsingDecimalfor boundary limits mitigates rounding errors. Consider verifying string conversions for large amounts.
103-108:isLoadinglogic might be too simplistic.
Relying onfiatList.length < 2 || coinList.length < 2could incorrectly label states with fewer supported networks as “loading.” Evaluate if a single currency scenario is valid.
117-132: copyWith respects new properties.
All newly introduced state fields are included incopyWith, preserving consistency.
155-157: Expanded validation inputs.
IncludingselectedAssetininputsclarifies form validation. Confirm you are testing these validations thoroughly.
162-175: Props array updated to match new fields.
Ensures Equatable tracks the updated properties in equality checks.lib/model/forms/fiat/fiat_amount_input.dart (2)
21-29: Refactor ensures precise currency handling
Switching fromdoubletoDecimalin the constructors and introducingvalueAsDecimalimproves numeric precision for currency amounts. This is a solid approach given floating-point inaccuracies.
33-37: Consider clarifying negative or zero-edge cases
The validator checks for empty input and invalid format but does not explicitly handle negative or zero amounts. If negative amounts are disallowed, consider adding an explicit check to ensure clarity.Do you want me to verify whether negative or zero amounts are permissible elsewhere in the codebase, or add a check here?
lib/views/fiat/fiat_inputs.dart (2)
19-34: Constructor and new parameters
The updated parameters (selectedAsset,selectedAssetAddress, andselectedAssetPubkeys) align with the project’s shift toward typed asset handling. Verify that future usage consistently references these new fields to avoid confusion with any legacy naming.
218-239: Generified asset selection
Splitting the selection dialog into "fiat" vs. "coin" is clear. The generic parameter<C extends ICurrency>fosters reusability. Ensure that UI labels and placeholders reflect the correct asset type (“Select Fiat” vs. “Select Coin”).lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart (2)
156-205: Ensure address presence before submit
Form submission referencesstate.selectedAssetAddress!.addressdirectly. While there's a prior check, consider an early return or assertion ifselectedAssetAddressis null. This guards against unforeseen races or uninitialized states.
304-322: Reloading pubkeys post-authentication
RefreshingselectedAssetAddressinside_onWalletAuthenticatedis a clean approach. Confirm if any concurrent event calls_onCoinSelectedagain during the same timeframe, which might override or conflict with updated pubkeys.lib/bloc/fiat/fiat_onramp_form/fiat_form_event.dart (16)
3-3: Good addition of class documentation.The newly added documentation clearly describes the purpose of the base class, improving code readability and maintainability.
11-14: LGTM: Well-designed new event for form initialization.The addition of
FiatFormStartedevent with appropriate documentation follows the BLoC pattern best practices and provides a clear entry point for form initialization.
16-17: Consistent naming convention applied.The renaming to
FiatFormOnRampPaymentStatusMessageReceivedaligns with the new naming pattern and improves code consistency.
26-30: Improved type safety with specialized types.The class was renamed from
SelectedFiatCurrencyChangedtoFiatFormFiatSelectedfor consistency, and now uses a specificFiatCurrencytype instead of a more generic one, enhancing type safety.
36-40: Good renaming and type specification.Similar to the fiat currency selection, this event has been renamed from
SelectedCoinChangedto follow the consistent naming pattern, and uses a specificCryptoCurrencytype.
46-48: Clear naming improvement.Renaming from
FiatAmountChangedtoFiatFormAmountUpdatedfollows the established pattern and better describes the action.
56-58: Consistent event naming implemented.Renamed from
PaymentMethodSelectedto follow theFiatForm[Action]pattern, improving code consistency.
66-69: Streamlined form submission event.Renamed from
FormSubmissionRequestedtoFiatFormSubmitted, which is more concise while maintaining clarity.
71-76: Well-structured mode update event with helpful factory constructor.The named constructor
fromTabIndexis a nice addition that encapsulates the conversion logic from UI tab index to domain model.
85-88: Good addition for state management.The new
FiatFormPaymentStatusClearedevent provides a clear way to reset payment status, which helps manage the UI state properly.
90-93: Important new event for authentication flow.Adding
FiatFormWalletAuthenticatedhelps track the authentication state, which is crucial for secure transactions.
95-98: Useful event for resetting account state.The
FiatFormAccountClearedevent enables proper cleanup of account data when needed.
100-111: Well-designed refresh mechanism.The new
FiatFormRefreshedevent with theforceRefreshflag provides flexibility in how data is refreshed, allowing for cache optimization.
113-116: Good addition for currency data fetching.The
FiatFormCurrenciesFetchedevent clearly separates the concern of fetching available currencies from other form actions.
118-121: Important addition for transaction monitoring.The
FiatFormOrderStatusWatchStartedevent enables tracking of transaction status, which is essential for providing feedback to users.
123-131: Key enhancement supporting HD wallet address selection.The
FiatFormCoinAddressSelectedevent using thePubkeyInfotype enables selection of specific addresses from HD wallets, directly addressing the PR objective of supporting HD wallet address selection.
- avoid type casting in ui layer - add documentation comments - remove spaces at the end of github action lines
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Visit the preview URL for this PR (updated for commit 46edfb3): https://walletrc--pull-2570-merge-8h6nd0n7.web.app (expires Mon, 05 May 2025 06:37:52 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f66a4ff03faa546f12f0ae5a841bd9eff2714dcc |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lib/views/fiat/fiat_inputs.dart (1)
199-213:⚠️ Potential issueThe showBalanceIndicator parameter issue remains.
The
showBalanceIndicatorparameter at line 209 was previously flagged as causing build errors. Although you mentioned it will be added in a future SDK update, it's still present in the code which could continue to cause build failures.- showBalanceIndicator: false, + // showBalanceIndicator: false, // Will be added in future SDK update
🧹 Nitpick comments (3)
lib/bloc/fiat/ramp/models/onramp_purchase_quotation/ramp_quote_result_for_payment_method.dart (1)
59-68: Consider unifying decimal parsing approach for consistencyWhile the implementation is generally good, there's an inconsistency in how decimal values are parsed.
cryptoAmountuses direct parsing from a string (line 61), while other decimal fields use.toString()first (lines 62-67). This could cause issues if the API changes its response format.return RampQuoteResultForPaymentMethod( fiatCurrency: json['fiatCurrency'] as String, - cryptoAmount: Decimal.parse(json['cryptoAmount'] as String), + cryptoAmount: Decimal.parse(json['cryptoAmount'].toString()), fiatValue: Decimal.parse(json['fiatValue'].toString()), baseRampFee: Decimal.parse(json['baseRampFee'].toString()), appliedFee: Decimal.parse(json['appliedFee'].toString()), hostFeeCut: json['hostFeeCut'] != null ? Decimal.parse(json['hostFeeCut'].toString()) : null, );lib/model/forms/fiat/fiat_amount_input.dart (1)
57-58: TODO for further refactoringConsider moving the
parseLocaleAwareDecimalfunction into the SDK or as an extension method on theDecimalclass as suggested by the TODO comment. This would make the functionality more reusable across the application.lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart (1)
214-270: Enhanced refresh form logic with better error handling.The updated refresh form logic properly handles state transitions and has improved error handling with structured logging.
However, there might be an opportunity to optimize the state emissions:
Consider potentially combining the state updates at lines 225-228 and line 240 to reduce UI rebuilds during the refresh operation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/komodo_ui_kit/pubspec.lockis excluded by!**/*.lockpubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.github/actions/generate-assets/action.yml(1 hunks)lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart(15 hunks)lib/bloc/fiat/fiat_onramp_form/fiat_form_state.dart(5 hunks)lib/bloc/fiat/ramp/models/host_assets_config.dart(1 hunks)lib/bloc/fiat/ramp/models/onramp_purchase_quotation/ramp_quote_result.dart(1 hunks)lib/bloc/fiat/ramp/models/onramp_purchase_quotation/ramp_quote_result_for_payment_method.dart(1 hunks)lib/bloc/fiat/ramp/models/ramp_asset_info.dart(1 hunks)lib/model/forms/fiat/currency_input.dart(2 hunks)lib/model/forms/fiat/fiat_amount_input.dart(3 hunks)lib/views/fiat/fiat_form.dart(7 hunks)lib/views/fiat/fiat_inputs.dart(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/actions/generate-assets/action.yml
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Desktop (windows)
- GitHub Check: Build Mobile (Android)
- GitHub Check: Build Mobile (iOS)
🔇 Additional comments (48)
lib/bloc/fiat/ramp/models/onramp_purchase_quotation/ramp_quote_result.dart (1)
1-65: Well-structured model class with proper validation and error handlingThe implementation of
RampQuoteResultfollows good practices for JSON parsing and validation. The class appropriately checks for required fields, validates types, and provides clear error messages when validation fails.lib/bloc/fiat/ramp/models/ramp_asset_info.dart (2)
76-86: LGTM: Clear validation methods for purchase amountsThe methods for validating minimum and maximum purchase amounts are well-implemented and handle the special value of -1 correctly to indicate no limit.
94-149: Robust JSON parsing with good validationThe
fromJsonfactory method properly validates required fields and handles type checking appropriately. The use ofDecimal.tryParsefor optional fields is a good approach to gracefully handle parsing errors.lib/bloc/fiat/ramp/models/host_assets_config.dart (2)
48-80: Well-structured JSON parsing with good validationThe
fromJsonfactory method makes good use of helper methods to validate required fields and parse decimal values. The code properly validates that the 'assets' field is a list and that each asset is a valid JSON object.
82-103: Effective helper methods for validation and parsingThe private helper methods
_validateRequiredFieldand_parseDecimalare well-implemented and provide clear error messages. The consistent use of these methods throughout the class reduces code duplication and improves maintainability.lib/model/forms/fiat/currency_input.dart (3)
14-15: Good use of generics for type safety improvementsConverting the class to use a generic type parameter (
T extends ICurrency) is a solid improvement that enhances type safety. This allows for more specific typing in consumer code, reducing the risk of runtime type errors when working with different currency types.
20-21: Type parameter correctly applied to validator methodThe validator method now correctly uses the generic type parameter, maintaining type safety throughout the validation process.
33-33: Consistent generic type usageThe
isCurrencySupportedmethod correctly adopts the generic type parameter, completing the class's conversion to a proper generic implementation.lib/bloc/fiat/fiat_onramp_form/fiat_form_state.dart (10)
12-13: Renamed property improves semantic clarityChanging from
selectedCointoselectedAssetis a good semantic improvement that better represents the concept being modeled, especially when working with multiple types of assets.
23-24: Good addition of asset-specific propertiesAdding
selectedAssetAddressandselectedCoinPubkeysproperties provides better structure for managing HD wallet addresses, which appears to be the main focus of this PR.
36-37: Initial state properly handles new fieldsThe initial state constructor correctly initializes the new properties to null, maintaining the expected initial state behavior.
48-50: Improved type safety with generic currency inputsThe code now uses the generic
CurrencyInput<FiatCurrency>andCurrencyInput<CryptoCurrency>types instead of the less specificCurrencyInput. This leverages the generic improvements made incurrency_input.dartand improves type safety throughout the app.Also applies to: 51-52
61-62: HD wallet address selection support addedThese properties add the necessary state to support HD wallet address selection, which is essential for the feature described in the PR title.
Also applies to: 64-65
80-81: Type specificity improves compiler checksUsing specific currency types (
FiatCurrencyandCryptoCurrency) instead of the genericICurrencyimproves type safety and provides better IDE support for developers.Also applies to: 83-84
98-99: Using Decimal type for currency amountsChanging from
double?toDecimal?for currency-related values is a good practice that avoids floating-point precision issues common in financial applications.Also applies to: 101-102
112-113: Improved validation logicThe updated
canSubmitlogic now checks for the presence ofselectedAssetAddressinstead of a previously removed property, ensuring proper validation of the user's selection.
118-120: Updated copyWith method maintains immutability patternThe
copyWithmethod has been properly updated to include all new properties, maintaining the immutability pattern that's important for state management in Flutter.Also applies to: 127-129, 131-132
148-149: Proper inclusion in props list ensures equality comparisons workThe new properties have been correctly added to the
propslist, ensuring that equality comparisons (used by the Bloc library) work correctly.lib/views/fiat/fiat_form.dart (10)
37-46: Simplified initialization with event-driven approachThe initialization logic has been streamlined to dispatch a refresh event rather than setting up multiple listeners. This is a cleaner approach that centralizes state management in the Bloc.
63-75: Improved state management with BlocListener and BlocConsumerUsing BlocListener for the CoinsBloc and BlocConsumer for the FiatFormBloc creates a cleaner separation of concerns and more predictable state updates.
99-107: Proper handling of currency and asset selectionThe implementation now correctly passes the selected asset address and pubkeys to the FiatInputs widget, addressing the issue mentioned in the PR description about failure to update the address field when changing the coin.
130-131: Renamed method with proper underscore prefixRenaming
completeOrderto_completeOrderfollows the convention of marking private methods with an underscore, improving code readability and organization.
158-169: Improved event handling for field changesThe implementation now properly dispatches both the specific field update event and a forced refresh event when values change, ensuring consistent form state.
179-182: Simplified account status handlingThe account status change handler now simply dispatches appropriate events to the bloc rather than implementing complex logic, following a cleaner separation of concerns.
233-235: Fixed QR code visibility issueThis implementation addresses the pop-up issue mentioned in the PR description, where the Ramp fiat onramp process was covering the QR code.
247-298: Improved payment status dialog handlingConverting the dialog method to asynchronous and simplifying the implementation provides better user experience by ensuring proper dialog flow and potential animation completion.
50-61: TODO comment provides context for future developmentThe comprehensive TODO comment provides valuable context for future development related to checkout URL reuse, which is a good practice for maintaining code knowledge.
210-218: Enhanced console message handlingThe improved console message handling dispatches events directly to the bloc, maintaining proper separation of concerns and event-driven architecture.
lib/model/forms/fiat/fiat_amount_input.dart (4)
21-27: Improved precision with Decimal typeChanging from
double?toDecimal?for currency-related values is an excellent improvement for financial calculations, avoiding the floating-point precision issues that can occur with doubles.
28-29: Better currency value parsingReplacing
valueAsDoublewithvalueAsDecimaland using the newparseLocaleAwareDecimalfunction provides more accurate and locale-aware parsing.
37-38: Updated validator with improved parsingThe validator now uses the locale-aware parsing function, ensuring consistent validation regardless of the user's number format preferences.
55-92: Excellent locale-aware decimal parsing implementationThe
parseLocaleAwareDecimalfunction is a comprehensive solution for handling different number formats. It tries multiple parse strategies to accommodate various locale conventions:
- Standard format with period as decimal separator
- US/UK format with comma thousand separators
- European format with periods as thousand separators and commas as decimal separators
This is exactly what was needed to address the previous review comment about potential issues with locale-based comma usage, and the implementation goes beyond the basic suggestion to create a robust solution.
lib/views/fiat/fiat_inputs.dart (7)
3-6: Good use of structured imports.The addition of Komodo SDK and types modules along with flutter_bloc imports sets the foundation for the upcoming changes and properly organizes the dependency imports.
17-34: Improved parameter types for better type safety.The refactoring from generic ICurrency to specific FiatCurrency and CryptoCurrency types is a great improvement that enhances type safety and makes the code more maintainable. The new parameters like
selectedAssetPubkeysandonSourceAddressChangedproperly support the feature to handle HD wallet addresses.Also applies to: 36-52
75-93: Good migration to Decimal for financial calculations.Converting from double to Decimal for financial calculations is a best practice as it avoids floating-point precision issues. The updated comparison logic properly handles this type change.
95-105: Type-safe callback implementations.The methods now correctly accept specific currency types, which matches the function signatures in the class properties and improves type safety throughout the codebase.
134-187: Properly updated widget references.The UI elements now use the updated property names and types, maintaining consistency with the refactored model.
223-239: Improved type-specific dialog handling.Good job using separate type-specific method calls for fiat and coin selection dialogs. This improves type safety and makes the code more maintainable.
242-282: Well-implemented generic selection dialog.The generic implementation using type parameter
C extends ICurrencymakes the code more flexible while maintaining type safety. This is a good pattern for reusable UI components that need to handle different but related types.lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart (9)
6-12: Proper imports for enhanced functionality.The addition of decimal, SDK, types, and logging imports sets up the foundation for the code improvements throughout the file.
27-58: Well-structured event handling setup.The constructor now correctly uses KomodoDefiSdk instead of CoinsRepo, and event handlers follow a consistent naming convention. The use of appropriate transformer strategies (debounce vs restartable) for different events helps prevent UI jank.
82-107: SDK integration for pubkey management.Good implementation of the SDK calls to fetch pubkeys for the selected asset. There's proper error handling with structured logging which improves debuggability.
109-136: Improved financial calculations with Decimal.The migration to use Decimal instead of double for financial calculations is a best practice that avoids floating-point precision issues. The
_getAmountInputWithBoundsmethod correctly handles this transition.
305-324: Good wallet authentication handling.The new method provides proper handling for wallet authentication events and retrieves pubkeys as needed. The error handling and logging are implemented well.
333-347: Well-implemented asset pubkeys update method.This new helper method properly encapsulates the logic for updating asset pubkeys, making the code more maintainable. The try-catch block with structured logging enhances error handling.
404-432: Improved order status monitoring with structured error handling.The order status monitoring implementation now has better error handling with proper logging. The use of emit.forEach with onError handler ensures the bloc properly manages failures during streaming operations.
442-452: TODO comment should be addressed.There's a TODO comment about handling Banxa error responses that hasn't been addressed. This might be an opportunity to improve the user experience by providing more specific error messages.
Is this TODO still relevant? If so, consider creating a task to address it in a future update or implement a solution now.
476-485: Well-implemented address selection handler.The new method for handling coin address selection properly updates the state with the selected address. This is essential for the HD wallet address selection feature.
remove debouncing on input events, and change refresh form from debounce to restartable to avoid unexpected UI changes to to delayed state changes
|
Can't seem to progress beyond this screen regardless of the address input: I did notice tho that in the "source address" dropdown, only non-segwit BTC addresses are listed - we should list both. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
@smk762 were you using a local debug build at the time? I could reproduce the issue with BTC when using a debug build, but profile and release builds worked for me. It might be restrictions in the Ramp testing environment specific to BTC. MATIC and ETH work for testing with "Manual Bank Transfer" in debug builds, although the transactions do not reach completion because they are not the Ramp test network coins. The Ramp test coins are currently being filtered out, so we could look into making them visible in debug/profile modes to simplify testing. I've added segwit coins to the dropdown in the meantime. |
|
Yep, local as per first post in PR Confirmed seeing btc segwit in the list, thanks. Can we please make it the default instead of BTC? |
also add retry logic to the pubkey fetching in fiat onramp bloc
|
Confirmed BTC-segwit is now the default 🙏 Banxa purchase via credit card successfully submitted 🎉 I got a payment failed pop up, though after checking email, wallet and bank account, it was actually successful. Unable to complete a "real world" purchase with RAMP, as using AUD for fiat only supports Banxa. |
CharlVS
added
P0
There was a problem hiding this comment.
Thank you for your contributions, @takenagain. Please review the Banxa bug as soon as possible, when you are available. There has been a change to the structure of their order status payload in their latest API version, but that shouldn't affect us since the URL should be pegged to a specific version. Please verify that we are using a pegged URL
CharlVS
force-pushed
the
feat/fiat-form-hd-support
branch
from
bc50a96 to
04696c9
Compare
This PR adds the new address selection widget used by the withdrawal form to the fiat on ramp page, and fixes the issues listed below.
Fiat Form bugs fixed by this PR
Fiat on ramp demo/testing environment
The Ramp and Banxa test/demo/sandbox environments only work for debug builds, so testing needs to be done with a local build rather than preview links, unless testing with real fiat transfers is desired. Preview links, like the one in the comment below, use production APIs and configurations.
Ramp
You can make a test purchase using the guidelines below (demo environment only):
Banxa
See #1362 comment for more information. The relevant notes were copied below:
Summary by CodeRabbit
Decimalinstead ofdouble.