Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Adds force_origin support #13845

Merged
merged 17 commits into from
Apr 24, 2023
Merged

Adds force_origin support #13845

merged 17 commits into from
Apr 24, 2023

Conversation

gupnik
Copy link
Contributor

@gupnik gupnik commented Apr 6, 2023

Fixes #13721

In benchmarking v1, we used to be able to do:

let force_origin =
	T::ForceEnterOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;

_<T::RuntimeOrigin>(force_origin)

With this change, the syntax in v2 would look like:

let force_origin =
	T::ForceEnterOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
#[extrinsic_call]
_(force_origin as T::RuntimeOrigin);

@gupnik
Copy link
Contributor Author

gupnik commented Apr 6, 2023

Will add an example soon

@gupnik gupnik requested a review from sam0x17 April 6, 2023 13:31
@gupnik gupnik added A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Apr 6, 2023
@gupnik gupnik requested review from kianenigma and ggwpez April 6, 2023 13:32
}

start_lottery {
/*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will move rest of them soon

@sam0x17
Copy link
Contributor

sam0x17 commented Apr 6, 2023

This looks good to me as long as it works with the existing examples. Love the simple as syntax.

Also in this PR we will want to add passing and failing UI tests (maybe choose a non-cast expr type that is also invalid with the existing syntax as one of the failing examples), and we will want the docs for benchmark v2 updated so people know about this syntax :)

@gupnik gupnik marked this pull request as ready for review April 7, 2023 16:09
@gupnik
Copy link
Contributor Author

gupnik commented Apr 7, 2023

bot fmt

@command-bot
Copy link

command-bot bot commented Apr 7, 2023

@gupnik https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2656974 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 2-fdef3bb0-fd2d-4154-8421-91bcdcceee08 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Apr 7, 2023

@gupnik Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2656974 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2656974/artifacts/download.

Copy link
Contributor

@sam0x17 sam0x17 left a comment

Choose a reason for hiding this comment

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

This looks good to me once everything is passing, good end-to-end example with the lottery pallet, and the negative UI test looks good

@@ -160,6 +160,12 @@ pub use v1::*;
/// The underscore will be substituted with the name of the benchmark (i.e. the name of the
/// function in the benchmark function definition).
///
/// In case of a `force_origin`, this is the general syntax:
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps a sentence or two explaining what force_origin means or a link to elsewhere in the docs where this is explained

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Please let me know if this works.

@sam0x17 sam0x17 self-requested a review April 22, 2023 12:36
@ggwpez
Copy link
Member

ggwpez commented Apr 22, 2023

Did you look at the CI output? Something with the Linear syntax does not seem to work together with config vars.

error[E0433]: failed to resolve: use of undeclared type `T`
     --> /builds/parity/mirrors/substrate/frame/lottery/src/benchmarking.rs:92:30
      |
   92 |     fn set_calls(n: Linear<0, { T::MaxCalls::get() }>) -> Result<(), BenchmarkError> {
      |                                 ^ use of undeclared type `T`

@sam0x17
Copy link
Contributor

sam0x17 commented Apr 22, 2023

Did you look at the CI output? Something with the Linear syntax does not seem to work together with config vars.

error[E0433]: failed to resolve: use of undeclared type `T`
     --> /builds/parity/mirrors/substrate/frame/lottery/src/benchmarking.rs:92:30
      |
   92 |     fn set_calls(n: Linear<0, { T::MaxCalls::get() }>) -> Result<(), BenchmarkError> {
      |                                 ^ use of undeclared type `T`

yeah I'm going back and forth helping him debug it and removed my review haha

@gupnik
Copy link
Contributor Author

gupnik commented Apr 22, 2023

Did you look at the CI output? Something with the Linear syntax does not seem to work together with config vars.

error[E0433]: failed to resolve: use of undeclared type `T`
     --> /builds/parity/mirrors/substrate/frame/lottery/src/benchmarking.rs:92:30
      |
   92 |     fn set_calls(n: Linear<0, { T::MaxCalls::get() }>) -> Result<(), BenchmarkError> {
      |                                 ^ use of undeclared type `T`

Yes, looking into this

@gupnik
Copy link
Contributor Author

gupnik commented Apr 22, 2023

bot fmt

@command-bot
Copy link

command-bot bot commented Apr 22, 2023

@gupnik https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2717436 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 50-2fa1f27e-16e9-4b8d-9cdb-63d00c52ad5c to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Apr 22, 2023

@gupnik Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2717436 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2717436/artifacts/download.

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

thanks looks good!
Could you still please extend the description a bit to show how the V1 syntax of this looked like?
I will try to migrate another pallet to see that it works there as well.

@gupnik gupnik merged commit 9370f42 into master Apr 24, 2023
@gupnik gupnik deleted the gupnik/force_origin branch April 24, 2023 12:04
gpestana pushed a commit that referenced this pull request May 4, 2023
* Adds force_origin support

* Moves a couple of tests to showcase v2 with force_origin

* Adds remaining tests

* adds documentation

* minor

* adds test for invalid origin

* ".git/.scripts/commands/fmt/fmt.sh"

* updates param to use MaxCalls

* Fixes compilation error

* Updates doc comment

* Fixes test outputs

* Fixes test output

* ".git/.scripts/commands/fmt/fmt.sh"

---------

Co-authored-by: command-bot <>
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* Adds force_origin support

* Moves a couple of tests to showcase v2 with force_origin

* Adds remaining tests

* adds documentation

* minor

* adds test for invalid origin

* ".git/.scripts/commands/fmt/fmt.sh"

* updates param to use MaxCalls

* Fixes compilation error

* Updates doc comment

* Fixes test outputs

* Fixes test output

* ".git/.scripts/commands/fmt/fmt.sh"

---------

Co-authored-by: command-bot <>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add _(force_origin) support to benchmarking v2
4 participants