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.

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

refactor evm origin with DispatchContext #2481

Open
xlc opened this issue Feb 27, 2023 · 13 comments
Open

refactor evm origin with DispatchContext #2481

xlc opened this issue Feb 27, 2023 · 13 comments

Comments

@xlc
Copy link
Member

xlc commented Feb 27, 2023

new feature from paritytech/substrate#13468

we can use this to implement evm origin

@xlc
Copy link
Member Author

xlc commented Feb 27, 2023

the way the DispatchContext API is structured may not make it possible for evm origin use case

@bkchr
Copy link

bkchr commented Feb 27, 2023

the way the DispatchContext API is structured may not make it possible for evm origin use case

What is the problem? What do you need?

@xlc
Copy link
Member Author

xlc commented Feb 28, 2023

I think we need to implement it to know if more changes are required. Here are two use cases we have currently:

Set origin for tx in pre_dispatch for signed tx

Acala/modules/evm/src/lib.rs

Lines 2021 to 2041 in 1cf443e

fn pre_dispatch(
self,
who: &Self::AccountId,
_call: &Self::Call,
_info: &DispatchInfoOf<Self::Call>,
_len: usize,
) -> Result<(), TransactionValidityError> {
ExtrinsicOrigin::<T>::set(Some(vec![who.clone()]));
Ok(())
}
fn post_dispatch(
_pre: Option<Self::Pre>,
_info: &DispatchInfoOf<Self::Call>,
_post_info: &PostDispatchInfoOf<Self::Call>,
_len: usize,
_result: &DispatchResult,
) -> Result<(), TransactionValidityError> {
ExtrinsicOrigin::<T>::kill();
Ok(())
}

Set origin for xcm call

fn execute_xcm_in_credit(
origin: impl Into<MultiLocation>,
message: Xcm<Config::RuntimeCall>,
weight_limit: XcmWeight,
weight_credit: XcmWeight,
) -> Outcome {
let origin = origin.into();
let account = AccountIdConvert::convert(origin.clone());
let clear = if let Ok(account) = account {
EVMBridge::push_origin(account);
true
} else {
false
};
let res =
xcm_executor::XcmExecutor::<Config>::execute_xcm_in_credit(origin, message, weight_limit, weight_credit);
if clear {
EVMBridge::pop_origin();
}
res

@bkchr
Copy link

bkchr commented Feb 28, 2023

Yeah those would not work, as they are still outside the dispatch context! Not sure if you could set store these origins in some call.

@xlc
Copy link
Member Author

xlc commented Feb 28, 2023

We cannot set origin in some call. Because for example a swap can trigger ERC20 transfer. This means we need to set it on every call.

@bkchr
Copy link

bkchr commented Feb 28, 2023

If wanted, we could also let the initial dispatch context being created in frame-executive. This should then help for your use case. I also thought about having some kind of support to know the outer origin/account id while implementing this feature.

@xlc
Copy link
Member Author

xlc commented Feb 28, 2023

Yeah that will be lot more useful. It will be great if you can have it cover initialize/finalize hook and xcm execution as well so we don't need to do special handling in our runtime.

@bkchr
Copy link

bkchr commented Mar 1, 2023

initialize/finalize hook

That goes a little bit against the idea of the "dispatch context". It should be in the context of calling dispatch and then should also be deleted before doing the next dispatch. What kind of information are you storing in initialize/finalize hooks that require special handling?

@xlc
Copy link
Member Author

xlc commented Mar 1, 2023

The idea is there is always a context for any code execution.

In our use case, evm execution always requires an origin and we can’t rule out the possibility that someone may want to propose a governance call that executes evm call. In that case it will try to access the dispatch context. It could return None and we do special handling knowing it means the call must be dispatched by a hook. Or there could e a dispatch context API allow me define a default context. Then we could create a context with some special evm address as the origin.

@bkchr
Copy link

bkchr commented Mar 1, 2023

In our use case, evm execution always requires an origin and we can’t rule out the possibility that someone may want to propose a governance call that executes evm call.

But that means that this will be some kind of EvmPallet::execute_evm { evm_call } call? And you execute this call with dispatch? So, there will be a context. You could also start the context before calling dispatch and set the origin.

@xlc
Copy link
Member Author

xlc commented Mar 1, 2023

In the example of governance, the call is created by democracy or referendum pallet, executed by scheduler pallet. There is no place for me to inject context. Unless we create special variant of call for the purpose of governance which I think will work but make generic governance UI harder to implement.

@bkchr
Copy link

bkchr commented Mar 6, 2023

Democracy and Referenda are using dispatch to dispatch calls, which results in a context being created. Every dispatch call in the runtime will create/reuse this context.

@xlc
Copy link
Member Author

xlc commented Mar 6, 2023

I see. I guess it could work. We just need to give it a try and see.

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

No branches or pull requests

2 participants