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.

Sign up for GitHub

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

Jump to bottom

connectors-gateway: Add Ethereum Transaction pallet #1460

Merged
merged 9 commits into from Jul 31, 2023
Merged

connectors-gateway: Add Ethereum Transaction pallet #1460

merged 9 commits into from Jul 31, 2023

Conversation

cdamian
Copy link
Contributor

@cdamian cdamian commented Jul 18, 2023 •
edited

Description

Part 2 of - #1376

The pallet added in this PR will be used by the Axelar EVM gateway router which will be added in a following PR.

An example of how this pallet is used can be found here

Changes and Descriptions

  • Add Ethereum Transaction pallet

Checklist:

  • I have added Rust doc comments to structs, enums, traits and functions
  • I have made corresponding changes to the documentation
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or that my feature works

@cdamian cdamian mentioned this pull request Jul 18, 2023
Closed
4 tasks
@cdamian cdamian marked this pull request as ready for review July 18, 2023 19:04
lemunozm
lemunozm reviewed Jul 19, 2023
View reviewed changes
Copy link
Contributor

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

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

I'm not the best to test this logic but all changes LGTM! 🙌🏻
Everything is quite clean so most of my comments are questions to myself or nitpick things.
Good job!

pallets/ethereum-transaction/src/lib.rs Outdated Show resolved Hide resolved
pallets/ethereum-transaction/Cargo.toml Outdated Show resolved Hide resolved
pallets/ethereum-transaction/Cargo.toml Outdated Show resolved Hide resolved
pallets/ethereum-transaction/src/mock.rs Outdated Show resolved Hide resolved
runtime/integration-tests/Cargo.toml Outdated Show resolved Hide resolved
runtime/integration-tests/src/ethereum_transaction/pallet.rs Show resolved Hide resolved
runtime/integration-tests/src/ethereum_transaction/pallet.rs Outdated Show resolved Hide resolved
pallets/ethereum-transaction/src/lib.rs Show resolved Hide resolved
cdamian
cdamian commented Jul 19, 2023
View reviewed changes
pallets/ethereum-transaction/src/lib.rs
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.

NOTE - this was built with the idea that the caller of this function would be the one that will take of charging the according fees based on the returned weight. This was an ongoing discussion a few weeks back and I think we should get back to it and decide how we want to tackle this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed. We can add an additional parameter if we want to let the system pay I think!

wischli
wischli reviewed Jul 19, 2023
View reviewed changes
Copy link
Contributor

@wischli wischli left a comment

Choose a reason for hiding this comment

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

Again, I had a great reviewing experience and overall looks good! I have a few minor questions before I feel good with hitting the approval. Apart from that, some praise and as always a few nitpicks which can totally be ignored.

pallets/ethereum-transaction/Cargo.toml Outdated Show resolved Hide resolved
pallets/ethereum-transaction/src/lib.rs Outdated Show resolved Hide resolved
pallets/ethereum-transaction/src/lib.rs Show resolved Hide resolved
pallets/ethereum-transaction/src/lib.rs Show resolved Hide resolved
pallets/ethereum-transaction/src/lib.rs Outdated

fn map_evm_error(e: ExitError) -> DispatchError {
match e {
ExitError::StackUnderflow => DispatchError::Other("stack underflow"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want all of these errors to really be DispatchError::Other and not pallet::<T>::Error? Asking because I recently had to find the cause of something that threw DispatchError::Other and unfortunately, Polkadot Js Apps omits the message so it was like finding the needle in a stack of hay. Not sure if it is completely omitted from the metadata. If so, we should not throw any of these actually.

Here's proof of my statement. I tried to invest without having permissions which throws DispatchError::Other("Account does not have the TrancheInvestor permission."):
Screenshot 2023-07-19 at 14 40 54

Copy link
Contributor Author

@cdamian cdamian Jul 19, 2023 •
edited

Choose a reason for hiding this comment

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

Hmm, initially I added this DispatchError because I wasn't able to debug test failures but completely forgot about the "outside". Awesome point, will definitely change this, thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I'm not passing the error message in the case of ExitError::Other(Cov<'static, str>) though. It seems that might be a bit tricky to do if we really want to, WDYT?

pallets/ethereum-transaction/src/tests.rs Show resolved Hide resolved
pallets/ethereum-transaction/src/tests.rs Outdated Show resolved Hide resolved
runtime/integration-tests/src/ethereum_transaction/pallet.rs Outdated Show resolved Hide resolved
runtime/integration-tests/src/utils/env.rs Show resolved Hide resolved
runtime/integration-tests/src/utils/env.rs Show resolved Hide resolved
lemunozm
lemunozm previously approved these changes Jul 20, 2023
View reviewed changes
Copy link
Contributor

@lemunozm lemunozm left a comment •
edited

Choose a reason for hiding this comment

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

LGTM! Thanks for addressing the changes 🚀

wischli
wischli previously approved these changes Jul 20, 2023
View reviewed changes
Copy link
Contributor

@wischli wischli left a comment

Choose a reason for hiding this comment

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

Thanks for putting sweat into my nitpicks! LGTM 🚀

@cdamian
Copy link
Contributor Author

cdamian commented Jul 25, 2023

cc @branan

@cdamian cdamian dismissed stale reviews from wischli and lemunozm via 76619bd July 28, 2023 10:19
@cdamian cdamian force-pushed the connectors-gateway-2-ethereum-transaction-pallet branch from cb8009e to 76619bd Compare July 28, 2023 10:19
lemunozm
lemunozm approved these changes Jul 28, 2023
View reviewed changes
Copy link
Contributor

@lemunozm lemunozm left a comment •
edited

Choose a reason for hiding this comment

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

Re-approved! Although would be awesome to have @branan eyes on this.

@cdamian cdamian force-pushed the connectors-gateway-2-ethereum-transaction-pallet branch from 76619bd to daaf0b1 Compare July 31, 2023 07:28
@cdamian cdamian merged commit 5237396 into main Jul 31, 2023
11 checks passed
@NunoAlexandre NunoAlexandre deleted the connectors-gateway-2-ethereum-transaction-pallet branch July 31, 2023 14:32
mustermeiszer
mustermeiszer reviewed Aug 9, 2023
View reviewed changes
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.

Looks good!

pallets/ethereum-transaction/src/lib.rs
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.

Agreed. We can add an additional parameter if we want to let the system pay I think!

pallets/ethereum-transaction/src/lib.rs
Comment on lines +160 to +168

let transaction = TransactionV2::Legacy(LegacyTransaction {
nonce,
gas_price,
gas_limit,
action: TransactionAction::Call(to),
value,
input: data.into(),
signature,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Using this we can never get a Create call right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mustermeiszer mustermeiszer mustermeiszer left review comments

@NunoAlexandre NunoAlexandre NunoAlexandre approved these changes

@lemunozm lemunozm lemunozm approved these changes

@wischli wischli wischli left review comments

@mikiquantum mikiquantum Awaiting requested review from mikiquantum

@branan branan Awaiting requested review from branan

@thea-leake thea-leake Awaiting requested review from thea-leake

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@cdamian @NunoAlexandre @lemunozm @wischli @mustermeiszer