feat: async API when `Response` has been processed. #1281

niklasad1 · 2024-01-30T15:10:02Z

Resolves #1279

examples/examples/ws_notify_on_method_answered.rs

core/src/server/rpc_module.rs

core/src/server/helpers.rs

proc-macros/src/rpc_macro.rs

core/src/server/helpers.rs

lexnv · 2024-02-01T12:43:30Z

Overall looks good to me, nice one 👍 ! And in combination with the polkadot-sdk commit this should resolve any races from happening.

I find the is_sent() method name a bit confusing; in the sense that the message is sent to our internal buffers; but can't think of any other names. This should be fine since this is a niece low-level API

niklasad1 · 2024-02-01T13:58:43Z

I find the is_sent() method name a bit confusing; in the sense that the message is sent to our internal buffers; but can't think of any other names. This should be fine since this is a niece low-level API

Yeah, agree. I don't like it either I thought it was nice wrapper but it makes things more confusing. Removed now thanks.

core/src/server/method_response.rs

jsdw · 2024-02-02T15:51:16Z

core/src/server/method_response.rs

+
+	/// Optional callback that may be utilized to notify that the method response was a
+	/// an JSON-RPC error object and processed.
+	pub fn notify_on_error(mut self, tx: MethodResponseNotifyTx) -> Self {


How this is set up at the moment means that if somebody does:

response .notify_on_success(...) .notify_on_error(...) .notify_on_response(...)

They get no compile error, and they might expect one of those to fire in each case, but in fact only the last registered one has any chance of ever firing.

I guess I'd prevent this by:

Either have more than one callback that can be registered, or

Just have notify_on_completion or something and indicate by the channel response whether success or error happened.

Though I'd also probably want it so that response.notify_on_completion(a).notify_on_completion(b) also worked and fired both a and b; one could store a vec of things-to-be-notified perhaps? Or change the method name to eg set_on_complete_notifier(..) so that it was a bit more obvious that it set a single thing rather than allowed multiple notifications to be given.

core/src/server/method_response.rs

examples/examples/ws_notify_on_method_answered.rs

jsdw · 2024-02-02T16:05:21Z

proc-macros/src/render_client.rs

@@ -109,10 +105,20 @@ impl RpcDescription {
 			let ret_ty = args.last_mut().unwrap();
 			let err_ty = self.jrps_client_item(quote! { core::client::Error });

+			quote! { core::result::Result<#ret_ty, #err_ty> }
+		} else if type_name.ident == "ResponsePayload" {


I'm sad that we have to match on the string type names :D

I guess it wasn't possible to use a trait to convert these into results automatically or something; I'm sure it was something like that but can't remember the issue now! But ah well!

Nah, it's that the client codegen is super opinionated and the error type must be jsonrpsee::core::client::Error otherwise it won't compile. Yeah, it's ugly but just proc macro code thank god.

IntoResponse stuff doesn't work there because we have deal with I/O errors and bunch of things in the client code

proc-macros/src/render_client.rs

jsdw · 2024-02-02T16:13:37Z

Looks good to me; the only actual thing I think should be addressed is making res.notify_on_success(..).notify_on_error(..) either impossible or do as you might expect (ie fire all of the provided channels).

niklasad1 · 2024-02-05T10:58:43Z

proc-macros/tests/ui/correct/custom_ret_types.rs

@@ -31,7 +30,7 @@ impl IntoResponse for CustomError {
 		let data = data.map(|val| serde_json::value::to_raw_value(&val).unwrap());

 		let error_object = jsonrpsee::types::ErrorObjectOwned::owned(code, "custom_error", data);
-		ResponsePayload::Error(error_object)
+		ResponsePayload::error(error_object)


This is the breaking change

Aaahhh I see! I was thinking of something else :)

(I guess if you wanted to do a patch release and didn't mind it being a bit ugly you could have an Error method)

niklasad1 · 2024-02-05T10:58:57Z

proc-macros/tests/ui/correct/custom_ret_types.rs

@@ -4,8 +4,7 @@ use std::net::SocketAddr;

 use jsonrpsee::core::{async_trait, ClientError, Serialize};
 use jsonrpsee::proc_macros::rpc;
-use jsonrpsee::server::{IntoResponse, ServerBuilder};
-use jsonrpsee::types::ResponsePayload;
+use jsonrpsee::server::{IntoResponse, ResponsePayload, ServerBuilder};


This is a breaking change as well

Aah I see; it moved! So will you do a proper release then instead of a patch one do you think? Or alias this type in jsonrpsee::types in the release to avoid this break?

I think we have to make a proper release because the MethodResponse is also a breaking change and I can't do proper alias in types because then the feature flag core/server needs to be enabled for ResponsePayload to be available.

core/src/server/method_response.rs

jsdw

Looks good to me offhand!

It feels like we'll need to do a major release given the breaks unless you mangle a couple of things in a patch release to keep the old APIs/imports :)

lexnv · 2024-02-05T16:43:48Z

examples/examples/ws_notify_on_method_answered.rs

@@ -0,0 +1,84 @@
+// Copyright 2019-2021 Parity Technologies (UK) Ltd.


nit: Maybe copyright years need to change

let's take of it in another PR

lexnv

Looks good to me!

proc-macros/src/render_server.rs

…response.rs

niklasad1 · 2024-02-06T07:07:17Z

core/src/server/method_response.rs

+	///
+	/// This is useful when one only want to return the error and
+	/// `T` can't resolved by rustc to avoid type annonations.
+	pub fn unit_error(e: impl Into<ErrorObjectOwned>) -> Self {


/cc @jsdw @lexnv any thoughts on this one?

Basically, I would want a nice wrapper to avoid doing ResponsePayload::<()>::error(err) but maybe unit error is weird because it kind of reads like the error is ()

When you first mentioned it I assumed it meant error is () which is what confused me :D

That said, I'm struggling to come up with a better name! I'm ok with just doing ResponsePayload::<()>::error(err) too I guess; if thename for any longer you'd not be saving any chars any more anyways!

…-api-v1

server/src/transport/ws.rs

niklasad1 added 7 commits January 30, 2024 16:07

feat: add RpcModule::register_raw_method

4773227

add proc macro support

ee9b8ea

rename API

b595e5f

simplify API with MethodResponse::notify_when_sent

304c938

improve notify API

973e62d

fix nits

90da71b

introduce ResponsePayloadV2

53520d8

niklasad1 changed the title ~~feat: add RpcModule::register_raw_method~~ feat: add RpcModule::register_raw_method + ResponsePayload async API Jan 31, 2024

niklasad1 commented Jan 31, 2024

View reviewed changes

examples/examples/ws_notify_on_method_answered.rs Outdated Show resolved Hide resolved

niklasad1 commented Jan 31, 2024

View reviewed changes

core/src/server/rpc_module.rs Outdated Show resolved Hide resolved

niklasad1 commented Jan 31, 2024

View reviewed changes

core/src/server/helpers.rs Outdated Show resolved Hide resolved

impl ResponsePayloadV2 for T

f71d046

lexnv reviewed Feb 1, 2024

View reviewed changes

proc-macros/src/rpc_macro.rs Outdated Show resolved Hide resolved

lexnv reviewed Feb 1, 2024

View reviewed changes

core/src/server/helpers.rs Outdated Show resolved Hide resolved

niklasad1 added 2 commits February 1, 2024 16:14

cleanup

dc88a6d

client: proc macro support for custom ret_ty

51ce17e

niklasad1 marked this pull request as ready for review February 1, 2024 16:55

niklasad1 requested a review from a team as a code owner February 1, 2024 16:55

add tests

ffea2ca

jsdw reviewed Feb 2, 2024

View reviewed changes

core/src/server/method_response.rs Outdated Show resolved Hide resolved

jsdw reviewed Feb 2, 2024

View reviewed changes

core/src/server/method_response.rs Outdated Show resolved Hide resolved

jsdw reviewed Feb 2, 2024

View reviewed changes

core/src/server/method_response.rs Outdated Show resolved Hide resolved

jsdw reviewed Feb 2, 2024

View reviewed changes

examples/examples/ws_notify_on_method_answered.rs Outdated Show resolved Hide resolved

jsdw reviewed Feb 2, 2024

View reviewed changes

proc-macros/src/render_client.rs Outdated Show resolved Hide resolved

address grumbles

f549bf4

niklasad1 changed the title ~~feat: add async API when a ResponsePayload has been processed.~~ feat: async API when a ResponsePayload has been processed. Feb 4, 2024

niklasad1 mentioned this pull request Feb 5, 2024

chainHead_call (et al): clarify that response from this comes back before any events on the follow subscription. paritytech/json-rpc-interface-spec#125

Closed

niklasad1 added 2 commits February 5, 2024 09:21

proc: revert unrelated changes

db7e08c

remove panics; move should be enough

925108a

niklasad1 commented Feb 5, 2024

View reviewed changes

bring back UI tests

d256661

jsdw mentioned this pull request Feb 5, 2024

JSON-RPC messages arrive out of order paritytech/polkadot-sdk#2992

Closed

2 tasks

jsdw reviewed Feb 5, 2024

View reviewed changes

core/src/server/method_response.rs Outdated Show resolved Hide resolved

jsdw approved these changes Feb 5, 2024

View reviewed changes

niklasad1 added 5 commits February 5, 2024 13:27

grumbles: remove NotifiedError

0eba672

break stuff for uniform API

8391579

make more stuff private

f4b5a92

remove ResponseErrorUnit type alias

e835a72

fix ui tests

7f93b20

lexnv reviewed Feb 5, 2024

View reviewed changes

lexnv approved these changes Feb 5, 2024

View reviewed changes

niklasad1 changed the title ~~feat: async API when a ResponsePayload has been processed.~~ feat: async API when Response has been processed. Feb 5, 2024

niklasad1 commented Feb 5, 2024

View reviewed changes

proc-macros/src/render_server.rs Outdated Show resolved Hide resolved

niklasad1 added 2 commits February 5, 2024 18:20

Update proc-macros/src/render_server.rs

7006a00

Rename ws_notify_on_method_answered.rs to response_payload_notify_on_…

487df31

…response.rs

niklasad1 commented Feb 6, 2024

View reviewed changes

niklasad1 added 4 commits February 6, 2024 12:55

Merge remote-tracking branch 'origin/master' into low-level-api-v1

f9d7fc3

remove unit_error APIs

da41c62

Merge remote-tracking branch 'origin/low-level-api-v1' into low-level…

a83c766

…-api-v1

replace notify_on_x with notify_on_completion

802f77f

niklasad1 commented Feb 6, 2024

View reviewed changes

server/src/transport/ws.rs Outdated Show resolved Hide resolved

Update server/src/transport/ws.rs

2836906

niklasad1 merged commit 387f52f into master Feb 6, 2024
11 checks passed

niklasad1 deleted the low-level-api-v1 branch February 6, 2024 13:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: async API when `Response` has been processed. #1281

feat: async API when `Response` has been processed. #1281

niklasad1 commented Jan 30, 2024 •

edited

Loading

lexnv commented Feb 1, 2024

niklasad1 commented Feb 1, 2024 •

edited

Loading

jsdw Feb 2, 2024 •

edited

Loading

jsdw Feb 2, 2024 •

edited

Loading

jsdw Feb 2, 2024

niklasad1 Feb 2, 2024 •

edited

Loading

jsdw commented Feb 2, 2024

niklasad1 Feb 5, 2024

jsdw Feb 5, 2024 •

edited

Loading

niklasad1 Feb 5, 2024

jsdw Feb 5, 2024

niklasad1 Feb 5, 2024

jsdw left a comment

lexnv Feb 5, 2024

niklasad1 Feb 6, 2024

lexnv left a comment

niklasad1 Feb 6, 2024

jsdw Feb 6, 2024

		@@ -0,0 +1,84 @@
		// Copyright 2019-2021 Parity Technologies (UK) Ltd.

feat: async API when Response has been processed. #1281

feat: async API when Response has been processed. #1281

Conversation

niklasad1 commented Jan 30, 2024 • edited Loading

lexnv commented Feb 1, 2024

niklasad1 commented Feb 1, 2024 • edited Loading

jsdw Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

jsdw Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

jsdw commented Feb 2, 2024

Choose a reason for hiding this comment

jsdw Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lexnv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: async API when `Response` has been processed. #1281

feat: async API when `Response` has been processed. #1281

niklasad1 commented Jan 30, 2024 •

edited

Loading

niklasad1 commented Feb 1, 2024 •

edited

Loading

jsdw Feb 2, 2024 •

edited

Loading

jsdw Feb 2, 2024 •

edited

Loading

niklasad1 Feb 2, 2024 •

edited

Loading

jsdw Feb 5, 2024 •

edited

Loading