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.

Sign up for GitHub

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 substrate-compat default feature flag #1078

Merged
merged 10 commits into from
Jul 21, 2023
Merged

Remove substrate-compat default feature flag #1078

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6836f9b
remove defautl feature, add target cfg, change CI job waml
tadeohepperle Jul 20, 2023
4091501
adjust CI job
tadeohepperle Jul 20, 2023
538a55a
Merge branch 'master' into remove-substrate-compat-default-deature-flag
tadeohepperle Jul 20, 2023
e6bdea3
Adjust docs
tadeohepperle Jul 21, 2023
6ebb190
adjust the CI job
tadeohepperle Jul 21, 2023
32210d5
add wasm targe to ci again
tadeohepperle Jul 21, 2023
78a5e96
Merge branch 'master' into remove-substrate-compat-default-deature-flag
tadeohepperle Jul 21, 2023
1686260
Merge branch 'master' into remove-substrate-compat-default-deature-flag
tadeohepperle Jul 21, 2023
6ddeb74
remove the unresolved links from the book
tadeohepperle Jul 21, 2023
6f7f7ba
Update subxt/src/book/usage/transactions.rs
jsdw Jul 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions .github/workflows/rust.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ jobs:
- name: Cargo check all targets.
run: cargo check --all-targets

# A basic check for WASM target specifically
- name: Cargo check WASM target
run: cargo check --no-default-features --features jsonrpsee,web --target wasm32-unknown-unknown
jsdw marked this conversation as resolved.
Show resolved Hide resolved

# Next, check subxt features.
# - `native` feature must always be enabled
# - `web` feature is always ignored.
Expand All @@ -71,9 +75,9 @@ jobs:
# check it without. We can't enable subxt or web features here, so no cargo hack.
- name: Cargo check subxt-signer
run: |
cargo check -p subxt-signer
cargo check -p subxt-signer --no-default-features --features sr25519
cargo check -p subxt-signer --no-default-features --features ecdsa
cargo check -p subxt-signer
cargo check -p subxt-signer --no-default-features --features sr25519
cargo check -p subxt-signer --no-default-features --features ecdsa

# We can't enable web features here, so no cargo hack.
- name: Cargo check subxt-lightclient
Expand Down Expand Up @@ -271,8 +275,8 @@ jobs:
# `listen-addr` is used to configure p2p to accept websocket connections instead of TCP.
# `node-key` provides a deterministic p2p address.
substrate --dev --node-key 0000000000000000000000000000000000000000000000000000000000000001 --listen-addr /ip4/0.0.0.0/tcp/30333/ws > /dev/null 2>&1 &
wasm-pack test --headless --firefox
wasm-pack test --headless --chrome
wasm-pack test --no-default-features --features jsonrpsee,web --headless --firefox
wasm-pack test --no-default-features --features jsonrpsee,web --headless --chrome
Copy link
Collaborator

Choose a reason for hiding this comment

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

These should be unnecessary shouldn't they? The wasm-rpc-tests crate doesn't expose any features, and so this should have no effect.

pkill substrate
working-directory: testing/wasm-rpc-tests

Expand Down
2 changes: 1 addition & 1 deletion subxt/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ keywords = ["parity", "substrate", "blockchain"]
[features]
# For dev and documentation reasons we enable more features than are often desired.
# it's recommended to use `--no-default-features` and then select what you need.
default = ["jsonrpsee", "native", "substrate-compat"]
default = ["jsonrpsee", "native"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

🎉


# Enable this for native (ie non web/wasm builds).
# Exactly 1 of "web" and "native" is expected.
Expand Down
71 changes: 37 additions & 34 deletions subxt/src/book/usage/transactions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,40 +77,43 @@
//! Let's see how to use each of these approaches:
//!
//! ```rust
//! use subxt::config::PolkadotConfig;
//! use std::str::FromStr;
//!
//! //// 1. Use a `subxt_signer` impl:
//! use subxt_signer::{ SecretUri, sr25519 };
//!
//! // Get hold of a `Signer` for a test account:
//! let alice = sr25519::dev::alice();
//!
//! // Or generate a keypair, here from an SURI:
//! let uri = SecretUri::from_str("vessel ladder alter error federal sibling chat ability sun glass valve picture/0/1///Password")
//! .expect("valid URI");
//! let keypair = sr25519::Keypair::from_uri(&uri)
//! .expect("valid keypair");
//!
//! //// 2. Use the corresponding `sp_core::Pair` impl:
//! use subxt::tx::PairSigner;
//! use sp_core::Pair;
//!
//! // Get hold of a `Signer` for a test account:
//! let alice = sp_keyring::AccountKeyring::Alice.pair();
//! let alice = PairSigner::<PolkadotConfig,_>::new(alice);
//!
//! // Or generate a keypair, here from an SURI:
//! let keypair = sp_core::sr25519::Pair::from_string("vessel ladder alter error federal sibling chat ability sun glass valve picture/0/1///Password", None)
//! .expect("valid URI");
//! let keypair = PairSigner::<PolkadotConfig,_>::new(keypair);
//! #
//! # // Test that these all impl Signer trait while we're here:
//! #
//! # fn is_subxt_signer(_signer: impl subxt::tx::Signer<PolkadotConfig>) {}
//! # is_subxt_signer(subxt_signer::sr25519::dev::alice());
//! # is_subxt_signer(subxt_signer::ecdsa::dev::alice());
//! # is_subxt_signer(PairSigner::<PolkadotConfig,_>::new(sp_keyring::AccountKeyring::Alice.pair()));
//! #[cfg(feature = "substrate-compat")]
//! {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ooh, putting the feature on a block is much cleaner than my suggesiton; good idea!

I'd suggest hiding these lines though (with a # prefix) and not indenting the rest, since (a) most of it doesn't require that feature and (b) we talk about needing it above anyway, so keeping the example code neater feels better :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@tadeohepperle this is the only thing left that I'd like to see done :)

//! use subxt::config::PolkadotConfig;
//! use std::str::FromStr;
//!
//! //// 1. Use a `subxt_signer` impl:
//! use subxt_signer::{ SecretUri, sr25519 };
//!
//! // Get hold of a `Signer` for a test account:
//! let alice = sr25519::dev::alice();
//!
//! // Or generate a keypair, here from an SURI:
//! let uri = SecretUri::from_str("vessel ladder alter error federal sibling chat ability sun glass valve picture/0/1///Password")
//! .expect("valid URI");
//! let keypair = sr25519::Keypair::from_uri(&uri)
//! .expect("valid keypair");
//!
//! //// 2. Use the corresponding `sp_core::Pair` impl:
//! use subxt::tx::PairSigner;
//! use sp_core::Pair;
//!
//! // Get hold of a `Signer` for a test account:
//! let alice = sp_keyring::AccountKeyring::Alice.pair();
//! let alice = PairSigner::<PolkadotConfig,_>::new(alice);
//!
//! // Or generate a keypair, here from an SURI:
//! let keypair = sp_core::sr25519::Pair::from_string("vessel ladder alter error federal sibling chat ability sun glass valve picture/0/1///Password", None)
//! .expect("valid URI");
//! let keypair = PairSigner::<PolkadotConfig,_>::new(keypair);
//! #
//! # // Test that these all impl Signer trait while we're here:
//! #
//! # fn is_subxt_signer(_signer: impl subxt::tx::Signer<PolkadotConfig>) {}
//! # is_subxt_signer(subxt_signer::sr25519::dev::alice());
//! # is_subxt_signer(subxt_signer::ecdsa::dev::alice());
//! # is_subxt_signer(PairSigner::<PolkadotConfig,_>::new(sp_keyring::AccountKeyring::Alice.pair()));
//! }
jsdw marked this conversation as resolved.
Show resolved Hide resolved
//! ```
//!
//! See the `subxt_signer` crate or the [`sp_core::Pair`] docs for more ways to construct
Expand Down
6 changes: 4 additions & 2 deletions subxt/src/client/online_client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,7 +468,7 @@ mod jsonrpsee_helpers {
}

// helpers for a jsonrpsee specific OnlineClient.
#[cfg(all(feature = "jsonrpsee", feature = "web"))]
#[cfg(all(feature = "jsonrpsee", feature = "web", target_arch = "wasm32"))]
Copy link
Collaborator

@jsdw jsdw Jul 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It might be a slightly better user experience to throw a compile_error if the target isn't wasm32 and jsonrpsee expects that, eg:

#[cfg(all(feature = "jsonrpsee", feature = "web"))]
mod jsonrpsee_helpers {
    #[cfg(not(target_arch = "wasm32")]
    compile_error!("when using jsonrpsee and web features, the target arch must be wasm32");

    // ...
}

So that users know what the issue is if things don't compile properly with these features.

It also self documents for future us why that target_arch is there :)

mod jsonrpsee_helpers {
pub use jsonrpsee::{
client_transport::web,
Expand All @@ -480,7 +480,9 @@ mod jsonrpsee_helpers {

/// Build web RPC client from URL
pub async fn client(url: &str) -> Result<Client, Error> {
let (sender, receiver) = web::connect(url).await.unwrap();
let (sender, receiver) = web::connect(url)
.await
.map_err(|e| Error::Transport(e.into()))?;
Ok(ClientBuilder::default()
.max_notifs_per_subscription(4096)
.build_with_wasm(sender, receiver))
Expand Down