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

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

Merged
merged 9 commits into from
Feb 13, 2024

Conversation

lexnv
Copy link
Collaborator

@lexnv lexnv commented Feb 12, 2024

This PR implements multiple std::cmp traits for either subxt payloads to addresses:

  • Eq
  • Ord
  • PartialEq
  • PartialOrd

The traits are only implemented if the generic parameter of those objects implements the said traits.

This offers users the flexibility to use the subxt payloads in combination with custom_derives for the subxt macro. This makes the subxt more flexible (eg users could compare multiple tx::Payload<X> when X: PartialEq).

Closes #1422

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv requested a review from a team as a code owner February 12, 2024 12:18
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

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

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)
                        }
                    },
                }
            }

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Comment on lines +54 to +58
impl<ReturnTy, IsDecodable> PartialOrd for StaticAddress<ReturnTy, IsDecodable> {
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
Some(self.cmp(other))
}
}
Copy link
Collaborator

@jsdw jsdw Feb 12, 2024

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!

Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

LGTM offhand!

Shame about the manual PartialOrd impls just to avoid a clippy lint but oh well; might be worth a comment on that impl just to say why it's there :)

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
subxt/src/backend/mod.rs Outdated Show resolved Hide resolved
@lexnv lexnv merged commit 25ff4ec into master Feb 13, 2024
12 checks passed
@lexnv lexnv deleted the lexnv/paylaod-pq branch February 13, 2024 10:06
tadeohepperle pushed a commit that referenced this pull request Feb 13, 2024
* subxt/tx: Derive std::cmp traits

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* subxt/runtime_api: Derive std::cmp traits

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* subxt/constants: Derive std::cmp traits

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* subxt/custom_values: Derive std::cmp traits

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* subxt/storage: Derive std::cmp traits

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* subxt: Fix non_canonical_partial_ord_impl clippy introduced in 1.73

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* subxt: Add comment wrt derivative issue that triggers clippy warning

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* Update subxt/src/backend/mod.rs

* Update subxt/src/constants/constant_address.rs

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
tadeohepperle added a commit that referenced this pull request Mar 6, 2024
* first iteration on storage multi keys

* decoding values from concat style hashers

* move util functions and remove comments

* change codegen for storage keys and fix examples

* trait bounds don't match scale value...

* fix trait bounds and examples

* reconstruct storage keys in iterations

* build(deps): bump js-sys from 0.3.67 to 0.3.68 (#1428)

Bumps [js-sys](https://github.com/rustwasm/wasm-bindgen) from 0.3.67 to 0.3.68.
- [Release notes](https://github.com/rustwasm/wasm-bindgen/releases)
- [Changelog](https://github.com/rustwasm/wasm-bindgen/blob/main/CHANGELOG.md)
- [Commits](https://github.com/rustwasm/wasm-bindgen/commits)

---
updated-dependencies:
- dependency-name: js-sys
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps): bump clap from 4.4.18 to 4.5.0 (#1427)

Bumps [clap](https://github.com/clap-rs/clap) from 4.4.18 to 4.5.0.
- [Release notes](https://github.com/clap-rs/clap/releases)
- [Changelog](https://github.com/clap-rs/clap/blob/master/CHANGELOG.md)
- [Commits](clap-rs/clap@v4.4.18...clap_complete-v4.5.0)

---
updated-dependencies:
- dependency-name: clap
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps): bump either from 1.9.0 to 1.10.0 (#1425)

Bumps [either](https://github.com/rayon-rs/either) from 1.9.0 to 1.10.0.
- [Commits](rayon-rs/either@1.9.0...1.10.0)

---
updated-dependencies:
- dependency-name: either
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps): bump thiserror from 1.0.56 to 1.0.57 (#1424)

Bumps [thiserror](https://github.com/dtolnay/thiserror) from 1.0.56 to 1.0.57.
- [Release notes](https://github.com/dtolnay/thiserror/releases)
- [Commits](dtolnay/thiserror@1.0.56...1.0.57)

---
updated-dependencies:
- dependency-name: thiserror
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps): bump jsonrpsee from 0.21.0 to 0.22.0 (#1426)

* build(deps): bump jsonrpsee from 0.21.0 to 0.22.0

Bumps [jsonrpsee](https://github.com/paritytech/jsonrpsee) from 0.21.0 to 0.22.0.
- [Release notes](https://github.com/paritytech/jsonrpsee/releases)
- [Changelog](https://github.com/paritytech/jsonrpsee/blob/master/CHANGELOG.md)
- [Commits](paritytech/jsonrpsee@v0.21.0...v0.22.0)

---
updated-dependencies:
- dependency-name: jsonrpsee
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* Update Cargo.lock

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: James Wilson <james@jsdw.me>
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>

* subxt: Derive `std::cmp` traits for subxt payloads and addresses (#1429)

* subxt/tx: Derive std::cmp traits

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* subxt/runtime_api: Derive std::cmp traits

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* subxt/constants: Derive std::cmp traits

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* subxt/custom_values: Derive std::cmp traits

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* subxt/storage: Derive std::cmp traits

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* subxt: Fix non_canonical_partial_ord_impl clippy introduced in 1.73

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* subxt: Add comment wrt derivative issue that triggers clippy warning

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* Update subxt/src/backend/mod.rs

* Update subxt/src/constants/constant_address.rs

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>

* fix clippy

* add integration tests

* fix doc tests

* change hashing logic for hashers=1

* refactor

* clippy and fmt

* regenerate polkadot file which got changed by the automatic PR

* nested design for storage keys

* refactor codegen

* codegen adjustments

* fix storage hasher codegen test

* Suggestions for storage value decoding (#1457)

* Storage decode tweaks

* doc tweak

* more precise error when leftover or not enough bytes

* integrate nits from PR

* add fuzztest for storage keys, fix decoding bug

* clippy and fmt

* clippy

* Niklas Suggestions

* lifetime issues and iterator impls

* fmt and clippy

* regenerate polkadot.rs

* fix storage key encoding for empty keys

* rename trait methods for storage keys

* fix hasher bug...

* impl nits, add iterator struct seperate from `StorageHashers`

* clippy fix

* remove println

---------

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: James Wilson <james@jsdw.me>
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
Co-authored-by: Alexandru Vasile <60601340+lexnv@users.noreply.github.com>
@jsdw jsdw mentioned this pull request Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PartialEq is not implemented in Payload
3 participants