fix(nft): add nft v2 and hd wallet support#2566
Conversation
|
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 WalkthroughThe changes introduce a series of refactorings and enhancements to the asset and NFT management logic. New constants and getters have been added to manage asset visibility, while several blocs and repositories have been refactored to improve error handling, logging, and dependency usage. Event and state names have been updated for clarity, and UI components for NFT receipt have been consolidated and modernized by changing model types and input handling. Additionally, some deprecated methods and redundant imports were removed, and the address handling was shifted from simple strings to structured public key info objects. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant View as NftReceiveView
participant Bloc as NftReceiveBloc
participant Logger as Logger
User->>View: Initiate address change
View->>Bloc: Dispatch NftReceiveAddressChanged event
Bloc->>Logger: Log event details
Bloc-->>Bloc: Process event & validate state
Bloc->>View: Emit NftReceiveLoadSuccess state
View->>User: Update UI with new address
Possibly related PRs
Suggested labels
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 (
|
NFT_* coins require the nft's to be enabled either in the eth activation function or separately like this using the enable_nft RPC
takenagain
force-pushed
the
bugfix/nft-hd-support
branch
from
5cb7ccb to
dffb9b8
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
lib/views/wallet/coin_details/receive/receive_details.dart (1)
169-173:⚠️ Potential issuePotential mismatch between callback implementation and usage.
The
_onAddressChangedmethod still accepts aStringparameter, but according to the summary, theReceiveAddresscallback type has changed fromFunction(String)tovoid Function(PubkeyInfo?). This may cause a type mismatch when the callback is invoked.-void _onAddressChanged(String address) { +void _onAddressChanged(PubkeyInfo? pubkeyInfo) { setState(() { - _currentAddress = address; + _currentAddress = pubkeyInfo?.address; }); }
🧹 Nitpick comments (4)
lib/mm2/mm2_api/mm2_api_nft.dart (2)
151-157: Optional error handling inenableNft.
Currently, if_sdk.client.rpc.nft.enableNft(...)fails, the method will throw without logging. Consider wrapping it for better context around the failure or at least adding a local try/catch to log the failure message.
159-181: Enhance fault tolerance in_tryEnableNftChains.
If enabling one chain fails, the current design aborts enabling for any remaining chains. Consider a more granular error handling strategy (e.g., continuing attempts for remaining chains) to avoid partial enablement failures from blocking others.lib/views/wallet/coin_details/receive/receive_address_trezor.dart (2)
56-67: Consider providing real derivation path and chain informationThe PubkeyInfo is created with empty strings for
derivationPathandchain. Consider deriving these values from the asset or the provided address for better data integrity.final newPubkey = PubkeyInfo( address: newAddress, - derivationPath: '', - chain: '', + derivationPath: asset.derivationPath ?? '', + chain: asset.id.chain ?? '', balance: BalanceInfo.zero(), );
56-57: Remove TODO comment after implementing the featureThe TODO comment about updating to use asset instead of coin should be removed since the implementation now uses the asset parameter.
- // TODO: update to use asset instead of coin return RequestAddressButton(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
lib/app_config/app_config.dart(4 hunks)lib/bloc/app_bloc_root.dart(1 hunks)lib/bloc/coins_bloc/coins_bloc.dart(1 hunks)lib/bloc/coins_bloc/coins_state.dart(2 hunks)lib/bloc/nft_receive/bloc/nft_receive_bloc.dart(2 hunks)lib/bloc/nft_receive/bloc/nft_receive_event.dart(1 hunks)lib/bloc/nft_receive/bloc/nft_receive_state.dart(1 hunks)lib/bloc/nft_transactions/nft_txn_repository.dart(4 hunks)lib/bloc/nft_withdraw/nft_withdraw_repo.dart(2 hunks)lib/bloc/nfts/nft_main_bloc.dart(8 hunks)lib/bloc/nfts/nft_main_repo.dart(3 hunks)lib/main.dart(1 hunks)lib/mm2/mm2.dart(1 hunks)lib/mm2/mm2_api/mm2_api.dart(4 hunks)lib/mm2/mm2_api/mm2_api_nft.dart(6 hunks)lib/model/nft.dart(2 hunks)lib/views/nfts/nft_main/nft_main_controls.dart(2 hunks)lib/views/nfts/nft_receive/common/nft_receive_card.dart(5 hunks)lib/views/nfts/nft_receive/desktop/nft_receive_desktop_view.dart(0 hunks)lib/views/nfts/nft_receive/mobile/nft_receive_mobile_view.dart(0 hunks)lib/views/nfts/nft_receive/nft_receive_page.dart(1 hunks)lib/views/nfts/nft_receive/nft_receive_view.dart(3 hunks)lib/views/wallet/coin_details/receive/receive_address.dart(1 hunks)lib/views/wallet/coin_details/receive/receive_address_trezor.dart(4 hunks)lib/views/wallet/coin_details/receive/receive_details.dart(1 hunks)
💤 Files with no reviewable changes (2)
- lib/views/nfts/nft_receive/mobile/nft_receive_mobile_view.dart
- lib/views/nfts/nft_receive/desktop/nft_receive_desktop_view.dart
🔇 Additional comments (78)
lib/main.dart (1)
69-69:Details
✅ Verification successful
Dependency injection simplified by removing coinsRepo parameter.
The
Mm2Apiinitialization has been refactored to remove thecoinsRepodependency, now using onlymm2andsdkparameters. This is a good architectural improvement that reduces dependency coupling.
🏁 Script executed:
#!/bin/bash # Verify that Mm2Api constructor no longer requires coinsRepo parameter # Check the Mm2Api constructor definition rg -A 10 "class Mm2Api" lib/mm2/mm2_api/mm2_api.dartLength of output: 273
Dependency Injection Simplification Confirmed
We verified that the
Mm2Apiconstructor inlib/mm2/mm2_api/mm2_api.dartnow only requires themm2andsdkparameters, with no reference to acoinsRepoparameter. This aligns with the instantiation inlib/main.dart(line 69), confirming the intended simplification and reduced dependency coupling.lib/views/wallet/coin_details/receive/receive_details.dart (1)
141-141:Details
✅ Verification successful
Parameter name updated from 'coin' to 'asset'.
The parameter name change from
cointoassetaligns with changes in theReceiveAddresscomponent that now accepts assets and pubkeys instead of coins. This change supports NFT v2 and HD wallet compatibility.
🏁 Script executed:
#!/bin/bash # Check if ReceiveAddress component now accepts 'asset' parameter instead of 'coin' # Look at the ReceiveAddress constructor rg -A 10 "class ReceiveAddress" lib/views/wallet/coin_details/receive/receive_address.dartLength of output: 349
Re: Parameter Name Update Verified
The change from using the
coinparameter toassetwas correctly applied. TheReceiveAddressconstructor now requires anassetparameter (as verified inlib/views/wallet/coin_details/receive/receive_address.dart), so updating the argument inlib/views/wallet/coin_details/receive/receive_details.dartto:asset: widget.asset.toCoin(),is consistent with the new implementation supporting NFT v2 and HD wallet compatibility. No further changes are required.
lib/views/nfts/nft_receive/nft_receive_page.dart (1)
20-20:Details
✅ Verification successful
Event name updated for better clarity.
The event name has been updated from
NftReceiveEventInitialtoNftReceiveStarted, which follows a clearer naming convention. This improves code readability and maintains consistency with the event naming pattern in the rest of the application.
🏁 Script executed:
#!/bin/bash # Verify that the NftReceiveEventInitial class has been renamed to NftReceiveStarted # Look for both event names in the NftReceiveBloc file rg "NftReceiveEventInitial|NftReceiveStarted" lib/bloc/nft_receive/bloc/nft_receive_bloc.dartLength of output: 212
Event Renaming Verified: The update to NftReceiveStarted is correctly applied.
The verification confirms that all references to the initial event have been updated. In particular, the file
lib/bloc/nft_receive/bloc/nft_receive_bloc.dartshows:
- The use of
<NftReceiveStarted>for the event handler.- The addition of
NftReceiveStarted(chain: localChain)in the bloc.This update maintains consistency with the naming pattern throughout the codebase and improves clarity.
lib/app_config/app_config.dart (2)
79-81: Well-documented NFT v2 implementationGood job adding clear documentation explaining that NFT v2 coins will be used in the background and don't need to be visible to users. This helps maintain code clarity for future development.
127-133: NFT v2 coins correctly added to enabledByDefaultCoinsThe addition of NFT v2 coins to the default enabled coins list is an essential change to ensure NFT v2 functionality works properly. The comment clearly explains that these coins are required for the NFT v2 methods.
lib/views/nfts/nft_main/nft_main_controls.dart (2)
9-9: Properly imported AuthBlocAdding this import is necessary for accessing the centralized authentication state.
73-74: Improved authentication handlingGood refactoring to use the centralized AuthBloc for authentication checks instead of the NftMainBloc. This change improves the architecture by properly separating concerns and will help resolve the infinite loading issue with HD wallets and multi-address support.
lib/bloc/app_bloc_root.dart (1)
264-264: Updated NftMainBloc initializationGood update to the NftMainBloc initialization to align with the architectural changes. Removing the
isLoggedInparameter and renamingkdfSdktosdkmatches the changes in the bloc implementation and improves consistency.lib/mm2/mm2.dart (1)
58-61: Properly deprecated legacy methodThe deprecation annotation is well-documented, clearly explaining why developers should use the new methods instead. The message provides specific alternatives (
KomodoDefiSdk.client.rpcorKomodoDefiSdk.client.executeRpc) and explains the issues with the current implementation (injecting empty user passwords into legacy models).lib/bloc/nft_transactions/nft_txn_repository.dart (5)
16-18: Clean code reorganization!Moving member variables from constructor assignments to class-level declarations improves readability and follows Dart best practices.
21-28: Improved parameter formatting and error handlingThe function parameter formatting has been improved for readability, and the mapping in line 27 has been properly aligned.
38-42: Better error handling with explicit type castingGood improvement to explicitly cast the error message to
Stringbefore logging and throwing anApiError. This reduces the risk of unexpected type errors.
72-74: Improved parameter alignmentThe parameters for
getNftTxDetailsByHashhave been nicely aligned for better readability.
100-100: Good use offinalfor loop variableChanging from
vartofinalfor the loop variable enforces immutability and is a good practice.lib/model/nft.dart (2)
83-83: Code simplification inuuidgetterRemoving the redundant
.toString()call onchainsimplifies the code without changing functionality, as string interpolation already callstoString()implicitly.
243-256: Well-implementednftAssetTicker()methodThe new method provides a clear mapping from blockchain types to NFT asset tickers, following the same pattern as existing methods like
toString()andcoinAbbr(). This will be useful for consistent asset identification throughout the application.lib/bloc/nft_withdraw/nft_withdraw_repo.dart (3)
37-40: Improved error handling with null fallbackGood improvement to handle potential null error messages by adding a fallback. The use of
.ignore()on the log is also consistent with other error handling in the codebase.
59-62: Better parameter formattingThe parameters for
confirmSendhave been reformatted to use a multi-line declaration style, which improves readability.
69-71: Improved response formattingThe
SendRawTransactionResponsecreation has been reformatted to a multi-line style for better readability.lib/views/nfts/nft_receive/nft_receive_view.dart (6)
4-4: Added required import for PubkeyInfoThe import for
komodo_defi_typesis correctly added to support thePubkeyInfotype used in the new_onAddressChangedmethod.
12-12: Updated import for consolidated NFT receive cardThe import has been updated to use the new consolidated
NftReceiveCardcomponent.
40-42: Updated state type checks for improved clarityThe state type checks have been updated to use more descriptive state names:
NftReceiveHasBackup→NftReceiveBackupSuccessNftReceiveFailure→NftReceiveLoadFailureNftReceiveAddress→NftReceiveLoadSuccessThis makes the code more self-documenting and easier to understand.
Also applies to: 52-52
49-49: Updated event name for consistencyThe event name has been updated to
NftReceiveRefreshRequested()to match the naming convention in the bloc.
54-76: UI refactoring with consolidated componentExcellent refactoring to use a single
NftReceiveCardcomponent for both mobile and desktop layouts with appropriate configuration differences. This reduces code duplication and improves maintainability.
87-91: Well-implemented address change handlerThe new
_onAddressChangedmethod follows good practices for event dispatching and keeps the UI component clean by separating the event logic.lib/bloc/nfts/nft_main_repo.dart (5)
25-30: Improved error handling in updateNft method.The error handling has been enhanced by explicitly casting the error to a String type and using the
.ignore()method on the log statement. This helps prevent potential type errors and follows the project's updated logging pattern.
37-38: Enhanced type safety with explicit nullable typing.Explicitly typing
jsonErrorasString?improves code clarity and type safety by making the nullability of this variable explicit.
43-44: Streamlined error handling condition.The error condition check has been simplified to directly verify if the error message starts with 'transport' rather than first checking if it's a String. This cleaner approach is possible because of the explicit typing above.
58-58: Added 'final' keyword to loop variable.Adding the 'final' keyword to the loop variable enforces immutability within the loop scope, which is a good practice for preventing accidental modifications.
67-67: Simplified error message construction.The error message string interpolation has been simplified by directly using the exception variable without an explicit toString() call, which makes the code more concise and readable.
lib/mm2/mm2_api/mm2_api.dart (4)
58-66: Updated constructor to use KomodoDefiSdk instead of CoinsRepo.The constructor has been simplified by removing the
coinsRepoparameter and now properly initializes thenftproperty with thesdkparameter instead. This matches the PR objective to add support for HD wallet and multi-address capabilities.
107-109: Improved logging pattern in _fallbackToBalanceTaker method.The log statement now uses the
.ignore()method, following a consistent pattern across the codebase for handling log statements.
441-441: Removed unnecessary await keyword.The
awaitkeyword was removed from the_fallbackToBalanceTakercall in the catch block, which is appropriate since the method is already returning a Future that doesn't need to be awaited in this context.
606-606: Simplified error logging in showPrivKey method.The error message now directly uses the exception object without an explicit toString() call, which makes the code more concise while maintaining the same functionality.
lib/bloc/nft_receive/bloc/nft_receive_state.dart (3)
12-12: Renamed state class for better semantic clarity.Renaming
NftReceiveHasBackuptoNftReceiveBackupSuccessprovides better semantic clarity by indicating the successful state of the backup operation.
14-43: Enhanced NFT receive state with richer model.The
NftReceiveLoadSuccessstate now contains more comprehensive data withasset,pubkeys, and optionalselectedAddressproperties. This provides better support for HD wallets and multi-address functionality as mentioned in the PR objectives. The properly implementedcopyWithmethod andpropslist ensure correct state handling and equality comparison.
45-49: Renamed failure state for consistency.Renaming
NftReceiveFailuretoNftReceiveLoadFailurebrings consistency to the state naming convention, clearly indicating it's related to the loading process.lib/bloc/coins_bloc/coins_state.dart (4)
4-14: Added asset filtering to exclude NFT assets from display.The constructor now includes clear documentation and applies filtering to exclude assets not intended for user display, such as NFTs. This directly addresses the issue mentioned in the PR objective of fixing the infinite loading screen when accessing the NFT page with HD mode and multi-address support.
16-22: Updated initial factory method to use non-const constructor.The
initialfactory method was modified to use a non-const constructor, which is necessary to support the filtering logic in the constructor.
34-61: Enhanced copyWith method with filtering logic.The
copyWithmethod now includes detailed comments explaining the filtering process and applies the_filterExcludedAssetsfunction to bothcoinsandwalletCoins. This ensures that any state modifications maintain the exclusion of assets that shouldn't be displayed to users.
63-70: Added helper method for filtering excluded assets.The new
_filterExcludedAssetsmethod encapsulates the filtering logic in a reusable way, ensuring consistent filtering of excluded assets throughout the class. This is an excellent example of code reuse and clean architecture.lib/mm2/mm2_api/mm2_api_nft.dart (10)
4-5: Imports look good.
These additions provide necessary references to Komodo Defi packages and logging utilities.Also applies to: 7-8
19-23: Constructor dependency injection is well-structured.
Switching from aCoinsRepoto aKomodoDefiSdkdependency aligns well with the broader refactor toward the SDK-based approach, and leveragingLoggerfor_logis neatly done.
37-37: Enabling NFT chains prior to updating the NFT list.
Calling_tryEnableNftChainsensures chains are activated before fetching. This is a logical improvement that prevents possible empty or invalid returns.
41-43: Verbose logging of requests and responses.
Using_log.finefor request/response data is appropriate for debugging without cluttering higher log levels.
46-46: Logging error at shout level.
The usage of_log.shout('Error updating nfts', e, s);provides a clear high-level alert for runtime failures.
64-65: Consistent logging style.
Catching the exception and using_log.shout(e.toString(), e, s);is consistent with the rest of your error logging.
83-85: Controller logs for GetNftList request.
Logging the request and response offers better traceability in diagnosing NFT chain issues.
88-89: Error handling for getNftList.
Raising aTransportErrorafter logging the failure ensures consistent downstream handling of errors.
133-147: Active NFT chains derivation logic.
Extracting_sdk.assets.getActivatedAssets()and filtering byenabledCoinIdsproduces a neat approach to ensuring only recognized chains are processed. The additional debug logs forenabledCoinIds,nftCoins,activeChains, andnftChainsare informative for troubleshooting.
186-186: Error base message added.
This constant clarifies the origin of thrown exceptions inProxyApiNftusage and aids debugging.lib/bloc/nft_receive/bloc/nft_receive_event.dart (4)
7-7: Switching to nullable type in base event props.
UsingList<Object?>inpropsfacilitates improved event property handling when dealing with potential nulls.
10-12: Renamed and refined event for receiving NFTs.
IntroducingNftReceiveStartedwith a requiredchainis clearer than a generic initialization event.
19-20: Concise refresh event name.
NftReceiveRefreshRequestedis consistent with the typical “Requested” naming pattern in bloc events.
26-29: Address property updated toPubkeyInfo?.
Switching from aString?toPubkeyInfo?better expresses the structure of NFT addresses and supports more robust data. Thepropsoverride correctly returns this value for equality checks.Also applies to: 32-32
lib/views/wallet/coin_details/receive/receive_address.dart (6)
4-4: New import for asset types.
Importingkomodo_defi_types.dartis correct for referencingAssetandPubkeyInfoclasses.
15-21: Constructor signature now acceptsAssetandpubkeys.
This transition from a coin-centric model to asset-based parameters, plus a newpubkeysfield, clarifies usage and is consistent with the broader shift toAssetreferences.
23-27: New fields for asset-based handling.
Definingasset,pubkeys, andselectedAddresssignificantly improves clarity, ensuring the widget works with strongly typed data.
34-36: Passing asset and pubkeys to Trezor flow.
Providing the correct asset data for Trezor usage ensures the appropriate address context is available.
41-41: Safeguard for missing address.
Early return whenselectedAddressis null prevents null pointer issues and displays a user-friendly message.
46-46: UsingselectedAddress!.address.
After the null check, force-unwrapping is safe here. The approach remains consistent for both mobile and desktop.Also applies to: 53-53
lib/bloc/nfts/nft_main_bloc.dart (5)
18-20: Clean parameter renaming fromkdfSdktosdkThis change improves code clarity and consistency by using a more concise and appropriate variable name.
29-36: Improved authentication state managementGood refactoring that removes the need for an internal
_isLoggedInvariable by directly subscribing to_sdk.auth.authStateChanges. This approach is more reliable as it directly reflects the actual authentication state from the SDK.
49-51: Replaced boolean flag with direct authentication checkThe code now properly uses
await _sdk.auth.isSignedIn()instead of relying on an internal state variable. This makes the authentication check more accurate and reduces potential state synchronization issues.Also applies to: 76-78, 111-113
151-153: Enhanced method signature with optional parameterAdding an optional parameter with a default value to
_getAllNftsimproves the method's flexibility while maintaining backward compatibility.
59-64: Improved code formatting with consistent indentationThe code now follows better formatting practices with properly indented multi-line method calls, making the code more readable.
Also applies to: 84-93, 123-129
lib/views/wallet/coin_details/receive/receive_address_trezor.dart (2)
13-24: Updated parameter types to support HD wallet functionalityConstructor parameters have been updated to work with
AssetandAssetPubkeysinstead ofCoin, aligning with the changes needed to support HD wallets and multi-address functionality.
39-44: Replaced custom widget with reusable componentUsing the
SourceAddressFieldcomponent instead of a custom implementation improves code reusability and consistency across the application.lib/bloc/nft_receive/bloc/nft_receive_bloc.dart (6)
22-25: Improved event naming for better clarityEvent names have been updated to follow better naming conventions:
NftReceiveStarted,NftReceiveRefreshRequested, andNftReceiveAddressChanged. This improves code readability and understanding.
29-29: Added logging for better debuggingThe addition of a Logger instance enhances the debuggability of the NFT receive workflow. This is particularly valuable for diagnosing issues with HD wallet and multi-address support.
36-40: Added state validation to prevent redundant initializationNow checking if already in the success state before proceeding with initialization, which prevents unnecessary processing and potential state flicker.
44-47: Enhanced error handling with loggingAdded proper logging and error state emission when a coin isn't found for the given chain, improving error traceability and user experience.
98-115: Improved address change handling with better validationThe address change logic now properly checks the current state and provides detailed logging about the selected address, making the code more robust and maintainable.
83-91: Improved refresh handling with better logging and error handlingThe refresh logic now includes proper logging and handles the null chain case explicitly, preventing potential issues with the NFT receive workflow.
lib/views/nfts/nft_receive/common/nft_receive_card.dart (4)
18-32: Updated component to support HD wallet functionalityComponent parameters have been updated to work with
PubkeyInfoandAssetPubkeysinstead of string addresses, supporting the multi-address functionality needed for HD wallets.
88-88: Fixed QR code generation to use address propertyThe QR code address is now correctly extracted from the PubkeyInfo object's address property, ensuring that the QR code represents the actual address.
118-119: Updated chain detection to work with Asset typeThe
fromCoinToChainmethod has been modified to work with the Asset type instead of Coin, usingcoin.id.idas the identifier for chain detection.
105-108: Updated ReceiveAddress integrationThe ReceiveAddress component is now correctly configured with the asset and pubkeys parameters, aligning with the changes in that component.
takenagain
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
lib/views/wallet/coin_details/receive/receive_address_trezor.dart (1)
63-63: Null safety check may be redundant.The code uses the null assertion operator (
!) onselectedAddress, which is safe in this context because_buildCopyButtonappears to only be called whenselectedAddressis non-null (based on the conditional in the build method). However, for additional safety, consider adding a null check within the onTap callback.onTap: () { - copyToClipBoard(context, selectedAddress!.address); + if (selectedAddress != null) { + copyToClipBoard(context, selectedAddress.address); + } },lib/bloc/nfts/nft_main_bloc.dart (1)
7-7: Addition of logging capabilities is beneficial.
Importingpackage:logging/logging.darthelps improve observability. Ensure that log levels are appropriately configured for production.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
lib/bloc/nft_receive/bloc/nft_receive_bloc.dart(2 hunks)lib/bloc/nfts/nft_main_bloc.dart(3 hunks)lib/bloc/nfts/nft_main_event.dart(1 hunks)lib/bloc/nfts/nft_main_state.dart(1 hunks)lib/bloc/trezor_bloc/trezor_repo.dart(1 hunks)lib/bloc/withdraw_form/withdraw_form_bloc.dart(1 hunks)lib/blocs/trezor_coins_bloc.dart(1 hunks)lib/mm2/mm2_api/mm2_api_nft.dart(6 hunks)lib/mm2/mm2_api/rpc/nft/refresh_nft_metadata/refresh_nft_metadata_req.dart(1 hunks)lib/mm2/mm2_api/rpc/nft/update_nft/update_nft_req.dart(1 hunks)lib/model/hd_account/hd_account.dart(2 hunks)lib/model/nft.dart(2 hunks)lib/shared/widgets/copyable_address_dialog.dart(1 hunks)lib/views/nfts/common/widgets/nft_image.dart(2 hunks)lib/views/nfts/nft_main/nft_main_controls.dart(3 hunks)lib/views/nfts/nft_main/nft_main_failure.dart(1 hunks)lib/views/nfts/nft_main/nft_refresh_button.dart(1 hunks)lib/views/nfts/nft_page.dart(1 hunks)lib/views/nfts/nft_tabs/nft_tabs.dart(1 hunks)lib/views/wallet/coin_details/receive/receive_address.dart(1 hunks)lib/views/wallet/coin_details/receive/receive_address_trezor.dart(3 hunks)lib/views/wallet/coin_details/receive/receive_details.dart(6 hunks)lib/views/wallet/coin_details/receive/request_address_button.dart(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/views/nfts/nft_main/nft_main_controls.dart
- lib/model/nft.dart
- lib/mm2/mm2_api/mm2_api_nft.dart
- lib/bloc/nft_receive/bloc/nft_receive_bloc.dart
🔇 Additional comments (64)
lib/mm2/mm2_api/rpc/nft/refresh_nft_metadata/refresh_nft_metadata_req.dart (1)
32-32: Parameter name changed from "proxy_auth" to "komodo_proxy"The parameter has been renamed from "proxy_auth" to "komodo_proxy" while maintaining the same boolean value (false). This change appears to be part of the fix for NFT page loading issues with HD wallet mode, likely aligning with updated backend API requirements.
lib/mm2/mm2_api/rpc/nft/update_nft/update_nft_req.dart (1)
26-26: Parameter name changed from "proxy_auth" to "komodo_proxy"The parameter has been renamed from "proxy_auth" to "komodo_proxy" while maintaining the same boolean value (false). This change is consistent with the same update in the RefreshNftMetadataRequest class, ensuring uniform parameter naming across NFT-related API calls.
lib/model/hd_account/hd_account.dart (2)
1-1: LGTM: Importing necessary package.The import of
komodo_defi_typespackage is necessary to support the newly added functionality.
69-76: LGTM: Well-implemented conversion method.This method provides a clean way to convert an
HdAddressinstance to aPubkeyInfoobject, which is essential for NFT functionality to work with HD wallets. The implementation correctly maps all required fields.Note that the balance conversion uses JSON serialization/deserialization (
balance.toJson()followed byBalanceInfo.fromJson()). While this works, direct conversion methods are generally more efficient if they become available in the future.lib/bloc/trezor_bloc/trezor_repo.dart (1)
169-172: Updated parameter type from Coin to AssetParameter type has been updated from
CointoAssetto align with the broader transition from coin-based to asset-based model throughout the application.lib/blocs/trezor_coins_bloc.dart (2)
31-37: Updated method signature and implementation to use Asset instead of CoinThe
initNewAddressmethod has been updated to useAssetinstead ofCoin, and now retrieves the address usingasset.id.idinstead ofcoin.abbr. This change aligns with the transition to asset-based model throughout the application.
39-50: Updated parameter types and improved callback type safetyThe changes include:
- Updated parameter type from
CointoAssetto align with the asset-based model- Improved type safety by explicitly defining the callback as
void Function(GetNewAddressResponse)rather than using the more genericFunction- Updated the parameter passed to
getNewAddressStatusto use the newassetparameterThese changes improve type safety and maintain consistency with the asset-based model transition.
lib/shared/widgets/copyable_address_dialog.dart (2)
7-136: New CopyableAddressDialog widget for handling addressesThis new widget provides a well-structured way to display, copy, and select cryptocurrency addresses. The implementation includes:
- A flexible UI that supports address truncation for better display
- Address selection functionality via
showAddressSearch- Copy-to-clipboard functionality
- Customizable styling options
The code is well-organized with clean separation of UI and functionality.
A few suggestions for improvements:
- // Handle null address case + // Return empty widget if address is null if (address == null) { return const SizedBox.shrink(); }Consider adding error handling for the copy operation and providing user feedback when the copy operation succeeds.
123-134: Address search with proper context validationThe
_showAddressSearchmethod correctly checks if the context is still mounted before attempting to show the dialog, which prevents potential issues if the widget is disposed before the async operation completes.lib/views/wallet/coin_details/receive/request_address_button.dart (4)
7-7: Import looks good.The new import for
komodo_defi_typesis necessary forPubkeyInfo. No concerns here.
19-25: Constructor refactor appears consistent.Switching from
(Coin coin, Function(String) onSuccess)to(Asset asset, void Function(PubkeyInfo) onSuccess)aligns with the broaderAsset-centric approach. Parameter naming is clear and types are well-defined.
99-103: Good subscription handling.Initializes the request with
initNewAddress()and then subscribes to status updates. Thenullcheck ontaskIdensures the flow terminates gracefully if address initialization fails. This prevents unintended subscriptions.
154-154: Callback update is well-handled.Passing
details.newAddress.toPubkeyInfo()to the success callback reflects the new approach for handling addresses. This ensures the rest of the app receives the correct, structured data.lib/views/wallet/coin_details/receive/receive_details.dart (11)
24-26: New constructor arguments.Introducing
pubkeysand positioningsuper.keyin the constructor are consistent with the revised design. No issues found.
30-30: Additional field forAssetPubkeys.Declaring
final AssetPubkeys pubkeys;provides the needed reference for the new functionality. Looks good.
56-56: Child widget instantiation.Passing
pubkeysto_ReceiveDetailsContentensures consistent use of the new data model within child components.
67-70: Stateful content widget.The
_ReceiveDetailsContentconstructor requiring bothassetandpubkeysis coherent with the rest of the refactor. This enforces data consistency.
77-77:_currentAddressnow usesPubkeyInfo?.Migrating from a
String?toPubkeyInfo?approach properly reflects the richer address data model. This helps avoid repeated string-to-object conversions.
98-100: Card styling changes.Applying a conditional background color and border radius is a reasonable UI enhancement. No concerns about performance or complexity here.
116-127: Container decoration block.These lines set up the address display card with shadow and color. Straightforward and coherent with Flutter design patterns.
135-142: Text style adjustments.Updating the network label style is consistent with app theming. Clear usage of
themeData.textThemeis good practice.
148-150: Refined parameter usage inReceiveAddress.Passing both
assetandpubkeysensures the widget can retrieve correct address data. The consistent naming improves readability.
157-157: Safe null-check.Using
_currentAddress!is protected by the precedingif (_currentAddress != null). Good handling of nullability.
177-180: Address change callback.The setter for
_currentAddressis straightforward and ensures state updates are triggered correctly. No concerns.lib/views/wallet/coin_details/receive/receive_address.dart (8)
4-4:komodo_defi_typesimport is aligned with changes.We need
PubkeyInfotypes. No further issues.
10-10:copyable_address_dialog.dartimport is correct.This aligns with the replacement of the old
CopiedTextwidget. Looks good.
16-22: Constructor parameters updated.Switching from
cointoasset, alongsidepubkeysandonChanged(PubkeyInfo?), is consistent with the new data model across the codebase.
24-29: Field additions reflect refactor.Defining
asset,pubkeys, and a nullableselectedAddressof typePubkeyInfo?ensures the widget can correctly manage address data.
35-39:ReceiveAddressTrezorupdates.Passing
asset, selectedAddress, pubkeys, onChangedmatches the Trezor flow with the new data types. No concerns here.
42-44: Handling absence ofselectedAddress.Returning a message when no address is available is user-friendly and avoids null reference errors. Good fallback.
46-52: Mobile UI withCopyableAddressDialog.Replacing
CopiedTextwith a dialog-based approach is more flexible. The constructor usage is correct, andonAddressChangedis passed properly.
54-63: Desktop UI withCopyableAddressDialog.Similarly, the non-mobile layout constraints and usage are consistent with the mobile approach. Good user experience design.
lib/views/wallet/coin_details/receive/receive_address_trezor.dart (5)
4-6: Well-structured import organization.The imports have been updated to include the necessary types from
komodo_defi_typesand UI components from the Komodo UI libraries, which aligns with the modernization of the component to support HD wallets and NFT v2.
12-17: Constructor parameters updated appropriately for HD wallet support.The constructor has been refactored to accept
assetandpubkeysinstead of acoinparameter, allowing for more structured handling of cryptocurrency assets and their associated public keys, which is essential for HD wallet support.
19-22: Type changes improve code safety and structure.The transition from string-based to object-oriented data types (
Asset,AssetPubkeys,PubkeyInfo) enhances type safety and provides more structured data handling. This is a significant improvement for maintaining the relationship between assets and their addresses in an HD wallet context.
38-42: Good component refactoring.Replacing the custom
_BuildSelectwidget with the more standardizedSourceAddressFieldimproves code maintainability and reusability. This change aligns with modern component-based architecture principles.
55-55: API consistency maintained.The request button implementation has been updated to use the new
assetparameter while maintaining the same callback pattern, ensuring consistent behavior with the updated data model.lib/views/nfts/nft_tabs/nft_tabs.dart (1)
40-40: Event naming convention update looks goodThe event dispatch has been updated from
ChangeNftTabEventtoNftMainTabChanged, which follows a better naming convention pattern (entity + past tense verb). This aligns with what appears to be a broader effort to standardize event naming across the application.lib/views/nfts/nft_main/nft_main_failure.dart (1)
30-30: Event naming improved for clarityThe event dispatch has been updated from
UpdateChainNftsEventtoNftMainChainUpdateRequested, which is more descriptive and follows a consistent naming pattern. This change maintains the same functionality while improving code readability.lib/views/nfts/nft_main/nft_refresh_button.dart (1)
30-30: Event naming standardization appliedThe event dispatch has been updated from
RefreshNFTsForChainEventtoNftMainChainNftsRefreshed, maintaining the same functionality while adopting a more consistent naming convention. The change correctly passes the selected chain parameter to the new event.lib/views/nfts/common/widgets/nft_image.dart (2)
52-53: Good addition of GIF detectionAdding a specific check for GIF files will allow for special handling of this image format, which is often used for animated NFTs.
62-67: Enhanced image handling with proper error reportingThese changes bring two significant improvements:
- The
gaplessPlayback: isGifparameter ensures smoother GIF animation playback- The enhanced error handling now logs detailed information when image loading fails
This will greatly help with debugging image loading failures while providing a better user experience for animated NFTs.
lib/views/nfts/nft_page.dart (2)
68-69: Use of descriptive event names is commendable.
Replacing generic event names withNftMainChainUpdateRequestedandNftMainUpdateNftsStartedclarifies the intent and flow of updates, making the code more readable and maintainable.
75-75: Good practice to stop updates upon disposal.
DispatchingNftMainUpdateNftsStopped()indisposeensures that any ongoing timers or NFT update processes are safely terminated, preventing potential memory leaks or unwanted background tasks.lib/bloc/nfts/nft_main_state.dart (1)
14-14: Switch to factory constructor is a clean approach.
Convertinginitial()into a factory constructor can help streamline state instantiation logic. This also aligns well if future changes require more complex initialization.lib/bloc/nfts/nft_main_event.dart (6)
7-8: Improved naming clarifies the event's purpose.
NftMainChainUpdateRequestedbetter conveys the underlying action than a generic “update” event name.
11-12: Stopping event name is straightforward.
NftMainUpdateNftsStoppedsuccinctly indicates the update process has been halted. Clear naming aids maintainability.
15-16: Starting event name is more explicit.
NftMainUpdateNftsStartedclearly indicates the commencement of an NFT update loop or timer, improving code clarity.
19-20: Reset event name is self-descriptive.
NftMainResetRequestedprovides a clear entry point for resetting state without ambiguity.
23-24: Tab change event naming is coherent.
NftMainTabChangedclarifies that it specifically relates to a UI or internal tab selection event.
28-29: Refreshed event name is more meaningful.
NftMainChainNftsRefreshedindicates that the chain’s NFTs are being fetched fresh, benefiting code traceability.lib/bloc/nfts/nft_main_bloc.dart (13)
19-21: Constructor changes streamline authentication logic.
Introducingsdkdirectly (instead of a separatekdfSdkandisLoggedIn) centralizes auth checks in_sdk.auth. This reduces coupling and clarifies responsibilities.
23-28: Registering new event handlers is well-structured.
These additions ensure theNftMainBloccleanly responds to the newly named events without leftover references to removed ones.
30-38: Listening to auth state changes is robust.
Automatically updating or resetting NFTs upon sign-in or sign-out ensures the UI remains consistent. Be mindful of possible race conditions if multiple events arrive quickly.Would you like to check that multiple sign-ins or sign-outs in quick succession do not interrupt ongoing NFT-related operations? I can provide a script for scanning concurrency patterns or add tests to confirm.
41-41: Encapsulating SDK reference is concise.
StoringKomodoDefiSdkas a final field fosters clarity about its immutability.
44-44: Logger instantiation is good practice.
Having a dedicated logger for this bloc provides fine-grained logging control.
46-76: _onTabChanged ensures chain-specific NFTs are fetched.
The logic checks if the user is signed in and if the state is initialized before fetching NFTs, which prevents unnecessary calls. The error handling and logging also look solid.
116-120: Reset logic is clear.
EmittingNftMainState.initial()after a reset event reverts the bloc to a known baseline. This helps prevent stale data if the user signs out.
121-157: _onRefreshForChain safely updates a single chain.
UsingupdatingChainsto track refresh states is a good approach to guard concurrency. The error handling, logging, and final block for togglingupdatingChainsare well-structured.
159-164: _onStopUpdate gracefully cancels timers.
Invoking_stopUpdate()helps avoid background processes continuing after the user leaves the relevant screen.
165-173: _onStartUpdate sets up periodic updates.
A 1-minute interval for NFT updates seems reasonable, but verify that repeated calls don’t stack if the user quickly toggles navigation.I can prepare a script to grep for multiple timer creations, ensuring
_onStartUpdateis invoked only once per relevant scenario. Let me know if you’d like me to generate it.
174-193: _getAllNfts is well-factored.
Updating the repo before retrieving NFTs ensures fresh data. The usage of a fold operation for grouping by chain is concise and clear.
196-210: _calculateNftCount includes a chain sort feature.
Grouping NFT counts and sorting the chains by descending count is clean and tight. The use of pattern tuples helps with well-structured data returns.
78-114:Details
❓ Verification inconclusive
_onChainNftsUpdateRequested robustly updates all chains.
The finalfinallysetsisInitializedto true regardless of errors, ensuring the UI won't remain stuck. Consider verifying if partial errors on one chain can block updates for others.Use the snippet below to search for references to
_onChainNftsUpdateRequestedcalls in case partial chain failures need additional merging logic:
🏁 Script executed:
#!/bin/bash rg -A 3 "_onChainNftsUpdateRequested"Length of output: 635
Partial Chain Error Handling in _onChainNftsUpdateRequested
- Verified that
_onChainNftsUpdateRequestedis only used as an event handler inlib/bloc/nfts/nft_main_bloc.dart(viaon<NftMainChainUpdateRequested>(_onChainNftsUpdateRequested)).- The
finallyblock unconditionally setsisInitializedto true, ensuring the UI remains responsive even on errors.- Please review if this approach might inadvertently mask partial failures on individual chains. Partial errors during NFT updates may require more granular handling or merging logic to ensure that a failure in one chain doesn't prevent successful updates from others.
|
Visit the preview URL for this PR (updated for commit f540d8b): https://walletrc--pull-2566-merge-x1f3mv8p.web.app (expires Fri, 02 May 2025 10:41:47 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f66a4ff03faa546f12f0ae5a841bd9eff2714dcc |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@smk762 I've pushed a fix for #2566. Address conversion happens automatically, but I decided against re-using the address input from the withdrawal form, which has some nice goodies. It's a simple task, but it wasn't a drop-in replacement, and the last thing I want now is to introduce a weird edge-case bug I also fixed a bug in NFT withdrawal form, which also extends to the coin withdrawal form that fixes the regression where withdrawal-related errors were always showing as “Something wrong happened” instead of the specific error e.g. “FTM balance not sufficient to pay transaction fees.”. This regression has been around for a few months.
@smk762, please test if the performance is still acceptable in the release mode build on both web and desktop builds. The issue may not be straightforward to trace. Since the SDK is now handling activation and the SDK example app handles activations without slowdown, I suspect it may be related to some of the legacy code interacting directly with mm2 which has not yet been migrated to the SDK. The activation error may also be the same. If there's still a significant slowdown, I will do a search for any low-hanging fruit. But regardless, I'll revisit it in-depth post-release. |
|
It varies, and though better to release than delay without clear solution, it would be great to invesigate and mitigate before the next release after this one. |
|
Still fails. I can see the address change to mixed case when I click send, though there is no warning etc. When the send fails, it reverts back to lowercase. vokoscreenNG-2025-04-22_13-08-00.mp4 |
|
Related KDF issue: GLEECBTC/komodo-defi-framework#2421 |
Fix missing NFT preview URLs for gateway IPFS URLs.
…into bugfix/nft-hd-support
3 participants
Changes
devbranch, including KDF update tof3f4bc35a358c15007eb624928e87458ed15f38aNOTE: the NFT spam filter is always enabled, so some NFTs may not be visible in the list.
Summary by CodeRabbit
New Features
Refactor
Chores