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

Jump to bottom

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

Merged
merged 8 commits into from
Jun 21, 2024
Merged

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

merged 8 commits into from
Jun 21, 2024

Conversation

Moliholy
Copy link
Contributor

@Moliholy Moliholy commented Jun 11, 2024 •
edited
Loading

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
bkchr
bkchr reviewed Jun 17, 2024
View reviewed changes
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.

polkadot-sdk/substrate/frame/nfts/src/benchmarking.rs

Lines 232 to 236 in ad86209

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

substrate/frame/nfts/src/lib.rs
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
@Moliholy @TarekkMA
Do not make pallet-nfts benchmarks signature-dependent (paritytech#4756)
4836bf9 
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 was referenced Aug 21, 2024
Closed
Closed
@TDemeco TDemeco mentioned this pull request Oct 7, 2024
Merged
sfffaaa pushed a commit to peaqnetwork/polkadot-sdk that referenced this pull request Dec 27, 2024
@Moliholy
Do not make pallet-nfts benchmarks signature-dependent (paritytech#4756)
97dc33b 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ggwpez ggwpez ggwpez approved these changes

@bkchr bkchr bkchr approved these changes

Assignees
No one assigned
Labels
T2-pallets This PR/Issue is related to a particular pallet.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Moliholy @paritytech-cicd-pr @bkchr @ggwpez