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

Remove sync api jsonrpsee #699

Merged
merged 2 commits into from
Feb 5, 2024
Merged

Remove sync api jsonrpsee #699

merged 2 commits into from
Feb 5, 2024

Conversation

haerdib
Copy link
Contributor

@haerdib haerdib commented Dec 20, 2023

As decided in #692, this PR removes the sync interface for the jsonrpsee client. Because now not all feauters are compatible with each other anymore (jsonrpsee may not be built with sync interface), the examples were split up between sync and async. This allows to distinguish the build and test them separately.

The following has been changed:

  • Split up of examples in async and sync folders / versions
    - changed jsonrpsee examples to purely async ones
    - moved ws und tungstentite examples to the sync folder since they officially only support sync.
    - removed futures from jsonrpsee because tokio should be used instead for consistency
  • Adapted CI to build the features separately and distinguish between building examples and unit testing.
  • Removed the documentation part of the custom client example, because it was not easily possible to make it compile for both sync and async features and is already described in the README https://github.com/scs/substrate-api-client#rpc-client
  • Unit tests now need a block_on because the default version is async. Since the api behind it is a dummy client, block_on should be fine.

closes #692

@haerdib haerdib self-assigned this Dec 20, 2023
@haerdib haerdib added F2-bug Something isn't working E2-breaksapi labels Dec 20, 2023
@@ -8,7 +8,7 @@

//! General node-api Error implementation.

use alloc::{string::String, vec::Vec};
use alloc::{boxed::Box, string::String, vec::Vec};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clippy warning - error was too long for being unconstrained.

@@ -30,58 +30,6 @@ use sp_version::RuntimeVersion;
/// Api to talk with substrate-nodes
///
/// It is generic over the `Request` trait, so you can use any rpc-backend you like.
///
/// # Custom Client Example
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the custom client example, because it was not easily possible to make it compile for both sync and async features and is already described in the README https://github.com/scs/substrate-api-client#rpc-client

@haerdib haerdib requested a review from masapr January 29, 2024 08:50
@haerdib haerdib marked this pull request as ready for review January 29, 2024 08:50
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.

LGTM 👍

@haerdib haerdib force-pushed the bh/remove-sync-api-jsonrpsee branch 2 times, most recently from 6e512a8 to 16109de Compare February 5, 2024 08:37
fix tests

update ci

cargo fmt

fix compile error

fix clippy

remove compile error and ensure jsonrpsee is only exported when not sync api

clean up dependencies

move keystore_tests.rs to sync examples

update .lock

remove unneeded deps

clean up sync example toml

clean up async example toml

remove sync part from async exmaple

speed up example

add box around dynamic error values

readd spacing

use async code smart

speed up example

make author_test actually async

add sleep

add some sleeps to avoid future error

remove obsolete async naming

update README

update README

update CI
@haerdib haerdib merged commit 3098812 into master Feb 5, 2024
53 checks passed
@haerdib haerdib deleted the bh/remove-sync-api-jsonrpsee branch February 5, 2024 10:11
@haerdib haerdib mentioned this pull request Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E2-breaksapi F2-bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove future block_on to update to jsonrpsee v17+
2 participants