Transact from sub to eth #1145

yrong · 2024-02-20T16:30:54Z

Resolves: SNO-586,SNO-866,SNO-696
Requires: Snowfork/polkadot-sdk#116

codecov · 2024-02-20T16:33:25Z

Codecov Report

Attention: Patch coverage is 88.00000% with 3 lines in your changes are missing coverage. Please review.

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

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

Files	Patch %	Lines
contracts/src/FundAgent.sol	0.00%	3 Missing ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##             bridge-next-gen    #1145   +/-   ##
==================================================
  Coverage                   ?   79.72%           
==================================================
  Files                      ?       14           
  Lines                      ?      429           
  Branches                   ?       76           
==================================================
  Hits                       ?      342           
  Misses                     ?       71           
  Partials                   ?       16

Flag	Coverage Δ
solidity	`79.72% <88.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.

alistair-singh

Looks good! Just one comment tests.

alistair-singh · 2024-02-27T09:26:22Z

contracts/src/utils/Call.sol

+    /// @notice Use when you _really_ really _really_ don't trust the called
+    /// contract. This prevents the called contract from causing reversion of
+    /// the caller in as many ways as we can.


We should understand how using zexcessivelySafeCall` can still fail? And potentially have tests for at least one case to make sure we don't poisen pill the inbound queue.

Yeah, an unsafe call could be dangerous so we need to be very careful here. Except that I would also like to introduce white-list access control, something like the SafeCallFilter but on Ethereum side.

My thoughts about this:

I think excessivelySafeCall (used by Nomad) is too complicated for our needs. When issuing a call, we are only interested in whether the call succeeded or not. The returndata we can ignore. For this reason, I prefer that we use this safe call solution: https://github.com/ethereum-optimism/optimism/blob/b36eb5515cc2a34a15383b2eee488dbac83d6caf/packages/contracts-bedrock/src/libraries/SafeCall.sol#L12

I am optimistic we've accounted for all the ways a contract call can be unsafe. This includes guards against re-entrancy (nonce check), gas limits, and returnbomb attacks. Therefore I don't see a reason to include a safe call filter. If we think a call can still be unsafe, we need to understand why. For now I would just remove the safe call filter.

@yrong How does SafeCallFilter work? Will we have to restrict allowing transact to only explicitly specified contracts and/or functions? This won't work - we 100% need permissionless support for anyone to integrate transacting with any contract and any function without permission.

And yeah agree with @vgeddes on just ignoring all returndata

How does SafeCallFilter work? Will we have to restrict allowing transact to only explicitly specified contracts and/or functions?

It's not by us or requires governance from the relaychain, but instead from the Parachain sovereign through xcm(i.e. require another call added in our system pallet), they can increase the whitelist step by step entirely under their control.

In this way, we can release the transact support without introducing potential risk.

Ignore returndata 0a8dc1e

Allow arbitray transact without any black(white)-list 606e867

contracts/src/DeployScript.sol

alistair-singh · 2024-04-19T01:43:08Z

rfc/transact_from_substrate_to_ethereum.md

+
+## Explanation
+
+We use penpal for the integration, basically it works as follows:


Fee flow:

User represents a user who kicks off an extrinsic on the parachain.

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

Sequence Where Who What

1 Parachain User pays(DOT, Native) to node to execute custom extrinsic; pays (DOT) to Treasury for delivery as part of custom extrinsic.

2 Bridge Hub Parachain pays(DOT) to Treasury Account for delivery(local fee), pays(DOT) to Parachain sovereign for delivery(remote fee), essentially a refund. Remote fee converted to ETH here.

3 Gateway Relayer pays(ETH) to validate and execute message.

4 Gateway Parachain Agent pays(ETH) to relayer for delivery(reward+refund) and execution.

Does this balance? No, Parachain agent pays for delivery and execution on the Ethereum side. User only pays for calling the extrinsic and delivery on the source Parachain. Because the custom extrinsic essentially allows the user to impersonate the parachain on ethereum side it becomes the Parachains job to charge appropriately for both delivery and execution. This doesn't mean there is something wrong with the implementation, just that this needs to be communication to creator of the custom extrinsic.

For step 1 user pays(DOT) to Treasury for both the delivery cost on BH and execution cost on Ethereum.

https://github.com/Snowfork/polkadot-sdk/blob/569d638d429944fc8d4e2d7db3b653d565879f86/cumulus/parachains/runtimes/testing/penpal/src/pallets/transact_helper.rs#L99-L100

For step 2 the sovereign of Penpal only pays(DOT) for the delivery(local fee) portion, there is no refund in this case.

https://github.com/Snowfork/polkadot-sdk/blob/569d638d429944fc8d4e2d7db3b653d565879f86/bridges/snowbridge/primitives/router/src/outbound/mod.rs#L119-L125

Summarize it in bd88ed8

contracts/src/Gateway.sol

smoketest/tests/transact_from_penpal_to_ethereum.rs

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

vgeddes · 2024-04-29T20:43:20Z

contracts/src/Gateway.sol

+        if (!success) {
+            revert AgentTransactCallFailed();
+        }
+        emit TransactExecuted(params.agentID, params.target, params.payload);


params.payload could be very large, cheaper to hash it when emitting event

Suggested change

emit TransactExecuted(params.agentID, params.target, params.payload);

emit TransactExecuted(params.agentID, params.target, keccak256(params.payload));

vgeddes · 2024-04-29T20:50:00Z

contracts/src/Params.sol

+    /// @dev Payload of the call
+    bytes payload;
+    /// @dev Max gas cost of the call
+    uint64 dynamicGas;


The message packet already has a maxDispatchGas, which is the upper limit a transact can consume. So I am not sure what role this dynamicGas plays.

The gas estimator on BridgeHub should add a buffer to maxDispatchGas to cover the cost of dispatching via the agent.

Yes, it's the same thing. Just rename it for less confusion.
27c14e9

vgeddes · 2024-04-29T20:50:50Z

contracts/src/interfaces/IGateway.sol

@@ -35,6 +35,8 @@ interface IGateway {
    // Emitted when funds are withdrawn from an agent
    event AgentFundsWithdrawn(bytes32 indexed agentID, address indexed recipient, uint256 amount);

+    event TransactExecuted(bytes32 indexed agentID, address indexed target, bytes payload);


Suggested change

event TransactExecuted(bytes32 indexed agentID, address indexed target, bytes payload);

event Transacted(bytes32 indexed agentID, address indexed target, bytes32 payloadHash);

vgeddes · 2024-04-29T20:51:19Z

contracts/src/utils/Call.sol

+    /// @param _calldata The data to send to the remote contract
+    /// @return success and returndata, as `.call()`. Returndata is capped to
+    /// `_maxCopy` bytes.
+    function excessivelySafeCall(address _target, uint256 _gas, uint256 _value, uint16 _maxCopy, bytes memory _calldata)


We're no longer using this, can we remove it?

Update for transact

fe7a814

Add smoke test

89c9e3f

yrong marked this pull request as ready for review February 21, 2024 15:07

yrong requested review from vgeddes, alistair-singh and claravanstaden and removed request for vgeddes and alistair-singh February 22, 2024 07:20

yrong added 2 commits February 22, 2024 17:28

Avoid return bomb attack

5823492

Test revert case

00c5eed

yrong requested a review from alistair-singh February 22, 2024 12:33

alistair-singh reviewed Feb 27, 2024

View reviewed changes

yrong added 3 commits February 29, 2024 14:46

Safe call filter and more tests

ebc8986

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

3d18d1a

Execute with command decoded

8ceb5ee

yrong marked this pull request as draft March 14, 2024 15:29

yrong mentioned this pull request Mar 29, 2024

Asset from polkadot #1155

Merged

yrong added the do-not-merge label Apr 11, 2024

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

yrong added 2 commits April 15, 2024 08:39

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

42f9e07

safeCall ignore returndata

0a8dc1e

yrong marked this pull request as ready for review April 15, 2024 02:36

yrong removed the do-not-merge label Apr 15, 2024

Add the rfc

69305f8

alistair-singh reviewed Apr 19, 2024

View reviewed changes

Update rfc with the fee section

bd88ed8

claravanstaden approved these changes Apr 23, 2024

View reviewed changes

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

smoketest/tests/transact_from_penpal_to_ethereum.rs Outdated Show resolved Hide resolved

Check transact target against blacklist

f6beeb1

yrong and others added 4 commits April 24, 2024 15:45

Update smoketest/tests/transact_from_penpal_to_ethereum.rs

add22c2

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

Merge with main branch

9019ab4

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

1fb852d

Disable token operation through transact

b89cc6c

yrong force-pushed the ron/transact-from-sub-to-eth branch from 1113521 to b89cc6c Compare April 24, 2024 12:56

yrong added 2 commits April 25, 2024 15:05

Allow arbitrary transact

606e867

Update rfc doc

539c180

vgeddes reviewed Apr 29, 2024

View reviewed changes

Make user agent to pay for a portion of the fees

fe513c7

yrong force-pushed the ron/transact-from-sub-to-eth branch from 3dcd8ed to fe513c7 Compare May 2, 2024 03:11

yrong added 6 commits May 2, 2024 11:20

Hash payload when emitting event

7880d46

Rename to maxDispatchGas

27c14e9

Rename event as Transacted

14a40cf

Clean up

0510064

Fix the stack-too-deep error refactoring with _refundRelayer function

c3b7817

Update rfc

b7e85ca

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transact from sub to eth #1145

Transact from sub to eth #1145

yrong commented Feb 20, 2024 •

edited

Loading

codecov bot commented Feb 20, 2024 •

edited

Loading

alistair-singh left a comment

alistair-singh Feb 27, 2024

yrong Feb 27, 2024 •

edited

Loading

yrong Feb 29, 2024

vgeddes Mar 4, 2024 •

edited

Loading

musnit Mar 4, 2024 •

edited

Loading

yrong Mar 5, 2024 •

edited

Loading

yrong Apr 15, 2024 •

edited

Loading

yrong Apr 29, 2024

alistair-singh Apr 19, 2024

yrong Apr 22, 2024 •

edited

Loading

yrong Apr 22, 2024

vgeddes Apr 29, 2024

yrong May 2, 2024

vgeddes Apr 29, 2024

yrong May 2, 2024

vgeddes Apr 29, 2024

yrong May 2, 2024

vgeddes Apr 29, 2024

yrong May 2, 2024


		## Explanation

		We use penpal for the integration, basically it works as follows:

Sequence	Where	Who	What
1	Parachain	User	pays(DOT, Native) to node to execute custom extrinsic; pays (DOT) to Treasury for delivery as part of custom extrinsic.
2	Bridge Hub	Parachain	pays(DOT) to Treasury Account for delivery(local fee), pays(DOT) to Parachain sovereign for delivery(remote fee), essentially a refund. Remote fee converted to ETH here.
3	Gateway	Relayer	pays(ETH) to validate and execute message.
4	Gateway	Parachain Agent	pays(ETH) to relayer for delivery(reward+refund) and execution.

	emit TransactExecuted(params.agentID, params.target, params.payload);
	emit TransactExecuted(params.agentID, params.target, keccak256(params.payload));

	event TransactExecuted(bytes32 indexed agentID, address indexed target, bytes payload);
	event Transacted(bytes32 indexed agentID, address indexed target, bytes32 payloadHash);

Transact from sub to eth #1145

Are you sure you want to change the base?

Transact from sub to eth #1145

Conversation

yrong commented Feb 20, 2024 • edited Loading

codecov bot commented Feb 20, 2024 • edited Loading

Codecov Report

alistair-singh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrong Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgeddes Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

musnit Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

yrong Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

yrong Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrong Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrong commented Feb 20, 2024 •

edited

Loading

codecov bot commented Feb 20, 2024 •

edited

Loading

yrong Feb 27, 2024 •

edited

Loading

vgeddes Mar 4, 2024 •

edited

Loading

musnit Mar 4, 2024 •

edited

Loading

yrong Mar 5, 2024 •

edited

Loading

yrong Apr 15, 2024 •

edited

Loading

yrong Apr 22, 2024 •

edited

Loading