feat: Add RPC support for direct admin communication #497
Conversation
- Add gRPC-based RPC server for admin operations - Implement CancelOrder, SettleOrder, AddSolver, TakeDispute endpoints - Add RPC configuration to settings.toml - Include protobuf definitions for admin service - Add comprehensive tests for RPC functionality - Create documentation and usage examples - Integrate RPC server startup with main daemon - Maintain backward compatibility with existing Nostr admin commands Resolves #391
|
Warning Rate limit exceeded@grunch has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 7 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis change introduces a new gRPC-based RPC interface for direct administrative communication with the Mostro daemon. It adds configuration options, protobuf definitions, a Rust server implementation, and client example, enabling local admin operations alongside existing Nostr-based commands. Documentation and tests are included to support and explain the new functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant AdminClient
participant RpcServer
participant AdminServiceImpl
participant MostroCore
AdminClient->>RpcServer: gRPC request (e.g., CancelOrder)
RpcServer->>AdminServiceImpl: Forward request
AdminServiceImpl->>MostroCore: Call existing admin handler (e.g., cancel order)
MostroCore-->>AdminServiceImpl: Return result
AdminServiceImpl-->>RpcServer: Build gRPC response
RpcServer-->>AdminClient: Send response
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Suggested reviewers
Poem
✨ Finishing Touches
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. 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
examples/rpc_client.rs (1)
1-65: Well-structured RPC client example with good error handling patterns.The example demonstrates proper async/await usage and comprehensive error handling for both RPC failures and business logic errors. The structure clearly shows how to use the gRPC client effectively.
Consider adding more descriptive comments about what each RPC operation accomplishes:
- // Example 1: Cancel an order + // Example 1: Cancel an order - Cancels an active order by ID println!("Attempting to cancel order...");- // Example 2: Add a solver + // Example 2: Add a solver - Adds a dispute resolver to the system println!("\nAttempting to add solver...");src/rpc/mod.rs (2)
41-66: Remove trivial assertion in favor of more meaningful test.The test correctly verifies that all request types can be instantiated, but the final
assert!(true)doesn't add value.Replace the trivial assertion with a more meaningful check:
- // Test passes if all types can be instantiated - assert!(true); + // Test passes if all types can be instantiated without panicking
68-93: Remove trivial assertion in favor of more meaningful test.Similar to the request types test, the final
assert!(true)doesn't provide value.Replace the trivial assertion with a more meaningful check:
- // Test passes if all types can be instantiated - assert!(true); + // Test passes if all response types can be instantiated without panickingsrc/rpc/server.rs (1)
22-28: Consider graceful error handling for missing configuration.The
Settings::get_rpc()function panics if the global configuration is not initialized. Consider returning aResultfrom the constructor to handle configuration errors gracefully rather than panicking.-pub fn new() -> Self { - let rpc_config = Settings::get_rpc(); - Self { - listen_address: rpc_config.listen_address.clone(), - port: rpc_config.port, - } -} +pub fn new() -> Result<Self, Box<dyn std::error::Error>> { + let rpc_config = Settings::get_rpc(); + Ok(Self { + listen_address: rpc_config.listen_address.clone(), + port: rpc_config.port, + }) +}Note: This would require updating the
Defaulttrait implementation and callers to handle theResult.docs/RPC.md (1)
130-142: Add language specifiers to fenced code blocks.The fenced code blocks for log examples should specify a language for proper syntax highlighting.
-``` +```log INFO mostro::rpc::server: Starting RPC server on 127.0.0.1:50051 INFO mostro::rpc::server: RPC server started successfullyAdmin operations will be logged:
-
+log
INFO mostro::rpc::service: Received cancel order request for order: 550e8400-e29b-41d4-a716-446655440000🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
132-132: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
139-139: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
proto/admin.proto (1)
3-3: Consider aligning package with directory structure.The package
mostro.admin.v1suggests the file should be inmostro/admin/v1/directory relative to the proto root. While this doesn't affect functionality, following the convention improves code organization.Either:
- Move the file to
proto/mostro/admin/v1/admin.proto, or- Simplify the package name to match the current structure (e.g.,
package admin.v1;)🧰 Tools
🪛 Buf (1.54.0)
3-3: Files with package "mostro.admin.v1" must be within a directory "mostro/admin/v1" relative to root but were in directory "proto".
(PACKAGE_DIRECTORY_MATCH)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Cargo.toml(1 hunks)build.rs(1 hunks)docs/RPC.md(1 hunks)examples/rpc_client.rs(1 hunks)proto/admin.proto(1 hunks)settings.tpl.toml(1 hunks)src/config/settings.rs(3 hunks)src/config/types.rs(2 hunks)src/main.rs(3 hunks)src/rpc/mod.rs(1 hunks)src/rpc/server.rs(1 hunks)src/rpc/service.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/main.rs (5)
src/scheduler.rs (1)
start_scheduler(22-35)src/rpc/server.rs (2)
is_enabled(58-60)new(22-28)src/rpc/service.rs (1)
new(23-33)src/lightning/mod.rs (1)
new(37-51)src/config/settings.rs (1)
get_db_pool(42-44)
src/rpc/server.rs (4)
src/rpc/service.rs (1)
new(23-33)src/config/settings.rs (1)
get_rpc(77-79)src/app.rs (1)
e(372-372)src/config/types.rs (1)
default(67-73)
🪛 markdownlint-cli2 (0.17.2)
docs/RPC.md
132-132: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
139-139: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🪛 Buf (1.54.0)
proto/admin.proto
3-3: Files with package "mostro.admin.v1" must be within a directory "mostro/admin/v1" relative to root but were in directory "proto".
(PACKAGE_DIRECTORY_MATCH)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
🔇 Additional comments (14)
Cargo.toml (1)
60-61: Good build dependency addition.The
tonic-builddependency is correctly placed in[build-dependencies]for protobuf compilation during build.build.rs (1)
3-7:❓ Verification inconclusive
Good protobuf compilation setup with minor flag concern.
The protobuf compilation setup is well-structured with proper error handling. However, verify that the
--experimental_allow_proto3_optionalflag is necessary and stable for your use case.
🌐 Web query:
Is the --experimental_allow_proto3_optional flag in protoc stable and recommended for production use?💡 Result:
The
--experimental_allow_proto3_optionalflag inprotocwas introduced in version 3.12.0 to enable support for theoptionalkeyword in proto3, allowing for explicit field presence tracking. (abi-laboratory.pro) This feature was considered experimental at that time.In version 3.15.0, released in 2020, the
optionalkeyword became fully supported in proto3 without the need for the--experimental_allow_proto3_optionalflag. (abi-laboratory.pro) Therefore, if you're usingprotocversion 3.15.0 or later, you can use theoptionalkeyword in your proto3 definitions without any experimental flags.Given that the
optionalkeyword is now fully supported in stable releases ofprotoc, it is safe and recommended to use it in production environments.Citations:
- 1: https://abi-laboratory.pro/index.php?l=protobuf&v=4.0.0-rc2&view=changelog&utm_source=openai
- 2: https://abi-laboratory.pro/index.php?l=protobuf&v=4.0.0-rc2&view=changelog&utm_source=openai
Protobuf Optional Support Now Stable – Verify Protoc Version and Flag Usage
The
optionalkeyword in proto3 has been natively supported sinceprotoc3.15.0, so the--experimental_allow_proto3_optionalflag is no longer required on modern setups. Please confirm your CI and local environments are runningprotoc≥ 3.15.0 and drop the experimental flag if so.• File:
build.rs
Lines: 3–7 in thetonic_build::configure()blockSuggested change:
- .protoc_arg("--experimental_allow_proto3_optional")src/config/settings.rs (3)
2-2: LGTM - Consistent import addition.The
RpcSettingsimport follows the existing pattern and is properly integrated.
30-31: LGTM - Consistent struct field addition.The
rpcfield addition follows the established pattern of theSettingsstruct.
76-79: LGTM - Consistent accessor method.The
get_rpc()method follows the same pattern as other configuration getters with appropriate error handling.src/main.rs (4)
12-12: LGTM - Proper module declaration.The
rpcmodule is correctly declared as public.
22-22: LGTM - Appropriate import addition.The
RpcServerimport is correctly placed with other imports.
27-27: LGTM - Necessary Arc import.The
Arcimport is needed for shared resource management in the RPC server.
108-108:✅ Verification successful
Verify LndConnector cloning behavior.
The code creates a new
LndConnectorinstance, but it's unclear if this type supports cloning or if creating multiple instances to the same LND node is safe.
🏁 Script executed:
#!/bin/bash # Check if LndConnector implements Clone trait ast-grep --pattern 'impl Clone for LndConnector { $$$ }' # Also check for Clone derive ast-grep --pattern '#[derive($$$Clone$$$)] struct LndConnector { $$$ }'Length of output: 136
🏁 Script executed:
#!/bin/bash # Locate the definition of LndConnector to inspect its traits rg -n "struct LndConnector" -C 3 --glob "*.rs" # Check for a #[derive(Clone)] annotation anywhere on LndConnector rg -n "#\[derive.*Clone.*\]" -C 3 --glob "*.rs" # Look for an explicit impl Clone for LndConnector rg -n "impl\s\+Clone\s\+for\s\+LndConnector" -C 3 --glob "*.rs"Length of output: 4122
No Clone impl needed for LndConnector – you’re cloning the Arc, not the connector.
LndConnector itself doesn’t derive or implement
Clone, nor does it need to. By wrapping it inArc<Mutex<LndConnector>>, you’re cloning theArchandle to share the same connector instance across tasks, which is both safe and intentional.settings.tpl.toml (1)
54-60: LGTM! Good security defaults for RPC configuration.The RPC configuration section follows security best practices by being disabled by default and binding to localhost only. The default port 50051 is the standard gRPC port, which is appropriate.
src/config/types.rs (2)
55-74: Well-structured configuration type with appropriate defaults.The
RpcSettingsstruct properly follows the existing configuration pattern with appropriate derive traits and a sensible default implementation that matches the template configuration.
111-112: Proper integration with the configuration macro system.The macro call correctly includes
RpcSettingsmapped to therpcfield, maintaining consistency with the existing configuration structure.src/rpc/mod.rs (2)
1-16: Clean module structure with proper protobuf integration.The module organization follows Rust conventions with clear separation of concerns between server and service functionality. The protobuf inclusion is correctly implemented.
21-39: Good basic protobuf message testing.The test verifies protobuf message creation and field access, which is valuable for ensuring the generated code works correctly.
…anges made were: 1. Cargo.toml:54 - Upgraded tonic = "0.11" to tonic = "0.13.1" 2. Cargo.toml:55 - Upgraded prost = "0.12" to prost = "0.13.5" 3. build.rs:6 - Changed .compile() to .compile_protos() for tonic-build API compatibility
Resource duplication: Creating a new LndConnector instance instead of reusing the existing ln_client
Redundant Arc wrapping: get_db_pool() already returns Arc<sqlx::SqlitePool>, so Arc::new() creates Arc<Arc<SqlitePool>>
Premature logging: Success message is logged before the server actually starts listening
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Resolves #391
Summary by CodeRabbit
New Features
Documentation
Configuration
Examples