-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Conversation
Let me know if this makes sense and then I can do the polkadot companion. |
To me it looks fine. But I don't feel comfortable signing off on this. I requested reviews from the FRAME experts. |
On the surface this seems reasonable but I think it makes more sense to have this in its own minimal pallet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no issues with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we have the new call have a proper benchmark ?
I think even if it straightforward it a better practice than having some hardcoded weight in the pallet.
EDIT: otherwise code is good to me
/benchmark runtime pallet pallet_utility |
Benchmark Runtime Pallet for branch "dispatch-as" with command cargo run --quiet --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_utility --extrinsic="*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/utility/src/weights.rs --template=./.maintain/frame-weight-template.hbs Results
|
…path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_utility --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/utility/src/weights.rs --template=./.maintain/frame-weight-template.hbs
})] | ||
pub fn dispatch_as( | ||
origin: OriginFor<T>, | ||
as_origin: Box<T::PalletsOrigin>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to box it? It wasn't boxed before you added the benchmark. We should solve it there when possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is generally good idea to box big data structure in call parameter to avoid increase the Call enum size. the same reason box is used elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I thought it was related to the benchmarks. Didn't know that this is a big argument, though.
bot merge |
Error: Response error (status 404 Not Found):
|
bot merge |
Error: Response error (status 404 Not Found):
|
bot merge |
Error: Response error (status 404 Not Found):
|
Ummm... Is there some bots end up in an infinite loop or something? |
@xlc we are looking into it. We can merge manually if we need to, but better to let the bot engineers figure out what is going on here. |
No problems. This isn’t a blocking issue for us so take your time. |
bot merge |
Error: Github API says paritytech/polkadot#4075 is not mergeable |
bot merge |
Error: Github API says paritytech/polkadot#4075 is not mergeable |
* implement dispatch_as * fix * fix * weight for dispatch_as * cargo run --quiet --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_utility --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/utility/src/weights.rs --template=./.maintain/frame-weight-template.hbs * fix * Update frame/utility/src/benchmarking.rs Co-authored-by: Alexander Theißen <alex.theissen@me.com> * fix issues Co-authored-by: Parity Bot <admin@parity.io> Co-authored-by: Alexander Theißen <alex.theissen@me.com> Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
* implement dispatch_as * fix * fix * weight for dispatch_as * cargo run --quiet --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_utility --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/utility/src/weights.rs --template=./.maintain/frame-weight-template.hbs * fix * Update frame/utility/src/benchmarking.rs Co-authored-by: Alexander Theißen <alex.theissen@me.com> * fix issues Co-authored-by: Parity Bot <admin@parity.io> Co-authored-by: Alexander Theißen <alex.theissen@me.com> Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
Closes #6428
I am not going to add any additional types to frame-system until paritytech/polkadot-sdk#367 is addressed.
I am go for the most generalized approach so root can not only dispatch as signed origin, but also all other kinds of origin including collectives etc, which may or may not be useful.
Let me know if you think I should just have a
dispatch_as_signed
instead of this generalized approach.polkadot address: 14DsLzVyTUTDMm2eP3czwPbH53KgqnQRp3CJJZS9GR7yxGDP
skip check-dependent-cumulus
polkadot companion: paritytech/polkadot#4075
cumulus companion: paritytech/cumulus#715