-
Notifications
You must be signed in to change notification settings - Fork 160
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
Migrate Forest to jsonrpsee
crate
#3943
Conversation
src/rpc/auth_api.rs
Outdated
data: Data<RPCState<DB>>, | ||
Params(params): Params<AuthNewParams>, | ||
params: Params<'_>, | ||
data: Arc<Arc<RPCState<DB>>>, |
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.
I don't understand why we need Arc<Arc<T>>
here - could you justify or remove?
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 first Arc
is needed to share the state between two RPC modules.
Line 69 in ce33815
let state = Arc::new(state); |
We only have one module now, but to implement specific pubsub code, we will need to implement a dedicated RpcModule
.
In the context of async
methods, jsonrpsee
creates a second Arc
around the Context
type:
pub struct RpcModule<Context> {
ctx: Arc<Context, Global>,
methods: Methods,
}
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.
Quick question: do we really need to share the state between modules?
And if so - perhaps the best approach is to type alias this Arc<Arc
stuff away and keep it in one place instead of declaring it on every method.
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.
Yes, because all methods in a module need access to the state.
In our case, we will have to call chain_api::chain_notify
to get the subscriber:
async fn chain_notify<DB: Blockstore>(
data: Arc<RPCState<DB>>,
) -> Result<Subscriber<HeadChange>, JsonRpseeError> {
let head_change = data.chain_store.publisher().subscribe();
Ok(head_change)
}
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.
Hum, there's only one Arc
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.
Ah I see why, ignore my comment.
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.
What's the resolution here? this is the only thing blocking an approval from me I think
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.
I think the double Arc<Arc<T>>
is ok for now (I've hidden the ugliness using the Data
type alias). This should resolve at the jsonrpsee
crate level. Maybe with something like:
pub fn from_arc(ctx: Arc<Context>) -> Self {
Self { ctx, methods: Default::default() }
}
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.
I'm grumpy about this, but if there's truly no other way then fine
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.
My from_arc
works and I can remove one Arc level: pub type Data<T> = Arc<T>;
This will require change in upstream repository though.
Seems good apart from two nits already mentioned by @aatifsyed. Perhaps the error codes could be improved, so that we at least have a way of knowing which error type was returned. |
Yes, will do. Error codes could be much more precise. And we don't always validate params; for example, in
While for Forest, it's valid. This can be addressed in a follow-up PR, though. |
Co-authored-by: David Himmelstrup <david.himmelstrup@chainsafe.io>
Co-authored-by: David Himmelstrup <david.himmelstrup@chainsafe.io>
Co-authored-by: David Himmelstrup <david.himmelstrup@chainsafe.io>
Summary of changes
Changes introduced in this pull request:
jsonrpsee
serverWebsocket transport flag should work as well:
$ ./forest-tool api compare --ws --lotus /ip4/127.0.0.1/tcp/1234/ws --forest /ip4/127.0.0.1/tcp/2345/ws forest_snapshot_calibnet_2024-02-14_height_1352592.forest.car.zst
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist