-
Notifications
You must be signed in to change notification settings - Fork 236
feat(rpc): add IPC call #4861
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(rpc): add IPC call #4861
Conversation
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.
Please execute make gen-rpc-doc
Done |
payload: match fmt { | ||
IpcPayloadFormat::Hex => Value::String(format!("0x{}", hex_string(resp.payload()))), | ||
IpcPayloadFormat::Json => { | ||
serde_from_str::<Value>(String::from_utf8_lossy(resp.payload()).as_ref()) | ||
.map_err(|e| RPCError::custom_with_error(RPCError::IPC, e))? | ||
} | ||
}, |
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.
Maybe we should change the payload
's type to String
instead of serde_json::Value
?
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.
According to the design document, Payload needs to support two formats. The first is json object, for example:
{
"Payload": {
"MethodName": { "Parameter": "Value" }
}
}
The second is Hex mode, for example
{
"Payload": "0x0000000001111111122222..."
}
It seems that only serde_json::Value
can be used to define its type.
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 adds IPC call functionality to the RPC module by introducing a new ckb-ipc utility crate, updating the RPC service builder to support the IPC module, and extending documentation and tests accordingly.
- Added a new ckb-ipc module with VLQ encoding/decoding utilities, pipe communication, packet definitions, and error types.
- Extended the RPC configuration, service builder, and documentation to support the new IPC module.
- Updated workspace and dependency configurations to include the new ckb-ipc module.
Reviewed Changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
util/ipc/src/vlq.rs | Implements VLQ encoding/decoding functions for IPC messages |
util/ipc/src/pipe.rs | Adds a bidirectional pipe implementation for IPC communication |
util/ipc/src/packet.rs | Defines IPC packet structures and serialization methods |
util/ipc/src/lib.rs | Re-exports IPC components |
util/ipc/src/error.rs | Implements custom error types for IPC |
util/app-config/src/configs/rpc.rs | Introduces IPC module support in the configuration |
script/src/syscalls/mod.rs | Adds syscall id constants for IPC-related operations |
rpc/src/tests/setup.rs | Integrates IPC module enablement into the RPC test suite |
rpc/src/service_builder.rs | Adds enable_ipc method and related changes to include IPC RPC methods |
rpc/src/module/mod.rs | Exposes new IPC module and related RPC method definitions |
rpc/src/module/indexer.rs | Adjusts visibility of IndexerRpcImpl affecting module exposure |
rpc/src/error.rs | Adds error codes for IPC-related errors |
rpc/README.md | Updates documentation with IPC module details and examples |
(Other files) | Workspace and dependency updates to include the new ckb-ipc module |
Comments suppressed due to low confidence (3)
util/ipc/src/pipe.rs:84
- [nitpick] Consider using a more descriptive error message for the missing receiver channel, e.g., 'Receiver channel not available', to aid debugging.
.rx.as_ref().ok_or_else(|| Error::new(ErrorKind::Other, "channel is not found"))?
util/ipc/src/pipe.rs:110
- [nitpick] Consider revising the error message to clearly indicate that the sender channel is missing, for example, 'Sender channel not available'.
.tx.as_mut().ok_or_else(|| Error::new(ErrorKind::Other, "channel is not found"))?
rpc/src/module/indexer.rs:887
- The visibility of IndexerRpcImpl was changed from 'pub(crate)' to 'pub'. Please ensure this change is intentional to avoid exposing internal implementation details.
pub struct IndexerRpcImpl {
1c29cba
to
39962ad
Compare
LGTM |
); | ||
let builder = builder.enable_debug(); | ||
let builder = builder.enable_alert(alert_verifier, alert_notifier, network_controller); | ||
let indexer_rpc_impl = builder.indexer_rpc_impl.clone(); |
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.
any specific reason to don't use chain call style here?
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.
Here I need to clone a copy of indexer_rpc_impl, which cannot be achieved using chain call.
.map(|e| e.into_bytes())?) | ||
} | ||
None => Err(RPCError::custom( | ||
RPCError::IPCIndexerIsDisabled, |
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.
seems from the code, if IPC module is not enabled, ipc_script_data_by_type_id_args
will never be called here.
it's not necessary to add a RPCError::IPCIndexerIsDisabled code for it.
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.
never mind, it's not IPC module here.
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.
so if IPC module is enabled, it will require Index module required, could we just validate this rule when parsed config file.
otherwise user can only figure out this rule when error happened.
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.
When the script_locator
parameter of ipc_call is type_id_args
, it will depend on indexer;
When it is out_point
, it will not depend on indexer.
So it can only be checked at runtime.
rpc/src/module/ipc.rs
Outdated
debug_assert_eq!(scheduler.consumed_cycles(), 0); | ||
let tic = std::time::SystemTime::now(); | ||
loop { | ||
let last_cycles = limit_cycles as u64 - scheduler.consumed_cycles(); |
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.
do we have a risk of overflow here?
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.
We are better off using the saturating_sub
method
break; | ||
} | ||
let toc = tic.elapsed().map(|e| e.as_secs()).unwrap_or(u64::MAX); | ||
if toc >= limit_time as u64 { |
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.
if timeout, whether we should report an error for endpoint?
What is changed and how it works?
What's Changed:
Added an
ipc_call
method in jsonrpc. The method is described in detail inrpc/src/module/ipc.rs
's comments.Here are some additional points to note:
ipc script
, such asexec
andspawn
.I need more time to rewritten it in Rust., @gpBlockchain is responsible for migrating integration testing to project https://github.com/cryptape/ckb-py-integration-testCheck List
Tests
Release note