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

Clone + Debug on Payloads/Addresses, and compare child storage results #1203

Merged
merged 9 commits into from
Oct 18, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
22 changes: 22 additions & 0 deletions subxt/src/constants/constant_address.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,28 @@ pub struct Address<ReturnTy> {
_marker: std::marker::PhantomData<ReturnTy>,
}

impl<ReturnTy> Clone for Address<ReturnTy> {
fn clone(&self) -> Self {
Self {
pallet_name: self.pallet_name.clone(),
constant_name: self.constant_name.clone(),
constant_hash: self.constant_hash,
_marker: self._marker,
}
}
}

impl<ReturnTy> std::fmt::Debug for Address<ReturnTy> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_struct("Address")
.field("pallet_name", &self.pallet_name)
.field("constant_name", &self.constant_name)
.field("constant_hash", &self.constant_hash)
.field("_marker", &self._marker)
.finish()
}
}

/// The type of address typically used to return dynamic constant values.
pub type DynamicAddress = Address<DecodedValueThunk>;

Expand Down
20 changes: 20 additions & 0 deletions subxt/src/custom_values/custom_value_address.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,26 @@ pub struct StaticAddress<ReturnTy, IsDecodable> {
phantom: PhantomData<(ReturnTy, IsDecodable)>,
}

impl<ReturnTy, IsDecodable> Clone for StaticAddress<ReturnTy, IsDecodable> {
Copy link
Member

Choose a reason for hiding this comment

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

Why is the manual Clone + Debug impls needed?

It should be possible to derive those I think

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because Clone/Debug should be possible regardless of some of the generic params, and the default derives assume that they all impl Clone/debug too.

Buttt, I've been avoiding using derivative and maybe I should just use that instead to automate these better!

Copy link
Member

Choose a reason for hiding this comment

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

that's fine,

Do you mean that PhantomData impl differs depending what the T resolves to? I think rustc can resolve that otherwise with PhantomData but maybe I don't understand :P

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 mean that if you have:

#[derive(Clone)]
struct Foo<T> {
    val: usize,
    marker: PhantomData<T>
}

Then the Clone impl will be like:

impl <T: Clone> Clone for Foo<T> { ... }

Even though it's irrelevant whether T impls Clone or not. So really we want:

impl <T> Clone for Foo<T> { ... }

I just wrote them manually but should probably switch these to using derivative to save some loc :)

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 switched to using derivative to derive these things instead to get rid of all of the boilerplate!

fn clone(&self) -> Self {
Self {
name: self.name,
hash: self.hash,
phantom: self.phantom,
}
}
}

impl<ReturnTy, IsDecodable> std::fmt::Debug for StaticAddress<ReturnTy, IsDecodable> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_struct("StaticAddress")
.field("name", &self.name)
.field("hash", &self.hash)
.field("phantom", &self.phantom)
.finish()
}
}

impl<ReturnTy, IsDecodable> StaticAddress<ReturnTy, IsDecodable> {
#[doc(hidden)]
/// Creates a new StaticAddress.
Expand Down
25 changes: 24 additions & 1 deletion subxt/src/runtime_api/runtime_payload.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ pub trait RuntimeApiPayload {
///
/// This can be created from static values (ie those generated
/// via the `subxt` macro) or dynamic values via [`dynamic`].
#[derive(Clone, Debug)]
pub struct Payload<ArgsData, ReturnTy> {
trait_name: Cow<'static, str>,
method_name: Cow<'static, str>,
Expand All @@ -74,6 +73,30 @@ pub struct Payload<ArgsData, ReturnTy> {
_marker: PhantomData<ReturnTy>,
}

impl<ArgsData: Clone, ReturnTy> Clone for Payload<ArgsData, ReturnTy> {
fn clone(&self) -> Self {
Self {
trait_name: self.trait_name.clone(),
method_name: self.method_name.clone(),
args_data: self.args_data.clone(),
validation_hash: self.validation_hash,
_marker: self._marker,
}
}
}

impl<ArgsData: std::fmt::Debug, ReturnTy> std::fmt::Debug for Payload<ArgsData, ReturnTy> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_struct("Payload")
.field("trait_name", &self.trait_name)
.field("method_name", &self.method_name)
.field("args_data", &self.args_data)
.field("validation_hash", &self.validation_hash)
.field("_marker", &self._marker)
.finish()
}
}

impl<ArgsData: EncodeAsFields, ReturnTy: DecodeWithMetadata> RuntimeApiPayload
for Payload<ArgsData, ReturnTy>
{
Expand Down
28 changes: 28 additions & 0 deletions subxt/src/storage/storage_address.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,34 @@ pub struct Address<StorageKey, ReturnTy, Fetchable, Defaultable, Iterable> {
_marker: std::marker::PhantomData<(ReturnTy, Fetchable, Defaultable, Iterable)>,
}

impl<StorageKey: Clone, ReturnTy, Fetchable, Defaultable, Iterable> Clone
for Address<StorageKey, ReturnTy, Fetchable, Defaultable, Iterable>
{
fn clone(&self) -> Self {
Self {
pallet_name: self.pallet_name.clone(),
entry_name: self.entry_name.clone(),
storage_entry_keys: self.storage_entry_keys.clone(),
validation_hash: self.validation_hash,
_marker: self._marker,
}
}
}

impl<StorageKey: std::fmt::Debug, ReturnTy, Fetchable, Defaultable, Iterable> std::fmt::Debug
for Address<StorageKey, ReturnTy, Fetchable, Defaultable, Iterable>
{
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_struct("Address")
.field("pallet_name", &self.pallet_name)
.field("entry_name", &self.entry_name)
.field("storage_entry_keys", &self.storage_entry_keys)
.field("validation_hash", &self.validation_hash)
.field("_marker", &self._marker)
.finish()
}
}

/// A typical storage address constructed at runtime rather than via the `subxt` macro; this
/// has no restriction on what it can be used for (since we don't statically know).
pub type DynamicAddress<StorageKey> = Address<StorageKey, DecodedValueThunk, Yes, Yes, Yes>;
Expand Down
40 changes: 40 additions & 0 deletions testing/integration-tests/src/full_client/client/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,46 @@ async fn storage_iter() {
assert_eq!(len, 13);
}

#[tokio::test]
async fn storage_child_values_same_across_backends() {
let ctx = test_context().await;

let unstable_client = ctx.unstable_client().await;
let legacy_client = ctx.legacy_client().await;

let addr = node_runtime::storage().system().account_iter();
let block_ref = legacy_client
.blocks()
.at_latest()
.await
.unwrap()
.reference();

let a: Vec<_> = unstable_client
.storage()
.at(block_ref.clone())
.iter(addr.clone())
.await
.unwrap()
.collect()
.await;
let b: Vec<_> = legacy_client
.storage()
.at(block_ref.clone())
.iter(addr)
.await
.unwrap()
.collect()
.await;

for (a, b) in a.into_iter().zip(b.into_iter()) {
let a = a.unwrap();
let b = b.unwrap();

assert_eq!(a, b);
}
}

#[tokio::test]
async fn transaction_validation() {
let ctx = test_context().await;
Expand Down
99 changes: 75 additions & 24 deletions testing/integration-tests/src/utils/node_proc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// This file is dual-licensed as Apache-2.0 or GPL-3.0.
// see LICENSE for license details.

use std::cell::RefCell;
use std::ffi::{OsStr, OsString};
use std::sync::Arc;
use substrate_runner::SubstrateNode;
Expand All @@ -18,6 +19,12 @@ pub struct TestNodeProcess<R: Config> {
// Keep a handle to the node; once it's dropped the node is killed.
proc: SubstrateNode,

// Lazily construct these when asked for,
unstable_client: RefCell<Option<OnlineClient<R>>>,
legacy_client: RefCell<Option<OnlineClient<R>>>,

rpc_client: rpc::RpcClient,

#[cfg(not(feature = "unstable-light-client"))]
client: OnlineClient<R>,

Expand Down Expand Up @@ -49,14 +56,42 @@ where
unstable::UnstableRpcMethods::new(rpc_client)
}

async fn rpc_client(&self) -> rpc::RpcClient {
/// Hand back an RPC client connected to the test node.
pub async fn rpc_client(&self) -> rpc::RpcClient {
let url = format!("ws://127.0.0.1:{}", self.proc.ws_port());
rpc::RpcClient::from_url(url)
.await
.expect("Unable to connect RPC client to test node")
}

/// Returns the subxt client connected to the running node.
/// Always return a client using the unstable backend.
/// Only use for comparing backends; use [`TestNodeProcess::client()`] normally,
/// which enables us to run each test against both backends.
pub async fn unstable_client(&self) -> OnlineClient<R> {
if self.unstable_client.borrow().is_none() {
Copy link
Member

Choose a reason for hiding this comment

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

Not the prettiest code I have seen but works I guess.

I guess you can't use borrow_mut here because it can panic.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was just using RefCell as a silly internal cache because Mutex wasn't needed really, so in tests you can ask for one of the clients and you'll get one back or it'll make one for you and give that back if it needs to :)

let c = build_unstable_client(self.rpc_client.clone())
.await
.unwrap();
self.unstable_client.replace(Some(c));
}
self.unstable_client.borrow().as_ref().unwrap().clone()
}

/// Always return a client using the legacy backend.
/// Only use for comparing backends; use [`TestNodeProcess::client()`] normally,
/// which enables us to run each test against both backends.
pub async fn legacy_client(&self) -> OnlineClient<R> {
if self.legacy_client.borrow().is_none() {
let c = build_legacy_client(self.rpc_client.clone()).await.unwrap();
self.legacy_client.replace(Some(c));
}
self.legacy_client.borrow().as_ref().unwrap().clone()
}

/// Returns the subxt client connected to the running node. This client
/// will use the legacy backend by default or the unstable backend if the
/// "unstable-backend-client" feature is enabled, so that we can run each
/// test against both.
#[cfg(not(feature = "unstable-light-client"))]
pub fn client(&self) -> OnlineClient<R> {
self.client.clone()
Expand Down Expand Up @@ -115,36 +150,57 @@ impl TestNodeProcessBuilder {
// Spawn the node and retrieve a URL to it:
let proc = node_builder.spawn().map_err(|e| e.to_string())?;
let ws_url = format!("ws://127.0.0.1:{}", proc.ws_port());
let rpc_client = build_rpc_client::<R>(&ws_url)
.await
.map_err(|e| format!("Failed to connect to node at {ws_url}: {e}"))?;

// Cache whatever client we build, and None for the other.
#[allow(unused_assignments, unused_mut)]
let mut unstable_client = None;
#[allow(unused_assignments, unused_mut)]
let mut legacy_client = None;

#[cfg(feature = "unstable-light-client")]
let client = build_light_client(&proc).await;

#[cfg(feature = "unstable-backend-client")]
let client = build_unstable_client(&proc).await;
let client = {
let client = build_unstable_client(rpc_client.clone()).await?;
unstable_client = Some(client.clone());
client
};

#[cfg(all(
not(feature = "unstable-light-client"),
not(feature = "unstable-backend-client")
))]
let client = build_legacy_client(&proc).await;

match client {
Ok(client) => Ok(TestNodeProcess { proc, client }),
Err(err) => Err(format!("Failed to connect to node rpc at {ws_url}: {err}")),
}
let client = {
let client = build_legacy_client(rpc_client.clone()).await?;
legacy_client = Some(client.clone());
client
};

Ok(TestNodeProcess {
proc,
client,
legacy_client: RefCell::new(legacy_client),
unstable_client: RefCell::new(unstable_client),
rpc_client,
})
}
}

#[cfg(all(
not(feature = "unstable-light-client"),
not(feature = "unstable-backend-client")
))]
async fn build_legacy_client<T: Config>(proc: &SubstrateNode) -> Result<OnlineClient<T>, String> {
let ws_url = format!("ws://127.0.0.1:{}", proc.ws_port());

async fn build_rpc_client<T: Config>(ws_url: &str) -> Result<rpc::RpcClient, String> {
let rpc_client = rpc::RpcClient::from_url(ws_url)
.await
.map_err(|e| format!("Cannot construct RPC client: {e}"))?;

Ok(rpc_client)
}

async fn build_legacy_client<T: Config>(
rpc_client: rpc::RpcClient,
) -> Result<OnlineClient<T>, String> {
let backend = legacy::LegacyBackend::new(rpc_client);
let client = OnlineClient::from_backend(Arc::new(backend))
.await
Expand All @@ -153,14 +209,9 @@ async fn build_legacy_client<T: Config>(proc: &SubstrateNode) -> Result<OnlineCl
Ok(client)
}

#[cfg(feature = "unstable-backend-client")]
async fn build_unstable_client<T: Config>(proc: &SubstrateNode) -> Result<OnlineClient<T>, String> {
let ws_url = format!("ws://127.0.0.1:{}", proc.ws_port());

let rpc_client = rpc::RpcClient::from_url(ws_url)
.await
.map_err(|e| format!("Cannot construct RPC client: {e}"))?;

async fn build_unstable_client<T: Config>(
rpc_client: rpc::RpcClient,
) -> Result<OnlineClient<T>, String> {
let (backend, mut driver) = unstable::UnstableBackend::builder().build(rpc_client);

// The unstable backend needs driving:
Expand Down