Skip to content
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

Update jsonrpsee to v0.21 #707

Merged
merged 1 commit into from
Feb 12, 2024
Merged

Update jsonrpsee to v0.21 #707

merged 1 commit into from
Feb 12, 2024

Conversation

haerdib
Copy link
Contributor

@haerdib haerdib commented Feb 5, 2024

closes #702

@haerdib haerdib self-assigned this Feb 5, 2024
@haerdib haerdib added E1-breaksnothing F9-dependencies Pull requests that update a dependency file labels Feb 5, 2024
@haerdib haerdib changed the title Update jsonrspee to v0.21 Update jsonrpsee to v0.21 Feb 5, 2024
@haerdib haerdib marked this pull request as ready for review February 6, 2024 07:17
@haerdib haerdib requested a review from masapr February 6, 2024 07:18
@@ -42,7 +42,7 @@ serde_json = { version = "1.0.79", default-features = false }
url = { version = "2.0.0", optional = true }

# websocket dependent features
jsonrpsee = { version = "0.16", optional = true, features = ["async-client", "client-ws-transport", "jsonrpsee-types"] }
jsonrpsee = { version = "0.21", optional = true, features = ["async-client", "client-ws-transport-native-tls", "jsonrpsee-types"] }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is less on this PR but a general question: It seems wrong to me, that we explicitly reference the different websocket libraries here. This does mean, that I have to include tungstenite, even when I'm using the api client with jsonrpsee. Is this correct?

On the specific code here: What if I want to use the api client with jsonrpsee, but without the native-tls feature? As far as I understand, there's no reason to not allow this. But it seems to be prevented by this.

Copy link
Contributor Author

@haerdib haerdib Feb 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does mean, that I have to include tungstenite, even when I'm using the api client with jsonrpsee. Is this correct?

No, that is not correct. The dependency imports of the rpc-clients are optional and only included if the correspondent feature is chosen.

What if I want to use the api client with jsonrpsee, but without the native-tls feature? As far as I understand, there's no reason to not allow this. But it seems to be prevented by this.

Do you have something in mind? We could forward the features of the jsonrpsee client, but that would mean more features on our side, I believe… or is there a way around it?
Or we could allow that the user constructs his/her own jsonrpsee client and just wraps our wrapper struct around it: https://github.com/scs/substrate-api-client/blob/master/src/rpc/jsonrpsee_client/mod.rs#L30-L33:

// Something like this:
let jsonrpsee_client = ClientBuilder::default()
		.max_buffer_capacity_per_subscription(4096)
		.build_with_tokio(tx, rx);
let api_client = JsonrpseeClient { inner: Arc::new(client) };

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I'm not sure if my first approach would work as long as we are constructing our own jsonrpsee-client.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussion, see the proposal #714

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Collaborator

@masapr masapr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After our discussion and the new PR I think it's fine. Thank you!

@@ -42,7 +42,7 @@ serde_json = { version = "1.0.79", default-features = false }
url = { version = "2.0.0", optional = true }

# websocket dependent features
jsonrpsee = { version = "0.16", optional = true, features = ["async-client", "client-ws-transport", "jsonrpsee-types"] }
jsonrpsee = { version = "0.21", optional = true, features = ["async-client", "client-ws-transport-native-tls", "jsonrpsee-types"] }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@haerdib haerdib merged commit b618912 into master Feb 12, 2024
56 checks passed
@haerdib haerdib deleted the bh/702-update-jsonrpsee branch February 12, 2024 15:48
haerdib added a commit that referenced this pull request Feb 15, 2024
fix clippy

add call_raw and automatic decoding

add missing await

use vec instead of option

readd Option once again

add some runtime api calls and first test

add authority test

fix build

fix no-std build

fix  build

fix result return values

remove unnecessary result return

add finalize block

add core runtime api

fix build

add RutimeApiClient for clear distinguishion

add transaction, staking , mmr and session_keys api

fix build

fix build

fix clippy

fix naming of session keys function

add mmr tests

add session key tests

fix no-std error by defining types by self for now

add sakintapi test and fix

fix build

fix tets

update tests

add runtime api example

update README

add example of self creation of call

add metadata decoding

add list functions

add some nice printing

fix build

remove mmr

fix async build

update jsonrspee to v21 (#707)
haerdib added a commit that referenced this pull request Feb 15, 2024
fix clippy

add call_raw and automatic decoding

add missing await

use vec instead of option

readd Option once again

add some runtime api calls and first test

add authority test

fix build

fix no-std build

fix  build

fix result return values

remove unnecessary result return

add finalize block

add core runtime api

fix build

add RutimeApiClient for clear distinguishion

add transaction, staking , mmr and session_keys api

fix build

fix build

fix clippy

fix naming of session keys function

add mmr tests

add session key tests

fix no-std error by defining types by self for now

add sakintapi test and fix

fix build

fix tets

update tests

add runtime api example

update README

add example of self creation of call

add metadata decoding

add list functions

add some nice printing

fix build

remove mmr

fix async build

update jsonrspee to v21 (#707)
haerdib added a commit that referenced this pull request Feb 15, 2024
* add state_call to rpc api

fix clippy

add call_raw and automatic decoding

add missing await

use vec instead of option

readd Option once again

add some runtime api calls and first test

add authority test

fix build

fix no-std build

fix  build

fix result return values

remove unnecessary result return

add finalize block

add core runtime api

fix build

add RutimeApiClient for clear distinguishion

add transaction, staking , mmr and session_keys api

fix build

fix build

fix clippy

fix naming of session keys function

add mmr tests

add session key tests

fix no-std error by defining types by self for now

add sakintapi test and fix

fix build

fix tets

update tests

add runtime api example

update README

add example of self creation of call

add metadata decoding

add list functions

add some nice printing

fix build

remove mmr

fix async build

update jsonrspee to v21 (#707)

* rename authority_discovery to authorities

* use Bytes instead of Vec<u8>

* fix clippy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E1-breaksnothing F9-dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update jsonrpsee library to v20+
2 participants