Transact from eth to sub #1141

yrong · 2024-02-07T16:51:54Z

Resolves: SNO-591,SNO-613,SNO-714
Requires: Snowfork/polkadot-sdk#114

codecov · 2024-02-07T16:56:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (bridge-next-gen@7f41605). Click here to learn what that means.

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

Additional details and impacted files

@@                Coverage Diff                 @@
##             bridge-next-gen    #1141   +/-   ##
==================================================
  Coverage                   ?   79.32%           
==================================================
  Files                      ?       13           
  Lines                      ?      416           
  Branches                   ?       74           
==================================================
  Hits                       ?      330           
  Misses                     ?       69           
  Partials                   ?       17

Flag	Coverage Δ
solidity	`79.32% <100.00%> (?)`

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

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

contracts/src/Gateway.sol

yrong · 2024-02-14T09:36:25Z

Could we start with the teleport pattern as before which will make things a bit easier? Or by no means that third party chain will accept that?

I'm not sure but as we may notice to support transact it requires some custom xcm config anyway and only DOT from BH is required to pay the xcm fee so maybe not a big deal?

vgeddes · 2024-02-14T15:40:59Z

Could we start with the teleport pattern as before which will make things a bit easier? Or by no means that third party chain will accept that?

Nope, unfortunately teleportation is strictly not allowed between system and third party chains.

yrong · 2024-02-14T16:33:08Z

@vgeddes To not use teleport for transact in
Snowfork/polkadot-sdk@51b76fe

alistair-singh · 2024-02-14T22:07:39Z

Nope, unfortunately teleportation is strictly not allowed between system and third party chains.

What if we teleported the DOT after the UniversalOrigin but before DescendOrigin. Then the Target chain would allow teleport of DOT from the Snowbridge Location (GlobalConsensus(Ethereum {chain_id:...})). Only the bridge inbound-queue pallet should be able to assume this location's privilege.

UniversalOrigin(GlobalConsensus(Ethereum)),
ReceiveTeleportedAsset { DOT },
BuyExecution { DOT },
DescendOrigin(AccountKey20 { msg.sender })
Transact { OriginKind = "SovereignAccount" OR "Xcm" }

yrong · 2024-02-15T01:57:35Z

@alistair-singh As Vincent mentioned teleportation is strictly not allowed here so I've removed that in recent commits

vgeddes

First round of review comments..

contracts/src/Gateway.sol

.gitignore

vgeddes · 2024-02-19T12:46:28Z

contracts/src/Types.sol

+    if (
+        message.call.length == 0 || message.fee == 0 || message.weightAtMost.refTime == 0
+            || message.weightAtMost.proofSize == 0


All this validation can be done inline in Gateway.transact. Its very little code and I don't think it requires its own function.

Where did this validation move to? Should it not be done here somewhere?

I see it was removed here: 00abc13 and removed because of @vgeddes comment: #1141 (comment)

Should we not still at least check destinationFee > 0? Ie does it make sense to allow a likely invalid message to be sent and the user to pay for it when it will likely not succeed?

Agree. I think we need these checks on source chain so add it back here 3dd1adc

contracts/src/Types.sol

contracts/src/interfaces/IGateway.sol

contracts/src/Gateway.sol

yrong · 2024-03-06T14:16:18Z

One caveat for Parachain to integrate is that since the fee is paid upfront on Ethereum and UnpaidExecution is sent to the dest chain, the risk potentially is a malicious user can spam the dest chain to call expensive transact with a tiny fee. So It's the responsibility of Parachain to inspect/check the fee passed through can cover the execution cost though not charged.

Custom Xcm config for that In Snowfork/polkadot-sdk@c18f771 could be referenced.

claravanstaden

👏🏻 Awesome!

contracts/src/Gateway.sol

web/packages/test/scripts/configure-substrate.sh

web/packages/test/scripts/set-env.sh

claravanstaden · 2024-04-10T11:13:59Z

@alistair-singh As Vincent mentioned teleportation is strictly not allowed here so I've removed that in recent commits, the current fee flow is:

charge fees from msg.sender and deposit to agent of the dest chain, on Ethereum

transfer fees from sovereign of dest chain to BH, on BH

buy execution and pay fees from sovereign of BH, on dest chain

Does this PR still work this way?

yrong · 2024-04-11T03:35:35Z

Does this PR still work this way?

Nope. Sorry for the confusion, I should have removed it earlier.

claravanstaden · 2024-04-11T06:52:38Z

Does this PR still work this way?

Nope. Sorry for the confusion, I should have removed it earlier.

Would you mind adding a new comment with the updated functionality? That summary is helpful to understand the PR at a high level.

vgeddes · 2024-04-11T11:48:35Z

One caveat for Parachain to integrate is that since the fee is paid upfront on Ethereum and UnpaidExecution is sent to the dest chain, the risk potentially is a malicious user can spam the dest chain to call expensive transact with a tiny fee. So It's the responsibility of Parachain to inspect/check the fee passed through can cover the execution cost though not charged.

Custom Xcm config for that In Snowfork/polkadot-sdk@c18f771 could be referenced.

Good catch. At least with paid execution, the Transact includes the weight required at most, and if the user's supplied execution fee does not cover the maximum transact weight, the xcm will not be executed.

contracts/src/Gateway.sol

alistair-singh · 2024-04-19T00:31:19Z

contracts/src/Gateway.sol

+
+    /// @inheritdoc IGateway
+    function quoteSendCallFee(uint128 destinationFee) external view returns (uint256) {
+        Costs memory costs = _calculateTransactCost(destinationFee);


We can remove destinationfee from here because only delivery costs are paid in the new fee model.

alistair-singh · 2024-04-19T00:50:11Z

rfc/transact_from_ethereum_to_substrate.md

+
+## Explanation
+
+Basically it works as follows:


I would add a fee flow section:

dApp is represents msg.sender or its sovereign depending on context.

Parachain represents the target parachain, its sovereign or its agent depending on context.

Sequence Where Who What

1 Gateway dApp pays(ETH, converted to DOT here) Parachain Agent for delivery costs only (no execution cost)

2 Bridge Hub Relayer pays(DOT) node for execution

3 Bridge Hub Parachain Sovereign pays(DOT) Relayer for delivery (refund+reward)

4 Parachain dApp pays(DOT, Native) for execution only.

Does this balance? Yes because the parachain agent collects fees for delivery on Ethereum side and the Parachain Sovereign pays for delivery on the Bridge Hub side.

Execution costs totally belongs to the dApp prefunding its sovereign on the Parachain.

Thanks for summarizing. Added to rfc in 53791d2

alistair-singh · 2024-05-02T11:02:50Z

contracts/src/Gateway.sol

+    }
+
+    /// @inheritdoc IGateway
+    function quoteSendCallFee(uint128 destinationFee) external view returns (uint256) {


destinationFee can be removed from here, as the quote should only return delivery fees.

Change for contracts

55cc688

vgeddes reviewed Feb 7, 2024

View reviewed changes

contracts/src/Gateway.sol Outdated Show resolved Hide resolved

yrong added 7 commits February 8, 2024 16:08

Convert logic for transact command

07395ce

Reuse msg.sender for both originKinds

d41bd08

Revert to burn&teleport

16e7cbe

Update contract bindings

a01aaab

Update sdk

b660ea8

Merge branch 'main' into ron/transact-from-eth-to-sub

89feaf3

Remove unused

a94183a

yrong requested review from vgeddes, alistair-singh and claravanstaden February 14, 2024 09:38

Add smoke test

7a110d3

yrong marked this pull request as ready for review February 15, 2024 06:17

yrong added 3 commits February 15, 2024 15:14

Improve smoke test

b57f0c1

Add polkadot-sdk as soft link

3ad5b7d

Revert typos config

e93c725

yrong mentioned this pull request Feb 16, 2024

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

Closed

Add valid check

65714ef

vgeddes reviewed Feb 19, 2024

View reviewed changes

contracts/src/Gateway.sol Outdated Show resolved Hide resolved

yrong added 3 commits February 19, 2024 23:52

Remove TransactMessage and inline the params

4719a42

Remove from .gitignore

a78b1f9

Add quoteTransactFee

c192361

yrong added 2 commits March 6, 2024 18:09

Merge branch 'main' into ron/transact-from-eth-to-sub

dd3378e

Support TransactFeeMode

3d91da4

yrong added 3 commits March 7, 2024 18:20

Merge branch 'main' into ron/transact-from-eth-to-sub

ac7e1f5

Merge branch 'main' into ron/transact-from-eth-to-sub

72f6d3e

Remove prefunding mode

68a567f

claravanstaden approved these changes Apr 10, 2024

View reviewed changes

contracts/src/Gateway.sol Outdated Show resolved Hide resolved

web/packages/test/scripts/configure-substrate.sh Outdated Show resolved Hide resolved

web/packages/test/scripts/set-env.sh Outdated Show resolved Hide resolved

Merge branch 'main' into ron/transact-from-eth-to-sub

923187e

yrong mentioned this pull request Apr 11, 2024

Transact from eth to sub Snowfork/polkadot-sdk#114

Open

yrong changed the base branch from main to bridge-next-gen April 11, 2024 02:24

yrong added 3 commits April 11, 2024 10:48

Add param checks back

3dd1adc

Remove script unused

f07507f

Remove the fund script not in use

70413d8

yrong added 4 commits April 12, 2024 00:07

Update relayer

a364298

Merge branch 'bridge-next-gen' into ron/transact-from-eth-to-sub

bdf4c71

Add rfc doc

748eea4

Ignore destination fee for the transact

e27c0eb

alistair-singh reviewed Apr 19, 2024

View reviewed changes

yrong added 5 commits April 22, 2024 15:46

Update rfc

e5c1a70

Update rfc with fee section

53791d2

Cleanup

61d5ea5

Merge branch 'bridge-next-gen' into ron/transact-from-eth-to-sub

dfa331d

Update rfc doc

b935ca8

alistair-singh reviewed May 2, 2024

View reviewed changes

yrong added 2 commits May 5, 2024 23:02

Update rfc

2847751

Remove unused destinationFee

bfdff26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transact from eth to sub #1141

Transact from eth to sub #1141

yrong commented Feb 7, 2024 •

edited

Loading

codecov bot commented Feb 7, 2024 •

edited

Loading

yrong commented Feb 14, 2024 •

edited

Loading

vgeddes commented Feb 14, 2024

yrong commented Feb 14, 2024

alistair-singh commented Feb 14, 2024 •

edited

Loading

yrong commented Feb 15, 2024 •

edited

Loading

vgeddes left a comment

vgeddes Feb 19, 2024

claravanstaden Apr 10, 2024 •

edited

Loading

yrong Apr 11, 2024 •

edited

Loading

yrong commented Mar 6, 2024 •

edited

Loading

claravanstaden left a comment

claravanstaden commented Apr 10, 2024

yrong commented Apr 11, 2024

claravanstaden commented Apr 11, 2024

vgeddes commented Apr 11, 2024

alistair-singh Apr 19, 2024

yrong Apr 24, 2024

alistair-singh Apr 19, 2024

yrong Apr 22, 2024

alistair-singh May 2, 2024

yrong May 5, 2024

Sequence	Where	Who	What
1	Gateway	dApp	pays(ETH, converted to DOT here) Parachain Agent for delivery costs only (no execution cost)
2	Bridge Hub	Relayer	pays(DOT) node for execution
3	Bridge Hub	Parachain Sovereign	pays(DOT) Relayer for delivery (refund+reward)
4	Parachain	dApp	pays(DOT, Native) for execution only.

Transact from eth to sub #1141

Are you sure you want to change the base?

Transact from eth to sub #1141

Conversation

yrong commented Feb 7, 2024 • edited Loading

codecov bot commented Feb 7, 2024 • edited Loading

Codecov Report

yrong commented Feb 14, 2024 • edited Loading

vgeddes commented Feb 14, 2024

yrong commented Feb 14, 2024

alistair-singh commented Feb 14, 2024 • edited Loading

yrong commented Feb 15, 2024 • edited Loading

vgeddes left a comment

Choose a reason for hiding this comment

vgeddes Feb 19, 2024

Choose a reason for hiding this comment

claravanstaden Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

yrong Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

yrong commented Mar 6, 2024 • edited Loading

claravanstaden left a comment

Choose a reason for hiding this comment

claravanstaden commented Apr 10, 2024

yrong commented Apr 11, 2024

claravanstaden commented Apr 11, 2024

vgeddes commented Apr 11, 2024

alistair-singh Apr 19, 2024

Choose a reason for hiding this comment

yrong Apr 24, 2024

Choose a reason for hiding this comment

alistair-singh Apr 19, 2024

Choose a reason for hiding this comment

yrong Apr 22, 2024

Choose a reason for hiding this comment

alistair-singh May 2, 2024

Choose a reason for hiding this comment

yrong May 5, 2024

Choose a reason for hiding this comment

yrong commented Feb 7, 2024 •

edited

Loading

codecov bot commented Feb 7, 2024 •

edited

Loading

yrong commented Feb 14, 2024 •

edited

Loading

alistair-singh commented Feb 14, 2024 •

edited

Loading

yrong commented Feb 15, 2024 •

edited

Loading

claravanstaden Apr 10, 2024 •

edited

Loading

yrong Apr 11, 2024 •

edited

Loading

yrong commented Mar 6, 2024 •

edited

Loading