SNO-613: Arbitrary transact from Ethereum to Polkadot #925

yrong · 2023-08-10T03:52:39Z

Prerequisite

Xcm configs in the dest chain should be properly configured to allow bridge transact and take customized template runtime in Snowfork/cumulus#58 as reference.

Assumptions

We provide two options send Transact with Origin from Sender or Agent. For the first option, no extra/dynamic fee will be deducted on the Ethereum side but requires a prior step to fund the sender's mapping substrate address which will be used to buy_execution in the dest chain.
For the second option with Agent as Proxy, dynamic fees are always required in Ethereum to cover the cost of XCM execution in the dest chain, then there is no need for any extra step. We can still pass the original sender to the destination call if necessary. Requires extra field in particular call to differentiate both origins(e.g. take substrate's implementation for creating asset as reference, admin of the asset could be the real sender diff from the proxy address).
Also for the second option with Agent as Proxy, dynamic fees are bought with the native asset of the relay chain(i.e. DOT), so the dest chain should be configured to allow MultiLocation::parent() for XCM instructions like withdraw_asset and buy_execution with some custom XCM config(e.g. take implementation from Acala as reference). And since dynamic_fee is charged in Ether on the Ethereum side so it depends on some price oracle convert to the equivalent DOT on substrate side.

Todos

Since there is considerable change already so would prefer to focus on basic flow in this pull request, Mark the to-do list as follows, and will address it in later pull requests.

https://linear.app/snowfork/issue/SNO-619

codecov · 2023-08-10T03:55:30Z

Codecov Report

Patch coverage: 22.53% and project coverage change: -2.05% ⚠️

Comparison is base (d1590d0) 74.42% compared to head (e3fab42) 72.38%.
Report is 5 commits behind head on main.

❗ Current head e3fab42 differs from pull request most recent head aa61938. Consider uploading reports for the commit aa61938 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #925      +/-   ##
==========================================
- Coverage   74.42%   72.38%   -2.05%     
==========================================
  Files          41       41              
  Lines        1834     1890      +56     
  Branches       74       74              
==========================================
+ Hits         1365     1368       +3     
- Misses        449      502      +53     
  Partials       20       20

Flag	Coverage Δ
rust	`72.81% <19.60%> (-1.95%)`	⬇️
solidity	`70.50% <30.00%> (-2.46%)`	⬇️

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

Files Changed	Coverage Δ
contracts/src/Gateway.sol	`67.92% <6.66%> (-5.55%)`	⬇️
parachain/primitives/router/src/inbound/mod.rs	`39.70% <18.18%> (-8.90%)`	⬇️
parachain/pallets/inbound-queue/src/lib.rs	`51.61% <22.22%> (-14.35%)`	⬇️
contracts/src/Assets.sol	`67.85% <100.00%> (ø)`

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

yrong · 2023-08-16T08:56:22Z

There are several new smoke tests covering the feature:

do_something_as_arbitrary_transact for calling do_something with initial sender as Origin
do_something_through_sovereign for calling do_something with Agent as Origin

parachain/primitives/router/src/inbound/mod.rs

alistair-singh · 2023-08-24T07:25:03Z

contracts/src/Gateway.sol

@@ -260,7 +275,7 @@ contract Gateway is IGateway, IInitializable {
            revert AgentAlreadyCreated();
        }

-        address payable agent = payable(new Agent(params.agentID));
+        address payable agent = payable(new Agent{salt:CREATE2_SALT}(params.agentID));


The AgentId is a bytes32. Can we not use the AgentId as salt since its a one to one mapping?

Nope, it's not the AgentId, the create2 salt is configurable here which will be used to generate predictable contract address(e.g. Agent here which we will be used in transactThroughSovereign mode).

alistair-singh · 2023-08-24T08:52:32Z

Just a thought but I am not sure how useful it is to send over a single xcm transact. It might more be useful to accept an entire xcm payload as opaque bytes from the Ethereum side, then decode it to a set of instructions here and then prepend:

UniversalOrigin(GlobalConsensus(network)),
DescendOrigin(X1(origin_location)),

Where origin_location is the msg.sender. This pushes the burden of buying execution and worrying about fees onto the creator of the xcm message. It will be more general and is secured by the DescendOrigin which we will prepend to the instruction set.

I am just thinking about the use case for this kind of method and it probably won't be EOA accounts using transact. It will probably be smart contracts that need to interface with Polkadot. An end user would not need to use the bridge to issue a single transact when they can issue it directly to Polkadot and save paying fees on Ethereum. A smart contract however would want to execute some logic on the Ethereum side and then co-ordinate it with some logic on the Polkadot side using transact. If we only allow a single transact then to do complex tasks you would have to queue multiple messages.

Also given that the most likely use case for this would be a smart contract, the smart contract developer would need to make sure that the contracts sovereign account is funded on the destination chain to pay for execution.

From the 2 options presented for choice of origin:

Using the msg.sender.
Using the Agent contract.

I think we need to use both options in different cases. For transact we need to use option 1. For our custom messages such as sendToken and registerToken we should use the agent contract. i.e. AssetHubs agent contract is allowed to create and mint assets on AssetHub. We currently use GatewayProxy for custom messages but I see the GatewayProxy contract sovereign as a kind of "root" account.

claravanstaden

Hey @yrong! I don't know enough about this to give thorough commentary, but I've made some notes/asked questions.

I see you checked in smoketest/src/parachains/*. Is that deliberate?

claravanstaden · 2023-08-23T07:48:37Z

contracts/src/Assets.sol

+
+        payload = SubstrateTypes.SendToken(address(this), token, destinationChain, destinationAddress, amount, extraFee);
+


So if I understand this right, sendToken is a different kind of operation than an arbitrary contract call. So why is extraFee being sent here too, if it is not an arbitrary transact call being done here?

Actually it's some improvement for https://linear.app/snowfork/issue/SNO-582 for transfer, but relevant to the Transact change here(i.e. extra fee will consistently be used to cover the cost of XCM dispatch) so I link it as subtask as https://linear.app/snowfork/issue/SNO-613

@vgeddes Could you help to confirm if that's also your intention?

claravanstaden · 2023-08-23T08:05:35Z

contracts/src/Gateway.sol

+    ParaID internal immutable TEMPLATE_PARA_ID;
+    bytes32 internal immutable TEMPLATE_AGENT_ID;


If I understand correctly, the template parachain is used for "testing" and illustrating how the arbitrary transact will work, right? So should "testing" code like this form part of the gateway contract? 🤔

Exactly! Previously I did that in Initialize logic and removed in e3fab42 in consistent with your pull request.

Btw, seems you commented on outdated code.

parachain/pallets/inbound-queue/src/lib.rs

claravanstaden · 2023-08-24T18:32:30Z

smoketest/src/helper.rs

 use subxt::{Config, OnlineClient, PolkadotConfig};
 use templateXcm::{
    v3::{junction::Junction, junctions::Junctions, multilocation::MultiLocation},
    VersionedMultiLocation, VersionedXcm,
 };

 /// Custom config that works with Statemint
-pub enum TemplateConfig {}
+pub enum AssetHubConfig {}


Why are you using AssetHub here instead of the template parachain?

Actually for template parachain the default PolkadotConfig should be fine without a custom RuntimeConfig, there is some nuance here for AssetHub which requires it.

Now we don't have smoke tests to cover the asset register/transfer flow from substrate to ethereum direction yet, we'll need it then I think.

yrong · 2023-08-25T07:00:58Z

It might more be useful to accept an entire xcm payload as opaque bytes from the Ethereum side, then decode it to a set of instructions

Then it requires full implementation of SCALE encoder for XCM in the solidity side which is not easy IMO. Moreover I would prefer not to expose these low-level XCM details to the end user.

yrong · 2023-08-25T07:01:14Z

I am just thinking about the use case for this kind of method and it probably won't be EOA accounts using transact. It will probably be smart contracts that need to interface with Polkadot.

Agree! Actually in current impl of transactThroughSigned I don't care whether it's EOA account or a smart contract. Though I'm not sure but would assume transactThroughSovereign will be helpful in some cases, much like the end user(EOA or a smart contract) approves the agent as proxy on their behalf.

yrong · 2023-08-25T07:07:35Z

A smart contract however would want to execute some logic on the Ethereum side and then co-ordinate it with some logic on the Polkadot side using transact. If we only allow a single transact then to do complex tasks you would have to queue multiple messages.

I would assume for arbitrary transact actually we can call anything as we want(e.g. an Utilitity.batchAll which internally includes a lot of other transacts).

yrong · 2023-08-25T07:20:30Z

For our custom messages such as sendToken and registerToken we should use the agent contract. i.e. AssetHubs agent contract is allowed to create and mint assets on AssetHub. We currently use GatewayProxy for custom messages but I see the GatewayProxy contract sovereign as a kind of "root" account.

Agree! And there is another option as I mentioned before for the owner here when registering asset, instead of the Gateway we can change it to the Agent address or the original sender, then only that account(i.e. owner/admin of the asset) can mint tokens based on the checks here.

Co-authored-by: Clara van Staden <[email protected]>

yrong · 2024-02-16T10:21:05Z

Closed in favor of #1141

Support arbitrary transact(Part1)

d58565b

yrong added 3 commits August 10, 2023 14:07

Some fix for failed tests

66e0486

Add smoke test

1bcb9b7

Merge branch 'main' into ron/sno-613

99b5267

yrong marked this pull request as ready for review August 14, 2023 00:40

Use arithmetic api

8ddb34a

yrong added the do-not-merge label Aug 14, 2023

yrong changed the title ~~Support arbitrary transact~~ Arbitrary transact from Ethereum to Polkadot Aug 16, 2023

yrong requested review from vgeddes and alistair-singh August 16, 2023 09:54

Allow sender as origin

547dfc8

yrong removed the do-not-merge label Aug 17, 2023

Fix clippy

1c14ab4

yrong added the do-not-merge label Aug 17, 2023

alistair-singh reviewed Aug 17, 2023

View reviewed changes

parachain/primitives/router/src/inbound/mod.rs Outdated Show resolved Hide resolved

parachain/primitives/router/src/inbound/mod.rs Outdated Show resolved Hide resolved

parachain/primitives/router/src/inbound/mod.rs Outdated Show resolved Hide resolved

yrong added 10 commits August 18, 2023 21:49

Smoke test for OriginKind::SovereignAccount with template parachain

3d59a44

Update for bridge transact

152ce33

Update cumulus

f11f5d9

Update cumulus

d36cd35

Support Agent as transact origin

a9508a7

Mark more todo list

48bcb82

Update cumulus

abae23c

Merge branch 'main' into ron/sno-613

d00cd8b

Update cumulus

f0cd660

Initialize agent with create2 & More refactor

919f318

yrong changed the title ~~Arbitrary transact from Ethereum to Polkadot~~ SNO-613: Arbitrary transact from Ethereum to Polkadot Aug 23, 2023

yrong removed the do-not-merge label Aug 23, 2023

yrong requested a review from claravanstaden August 23, 2023 04:04

yrong requested a review from doubledup August 23, 2023 04:04

yrong added 3 commits August 23, 2023 14:49

Mark todo

64c48f8

Mark todo

8f3e52b

Create agent through dispatch call in consistent with PR924

e3fab42

alistair-singh reviewed Aug 24, 2023

View reviewed changes

claravanstaden reviewed Aug 24, 2023

View reviewed changes

yrong and others added 2 commits August 25, 2023 16:19

Update parachain/pallets/inbound-queue/src/lib.rs

b2f4388

Co-authored-by: Clara van Staden <[email protected]>

Fix AssetHubConfig

aa61938

yrong added the do-not-merge label Aug 25, 2023

yrong self-assigned this Sep 5, 2023

yrong closed this Feb 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SNO-613: Arbitrary transact from Ethereum to Polkadot #925

SNO-613: Arbitrary transact from Ethereum to Polkadot #925

yrong commented Aug 10, 2023 •

edited

Loading

codecov bot commented Aug 10, 2023 •

edited

Loading

yrong commented Aug 16, 2023 •

edited

Loading

alistair-singh Aug 24, 2023

yrong Aug 25, 2023 •

edited

Loading

alistair-singh commented Aug 24, 2023 •

edited

Loading

claravanstaden left a comment

claravanstaden Aug 23, 2023

yrong Aug 25, 2023 •

edited

Loading

claravanstaden Aug 23, 2023

yrong Aug 25, 2023 •

edited

Loading

claravanstaden Aug 24, 2023

yrong Aug 25, 2023

yrong commented Aug 25, 2023

yrong commented Aug 25, 2023 •

edited

Loading

yrong commented Aug 25, 2023

yrong commented Aug 25, 2023

yrong commented Feb 16, 2024


		payload = SubstrateTypes.SendToken(address(this), token, destinationChain, destinationAddress, amount, extraFee);

		ParaID internal immutable TEMPLATE_PARA_ID;
		bytes32 internal immutable TEMPLATE_AGENT_ID;

SNO-613: Arbitrary transact from Ethereum to Polkadot #925

SNO-613: Arbitrary transact from Ethereum to Polkadot #925

Conversation

yrong commented Aug 10, 2023 • edited Loading

Prerequisite

Assumptions

Todos

codecov bot commented Aug 10, 2023 • edited Loading

Codecov Report

yrong commented Aug 16, 2023 • edited Loading

alistair-singh Aug 24, 2023

Choose a reason for hiding this comment

yrong Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

alistair-singh commented Aug 24, 2023 • edited Loading

claravanstaden left a comment

Choose a reason for hiding this comment

claravanstaden Aug 23, 2023

Choose a reason for hiding this comment

yrong Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

claravanstaden Aug 23, 2023

Choose a reason for hiding this comment

yrong Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

claravanstaden Aug 24, 2023

Choose a reason for hiding this comment

yrong Aug 25, 2023

Choose a reason for hiding this comment

yrong commented Aug 25, 2023

yrong commented Aug 25, 2023 • edited Loading

yrong commented Aug 25, 2023

yrong commented Aug 25, 2023

yrong commented Feb 16, 2024

yrong commented Aug 10, 2023 •

edited

Loading

codecov bot commented Aug 10, 2023 •

edited

Loading

yrong commented Aug 16, 2023 •

edited

Loading

yrong Aug 25, 2023 •

edited

Loading

alistair-singh commented Aug 24, 2023 •

edited

Loading

yrong Aug 25, 2023 •

edited

Loading

yrong Aug 25, 2023 •

edited

Loading

yrong commented Aug 25, 2023 •

edited

Loading