server: Register raw method with connection ID

[lexnv]

This PR exposes the connection ID to raw methods.

To achieve this, a new register_raw_method is exposed on the RpcModule.
The with-context attribute is implemented in the proc-macros to use this method.
When the with-context attribute is used, all methods of a server are rendered with an extra connection Id parameter.
This is similar to the PendingSubscriptionSink that is injected into subscription methods.

Examples

The example server_with_context implements a server with the context attribute enabled.
A function of the server returns the connection ID from its parameters.
Users on different connections will receive a different connection ID.

Testing

• Add proc-macro tests to capture the incompatibilities between with-context, sync methods and blocking methods
• Add a unit-test to validate this behavior further
• Build on the example to allow methods to be called from a single connection context

A simplified version of #1295, that avoids a user-breaking change.

[niklasad1]

This should work and overall I like it but some small comments to address

[niklasad1]

LGTM, nice work

I wonder if we should provide a type with connection_id() to avoid breaking it if we decide to add some other field but I think that's unlikely though.

[jsdw]

Looks good to me; just a couple of thoughts:

So, basically, register_raw_method is the same as register_async_method but with a connection ID. I'd wonder about calling it something more similar to that, eg: register_async_method_with_details (especially if the below was done).

In general I guess being able to pass Extensions between middlewares and method would remove the need for this again? But happy to just merge this and cross that bridge when we get there anyways!

I wonder if we should provide a type with connection_id() to avoid breaking it if we decide to add some other field but I think that's unlikely though.

I wondered too about wrapping the ConnectionId in a type, just to give more flexibility eg:

pub struct ConnectionDetails {
    id: ConnectionId
}

impl ConnectionDetails {
    pub fn id(&self) -> &ConnectionId
}

[niklasad1]

I don't understand why we need the builder?

[lexnv]

The RpcService should be the last "layer" before the user-callback is called.

We construct the ConnectionDetails in this layer:

jsonrpsee/server/src/middleware/rpc/layer/rpc_service.rs

Lines 102 to 104 in 87585ad

	let connection_details = ConnectionDetails::builder().id(conn_id).build();
	let fut = (callback)(id, params, connection_details, max_response_body_size);
	ResponseFuture::future(fut)

I think we could just as easily provide a constructor for this:

impl ConnectionDetails {
  pub fn new(id: ConnectionId) ..
}

The new method needs to be public, because the ConnectionDetails is declared in the core crate. And the ConnectionDetails needs to be instantiated in the server crate as well.

We'd have to eventually change this method to new(id: ConnectionId, extensions: Extenstions) which will be a breaking change (although we should be the only ones building this object).

TLDR; since we'll do a breaking change either way, I think a new method should be fine for now! 🙏

[niklasad1]

ok, I see makes sense

[jsdw]

Maybe have it be #[doc(hidden)] and comment that it's not part of the public APi and can change without notice? I don't think anything outside of jsonrpsee needs to build it, so then we could also just use new() instead of a builder (Or maybe eg _new(connection_id) to really emphasise the hiddenness of it but shrug)

[niklasad1]

I'm not super happy about the naming register_async_with_details, it's too vague for my taste.
I would prefer register_async_method_with_conn_context or something like that

It should be obvious whether it is a method call or subscription IMO.

[jsdw]

How about register_async_method_eith_details? since we are provigin ConnectionDetails to it, so the word details lines up well imo :)

[niklasad1]

We may mark this entire feature/API as #[doc(hidden)] as we will break it anyway in the next release?

What do you reckon?

[jsdw]

We may mark this entire feature/API as #[doc(hidden)] as we will break anyway in the next release?

Works for me too, since I geuss we only need it internally right now and we'll break it again soon! Could also be feature flagged to hdie it at compile time and then only enable in substrate?

[niklasad1]

rename perhaps?

[lexnv]

Have changed it to server_wtih_connection_details, thanks!