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

Downgrade class #13912

Closed
wants to merge 1 commit into from
Closed

Downgrade class #13912

wants to merge 1 commit into from

Conversation

AurevoirXavier
Copy link
Contributor

@AurevoirXavier AurevoirXavier commented Apr 13, 2023

Able to downgrade a call to normal class.

More and more chains focus on EVM now.
And we people might want to dispatch a call from the EVM.

For example, Darwinia or Moonbeam is an Ethereum-like chain. We set use Gnosis multisig instead of pallet-multisig. And I set the sudo key to that contract address. But I can't dispatch some calls from the EVM due to https://github.com/paritytech/frontier/blob/2b09a676cac8cca665c882a28bd13b2acef39395/frame/evm/precompile/dispatch/src/lib.rs#L67.

So, I think it's okay to have a utility to downgrade a call to a normal class. (it requires roots origin, and people know what they are doing)


Or do you think providing a with_class is better?

@bkchr
Copy link
Member

bkchr commented Apr 15, 2023

I don't see any reason to have this. In general I don't get the reason frontier is checking the dispatch class? What is the reason behind this?

@boundless-forest
Copy link
Contributor

I've submitted another pr to address this issue in the frontier repo. polkadot-evm/frontier#1042 Let's wait the opinion of @sorpaas.

@bkchr
Copy link
Member

bkchr commented Apr 17, 2023

@boundless-forest ty!

@AurevoirXavier
Copy link
Contributor Author

I don't see any reason to have this. In general I don't get the reason frontier is checking the dispatch class? What is the reason behind this?

I'm not sure about that. Maybe it's because they haven't done some further research and that's a tmp safeguard.

But I feel this is useful for Substrate. Just like the with_weight. It provides more flexibility to runtime.

@bkchr
Copy link
Member

bkchr commented Apr 17, 2023

But I feel this is useful for Substrate. Just like the with_weight. It provides more flexibility to runtime.

with_weight is quite different to this. I don't get the need to change the class? The only thing you get from this is that your transaction is getting a lower priority?

@AurevoirXavier
Copy link
Contributor Author

The only thing you get from this is that your transaction is getting a lower priority?

Yes, you are right. But is it worth having a with_class (to make the priority higher)?

@ggwpez
Copy link
Member

ggwpez commented Apr 17, 2023

But is it worth having a with_class (to make the priority higher)?

We could add Utility::with_class for root to upgrade to Operational or Mandatory.
IMO only really makes sense for Sudo chains, since normally governance goes through the scheduler, and that is already Mandatory.

@bkchr
Copy link
Member

bkchr commented Apr 19, 2023

We could add Utility::with_class for root to upgrade to Operational or Mandatory.

But this means that some extrinsic will have a higher dispatch class and then we only find out while applying that this someone doesn't has root. I'm still not convinced.

IMO only really makes sense for Sudo chains, since normally governance goes through the scheduler, and that is already Mandatory.

I mean the dispatch class is mainly around the priority of the transaction and the weight space there is reserved. But when would you need this upgrade? Why isn't the particular tx then not directly set to the proper dispatch class?

@AurevoirXavier
Copy link
Contributor Author

AurevoirXavier commented Apr 19, 2023

Thanks for your patience and reply.

Maybe the developers want to make some tweaks. They can call this from runtime code or from other pallets to satisfy their service logic.

Why isn't the particular tx then not directly set to the proper dispatch class?

Yes! That's the question. In the Substrate ecosystem, we could use the pallet from any team but developers' knowledge levels might be different. And just like the with_weight, why can't they set the weight correctly at the beginning?

@bkchr
Copy link
Member

bkchr commented Apr 20, 2023

And just like the with_weight, why can't they set the weight correctly at the beginning?

It is basically a hack to prevent bricking a chain because certain things are not benchmarked properly. with_dispatch_class would only change the transaction orderind/unlock "extra block weight". However for the second thing you could already use with_weight

@xlc
Copy link
Contributor

xlc commented Apr 20, 2023

I don't think we have any real use case for this so how about wait until someone actually find a use case first? Not "I think it will be useful for X" kind, but "I need this to do X" kind.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants