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

Make storage futures only borrow client, not self, for better ergonomics #561

Merged
merged 7 commits into from
Jun 14, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
89 changes: 59 additions & 30 deletions codegen/src/api/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -240,12 +240,6 @@ fn generate_storage_entry_fns(
}
};

let (lifetime_param, reference, anon_lifetime) = if should_ref {
(quote!(<'a>), quote!(&), quote!(<'_>))
} else {
(quote!(), quote!(), quote!())
};

let storage_entry_impl = quote! (
const PALLET: &'static str = #pallet_name;
const STORAGE: &'static str = #storage_name;
Expand All @@ -255,6 +249,10 @@ fn generate_storage_entry_fns(
}
);

let anon_lifetime = match should_ref {
true => quote!(<'_>),
false => quote!(),
};
let storage_entry_type = quote! {
#entry_struct
impl ::subxt::StorageEntry for #entry_struct_ident #anon_lifetime {
Expand All @@ -264,29 +262,49 @@ fn generate_storage_entry_fns(

let docs = &storage_entry.docs;
let docs_token = quote! { #( #[doc = #docs ] )* };

let lifetime_param = match should_ref {
true => quote!(<'a>),
false => quote!(),
};
let client_iter_fn = if matches!(storage_entry.ty, StorageEntryType::Map { .. }) {
quote! (
#docs_token
pub async fn #fn_name_iter(
pub fn #fn_name_iter(
&self,
block_hash: ::core::option::Option<T::Hash>,
) -> ::core::result::Result<::subxt::KeyIter<'a, T, #entry_struct_ident #lifetime_param>, ::subxt::BasicError> {
let runtime_storage_hash = {
let locked_metadata = self.client.metadata();
let metadata = locked_metadata.read();
metadata.storage_hash::<#entry_struct_ident>()?
};
if runtime_storage_hash == [#(#storage_hash,)*] {
self.client.storage().iter(block_hash).await
} else {
Err(::subxt::MetadataError::IncompatibleMetadata.into())
) -> impl ::core::future::Future<
Output = ::core::result::Result<::subxt::KeyIter<'a, T, #entry_struct_ident #lifetime_param>, ::subxt::BasicError>
> + 'a {
// Instead of an async fn which borrows all of self,
// we make sure that the returned future only borrows
// client, which allows you to chain calls a little better.
let client = self.client;
async move {
let runtime_storage_hash = {
let locked_metadata = client.metadata();
let metadata = locked_metadata.read();
match metadata.storage_hash::<#entry_struct_ident>() {
Ok(hash) => hash,
Err(e) => return Err(e.into())
}
};
if runtime_storage_hash == [#(#storage_hash,)*] {
client.storage().iter(block_hash).await
} else {
Err(::subxt::MetadataError::IncompatibleMetadata.into())
}
}
}
)
} else {
quote!()
};

let key_args_ref = match should_ref {
true => quote!(&'a),
false => quote!(),
};
let key_args = fields.iter().map(|(field_name, field_type)| {
// The field type is translated from `std::vec::Vec<T>` to `[T]`, if the
// interface should generate a reference. In such cases, the vector ultimately is
Expand All @@ -295,26 +313,37 @@ fn generate_storage_entry_fns(
Some(ty) if should_ref => quote!([#ty]),
_ => quote!(#field_type),
};
quote!( #field_name: #reference #field_ty )
quote!( #field_name: #key_args_ref #field_ty )
});

let client_fns = quote! {
#docs_token
pub async fn #fn_name(
pub fn #fn_name(
&self,
#( #key_args, )*
Copy link
Collaborator Author

@jsdw jsdw Jun 10, 2022

Choose a reason for hiding this comment

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

Note to self; key args have a lifetime of 'a, which is the same as the lifetime of the client struct.

Should key args have a lifetime of 'b, and the returned future rely on both 'a and 'b, to avoid any unnecessary restrictions on how the future is used in conjunction with these references?

It would be good to test and see whether a single bound is overly restrictive (as well as to test that we can create and later use futures so that we confirm it's all good in one place.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did a little testing and it seems OK to use the 'a lifetime for key_args and client; from reading around I think Rust will pick the correct smallest lifetime that all params with 'a use anyway, so no need to add other lifetimes.

block_hash: ::core::option::Option<T::Hash>,
) -> ::core::result::Result<#return_ty, ::subxt::BasicError> {
let runtime_storage_hash = {
let locked_metadata = self.client.metadata();
let metadata = locked_metadata.read();
metadata.storage_hash::<#entry_struct_ident>()?
};
if runtime_storage_hash == [#(#storage_hash,)*] {
let entry = #constructor;
self.client.storage().#fetch(&entry, block_hash).await
} else {
Err(::subxt::MetadataError::IncompatibleMetadata.into())
) -> impl ::core::future::Future<
Output = ::core::result::Result<#return_ty, ::subxt::BasicError>
> + 'a {
// Instead of an async fn which borrows all of self,
// we make sure that the returned future only borrows
// client, which allows you to chain calls a little better.
let client = self.client;
async move {
let runtime_storage_hash = {
let locked_metadata = client.metadata();
let metadata = locked_metadata.read();
match metadata.storage_hash::<#entry_struct_ident>() {
Ok(hash) => hash,
Err(e) => return Err(e.into())
}
};
if runtime_storage_hash == [#(#storage_hash,)*] {
let entry = #constructor;
client.storage().#fetch(&entry, block_hash).await
} else {
Err(::subxt::MetadataError::IncompatibleMetadata.into())
}
}
}

Expand Down
46 changes: 46 additions & 0 deletions examples/examples/concurrent_storage_requests.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// Copyright 2019-2022 Parity Technologies (UK) Ltd.
// This file is part of subxt.
//
// subxt is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// subxt is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.
//
// You should have received a copy of the GNU General Public License
// along with subxt. If not, see <http://www.gnu.org/licenses/>.

use futures::join;
use sp_keyring::AccountKeyring;
use subxt::{
ClientBuilder,
DefaultConfig,
PolkadotExtrinsicParams,
};

#[subxt::subxt(runtime_metadata_path = "../artifacts/polkadot_metadata.scale")]
pub mod polkadot {}

#[tokio::main]
async fn main() -> Result<(), Box<dyn std::error::Error>> {
let api = ClientBuilder::new()
.build()
.await?
.to_runtime_api::<polkadot::RuntimeApi<DefaultConfig, PolkadotExtrinsicParams<DefaultConfig>>>();

let addr = AccountKeyring::Bob.to_account_id();

// For storage requests, we can join futures together to
// await multiple futures concurrently:
let a_fut = api.storage().staking().bonded(&addr, None);
let b_fut = api.storage().staking().ledger(&addr, None);
let (a, b) = join!(a_fut, b_fut);

println!("{a:?}, {b:?}");

Ok(())
}