feat(message-signing): Add AddressPath type and refactor to use Asset/PubkeyInfo#231
feat(message-signing): Add AddressPath type and refactor to use Asset/PubkeyInfo#231
Conversation
…/PubkeyInfo - Add AddressPath class for HD wallet address path handling - Supports both derivation path and component-based formats - Immutable with proper equality and JSON serialization - Documented with API reference links - Refactor SignMessageRequest to use AddressPath - Simplified from 4 optional parameters to single AddressPath - Better type safety and API compliance - Maintains full HD and non-HD wallet support - Update MessageSigningManager to use high-level types - Replace String coin parameter with Asset type - Replace individual derivation params with PubkeyInfo - Automatic conversion from PubkeyInfo to AddressPath - Use asset.id.id for correct coin identifier - Update example and namespace implementations - Remove deprecated individual HD parameters - Add comprehensive documentation and examples - Ensure consistent API across RPC and SDK layers This provides a cleaner, more type-safe API that better abstracts the underlying RPC complexity while maintaining full compatibility with the Komodo DeFi Framework message signing endpoints.
|
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 Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. ✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces the AddressPath type and refactors message signing functionality to use high-level types (Asset and PubkeyInfo) instead of low-level string parameters, providing better type safety and a cleaner API.
- Introduces
AddressPathclass to handle HD wallet derivation paths with both full path and component formats - Refactors
MessageSigningManagerto useAssetandPubkeyInfoparameters instead of string-based coin/address parameters - Updates RPC layer to use the new
AddressPathtype, simplifying parameter handling
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
packages/komodo_defi_sdk/lib/src/message_signing/message_signing_manager.dart |
Refactored to use Asset/PubkeyInfo parameters and AddressPath conversion |
packages/komodo_defi_sdk/example/lib/widgets/asset/asset_header_widget.dart |
Updated to use new SDK API with Asset and PubkeyInfo |
packages/komodo_defi_rpc_methods/lib/src/rpc_methods_library.dart |
Simplified to use AddressPath parameter |
packages/komodo_defi_rpc_methods/lib/src/rpc_methods/utility/message_signing_rpc_namespace.dart |
Updated to use AddressPath with comprehensive documentation |
packages/komodo_defi_rpc_methods/lib/src/rpc_methods/utility/message_signing.dart |
Refactored to use AddressPath instead of individual HD parameters |
packages/komodo_defi_rpc_methods/lib/src/common_structures/hd_wallet/address_path.dart |
New AddressPath class supporting both derivation path and component formats |
packages/komodo_defi_rpc_methods/lib/src/common_structures/common_structures.dart |
Added export for new AddressPath class |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
packages/komodo_defi_rpc_methods/lib/src/common_structures/hd_wallet/address_path.dart
Outdated
Show resolved
Hide resolved
packages/komodo_defi_sdk/example/lib/widgets/asset/asset_header_widget.dart
Show resolved
Hide resolved
packages/komodo_defi_sdk/example/lib/widgets/asset/asset_header_widget.dart
Show resolved
Hide resolved
packages/komodo_defi_sdk/example/lib/widgets/asset/asset_header_widget.dart
Show resolved
Hide resolved
- Remove custom_token_storage_interface.dart (unused) - Alphabetically sort exports in _coins_config_index.dart
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
packages/komodo_defi_rpc_methods/lib/src/common_structures/hd_wallet/address_path.dart
Outdated
Show resolved
Hide resolved
- Replace manual immutability implementation with freezed - Use union types for derivationPath and components constructors - Leverage freezed's when/maybeWhen for type-safe pattern matching - Maintain custom fromJson/toJson for API compatibility - Reduce boilerplate: ~40 lines vs ~135 lines manually - Keep all existing APIs and behavior unchanged Benefits: - Automatic copyWith, equality, hashCode, toString generation - Type-safe exhaustive pattern matching with when/maybeWhen - Better maintainability and less error-prone - Consistent with freezed usage across the codebase
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| params['address'] = address; | ||
| // Add HD address path if provided | ||
| if (addressPath != null) { | ||
| params['address'] = addressPath!.toJson(); |
There was a problem hiding this comment.
Bug: Empty Derivation Path Included in Request
The SignMessageRequest now includes empty derivation paths ({"derivation_path": ""}) in its JSON payload when addressInfo.derivationPath is an empty string. This differs from the previous implementation, which used an isNotEmpty check to omit such paths, and may lead to API errors.
Additional Locations (1)
takenagain
left a comment
takenagain
left a comment
There was a problem hiding this comment.
LGTM, WOMM. The removed file is confirmed to be unreferenced, and absent from the index that could make it visible.
| /// ``` | ||
| Future<String> signMessage({ | ||
| required String coin, | ||
| required Asset asset, |
There was a problem hiding this comment.
suggestion: AssetId seems more appropriate here, seeing as asset.id.id is the only referenced field.
There was a problem hiding this comment.
Good point, thanks!
There was a problem hiding this comment.
Summary
- Updated
MessageSigningManagerto acceptAssetIdfor both signing and verification, refreshed documentation examples, and forwarded the identifier directly to the RPC layer. packages/komodo_defi_sdk/lib/src/message_signing/message_signing_manager.dartL22-L101 - Adjusted the example asset header widget to use the new
assetIdargument when invoking the message signing flow. packages/komodo_defi_sdk/example/lib/widgets/asset/asset_header_widget.dartL180-L195
Testing
⚠️ dart format packages/komodo_defi_sdk/lib/src/message_signing/message_signing_manager.dart packages/komodo_defi_sdk/example/lib/widgets/asset/asset_header_widget.dart(emitted analysis include warnings because the shared analysis options reference unavailable packages, but formatting completed successfully)
Summary
This PR introduces the
AddressPathtype and refactors the message signing functionality to use high-level types (AssetandPubkeyInfo) instead of low-level string parameters.Changes
1. New
AddressPathClasspackages/komodo_defi_rpc_methods/lib/src/common_structures/hd_wallet/address_path.dartm/44'/141'/0'/0/0) and component format (accountId/chain/addressId)@immutableannotationfromJson(),toJson(), proper equality, and hashCode2. Refactored
SignMessageRequestBefore:
After:
3. Updated
MessageSigningManager(SDK Layer)Before:
After:
4. Updated Components
MessageSigningMethodsNamespace: Simplified to useAddressPathRpcMethodsLibrary: Updated to use new signatureBenefits
✅ Type Safety: Using
AssetandPubkeyInfoprevents runtime errors✅ Cleaner API: Fewer parameters, more intuitive usage
✅ Better Abstraction: SDK layer properly abstracts RPC complexity
✅ Consistency: Matches patterns used by other SDK managers
✅ Automatic HD Handling: Derivation paths extracted from
PubkeyInfo✅ API Compliant: Fully compliant with KDF API documentation
Testing
Related
Part of the message signing feature implementation for the Komodo Wallet.
Note
Adds AddressPath type and refactors message signing to use AddressPath plus Asset/PubkeyInfo, removing legacy HD params and updating SDK and example.
SignMessageRequestto acceptAddressPath(address) instead of separatederivationPath/accountId/chain/addressId.MessageSigningMethodsNamespaceandrpc_methods_libraryto useaddressPath.hd_wallet/AddressPathunion type withfromJson/toJsonand export viacommon_structures.dart.MessageSigningManagerto acceptAssetandPubkeyInfo, auto-derivingAddressPathfromPubkeyInfo.AssetHeaderWidgetupdated to new API (usesassetandpubkeyswithout manual address/derivation fields).custom_token_storage_interface.dart._coins_config_index.dart.Written by Cursor Bugbot for commit 69b99d4. This will update automatically on new commits. Configure here.