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

pallets: Add Ethereum Transaction pallet #1403

Conversation

cdamian
Copy link
Contributor

@cdamian cdamian commented Jun 15, 2023

Follow-up for the connectors gateway pallet discussion around EVM found here.

The approach is similar to the one found in ethereum-xcm.

Note this a PR against - #1376, meant to facilitate discussion around this specific topic.

@cdamian cdamian force-pushed the connectors-gateway-add-ethereum-transaction-pallet branch from 6656fda to 9613fd6 Compare June 15, 2023 16:08
signature,
});

let (_target, _value, info) = pallet_ethereum::Pallet::<T>::execute(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that we are using pallet_ethereum::execute() instead of the T::ValidatedTransaction::apply() used by ethereum-xcm, as per a previous discussion around this topic.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@NunoAlexandre
Copy link
Contributor

The approach is similar to the one found in ethereum-xcm.

Where can we find information about the thought process around implementing a custom one instead of using that already solution from PureStake?

@cdamian
Copy link
Contributor Author

cdamian commented Jun 19, 2023

The approach is similar to the one found in ethereum-xcm.

Where can we find information about the thought process around implementing a custom one instead of using that already solution from PureStake?

Here is the spec.

@NunoAlexandre
Copy link
Contributor

Cool, thank you!

In order for Ethereum calls from the Substrate side to be present in an ethereum block, a full fake transaction needs to be created

What is the goal behind this feature? Is it so that we have all the substrate calls / blocks accessible in the EVM? And won't it be an issue that all of the mirrored blocks on the EVM are all signed by that mock account?

value: U256,
gas_price: U256,
gas_limit: U256,
) -> DispatchResultWithPostInfo {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to propagate this and then charge for the fees in one of the callers of this OR do we want to charge all the necessary fees here, in this pallet?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon further testing I noticed that some fees are actually charged by the EVM runner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, the above mentioned fees would be charged from the derived account, meaning that we'll have to ensure that the original caller will have sufficient funds to execute the EVM call.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the research. What would happen if the evm fails? Would it roll-back the complete transaction including possible EVM changes?

If this is the case, I would just assume we fail in runtime then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will have to check that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial error handling after executing the transaction was incorrect. I updated it now to handle all exit reasons found in there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is an error before this point we will get back a RunnerError that contains a weight that we should charge. Otherwise, the total fee for the EVM transaction will be charged by the runner and we would only have to charge for reading the nonce.

Copy link
Contributor Author

@cdamian cdamian Jun 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is an error before this point we will get back a RunnerError that contains a weight that we should charge. Otherwise, the total fee for the EVM transaction will be charged by the runner and we would only have to charge for reading the nonce.

Given this, would it make sense to charge for the necessary fees in this pallet? This would the allow us to just use a DispatchResult here and keep other parts free of fee-related logic. WDYT @mustermeiszer

Partially verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.
@cdamian cdamian requested review from mustermeiszer and branan June 19, 2023 16:33
Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only think I would clear out is whether we can just fail in runtime/EVM runner if the calley has not enough funds. cc @branan

value: U256,
gas_price: U256,
gas_limit: U256,
) -> DispatchResultWithPostInfo {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the research. What would happen if the evm fails? Would it roll-back the complete transaction including possible EVM changes?

If this is the case, I would just assume we fail in runtime then.


//TODO(cdamian): Same signature as the one in ethereum-xcm.
let signature = TransactionSignature::new(
42,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^^

pallets/ethereum-transaction/src/lib.rs Outdated Show resolved Hide resolved
@cdamian
Copy link
Contributor Author

cdamian commented Jun 21, 2023

As agreed, gonna merge this for now and continue working on the original branch.

@cdamian cdamian marked this pull request as ready for review June 21, 2023 16:12
@cdamian cdamian merged commit d1bec5d into add-pallet-connectors-gateway Jun 21, 2023
@cdamian cdamian deleted the connectors-gateway-add-ethereum-transaction-pallet branch June 21, 2023 16:12
cdamian added a commit that referenced this pull request Jun 23, 2023
* pallets: Add Ethereum Transaction pallet

* ethereum-transaction: Drop origin

* ethereum-transaction: Drop weights and origin type

* ethereum-transaction: Handle call info exit reasons
@mustermeiszer mustermeiszer mentioned this pull request Jun 30, 2023
4 tasks
cdamian added a commit that referenced this pull request Jul 11, 2023
* pallets: Add Ethereum Transaction pallet

* ethereum-transaction: Drop origin

* ethereum-transaction: Drop weights and origin type

* ethereum-transaction: Handle call info exit reasons
cdamian added a commit that referenced this pull request Jul 13, 2023
* pallets: Add Ethereum Transaction pallet

* ethereum-transaction: Drop origin

* ethereum-transaction: Drop weights and origin type

* ethereum-transaction: Handle call info exit reasons
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

Successfully merging this pull request may close these issues.

3 participants