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

fix: ethereum transaction pallet must forward to pallet-ethereum::transact #1536

Merged
merged 9 commits into from
Sep 8, 2023

Conversation

mustermeiszer
Copy link
Collaborator

Description

Axelar depends on the pallet_ethereum::Event<T>::Transact{..} and we were previsoulsy shortcircuting the logic, which lead to the event missing. This change fixes that behaviour and just generates the same origin as the runtime and forwards directly to pallet-ethereum.

Changes and Descriptions

  • forward ethereum calls directly to pallet-ethereum::Pallet::<T>::transact(..)

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

@mustermeiszer mustermeiszer added the D0-ready Pull request can be merged without special precaution and notification. label Sep 8, 2023
wischli
wischli previously approved these changes Sep 8, 2023
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.

I am missing expertise to evaluate the changes for the router and gateway but given that this is the result of a long debugging session, I am optimistically approving. Rest LGTM!

TransactionSignature::new(
TRANSACTION_RECOVERY_ID,
H256::from_low_u64_be(1u64),
H256::from_low_u64_be(1u64),
H256::from_low_u64_be(2u64),
Copy link
Contributor

Choose a reason for hiding this comment

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

Just want to make sure this bump is intended? If you have the time, would be interested what this number reflects as the type is quite opaque:

pub struct TransactionSignature {
	v: TransactionRecoveryId,
	r: H256,
	s: H256,
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just wanted the signature to not match the one from pallet-xcm-transactor. I have to be honest, no clue whether this makes snese. The signature is not checked, so I guess it is not really relevant.

Copy link
Contributor

Choose a reason for hiding this comment

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

All right, thanks nevertheless!

Comment on lines 253 to 258
/// An incoming LP message was
/// detected and is further processed
IncomingMessage {
sender: DomainAddress,
msg: MessageOf<T>,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

On a note: I would like to introduce the opposite OutgoingMessage enum event in #1473

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should I also add it here do you mean? In this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, not at all. I should have been more explicit. I wanted to express I am happy the opposite of my planned Event impl for Liquidity Pools will be implemented by this PR as this is a good signal for my plans.

@mustermeiszer mustermeiszer merged commit 0f907f3 into main Sep 8, 2023
11 checks passed
@NunoAlexandre NunoAlexandre deleted the fix/ethereum-transaction-forward branch September 9, 2023 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D0-ready Pull request can be merged without special precaution and notification.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants