-
Notifications
You must be signed in to change notification settings - Fork 5
feat(rpc): implement missing RPCs #188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
* Add trading, Lightning, and orderbook RPC method namespaces Co-authored-by: charl <[email protected]> * Refactor order status classes to use JsonMap and type-safe parsing Co-authored-by: charl <[email protected]> * Refactor order status and related classes with type-safe parsing Co-authored-by: charl <[email protected]> * Add new common structures for trading, orderbook, and Lightning Co-authored-by: charl <[email protected]> * chore: re-generate indexes --------- Co-authored-by: Cursor Agent <[email protected]>
|
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 ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements missing RPC method namespaces for the Komodo DeFi Framework, adding comprehensive support for trading, Lightning Network, and orderbook operations. The implementation includes new RPC method classes with type-safe parsing and refactored order status classes.
- Adds three new RPC namespaces: trading, Lightning, and orderbook operations
- Refactors order status classes to use JsonMap and type-safe parsing
- Adds common structures for trading, orderbook, and Lightning Network data types
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
packages/komodo_defi_rpc_methods/lib/src/rpc_methods_library.dart |
Adds new namespace getters for trading, orderbook, Lightning, and zhtlc |
packages/komodo_defi_rpc_methods/lib/src/rpc_methods/trading/trading_rpc_namespace.dart |
Implements comprehensive trading namespace with swap operations |
packages/komodo_defi_rpc_methods/lib/src/rpc_methods/trading/ |
New trading RPC methods for swap status and initiation |
packages/komodo_defi_rpc_methods/lib/src/rpc_methods/orderbook/ |
New orderbook RPC methods and namespace |
packages/komodo_defi_rpc_methods/lib/src/rpc_methods/lightning/ |
New Lightning Network RPC methods and namespace |
packages/komodo_defi_rpc_methods/lib/src/common_structures/ |
New common data structures for trading, orderbook, and Lightning operations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| }) { | ||
| return execute( | ||
| StartSwapRequest( | ||
| rpcPass: rpcPass ?? this.rpcPass ?? '', |
Copilot
AI
Aug 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using an empty string as fallback for RPC password could lead to authentication bypass. Consider throwing an exception or returning an error when no valid RPC password is available.
| rpcPass: rpcPass ?? this.rpcPass ?? '', | |
| final effectiveRpcPass = rpcPass ?? this.rpcPass; | |
| if (effectiveRpcPass == null || effectiveRpcPass.isEmpty) { | |
| throw ArgumentError('RPC password must be provided and non-empty.'); | |
| } | |
| return execute( | |
| StartSwapRequest( | |
| rpcPass: effectiveRpcPass, |
| }) { | ||
| return execute( | ||
| EnableLightningRequest( | ||
| rpcPass: rpcPass ?? this.rpcPass ?? '', |
Copilot
AI
Aug 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using an empty string as fallback for RPC password could lead to authentication bypass. Consider throwing an exception or returning an error when no valid RPC password is available.
| rpcPass: rpcPass ?? this.rpcPass ?? '', | |
| final effectiveRpcPass = rpcPass ?? this.rpcPass; | |
| if (effectiveRpcPass == null) { | |
| throw ArgumentError('RPC password must be provided for enableLightning.'); | |
| } | |
| return execute( | |
| EnableLightningRequest( | |
| rpcPass: effectiveRpcPass, |
| }) { | ||
| return execute( | ||
| OrderbookRequest( | ||
| rpcPass: rpcPass ?? this.rpcPass ?? '', |
Copilot
AI
Aug 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using an empty string as fallback for RPC password could lead to authentication bypass. Consider throwing an exception or returning an error when no valid RPC password is available.
| rpcPass: rpcPass ?? this.rpcPass ?? '', | |
| rpcPass: (() { | |
| final resolvedPass = rpcPass ?? this.rpcPass; | |
| if (resolvedPass == null || resolvedPass.isEmpty) { | |
| throw ArgumentError('No valid RPC password provided for orderbook request.'); | |
| } | |
| return resolvedPass; | |
| })(), |
| if (baseNota != null) params['base_nota'] = baseNota; | ||
| if (relConfs != null) params['rel_confs'] = relConfs; | ||
| if (relNota != null) params['rel_nota'] = relNota; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Incorrect Parameter Types in SetOrderRequest
The SetOrderRequest class serializes parameters with incorrect types for the setprice RPC. minVolume (declared as bool?) is sent as the string 'true'/'false', but the API expects a numeric volume string. Similarly, baseConfs/relConfs and baseNota/relNota are declared and serialized as strings, but the API expects integers for confirmations and booleans for NOTA flags, respectively. This type mismatch causes setprice requests to be rejected or misinterpreted, preventing order placement.
| : null, | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: JSON Parsing Fails for Custom Types
In OrderMatchBy.fromJson, the data field's presence check json.valueOrNull<OrderMatchByData?>('data') is flawed. This method attempts to cast the raw JSON map directly to OrderMatchByData, which always fails as it doesn't deserialize custom types. Consequently, the data field is always parsed as null, even when valid match_by details are present in the JSON. This silently drops critical order matching information, leading to UIs and logic receiving incomplete data.
Add trading, Lightning, and orderbook RPC method namespaces
Refactor order status classes to use JsonMap and type-safe parsing
Refactor order status and related classes with type-safe parsing
Add new common structures for trading, orderbook, and Lightning
chore: re-generate indexes