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

Arbitrary call from Polkadot to Ethereum contract #924

Closed
wants to merge 38 commits into from

Conversation

claravanstaden
Copy link
Contributor

@claravanstaden claravanstaden commented Aug 8, 2023

  • Smoke test to send xcm to template parachain
  • Template chain forwards message to bridge hub
  • Bridge hub forwards message to outbound router
  • Outbound router message transform to Gateway contract
  • Execute contract in AgentExecute
  • Figure out how to do the dynamic fees

Cumulus companion: Snowfork/cumulus#57

@codecov
Copy link

codecov bot commented Aug 8, 2023

Codecov Report

Patch coverage: 61.05% and project coverage change: -0.44% ⚠️

Comparison is base (d1590d0) 74.42% compared to head (286491d) 73.99%.
Report is 3 commits behind head on main.

❗ Current head 286491d differs from pull request most recent head 43935b6. Consider uploading reports for the commit 43935b6 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #924      +/-   ##
==========================================
- Coverage   74.42%   73.99%   -0.44%     
==========================================
  Files          41       41              
  Lines        1834     1915      +81     
  Branches       74       79       +5     
==========================================
+ Hits         1365     1417      +52     
- Misses        449      475      +26     
- Partials       20       23       +3     
Flag Coverage Δ
rust 74.58% <71.42%> (-0.19%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
parachain/primitives/core/src/outbound.rs 15.49% <0.00%> (-1.70%) ⬇️
contracts/src/Gateway.sol 70.96% <18.75%> (-2.51%) ⬇️
contracts/src/AgentExecutor.sol 73.33% <55.55%> (-26.67%) ⬇️
parachain/primitives/router/src/outbound/mod.rs 84.32% <77.58%> (-3.65%) ⬇️
parachain/pallets/outbound-queue/src/lib.rs 92.30% <100.00%> (+1.89%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@claravanstaden claravanstaden force-pushed the clara/transact-on-polkadot-parachains branch from 9339348 to c9ce72b Compare August 11, 2023 09:01
@claravanstaden claravanstaden changed the title Transact on Parachains Send messages from Polkadot to Ethereum contracts Aug 14, 2023
@claravanstaden
Copy link
Contributor Author

claravanstaden commented Aug 14, 2023

@yrong I have added a new extrinsic in the template parachain. To use the BridgeXcmSender, I need to add a bunch of config from the pallet-bridge-transfer-primitives crate. I'm not sure including the pallet-bridge-transfer-primitives as a dependency (which, incidentally, seems to have some no-std problem) in the template parachain is the right move? Let me know your thoughts and if this seems on the right track.

@yrong
Copy link
Contributor

yrong commented Aug 15, 2023

I'm not sure including the pallet-bridge-transfer-primitives as a dependency (which, incidentally, seems to have some no-std problem) in the template parachain is the right move?

Yes, actually it's not required. But since you've done some integration already so I just did some fix in faccaee based on current implementation.

For the smoke test

cargo test --test transact_polkadot_to_ethereum

Log in bridgehub shows it reach EthereumBlobExporter as expected, so would assume will not block you and we can just continue the unfinished Convert logic based on that.

2023-08-15 10:17:42.071 INFO tokio-runtime-worker xcm::ethereum_blob_exporter: [Parachain] 🤩 converting.

The fee part is not correctly handled yet but agree as you mentioned we can focus on the basic flow as first step.

@claravanstaden claravanstaden force-pushed the clara/transact-on-polkadot-parachains branch from 0104666 to 7d07acf Compare August 17, 2023 08:08
Ok(execution_fee)
}

fn is_transact(&self) -> bool {
for instruction in self.iter.clone() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of cloning the iterator, it might make sense to use Peekable iterator. This will allow peeking of the next instruction without advancing the iterator.

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 I understand Peekable, it only looks one message ahead, and to move the cursor you have to call iter.next(). I am looking for message Transact, and at this point the next message is DescendOrigin (if that is even correct).

Copy link
Contributor

Choose a reason for hiding this comment

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

So I wrote this conversion code in recursive descendant parser style. Basically, you should only need to peek one instruction to decide which branch to go down.

The two branches here are whether to do a transact or a token unlock. So how I would use peek would be instead of this code, I would use the following psuedo code:

let result = match iter.peek() {
    DescendOrigin => transact_message(),    // This must be a transact because we peeked DescendOrigin
    WithdrawAsset => native_tokens_unlock_message(), // This must be a token unlock because we peeked WithdrawAsset
    ReserveAssetDeposited => polkadot_token_burn_message(), // This doesnt exist yet, but will once we support polkadot assets
    _ => return Error::UnexpectedInstruction, // The peeked instruction is unknown so we fail because we didnt expect the instruction
}

Then inside transact_message() you would consume whatever you need from DescendOrigin, then advance the iterator, and then match Transact. If you cant match transact at this point you fail with Error::UnexpectedInstruction. But in this case you know that your expecting transact so a better error name to use would be Error::TransactExpected.

parachain/primitives/router/src/outbound/mod.rs Outdated Show resolved Hide resolved
log::trace!(target: "xcm::ethereum_blob_exporter", "before transact.");

let data = if let Transact {
origin_kind: _origin_kind,
Copy link
Contributor

@alistair-singh alistair-singh Aug 21, 2023

Choose a reason for hiding this comment

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

We might want to assert origin_kind is OriginKind::SovereignAccount because execution on an agent is the same as executing using a sovereign account.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 6b3b09d.


let inner_message = Box::new(Xcm(vec![
Instruction::UnpaidExecution { weight_limit: Unlimited, check_origin: None },// TODO update to paid
Instruction::DescendOrigin(contract_location), // TODO not sure if this is right, want to pass the contract address
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alistair-singh the Transact instruction doesn't have a field to specify the contract to be executed on Ethereum. Is DescendOrigin an OK way to send across the address of the arbitrary contract to be executed?

Copy link
Contributor

@alistair-singh alistair-singh Aug 24, 2023

Choose a reason for hiding this comment

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

We do not need to use DescendOrigin and SetTopic here. The Xcm we should send and match should simply be:

UnpaidExecution { ... }
Transact { ... }

The contract address needs to be passed call field using (address, function_selector, params). You already have (function_selector, params) implemented so just prepend the target contract address.

@claravanstaden claravanstaden force-pushed the clara/transact-on-polkadot-parachains branch from 2577727 to 22c8d69 Compare August 23, 2023 10:00
@claravanstaden claravanstaden changed the title Send messages from Polkadot to Ethereum contracts Arbitrary call from Polkadot to Ethereum contract Aug 23, 2023
@yrong
Copy link
Contributor

yrong commented Aug 24, 2023

@claravanstaden For the issue you mentioned yesterday I just made a fix in 5db9786 and the smoke test shows e2e flow work as expected.


let inner_message = Box::new(Xcm(vec![
Instruction::UnpaidExecution { weight_limit: Unlimited, check_origin: None },// TODO update to paid
Instruction::DescendOrigin(contract_location), // TODO not sure if this is right, want to pass the contract address
Copy link
Contributor

@alistair-singh alistair-singh Aug 24, 2023

Choose a reason for hiding this comment

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

We do not need to use DescendOrigin and SetTopic here. The Xcm we should send and match should simply be:

UnpaidExecution { ... }
Transact { ... }

The contract address needs to be passed call field using (address, function_selector, params). You already have (function_selector, params) implemented so just prepend the target contract address.

});

let inner_message = Box::new(Xcm(vec![ // TODO send the Ethereum gas fee here too
Instruction::UnpaidExecution { weight_limit: Unlimited, check_origin: None },// TODO update to paid
Copy link
Contributor

@alistair-singh alistair-singh Aug 24, 2023

Choose a reason for hiding this comment

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

This UnpaidExecution is for buying execution on the destination chain i.e. Ethereum. We do not support buying of execution on Ethereum so we use the unpaid execution right now. Execution is paid for currently by the channels agent who rewards the relayer for relaying the message.

We could potentially buy execution on the target by matching the below xcm:

WithdrawAsset (ETH, 1000)
BuyExecution (ETH)

The code already can match this, however, we assert the execution is unpaid currently.

Ok(execution_fee)
}

fn is_transact(&self) -> bool {
for instruction in self.iter.clone() {
Copy link
Contributor

Choose a reason for hiding this comment

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

So I wrote this conversion code in recursive descendant parser style. Basically, you should only need to peek one instruction to decide which branch to go down.

The two branches here are whether to do a transact or a token unlock. So how I would use peek would be instead of this code, I would use the following psuedo code:

let result = match iter.peek() {
    DescendOrigin => transact_message(),    // This must be a transact because we peeked DescendOrigin
    WithdrawAsset => native_tokens_unlock_message(), // This must be a token unlock because we peeked WithdrawAsset
    ReserveAssetDeposited => polkadot_token_burn_message(), // This doesnt exist yet, but will once we support polkadot assets
    _ => return Error::UnexpectedInstruction, // The peeked instruction is unknown so we fail because we didnt expect the instruction
}

Then inside transact_message() you would consume whatever you need from DescendOrigin, then advance the iterator, and then match Transact. If you cant match transact at this point you fail with Error::UnexpectedInstruction. But in this case you know that your expecting transact so a better error name to use would be Error::TransactExpected.

@claravanstaden
Copy link
Contributor Author

Closing, will be re-done in SNO-866.

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

Successfully merging this pull request may close these issues.

3 participants