-
Notifications
You must be signed in to change notification settings - Fork 740
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
Contracts: xcm host fn fixes #3086
Conversation
bot fmt |
@pgherveou https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5043153 was started for your command Comment |
@pgherveou Command |
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.
This fixes the problem but maybe we can side step it entirely. See my comment.
I thought about this change again. While this fixes the problem that the So the current code (before the PR) is actually correct with regard to influencing the lowest experienced gas. The actual problem here is that taking "all the gas" is a very vast overestimation of what the XCM needs. Especially in case of dry run where we supply the whole block gas limit. There are only two fixes for my problem I can thing of:
The problem of the second approach is that You have to investigate what is going on there? What if a program is not Weighable? How does pallet-xcm handle this situation? Does the executor about the execution some where? |
why is that? We know that the pre-dispatch weight is |
You are right this should work because the weight we take is dependent on the remaining weight. As long as it is not a fixed number it should work. |
@paritytech/xcm can you take a look at these minor changes in pallet-xcm when you have a chance 🙏 |
This reverts commit e6dd8e7.
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.
Yes the change makes sense to me that we are only return Ok
for a complete execution.
Ping @KiChjang or @franciscoaguirre |
The CI pipeline was cancelled due to failure one of the required jobs. |
## Xcm changes: - Fix `pallet_xcm::execute`, move the logic into The `ExecuteController` so it can be shared with anything that implement that trait. - Make `ExecuteController::execute` retursn `DispatchErrorWithPostInfo` instead of `DispatchError`, so that we don't charge the full `max_weight` provided if the execution is incomplete (useful for force_batch or contracts calls) - Fix docstring for `pallet_xcm::execute`, to reflect the changes from paritytech#2405 - Update the signature for `ExecuteController::execute`, we don't need to return the `Outcome` anymore since we only care about `Outcome::Complete` ## Contracts changes: - Update host fn `xcm_exexute`, we don't need to write the `Outcome` to the sandbox memory anymore. This was also not charged as well before so it if fixes this too. - One of the issue was that the dry_run of a contract that call `xcm_execute` would exhaust the `gas_limit`. This is because `XcmExecuteController::execute` takes a `max_weight` argument, and since we don't want the user to specify it manually we were passing everything left by pre-charghing `ctx.ext.gas_meter().gas_left()` - To fix it I added a `fn influence_lowest_limit` on the `Token` trait and make it return false for `RuntimeCost::XcmExecute`. - Got rid of the `RuntimeToken` indirection, we can just use `RuntimeCost` directly. --------- Co-authored-by: command-bot <>
Xcm changes:
pallet_xcm::execute
, move the logic into TheExecuteController
so it can be shared with anything that implement that trait.ExecuteController::execute
retursnDispatchErrorWithPostInfo
instead ofDispatchError
, so that we don't charge the fullmax_weight
provided if the execution is incomplete (useful for force_batch or contracts calls)pallet_xcm::execute
, to reflect the changes from pallet-xcm: ensure xcm outcome is complete, or discard all state changes done by extrinsic #2405ExecuteController::execute
, we don't need to return theOutcome
anymore since we only care aboutOutcome::Complete
Contracts changes:
xcm_exexute
, we don't need to write theOutcome
to the sandbox memory anymore. This was also not charged as well before so it if fixes this too.xcm_execute
would exhaust thegas_limit
.This is because
XcmExecuteController::execute
takes amax_weight
argument, and since we don't want the user to specify it manually we were passing everything left by pre-charghingctx.ext.gas_meter().gas_left()
fn influence_lowest_limit
on theToken
trait and make it return false forRuntimeCost::XcmExecute
.RuntimeToken
indirection, we can just useRuntimeCost
directly.