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

Do not make pallet-nfts benchmarks signature-dependent #4756

Merged
merged 8 commits into from
Jun 21, 2024

Conversation

Moliholy
Copy link
Contributor

@Moliholy Moliholy commented Jun 11, 2024

This PR:

  • Adds extra functionality to pallet-nfts's BenchmarkHelper to provide signers and sign message.
  • Abstracts away the explicit link with Sr25519 schema in the benchmarks, allowing parachains with a different one to be able to run them and calculate the weights.
  • Adds a default implementation for the empty tuple that leaves the code equivalent.

@ggwpez ggwpez added the T2-pallets This PR/Issue is related to a particular pallet. label Jun 11, 2024
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

where_clause {
where
T::OffchainSignature: From<MultiSignature>,
T::AccountId: From<AccountId32>,
}
this should also be removed.

Comment on lines +98 to +100
sp_runtime::MultiSigner,
sp_runtime::AccountId32,
sp_runtime::MultiSignature,
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. This is not a good solution. This way () will not work for any runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Current behaviour is that it builds but fails to execute the benchmarks. In my opinion it's already an improvement that it does not build, so that you can take care of the error instead of thinking "benchmarks will be fine because it compiles".

@Moliholy
Copy link
Contributor Author

this should also be removed.

Nice catch. Done in 54b2397.

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6491090

@bkchr bkchr added this pull request to the merge queue Jun 21, 2024
Merged via the queue into paritytech:master with commit 2657cfb Jun 21, 2024
157 of 158 checks passed
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
This PR:

- Adds extra functionality to pallet-nfts's `BenchmarkHelper` to provide
signers and sign message.
- Abstracts away the explicit link with Sr25519 schema in the
benchmarks, allowing parachains with a different one to be able to run
them and calculate the weights.
- Adds a default implementation for the empty tuple that leaves the code
equivalent.
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T2-pallets This PR/Issue is related to a particular pallet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants