[0.8.3+0] RC: add arbitrum coin type and private key export #2512
Conversation
WalkthroughThis pull request introduces comprehensive support for the Arbitrum (ARB) coin type across multiple components of the application. The changes span various files, adding Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
takenagain
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/bloc/fiat/base_fiat_provider.dart (1)
Line range hint
180-181: Remove outdated comment.The comment about "ARBITRUM" in the unsupported chains section is now outdated since Arbitrum is now supported.
Apply this diff to remove the outdated comment:
// These exist in coin config but not in CoinType structure yet: - // ARBITRUMlib/shared/utils/utils.dart (1)
502-503: Consider using a unique color for Arbitrum.The current implementation uses the same color as QRC20 (0, 168, 226, 1). Each protocol typically has its own unique color for better visual distinction.
- return const Color.fromRGBO(0, 168, 226, 1); + return const Color.fromRGBO(28, 163, 255, 1); // Arbitrum's brand color
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
lib/app_config/app_config.dart(1 hunks)lib/bloc/fiat/base_fiat_provider.dart(3 hunks)lib/model/coin.dart(1 hunks)lib/model/coin_type.dart(1 hunks)lib/model/coin_utils.dart(2 hunks)lib/shared/constants.dart(1 hunks)lib/shared/utils/prominent_colors.dart(1 hunks)lib/shared/utils/utils.dart(5 hunks)lib/shared/widgets/coin_item/coin_logo.dart(1 hunks)lib/shared/widgets/coin_type_tag.dart(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_preview
🔇 Additional comments (16)
lib/model/coin_type.dart (1)
10-10: LGTM! The newarb20coin type is properly integrated.The addition follows the established naming convention and maintains alphabetical ordering with other similar token standards.
lib/shared/widgets/coin_type_tag.dart (1)
53-54: LGTM! The ARB20 tag is properly implemented.The implementation follows the established pattern of uppercase token standard names and maintains consistency with other coin types.
lib/shared/constants.dart (1)
20-21: LGTM! The Arbitrum transaction history URLs are properly defined.The constants follow the established pattern with both v1 and v2 endpoints, maintaining consistency with other token types.
lib/shared/widgets/coin_item/coin_logo.dart (1)
110-111: Verify that the 'arb' protocol icon asset exists.The implementation follows the established pattern, but we should verify that the 'arb' icon asset is available in the assets directory.
✅ Verification successful
The implementation follows the correct pattern for protocol icons
The 'arb' protocol icon is not expected to be in the repository as all coin icons are fetched during build time from the KomodoPlatform/coins repository, as configured in build_config.json. The implementation in coin_logo.dart follows the established pattern for protocol icons.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the 'arb' protocol icon asset exists # Note: Common asset locations based on the import of CoinIcon widget # Search for icon asset files containing 'arb' fd -t f "arb" "assets/" # Search for any references to 'arb' icon in asset declarations rg -l "arb.*\.(png|svg|jpg)" "assets/"Length of output: 60
Script:
#!/bin/bash # Check pubspec.yaml for asset declarations cat pubspec.yaml # Search for any protocol-related icons in assets fd "protocol" "assets/" # Look for similar coin type cases to understand the pattern ast-grep --pattern 'case CoinType.$_: return $_' # Search for any mentions of 'arb' or 'arbitrum' in the codebase rg -i "arb(itrum)?" -g "!*.lock" -g "!*.log"Length of output: 63175
Script:
#!/bin/bash # Search for 'arb' icon in coin_icons directories fd -t f "arb" "assets/coin_icons/" fd -t f "arb" "assets/coin_icons/png/" fd -t f "arb" "assets/blockchain_icons/svg/" # List contents of coin icons directories to understand structure ls -R assets/coin_icons/ ls -R assets/blockchain_icons/Length of output: 607
Script:
#!/bin/bash # Check build configuration files cat app_build/build_config.json # Search for asset fetch configuration files fd -t f "build.*config.*" . # Look for documentation about asset handling rg -i "fetch.*coin.*assets?" -g "!*.lock"Length of output: 3330
lib/app_config/app_config.dart (1)
48-48: LGTM!The priority value of 10 for 'ARB' is consistent with other similar coins like 'MATIC', 'FTM', and 'AVAX'.
lib/model/coin_utils.dart (2)
125-126: LGTM!The short name 'ARB-20' follows the established pattern of hyphenated format used by other coin types.
168-169: LGTM!The long name 'Arbitrum' correctly represents the full chain name.
lib/bloc/fiat/base_fiat_provider.dart (3)
32-32: LGTM!The condition for
CoinType.arb20follows the established pattern used by other coin types.
159-160: LGTM!The case for
CoinType.arb20ingetCoinChainId()correctly returns 'ARB'.
267-268: LGTM!The case for "ARB" in
getCoinType()correctly returnsCoinType.arb20.lib/shared/utils/prominent_colors.dart (1)
77-77: LGTM!The color value "#28a0f0" for the "arb" ticker is in the correct format and properly placed in alphabetical order.
lib/model/coin.dart (1)
458-463: LGTM! Arbitrum coin type integration looks good.The implementation follows the established pattern and correctly maps the "Arbitrum" JSON type to
CoinType.arb20.lib/shared/utils/utils.dart (4)
288-288: LGTM! ARB20 suffix added to filtered list.The addition of 'ARB20' to filteredSuffixes ensures proper handling of Arbitrum token abbreviations.
374-382: LGTM! Transaction history support for Arbitrum looks good.The implementation follows the established pattern for ERC-like tokens and correctly uses Arbitrum-specific URLs.
549-549: LGTM! Transaction history support enabled for Arbitrum.Correctly adds Arbitrum to the list of coin types that support transaction history.
577-577: LGTM! Explorer URL handling for Arbitrum addresses.Correctly adds Arbitrum to the list of coin types that use the standard address explorer URL format.
* kdf show_priv_key api * display private keys * private keys QR code dialog * private keys list title * private key dialog width * private key share instead of copy * Revert "private key share instead of copy" This reverts commit a623444ea75e4d074419bcc9689585ca819beceb. * private key clipboard warning
* router fix uri query parameters were lost at parse * router add dexroute parameters * router dex state notifyListeners for new fields * router dex process order_type param * router dex clean params without notifying * router dex maker process ticker and amount params * router dex fix typo * dex TakerSetSellCoin event add setOnlyIfNotSet * router dex maker consumes only their own params * router dex allow order_type being maker by default * router dex taker process from/to currencies * dex repository waitOrderbookAvailability function * router dex form wait for orderbooks before process * router dex taker process from_amount * router dex code cleanup * mobile/widget layout taker form dropdown position * maker form parse error check
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
lib/blocs/maker_form_bloc.dart (1)
Line range hint
567-574: Add error handling for price value parsing.The
setPriceValuemethod should include a try-catch block similar tosetSellAmountandsetBuyAmountto handle invalid input gracefully.Apply this diff to add error handling:
void setPriceValue(String? priceStr) { priceStr ??= ''; Rational? priceValue; if (priceStr.isEmpty) { priceValue = null; } else { - priceValue = Rational.parse(priceStr); + try { + priceValue = Rational.parse(priceStr); + } catch (_) { + priceValue = null; + } } if (priceValue == price) return; price = priceValue; _onPriceUpdated(); }
🧹 Nitpick comments (9)
lib/mm2/mm2_api/rpc/show_priv_key/show_priv_key_response.dart (1)
7-11: Consider adding validation in fromJson factory.The factory constructor silently defaults to empty strings when JSON fields are missing. This could mask potential API errors.
factory ShowPrivKeyResponse.fromJson(Map<String, dynamic> json) => ShowPrivKeyResponse( - coin: json['result']['coin'] ?? '', - privKey: json['result']['priv_key'] ?? '', + coin: json['result']?['coin'] ?? + throw FormatException('Missing coin in response'), + privKey: json['result']?['priv_key'] ?? + throw FormatException('Missing private key in response'), );lib/mm2/mm2_api/rpc/show_priv_key/show_priv_key_request.dart (1)
3-19: Consider adding coin parameter validation.The request class accepts any string as coin without validation. Consider adding basic validation to catch obvious errors early.
class ShowPrivKeyRequest implements BaseRequest { ShowPrivKeyRequest({ required this.coin, - }); + }) { + if (coin.trim().isEmpty) { + throw ArgumentError('Coin cannot be empty'); + } + } @override late String userpass; @override final String method = 'show_priv_key'; final String coin;lib/views/settings/widgets/security_settings/security_settings_page.dart (1)
34-34: Consider security implications of storing private keys in memory.The
_privKeysmap stores sensitive data in memory. Consider:
- Clearing the map when leaving the page
- Using a secure string implementation that overwrites memory
- Adding a timeout to automatically clear the keys
Also applies to: 77-77, 88-88, 93-93
assets/translations/en.json (1)
466-467: LGTM with a suggestion to enhance the security warning.The translations are clear and follow the existing patterns. Consider enhancing the warning message to include:
- A recommendation to clear the clipboard after use
- A suggestion to use a hardware wallet for increased security
- "copyWarning": "Your Clipboard isn't a safe place for your private key! Copying the seed phrase or private keys can make them vulnerable to clipboard hacks. Please handle with caution and only copy if absolutely necessary.", + "copyWarning": "Your Clipboard isn't a safe place for your private key! Copying the seed phrase or private keys can make them vulnerable to clipboard hacks. Please handle with caution, only copy if absolutely necessary, and clear your clipboard immediately after use. For enhanced security, consider using a hardware wallet.",lib/router/parsers/dex_route_parser.dart (1)
17-25: LGTM! Consider adding parameter validation.The implementation for handling single path segment URIs is clean and follows best practices. The default empty string values prevent null issues.
Consider adding validation for the query parameters to ensure they meet expected formats, especially for numerical values like
from_amountandto_amount.if (uri.pathSegments.length == 1) { + final fromAmount = uri.queryParameters['from_amount'] ?? ''; + final toAmount = uri.queryParameters['to_amount'] ?? ''; + // Validate numerical parameters + if (fromAmount.isNotEmpty && !RegExp(r'^\d*\.?\d*$').hasMatch(fromAmount)) { + return DexRoutePath.dex(); // Return default path for invalid amount + } return DexRoutePath.dex( fromCurrency: uri.queryParameters['from_currency'] ?? '', - fromAmount: uri.queryParameters['from_amount'] ?? '', + fromAmount: fromAmount, toCurrency: uri.queryParameters['to_currency'] ?? '', - toAmount: uri.queryParameters['to_amount'] ?? '', + toAmount: toAmount, orderType: uri.queryParameters['order_type'] ?? '', ); }lib/router/state/dex_state.dart (1)
89-94: Add documentation for clearDexParams method.The method's purpose and usage context should be documented.
+/// Clears all DEX parameters, resetting them to empty strings. +/// This is typically called when navigating away from the DEX view +/// or when the parameters have been consumed. void clearDexParams() { _fromCurrency = ''; _fromAmount = ''; _toCurrency = ''; _toAmount = ''; _orderType = ''; }lib/bloc/taker_form/taker_event.dart (1)
28-33: LGTM! Add documentation for the new parameter.The new
setOnlyIfNotSetparameter is a good addition for controlling coin setting behavior.+/// Sets the sell coin for the taker form. +/// @param coin The coin to set as sell coin +/// @param autoSelectOrderAbbr Optional abbreviation for auto-selecting an order +/// @param setOnlyIfNotSet When true, the coin will only be set if no coin is currently set TakerSetSellCoin(this.coin, {this.autoSelectOrderAbbr, this.setOnlyIfNotSet = false});lib/bloc/dex_repository.dart (1)
144-166: Consider adding timeout and making parameters configurable.While the retry logic is well-implemented, consider these improvements:
- Add an overall timeout to prevent indefinite waiting
- Make the orderbook parameters (coin, type, number, action) configurable instead of hardcoding them
Consider this improved implementation:
Future<void> waitOrderbookAvailability({ int retries = 10, int interval = 300, + Duration? timeout, + String? coin, + BestOrdersRequestType type = BestOrdersRequestType.number, + int number = 1, + String action = 'sell', }) async { + final timer = timeout == null ? null : Timer(timeout, () { + throw TimeoutException('Orderbook availability check timed out'); + }); + BestOrders orders; for (int attempt = 0; attempt < retries; attempt++) { orders = await getBestOrders( BestOrdersRequest( - coin: defaultDexCoin, - type: BestOrdersRequestType.number, - number: 1, - action: 'sell', + coin: coin ?? defaultDexCoin, + type: type, + number: number, + action: action, ), ); if (orders.result?.isNotEmpty ?? false) { + timer?.cancel(); return; } await Future.delayed(Duration(milliseconds: interval)); } + + timer?.cancel(); }lib/views/dex/entities_list/dex_list_wrapper.dart (1)
51-61: Add error handling in _onRouteChange.The _onRouteChange method should handle potential errors to prevent crashes and maintain app stability.
Add error handling:
void _onRouteChange() { + try { if (mounted) { final type = routingState.dexState.orderType; if (type.isNotEmpty || (routingState.dexState.fromCurrency.isNotEmpty || routingState.dexState.toCurrency.isNotEmpty)) { tradingKindBloc .setKind(type == 'taker' ? TradingKind.taker : TradingKind.maker); } } + } catch (e) { + debugPrint('Error in _onRouteChange: $e'); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
lib/generated/codegen_loader.g.dartis excluded by!**/generated/**
📒 Files selected for processing (19)
assets/translations/en.json(1 hunks)lib/bloc/dex_repository.dart(2 hunks)lib/bloc/taker_form/taker_bloc.dart(1 hunks)lib/bloc/taker_form/taker_event.dart(1 hunks)lib/blocs/maker_form_bloc.dart(2 hunks)lib/mm2/mm2_api/mm2_api.dart(2 hunks)lib/mm2/mm2_api/rpc/show_priv_key/show_priv_key_request.dart(1 hunks)lib/mm2/mm2_api/rpc/show_priv_key/show_priv_key_response.dart(1 hunks)lib/router/navigators/app_router_delegate.dart(2 hunks)lib/router/parsers/dex_route_parser.dart(1 hunks)lib/router/parsers/root_route_parser.dart(1 hunks)lib/router/routes.dart(1 hunks)lib/router/state/dex_state.dart(2 hunks)lib/views/dex/entities_list/dex_list_wrapper.dart(1 hunks)lib/views/dex/simple/form/maker/maker_form_layout.dart(2 hunks)lib/views/dex/simple/form/taker/taker_form.dart(2 hunks)lib/views/dex/simple/form/taker/taker_form_layout.dart(1 hunks)lib/views/settings/widgets/security_settings/security_settings_page.dart(4 hunks)lib/views/settings/widgets/security_settings/seed_settings/seed_show.dart(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- lib/views/dex/simple/form/taker/taker_form_layout.dart
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_and_preview
- GitHub Check: build_and_deploy
🔇 Additional comments (15)
lib/views/settings/widgets/security_settings/seed_settings/seed_show.dart (3)
133-139: Consider security implications of QR code display.Displaying private keys as QR codes could make them vulnerable to screen capture or recording. Consider adding additional security measures or warnings.
215-236: Great implementation of security warning!The warning message is well-designed and prominently displayed, helping users understand the security implications.
88-90: Good handling of empty private keys.The component correctly handles the case when no private keys are available.
lib/mm2/mm2_api/mm2_api.dart (1)
50-51: LGTM!The imports follow the existing pattern and are appropriately scoped.
lib/router/state/dex_state.dart (1)
7-12: LGTM! State properties are well-structured.The new state properties follow proper encapsulation patterns with private fields and consistent initialization.
Also applies to: 17-21
lib/views/dex/simple/form/taker/taker_form.dart (1)
74-75: LGTM! Proper cleanup in dispose.The implementation correctly removes the routing state listener in dispose.
lib/router/routes.dart (2)
42-48: LGTM! Constructor properly initializes new routing parameters.The constructor correctly initializes all new fields with default empty string values, maintaining immutability.
64-68: LGTM! Fields are properly declared as final.The new fields are correctly declared as final strings, ensuring immutability and type safety.
lib/router/navigators/app_router_delegate.dart (2)
107-111: LGTM! State properties are correctly set from the path.The additional properties are properly assigned from the path parameter to the routing state.
173-179: LGTM! Return statement correctly includes all required properties.The
DexRoutePath.dex()constructor is properly called with all the necessary properties from the routing state.lib/bloc/taker_form/taker_bloc.dart (2)
243-246: LGTM! Proper null check before setting default coin.The code correctly checks if the sell coin is null before attempting to set a default, preventing unnecessary state updates.
253-254: LGTM! Early return prevents unnecessary state updates.The early return when
setOnlyIfNotSetis true andstate.sellCoinis not null helps optimize performance by avoiding redundant state changes.lib/blocs/maker_form_bloc.dart (2)
529-533: LGTM! Proper error handling for sell amount parsing.The try-catch block ensures that invalid input is handled gracefully by setting the amount to null.
551-555: LGTM! Proper error handling for buy amount parsing.The try-catch block ensures that invalid input is handled gracefully by setting the amount to null.
lib/router/parsers/root_route_parser.dart (1)
25-28: LGTM! Code is more concise and efficient.The changes streamline the code by:
- Combining URI parsing with parser selection
- Using the original URI from routeInformation
| _privKeys.clear(); | ||
| for (final coin in coinsBloc.walletCoins) { | ||
| final result = | ||
| await mm2Api.showPrivKey(ShowPrivKeyRequest(coin: coin.abbr)); | ||
| if (result != null) { | ||
| _privKeys[coin] = result.privKey; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling for private key retrieval.
The private key fetching loop silently ignores failures. Consider adding error handling and user feedback.
_privKeys.clear();
+final List<String> failedCoins = [];
for (final coin in coinsBloc.walletCoins) {
final result =
await mm2Api.showPrivKey(ShowPrivKeyRequest(coin: coin.abbr));
if (result != null) {
_privKeys[coin] = result.privKey;
+ } else {
+ failedCoins.add(coin.abbr);
}
}
+if (failedCoins.isNotEmpty) {
+ ScaffoldMessenger.of(context).showSnackBar(
+ SnackBar(
+ content: Text('Failed to fetch private keys for: ${failedCoins.join(", ")}'),
+ ),
+ );
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _privKeys.clear(); | |
| for (final coin in coinsBloc.walletCoins) { | |
| final result = | |
| await mm2Api.showPrivKey(ShowPrivKeyRequest(coin: coin.abbr)); | |
| if (result != null) { | |
| _privKeys[coin] = result.privKey; | |
| } | |
| } | |
| _privKeys.clear(); | |
| final List<String> failedCoins = []; | |
| for (final coin in coinsBloc.walletCoins) { | |
| final result = | |
| await mm2Api.showPrivKey(ShowPrivKeyRequest(coin: coin.abbr)); | |
| if (result != null) { | |
| _privKeys[coin] = result.privKey; | |
| } else { | |
| failedCoins.add(coin.abbr); | |
| } | |
| } | |
| if (failedCoins.isNotEmpty) { | |
| ScaffoldMessenger.of(context).showSnackBar( | |
| SnackBar( | |
| content: Text('Failed to fetch private keys for: ${failedCoins.join(", ")}'), | |
| ), | |
| ); | |
| } |
| Future<ShowPrivKeyResponse?> showPrivKey( | ||
| ShowPrivKeyRequest request, | ||
| ) async { | ||
| try { | ||
| final String response = await _call(request); | ||
| final Map<String, dynamic> json = jsonDecode(response); | ||
| if (json['error'] != null) { | ||
| return null; | ||
| } | ||
| return ShowPrivKeyResponse.fromJson(json); | ||
| } catch (e, s) { | ||
| log( | ||
| 'Error getting privkey ${request.coin}: ${e.toString()}', | ||
| path: 'api => showPrivKey', | ||
| trace: s, | ||
| isError: true, | ||
| ); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Additional security measures needed for private key handling
The codebase has good foundational security measures like password protection and error logging, but could benefit from:
- Adding rate limiting specifically for private key access operations to prevent brute force attempts
- Implementing audit logging for successful private key retrievals (currently only errors are logged)
The suggestion about key rotation mechanisms is not applicable for a non-custodial wallet where users manage their own keys.
🔗 Analysis chain
Consider additional security measures for private key handling.
While the implementation follows the class's patterns well, handling private keys requires extra security considerations:
- Consider adding rate limiting for private key requests
- Add logging for security audit purposes
- Consider implementing key rotation or expiry mechanisms
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other private key handling implementations
ast-grep --pattern 'private_key|priv_key|privkey'
Length of output: 51
Script:
#!/bin/bash
# Search for ShowPrivKey related implementations
ast-grep --pattern 'class ShowPrivKeyRequest {
$$$
}'
ast-grep --pattern 'class ShowPrivKeyResponse {
$$$
}'
# Search for rate limiting patterns
rg -i "rate.*limit|throttle" -A 3
# Search for security-related configurations
rg -i "security|auth.*config" -A 3
# Check logging patterns for private key operations
rg "log\(" -A 2
Length of output: 81916
| void resetOnLogOut() { | ||
| action = DexAction.none; | ||
| reset(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing reset behavior.
The resetOnLogOut method now calls reset, but reset only clears the action state. Consider clearing all DEX parameters during reset.
@override
void reset() {
action = DexAction.none;
+ clearDexParams();
}Committable suggestion skipped: line range outside the PR's diff.
| void _consumeRouteParameters() async { | ||
| if (routingState.dexState.orderType == 'taker') { | ||
| final fromCurrency = routingState.dexState.fromCurrency; | ||
| final toCurrency = routingState.dexState.toCurrency; | ||
| final fromAmount = routingState.dexState.fromAmount; | ||
| routingState.dexState.clearDexParams(); | ||
|
|
||
| await dexRepository.waitOrderbookAvailability(); | ||
|
|
||
| if (mounted) { | ||
| final takerBloc = context.read<TakerBloc>(); | ||
| Coin? sellCoin = | ||
| fromCurrency.isNotEmpty ? coinsBloc.getCoin(fromCurrency) : null; | ||
| Coin? buyCoin = | ||
| toCurrency.isNotEmpty ? coinsBloc.getCoin(toCurrency) : null; | ||
|
|
||
| if (sellCoin != null || buyCoin != null) { | ||
| takerBloc.add( | ||
| TakerSetSellCoin(sellCoin, autoSelectOrderAbbr: buyCoin?.abbr)); | ||
|
|
||
| if (fromAmount.isNotEmpty) { | ||
| Rational? sellAmount; | ||
| try { | ||
| sellAmount = Rational.parse(fromAmount); | ||
| } catch (_) {} | ||
|
|
||
| if (sellAmount != null) { | ||
| takerBloc.add(TakerSetSellAmount(sellAmount)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error handling and async operation.
The implementation correctly handles routing parameters, but there are a few areas for improvement:
- Add error logging for amount parsing failures
- Consider showing user feedback for invalid amounts
- The
waitOrderbookAvailabilitycall could block UI
void _consumeRouteParameters() async {
if (routingState.dexState.orderType == 'taker') {
final fromCurrency = routingState.dexState.fromCurrency;
final toCurrency = routingState.dexState.toCurrency;
final fromAmount = routingState.dexState.fromAmount;
routingState.dexState.clearDexParams();
+ // Show loading indicator
+ if (mounted) setState(() => _isLoading = true);
+
await dexRepository.waitOrderbookAvailability();
+
+ // Hide loading indicator
+ if (mounted) setState(() => _isLoading = false);
if (mounted) {
final takerBloc = context.read<TakerBloc>();
Coin? sellCoin =
fromCurrency.isNotEmpty ? coinsBloc.getCoin(fromCurrency) : null;
Coin? buyCoin =
toCurrency.isNotEmpty ? coinsBloc.getCoin(toCurrency) : null;
if (sellCoin != null || buyCoin != null) {
takerBloc.add(
TakerSetSellCoin(sellCoin, autoSelectOrderAbbr: buyCoin?.abbr));
if (fromAmount.isNotEmpty) {
Rational? sellAmount;
try {
sellAmount = Rational.parse(fromAmount);
} catch (e) {
+ debugPrint('Failed to parse amount: $fromAmount - ${e.toString()}');
+ if (mounted) {
+ ScaffoldMessenger.of(context).showSnackBar(
+ SnackBar(content: Text('Invalid amount format')),
+ );
+ }
}
if (sellAmount != null) {
takerBloc.add(TakerSetSellAmount(sellAmount));
}
}
}
}
}
}Committable suggestion skipped: line range outside the PR's diff.
| if (routingState.dexState.fromAmount.isNotEmpty) { | ||
| makerFormBloc.setSellAmount(routingState.dexState.fromAmount); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add amount string validation before setting values.
The amount strings from route parameters are set directly without validation. Consider adding validation to ensure they are valid numeric values.
Add validation before setting amounts:
if (routingState.dexState.fromAmount.isNotEmpty) {
+ final amount = routingState.dexState.fromAmount;
+ if (double.tryParse(amount) != null) {
makerFormBloc.setSellAmount(routingState.dexState.fromAmount);
+ }
}
...
if (routingState.dexState.toAmount.isNotEmpty) {
+ final amount = routingState.dexState.toAmount;
+ if (double.tryParse(amount) != null) {
makerFormBloc.setBuyAmount(routingState.dexState.toAmount);
+ }
}Also applies to: 57-59
| final TradingKindBloc tradingKindBloc = | ||
| TradingKindBloc(TradingKindState.initial()); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Dispose the TradingKindBloc instance.
The TradingKindBloc instance should be disposed when the widget is disposed to prevent memory leaks.
Add bloc disposal:
@override
void dispose() {
routingState.dexState.removeListener(_onRouteChange);
+ tradingKindBloc.close();
super.dispose();
}Also applies to: 46-49
|
Visit the preview URL for this PR (updated for commit cada902): https://walletrc--pull-2512-merge-5ajihgdr.web.app (expires Fri, 07 Feb 2025 16:57:07 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f66a4ff03faa546f12f0ae5a841bd9eff2714dcc |
takenagain
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/shared/widgets/logout_popup.dart (1)
Line range hint
38-45: Consider documenting or simplifying the nested conditions.The text display logic has two levels of conditions:
- Outer: Checks if wallet type is
iguana- Inner: Checks
kIsWalletOnlyflagConsider either:
- Adding a comment explaining why both conditions are necessary
- Simplifying the logic if possible
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
lib/generated/codegen_loader.g.dartis excluded by!**/generated/**
📒 Files selected for processing (4)
assets/translations/en.json(2 hunks)lib/router/parsers/root_route_parser.dart(2 hunks)lib/shared/widgets/logout_popup.dart(2 hunks)lib/views/settings/widgets/general_settings/general_settings.dart(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- assets/translations/en.json
- lib/router/parsers/root_route_parser.dart
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_and_preview
- GitHub Check: build_and_deploy
🔇 Additional comments (4)
lib/views/settings/widgets/general_settings/general_settings.dart (2)
2-2: LGTM!The import is correctly added to support the
kIsWalletOnlyconstant usage.
28-31: Verify the trading bot visibility behavior.The conditional rendering logic looks correct, adding an extra layer of control to hide the trading bot settings in wallet-only mode.
Let's verify the intended behavior:
✅ Verification successful
Trading bot visibility behavior is properly implemented ✅
The implementation provides robust visibility control through multiple layers:
- Compile-time control via
kIsWalletOnly- Runtime wallet state validation
- Consistent with other wallet-only mode restrictions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the trading bot visibility implementation # 1. Check if kIsWalletOnly is configurable # 2. Check for similar patterns in other views # 3. Verify consistency with trading bot related code # Check how kIsWalletOnly is defined and used rg "kIsWalletOnly" -A 3 # Look for similar trading bot visibility patterns rg "SettingsManageTradingBot" -A 3 # Check for any trading bot related tests fd "test.*trading.*bot.*dart$"Length of output: 4396
Script:
#!/bin/bash # Check HiddenWithoutWallet widget implementation rg "class HiddenWithoutWallet" -A 10Length of output: 901
Script:
#!/bin/bash # Get complete implementation of HiddenWithoutWallet rg "class HiddenWithoutWallet" -A 20Length of output: 1767
lib/shared/widgets/logout_popup.dart (2)
4-4: LGTM! Import added for accessing app configuration.The import for
app_config.dartis correctly added to access thekIsWalletOnlyconstant.
39-41: Verify the existence of translation keys and UI layout.The conditional text change looks good, but please ensure:
- Both translation keys exist in the localization files
- The UI layout handles potentially different text lengths gracefully
✅ Verification successful
✓ Translation keys and UI layout verified successfully
Both translation strings exist and the SelectableText widget with proper spacing will handle the different text lengths appropriately.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of translation keys and their usage # Check if both translation keys exist in the translation files echo "Checking translation keys..." rg "logoutPopupDescription:" -A 1 rg "logoutPopupDescriptionWalletOnly:" -A 1 # Look for other usages of these keys to ensure consistency echo "Checking for other usages..." rg "logoutPopupDescription[^W]" --type dart rg "logoutPopupDescriptionWalletOnly" --type dartLength of output: 734
Script:
#!/bin/bash # Find and check translation files echo "Looking for translation files..." fd -e json -e arb # Check the widget's layout structure for text handling echo "Checking widget layout..." rg -B 5 -A 10 "logoutPopupDescription" lib/shared/widgets/logout_popup.dartLength of output: 1389
Script:
#!/bin/bash # Check translation strings in the English translation file echo "Checking translation strings..." cat assets/translations/en.json | jq '.logoutPopupDescription, .logoutPopupDescriptionWalletOnly'Length of output: 363
There was a problem hiding this comment.
Tested the PR and successfully added and disabled the ARB coin (ARB-20 and ERC-20). Verified the QR code functionality and was able to copy the contract address.
However, observed an issue where the text "XMLHttpRequest error" appears when scrolling down after opening the coin.
In the Security > Settings tab, successfully copied the Private Key for ARB coins and verified its QR code.
Testing included both importing an existing wallet and creating a new one. Additionally, noticed that the text on the logout window has been updated.
Everything looks good to me.
There was a problem hiding this comment.
Confirmed ARB and ARB20 tokens activate as expected. Privkey export for individual coins matching output on desktop.
This error was due to failed connection to infra which has been restored. We could improve the error message in the event this happens with something like connection to {COIN} servers is failing. Please join the Komodo Platform Discord server for support and more information.
Use the message suggested by @smk762 rather than printing the raw exception message
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/views/wallet/coin_details/transactions/transaction_table.dart (1)
80-80: Consider adding null safety for coin name.Good improvement using localization and including the coin name in the error message. However, consider handling potential null coin names to prevent runtime errors.
- text: LocaleKeys.connectionToServersFailing.tr(args: [coin.name]), + text: LocaleKeys.connectionToServersFailing.tr(args: [coin.name ?? coin.abbr]),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
lib/generated/codegen_loader.g.dartis excluded by!**/generated/**
📒 Files selected for processing (2)
assets/translations/en.json(3 hunks)lib/views/wallet/coin_details/transactions/transaction_table.dart(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- assets/translations/en.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_and_preview
- GitHub Check: build_and_deploy
🔇 Additional comments (1)
lib/views/wallet/coin_details/transactions/transaction_table.dart (1)
5-5: LGTM!The import statement is correctly placed and the package is properly utilized in the code.
There was a problem hiding this comment.
LGTM.
p.s. The only thing I would like to suggest is that we are storing _seed and _privKeys as plain text in memory. As far as I understand Flutter, the data exists in memory only when the security settings page is opened / displayed, and the risk of a RAM dump or some inter-process "hook" is minimal. However, as a future suggestion, we might consider encrypting the internal data of _SecuritySettingsPageState with pass and decrypting it only when needed to be displayed as a WIF or QR. Anyway, it's just a suggestion.
There was a problem hiding this comment.
I've discovered an issue with the coin icons on the Buy/Sell tab. Some of the coin icons load, and then promptly disappear after a few seconds, to be replaced by a default icon:
@smk762 was able to replicate this on mobile too:
Here you can see some of the icons have loaded, but after a refresh they disappear.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/komodo_ui_kit/lib/src/images/coin_icon.dart (1)
Line range hint
169-196: Fix potential race condition in asset loading.The code assumes asset existence by setting
_assetExistenceCache[filePath] = truewithout checking, which could lead to unnecessary network calls if the asset doesn't exist. Consider using the existingcheckIfAssetExistsmethod.@override Widget build(BuildContext context) { final filePath = _getImagePath(coinAbbr); - - _assetExistenceCache[filePath] = true; return Image.asset( filePath, filterQuality: FilterQuality.high, errorBuilder: (context, error, stackTrace) { _assetExistenceCache[filePath] = false; _cdnExistenceCache[coinAbbr] ??= true; return Image.network( _getCdnUrl(coinAbbr), filterQuality: FilterQuality.high, errorBuilder: (context, error, stackTrace) { _cdnExistenceCache[coinAbbr] = false; return Icon(Icons.monetization_on_outlined, size: size); }, ); }, ); }
🧹 Nitpick comments (1)
packages/komodo_ui_kit/lib/src/images/coin_icon.dart (1)
Line range hint
198-245: Consider extracting shared utility code.The comment indicates this code is duplicated from
lib/shared/utils/utils.dart. Consider:
- Moving this utility to a shared package
- Creating a dependency on the main project
- Using code generation to keep the implementations in sync
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/komodo_ui_kit/lib/src/images/coin_icon.dart(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_and_preview
- GitHub Check: build_and_deploy
| @@ -2,7 +2,7 @@ import 'dart:convert'; | |||
| import 'package:flutter/material.dart'; | |||
| import 'package:flutter/services.dart'; | |||
|
|
|||
| const coinImagesFolder = 'coin_icons/png/'; | |||
| const coinImagesFolder = 'assets/coin_icons/png/'; | |||
There was a problem hiding this comment.
💡 Codebase verification
Asset path change requires proper setup
The path change to 'assets/coin_icons/png/' is problematic because:
- The directory structure doesn't exist in the package
- The assets are not configured in pubspec.yaml
- No PNG files are present in the package
Please ensure that:
- The coin icon assets are added to the correct location
- The assets are properly configured in pubspec.yaml under the
flutter.assetssection
🔗 Analysis chain
Verify asset configuration in pubspec.yaml.
The path change to 'assets/coin_icons/png/' suggests a restructuring of assets. Ensure that:
- The physical files have been moved to the new location
- The assets are properly configured in pubspec.yaml
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify asset configuration and file locations
# Check pubspec.yaml for asset configuration
echo "Checking pubspec.yaml for asset configuration..."
rg -A 5 "flutter:\s*assets:" packages/komodo_ui_kit/pubspec.yaml
# Check if files exist in the new location
echo "Checking files in new location..."
fd . packages/komodo_ui_kit/assets/coin_icons/png/ -t f -e png
# Check if files exist in the old location (should be empty)
echo "Checking files in old location (should be empty)..."
fd . packages/komodo_ui_kit/coin_icons/png/ -t f -e png
Length of output: 745
Script:
#!/bin/bash
# Description: Deep dive into asset configuration and directory structure
# Check if pubspec.yaml exists and show its content
echo "Checking pubspec.yaml existence and content..."
cat packages/komodo_ui_kit/pubspec.yaml
# List all directories under packages/komodo_ui_kit to understand the structure
echo -e "\nListing directory structure..."
find packages/komodo_ui_kit -type d
# Search for any PNG files in the package
echo -e "\nSearching for PNG files..."
fd -e png . packages/komodo_ui_kit
Length of output: 1666
Thanks for spotting the issue @AndrewDelaney. It should be resolved in cada902. Could you take another look at the Buy/Sell and Wallet page icons to confirm? |
QA:
To be tested:
Release notes:
New Features
Improvements
Summary by CodeRabbit
Based on the comprehensive changes across multiple files, here are the key release notes:
New Features
Improvements
Translations
Security
The release introduces comprehensive support for the Arbitrum (ARB) coin, with updates spanning configuration, routing, transaction handling, and user interface components.