-
Notifications
You must be signed in to change notification settings - Fork 2
Conversation
I suggest renaming EthereumCallDispatch trait to CallDispatch. the CallDispatch is used to dispatch different calls. |
modules/dispatch/src/lib.rs
Outdated
@@ -297,6 +311,39 @@ impl<T: Config<I>, I: 'static> MessageDispatch<T::AccountId, T::BridgeMessageId> | |||
return dispatch_result | |||
} | |||
|
|||
// Dispatch Ethereum call earlier before pay | |||
match T::EthereumCallDispatcher::dispatch(&call, &origin_account) { |
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 feel like this is a patch here.
maybe, generalize the EthereumCallDispatch
to CallDispatch
.
or, something like:
replace
let origin = RawOrigin::Signed(origin_account).into();
with
let origin = T::OriginBuilder.getOrigin(call, origin_account);
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.
generalize the EthereumCallDispatch to CallDispatch.
It seems like a rename issue. For dispatch calls besides the Ethereum pallet, we use the original call dispatch in this pallet. The dispatcher I added is only used for the Ethereum pallet dispatch call, so I think EthereumCallDispatch
is more suitable here.
For the second way:
We also need the necessary validation before dispatch, it inserts too much if we put the validate logic in this 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.
why I want to rename the trait to CallDispatch. for example, another chain has a special dispatch with a custom origin, like PalletNew
's new_call(NewOrigin)
, it need a impl CallDispatch for NewCallDispatcher
. So, if the trait name is EthereumCallDispatch
, it will be impl EthereumCallDispatch for NewCallDispatcher
.
is this trait be used only in Ethereum 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.
Currently, this one is only used by the Ethereum pallet. I don't see any other pallet that needs it. Rename to CallDispatch
is wired this pallet already called pallet-messages-dispatch
, then there is another associated type called CallDispatch
, After the CallDispatch executes logic, there is another call dispatch.
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.
ok
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.
modules/dispatch/src/lib.rs
Outdated
EthereumCallDispatched(ChainId, BridgeMessageIdOf<T, I>, DispatchResult), | ||
EthereumCallValidityError(ChainId, BridgeMessageIdOf<T, I>, EthereumCallValidityError), |
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 is too specific to put them in this generic pallet.
modules/dispatch/src/lib.rs
Outdated
@@ -297,6 +311,39 @@ impl<T: Config<I>, I: 'static> MessageDispatch<T::AccountId, T::BridgeMessageId> | |||
return dispatch_result | |||
} | |||
|
|||
// Dispatch Ethereum call earlier before pay | |||
match T::EthereumCallDispatcher::dispatch(&call, &origin_account) { |
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.
Prefer #111 |
This is an early draft pull request in development. Because of the cycle dep problem, the concrete tests have to be done on
darwinia-common
side.