-
Notifications
You must be signed in to change notification settings - Fork 16
feat: cch separate service #901
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
base: develop
Are you sure you want to change the base?
feat: cch separate service #901
Conversation
Add StoreWithPubSub to emit store update events. It uses ractor OutputPort for pub-sub. This is a rework of StoreWithHooks in nervosnetwork#511 - Use decorator pattern to add additional features on Store - Minimize the interface change that original codes relying on Store traits dot not need to change. - Avoid using an Actor to fetch extra data asynchronously. This resolves the issue mentioned in nervosnetwork#615 (comment) . The channel actor cleans preimage on success payment automatically. Thus we may fail to get the preimage when settling the cch payment in the BTC end.
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 enables running the Cross-Chain Hub (CCH) as a separate service that communicates with another fiber node via HTTP and WebSocket JSONRPC. It introduces a significant architectural change to support CCH as a standalone service alongside the existing embedded CCH functionality.
Key changes:
- Implements a separate CCH service architecture with RPC communication to fiber nodes
- Adds comprehensive test infrastructure for cross-chain payment scenarios
- Enhances invoice management with hold invoice support and manual settlement
- Refactors store components to support pub/sub patterns for cross-service communication
Reviewed Changes
Copilot reviewed 116 out of 118 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/nodes/start.sh | Adds conditional startup of separate CCH node for specific test scenarios |
| tests/nodes/deployer/config.yml | Updates CCH configuration with proper wrapped BTC type script |
| tests/deploy/udt-init/src/main.rs | Extends node configuration generation to support separate CCH node setup |
| tests/bruno/e2e/cross-chain-hub/*.bru | Creates comprehensive test suite for cross-chain hub functionality |
| tests/bruno/e2e/cross-chain-hub-separate/*.bru | Adds test cases for separate CCH service architecture |
| crates/fiber-lib/src/store/* | Implements pub/sub store wrapper and CCH order storage |
| crates/fiber-lib/src/rpc/* | Updates RPC modules to support separate CCH service and new endpoints |
| crates/fiber-lib/src/invoice/* | Enhances invoice system with hold invoices and manual settlement |
| crates/fiber-lib/src/cch/tests/* | Adds comprehensive test infrastructure for CCH functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| return await this.invokeCommand("cancel_invoice", [params]); | ||
| } | ||
| async settleInvoice(params: SettleInvoiceParams): Promise<SettleInvoiceResult> { | ||
| return await this.invokeCommand("cancel_invoice", [params]); |
Copilot
AI
Sep 17, 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.
The settleInvoice method is incorrectly calling 'cancel_invoice' instead of 'settle_invoice'. This will cause the method to cancel invoices instead of settling them.
| return await this.invokeCommand("cancel_invoice", [params]); | |
| return await this.invokeCommand("settle_invoice", [params]); |
| auto_accept_amount: Some(1000), | ||
| script: UdtScript { | ||
| code_hash: code_hash, | ||
| code_hash, |
Copilot
AI
Sep 17, 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.
[nitpick] Using shorthand field initialization syntax is good, but this change is inconsistent with the surrounding code style that explicitly shows field names. Consider keeping the explicit 'code_hash: code_hash' for consistency.
| code_hash, | |
| code_hash: code_hash, |
| println!("cch enabled!"); | ||
| if config.is_module_enabled("cch") { | ||
| modules | ||
| .merge(CchRpcServerImpl::new(cch_actor).into_rpc()) | ||
| .unwrap(); | ||
| } | ||
| } else { | ||
| println!("cch not enabled!"); |
Copilot
AI
Sep 17, 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 println! for logging in production code is not recommended. Use the tracing macros (debug!, info!, etc.) instead for consistent logging that can be controlled by log levels.
| println!("cch enabled!"); | |
| if config.is_module_enabled("cch") { | |
| modules | |
| .merge(CchRpcServerImpl::new(cch_actor).into_rpc()) | |
| .unwrap(); | |
| } | |
| } else { | |
| println!("cch not enabled!"); | |
| tracing::info!("cch enabled!"); | |
| if config.is_module_enabled("cch") { | |
| modules | |
| .merge(CchRpcServerImpl::new(cch_actor).into_rpc()) | |
| .unwrap(); | |
| } | |
| } else { | |
| tracing::info!("cch not enabled!"); |
| println!("cch enabled!"); | ||
| if config.is_module_enabled("cch") { | ||
| modules | ||
| .merge(CchRpcServerImpl::new(cch_actor).into_rpc()) | ||
| .unwrap(); | ||
| } | ||
| } else { | ||
| println!("cch not enabled!"); |
Copilot
AI
Sep 17, 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 println! for logging in production code is not recommended. Use the tracing macros (debug!, info!, etc.) instead for consistent logging that can be controlled by log levels.
| println!("cch enabled!"); | |
| if config.is_module_enabled("cch") { | |
| modules | |
| .merge(CchRpcServerImpl::new(cch_actor).into_rpc()) | |
| .unwrap(); | |
| } | |
| } else { | |
| println!("cch not enabled!"); | |
| tracing::info!("cch enabled!"); | |
| if config.is_module_enabled("cch") { | |
| modules | |
| .merge(CchRpcServerImpl::new(cch_actor).into_rpc()) | |
| .unwrap(); | |
| } | |
| } else { | |
| tracing::info!("cch not enabled!"); |
18505a5 to
cff1c58
Compare
Merge SendBTCOrder and ReceiveBTCOrder into CchOrder to simplify persistence. Add a order status to mark that the outgoing payment succeeds and preimage is obtained. Ensure the preimage is stored in case to retry settling the incoming payment.
Load orders from database and resume the workflow.
cff1c58 to
1949e80
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #901 +/- ##
========================================
Coverage 0.00% 0.00%
========================================
Files 61 67 +6
Lines 38205 39013 +808
========================================
- Misses 38205 39013 +808
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a75fc9f to
7f94dce
Compare
7f94dce to
b263400
Compare
Based on #615 and #898
A rework of #522
This PR allows running cch as a separate service, which talks to another fiber node via HTTP and WebSocket JSONRPC.