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

subxt: Derive std::cmp traits for subxt payloads and addresses #1429

Merged
merged 9 commits into from
Feb 13, 2024
Merged

subxt: Derive std::cmp traits for subxt payloads and addresses #1429

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
98c9ab7
subxt/tx: Derive std::cmp traits
lexnv Feb 12, 2024
271e881
subxt/runtime_api: Derive std::cmp traits
lexnv Feb 12, 2024
db2fac3
subxt/constants: Derive std::cmp traits
lexnv Feb 12, 2024
329bab9
subxt/custom_values: Derive std::cmp traits
lexnv Feb 12, 2024
890c051
subxt/storage: Derive std::cmp traits
lexnv Feb 12, 2024
eed11c5
subxt: Fix non_canonical_partial_ord_impl clippy introduced in 1.73
lexnv Feb 12, 2024
e4b5959
subxt: Add comment wrt derivative issue that triggers clippy warning
lexnv Feb 13, 2024
3699a90
Update subxt/src/backend/mod.rs
niklasad1 Feb 13, 2024
72ea9be
Update subxt/src/constants/constant_address.rs
niklasad1 Feb 13, 2024
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
1 change: 1 addition & 0 deletions subxt/src/backend/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ impl<H: PartialEq> PartialEq for BlockRef<H> {
}
impl<H: Eq> Eq for BlockRef<H> {}

// Manual implementation to work aroud https://github.com/mcarton/rust-derivative/issues/115.
impl<H: PartialOrd> PartialOrd for BlockRef<H> {
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
self.hash.partial_cmp(&other.hash)
Expand Down
15 changes: 14 additions & 1 deletion subxt/src/constants/constant_address.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,27 @@ pub trait ConstantAddress {

/// This represents the address of a constant.
#[derive(Derivative)]
#[derivative(Clone(bound = ""), Debug(bound = ""))]
#[derivative(
Clone(bound = ""),
Debug(bound = ""),
Eq(bound = ""),
Ord(bound = ""),
PartialEq(bound = "")
)]
pub struct Address<ReturnTy> {
pallet_name: Cow<'static, str>,
constant_name: Cow<'static, str>,
constant_hash: Option<[u8; 32]>,
_marker: std::marker::PhantomData<ReturnTy>,
}

// Manual implementation to work aroud https://github.com/mcarton/rust-derivative/issues/115.
impl<ReturnTy> PartialOrd for Address<ReturnTy> {
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
Some(self.cmp(other))
}
}

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

Expand Down
14 changes: 13 additions & 1 deletion subxt/src/custom_values/custom_value_address.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,25 @@ pub struct Yes;

/// A static address to a custom value.
#[derive(Derivative)]
#[derivative(Clone(bound = ""), Debug(bound = ""))]
#[derivative(
Clone(bound = ""),
Debug(bound = ""),
Eq(bound = ""),
Ord(bound = ""),
PartialEq(bound = "")
)]
pub struct StaticAddress<ReturnTy, IsDecodable> {
name: &'static str,
hash: Option<[u8; 32]>,
phantom: PhantomData<(ReturnTy, IsDecodable)>,
}

impl<ReturnTy, IsDecodable> PartialOrd for StaticAddress<ReturnTy, IsDecodable> {
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
Some(self.cmp(other))
}
}
Comment on lines +54 to +58
Copy link
Collaborator

@jsdw jsdw Feb 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Interesting; it sortof looks like derivative would do this one too (https://docs.rs/derivative/latest/src/derivative/lib.rs.html#61) already, but I guess you ran into an issue using it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For this avoid the clippy warning, the derivative crate would need to take into account the suggestion from https://rust-lang.github.io/rust-clippy/master/index.html#/non_canonical_partial_ord_impl.
Which is to delegate the PartialOrd::partial_cmp implementation, to the Ord::cmp; I don't think that it does that by the looks of those functions.

Probably the clippy rule has been added for implementations without bounds, not sure why it wasn't triggered for our other derives.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Aah ok, basically this issue: mcarton/rust-derivative#115

Probably needs derivative to emit that clippy allow in the relevant place!


impl<ReturnTy, IsDecodable> StaticAddress<ReturnTy, IsDecodable> {
#[doc(hidden)]
/// Creates a new StaticAddress.
Expand Down
6 changes: 5 additions & 1 deletion subxt/src/runtime_api/runtime_payload.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,11 @@ pub trait RuntimeApiPayload {
#[derive(Derivative)]
#[derivative(
Clone(bound = "ArgsData: Clone"),
Debug(bound = "ArgsData: std::fmt::Debug")
Debug(bound = "ArgsData: std::fmt::Debug"),
Eq(bound = "ArgsData: std::cmp::Eq"),
Ord(bound = "ArgsData: std::cmp::Ord"),
PartialEq(bound = "ArgsData: std::cmp::PartialEq"),
Copy link
Member

@niklasad1 niklasad1 Feb 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Does this generate:

impl<T: Eq> for Payload<..> {}
impl<T: PartialEq> for Payload<..> {}
impl<T: Ord> for Payload<..> {}

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 does seem to generate that for the Eq (using cargo expand)

  • impl<ArgsData, ReturnTy> ::std::cmp::Eq for Payload<ArgsData, ReturnTy> where ArgsData: std::cmp::Eq {}.

However, for the others it does match the fields 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

However, for the others it does match the fields

What do you mean here? I'd be curious to see the generated output :)

Copy link
Collaborator Author

@lexnv lexnv Feb 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is from the address of the constants:

impl<ReturnTy> ::std::cmp::PartialEq for Address<ReturnTy> {
            fn eq(&self, other: &Self) -> bool {
                true && match *self {
                    Address {
                        pallet_name: ref __self_0,
                        constant_name: ref __self_1,
                        constant_hash: ref __self_2,
                        _marker: ref __self_3,
                    } => match *other {
                        Address {
                            pallet_name: ref __other_0,
                            constant_name: ref __other_1,
                            constant_hash: ref __other_2,
                            _marker: ref __other_3,
                        } => {
                            true && &(*__self_0) == &(*__other_0)
                                && &(*__self_1) == &(*__other_1)
                                && &(*__self_2) == &(*__other_2)
                                && &(*__self_3) == &(*__other_3)
                        }
                    },
                }
            }
        }

I wanted to say that it generates the expected output. I guess I was a bit confused by the impl<T: PartialEq> for Payload<..> {}, which should have an fn eq(..) but after reading it again, I think Niklas was asking more about the bounds

Copy link
Collaborator

Choose a reason for hiding this comment

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

^ that's a different type from where this comment is which confused me for a bit; the expanded output for this one should have the bounds like <ArgsData: std::cmp::partialEq> etc :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, yep this is the PartialEq for this:

        impl<ArgsData, ReturnTy> ::std::cmp::PartialEq for Payload<ArgsData, ReturnTy>
        where
            ArgsData: std::cmp::PartialEq,
        {
            fn eq(&self, other: &Self) -> bool {
                true && match *self {
                    Payload {
                        trait_name: ref __self_0,
                        method_name: ref __self_1,
                        args_data: ref __self_2,
                        validation_hash: ref __self_3,
                        _marker: ref __self_4,
                    } => match *other {
                        Payload {
                            trait_name: ref __other_0,
                            method_name: ref __other_1,
                            args_data: ref __other_2,
                            validation_hash: ref __other_3,
                            _marker: ref __other_4,
                        } => {
                            true && &(*__self_0) == &(*__other_0)
                                && &(*__self_1) == &(*__other_1)
                                && &(*__self_2) == &(*__other_2)
                                && &(*__self_3) == &(*__other_3)
                                && &(*__self_4) == &(*__other_4)
                        }
                    },
                }
            }

PartialOrd(bound = "ArgsData: std::cmp::PartialOrd")
)]
pub struct Payload<ArgsData, ReturnTy> {
trait_name: Cow<'static, str>,
Expand Down
6 changes: 5 additions & 1 deletion subxt/src/storage/storage_address.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,11 @@ pub struct Yes;
#[derive(Derivative)]
#[derivative(
Clone(bound = "StorageKey: Clone"),
Debug(bound = "StorageKey: std::fmt::Debug")
Debug(bound = "StorageKey: std::fmt::Debug"),
Eq(bound = "StorageKey: std::cmp::Eq"),
Ord(bound = "StorageKey: std::cmp::Ord"),
PartialEq(bound = "StorageKey: std::cmp::PartialEq"),
PartialOrd(bound = "StorageKey: std::cmp::PartialOrd")
)]
pub struct Address<StorageKey, ReturnTy, Fetchable, Defaultable, Iterable> {
pallet_name: Cow<'static, str>,
Expand Down
11 changes: 10 additions & 1 deletion subxt/src/tx/tx_payload.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use crate::{
metadata::Metadata,
};
use codec::Encode;
use derivative::Derivative;
use scale_encode::EncodeAsFields;
use scale_value::{Composite, ValueDef, Variant};
use std::{borrow::Cow, sync::Arc};
Expand Down Expand Up @@ -48,7 +49,15 @@ pub struct ValidationDetails<'a> {
}

/// A transaction payload containing some generic `CallData`.
#[derive(Clone, Debug)]
#[derive(Derivative)]
#[derivative(
Clone(bound = "CallData: Clone"),
Debug(bound = "CallData: std::fmt::Debug"),
Eq(bound = "CallData: std::cmp::Eq"),
Ord(bound = "CallData: std::cmp::Ord"),
PartialEq(bound = "CallData: std::cmp::PartialEq"),
PartialOrd(bound = "CallData: std::cmp::PartialOrd")
)]
pub struct Payload<CallData> {
pallet_name: Cow<'static, str>,
call_name: Cow<'static, str>,
Expand Down
Loading