Support Native ETH in v1 #1354

alistair-singh · 2024-12-12T08:46:45Z

Changes

Use the 0x0000000000000000000000000000000000000000 token address as Native ETH.
When sendToken is called with the 0x00.. an amount of msg.value is locked in asset hub agent.
To avoid clashing with fees/rewards and locked funds, fees/rewards are collected and dispensed from the Gateway Proxy account.
The existing funds in the agents will be migrated on upgrade.
Scripts which mentions FundAgent are now called FundGateway

Related polkadot-sdk: paritytech/polkadot-sdk#6855

TODO

Fix tests
Add tests for the Eth asserting msg.value.
Migrate agent funds to gateway proxy on upgrade

Resolves: https://linear.app/snowfork/issue/SNO-1222

codecov · 2024-12-17T23:00:48Z

Codecov Report

Attention: Patch coverage is 87.80488% with 5 lines in your changes missing coverage. Please review.

Project coverage is 76.58%. Comparing base (24b9d31) to head (bb130a4).
Report is 158 commits behind head on main.

Files with missing lines	Patch %	Lines
contracts/src/Assets.sol	84.21%	3 Missing ⚠️
contracts/src/upgrades/Gateway202410.sol	77.77%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1354      +/-   ##
==========================================
- Coverage   77.83%   76.58%   -1.25%     
==========================================
  Files          14       17       +3     
  Lines         415      739     +324     
  Branches       76      110      +34     
==========================================
+ Hits          323      566     +243     
- Misses         75      162      +87     
+ Partials       17       11       -6

Flag	Coverage Δ
solidity	`76.58% <87.80%> (-1.25%)`	⬇️

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

contracts/src/Types.sol

contracts/src/GatewayProxy.sol

alistair-singh · 2024-12-18T22:31:09Z

@vgeddes we have a Command called TransferNativeFromAgent which is a management-style command, issuable from bridgehub only, that can empty an agent account of ETH. We would need to add a similar command TransferNativeFromGateway to do the same thing for the Gateway. We should also maybe disable TransferNativeFromAgent so that there is no way to remove ETH from an agent other than an Unlock command.

yrong

+1

Changes: 1. Use the 0x0000000000000000000000000000000000000000 token address as Native ETH. 2. Convert it to/from `{ parents: 2, interior: X1(GlobalConsensus(Ethereum{chain_id: 1})) }` when encountered. Onchain changes: This will require a governance request to register native ETH (with the above location) in the foreign assets pallet and make it sufficient. Related solidity changes: Snowfork/snowbridge#1354 TODO: - [x] Emulated Tests --------- Co-authored-by: Vincent Geddes <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Bastian Köcher <[email protected]>

Changes: 1. Use the 0x0000000000000000000000000000000000000000 token address as Native ETH. 2. Convert it to/from `{ parents: 2, interior: X1(GlobalConsensus(Ethereum{chain_id: 1})) }` when encountered. Onchain changes: This will require a governance request to register native ETH (with the above location) in the foreign assets pallet and make it sufficient. Related solidity changes: Snowfork/snowbridge#1354 TODO: - [x] Emulated Tests --------- Co-authored-by: Vincent Geddes <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> (cherry picked from commit 4059282)

vgeddes · 2025-01-15T11:26:13Z

contracts/src/AgentExecutor.sol

    function transferNative(address payable recipient, uint256 amount) external {
        recipient.safeNativeTransfer(amount);
    }

+    /// @dev Transfer ether to Gateway. Used once off for migration purposes. Can be removed after version 1.
+    function transferNativeToGateway(address payable gateway, uint256 amount) external {


In this context, msg.sender will always be the gateway address, as verified by Agent.invoke. So having a gateway parameter is redundant.

Also, prefer this naming in this context:

Suggested change

function transferNativeToGateway(address payable gateway, uint256 amount) external {

function transferEtherToGateway(address payable gateway, uint256 amount) external {

Addressed in aca3846

vgeddes · 2025-01-15T11:32:34Z

contracts/src/Assets.sol

@@ -60,7 +62,7 @@ library Assets {
    ) external view returns (Costs memory costs) {
        AssetsStorage.Layout storage $ = AssetsStorage.layout();
        TokenInfo storage info = $.tokenRegistry[token];
-        if (!info.isRegistered) {
+        if (!info.isRegistered && token != address(0)) {


Why not just register ether as a token, so we can avoid the complexity of this check?

Addressed in a3e5aa4

vgeddes · 2025-01-15T11:37:55Z

contracts/src/Assets.sol

@@ -144,7 +148,18 @@ library Assets {
        AssetsStorage.Layout storage $ = AssetsStorage.layout();

        // Lock the funds into AssetHub's agent contract
-        _transferToAgent($.assetHubAgent, token, sender, amount);
+        if (token != address(0)) {


I think the way you've introduced native ether support adds complexity by adding a lot of decision branches at the lowest level.

We already have branches at a higher level to choose between foreign, native tokens. We can add a branch for eth support there.

Concretely, I would suggest adding a new _sendEther function instead of adding branches in _sendNativeToken.

Addressed in d8a9a58

It looks like you reverted the _sendEther and _sendNativeToken split and merged it again. Just out of interest why?

vgeddes · 2025-01-17T12:44:35Z

contracts/src/Assets.sol

+        payable($.assetHubAgent).safeNativeTransfer(amount);
+        ticket.value = amount;
+
+        ticket.dest = $.assetHubParaID;


It seems that both _sendEther and _sendNativeToken share this ticket/payload creation code. Can we not factor that out?

addressed in 0921ff9

claravanstaden

Awesome feature! 🥳

contracts/src/Assets.sol

contracts/src/Gateway.sol

claravanstaden · 2025-01-21T08:07:24Z

web/packages/api/src/environment.ts

@@ -65,7 +65,7 @@ export const SNOWBRIDGE_ENV: { [id: string]: SnowbridgeEnvironment } = {
                erc20tokensReceivable: [
                    {
                        id: "WETH",
-                        address: "0x87d1f7fdfEe7f651FaBc8bFCB6E086C278b77A7d",
+                        address: "0x774667629726ec1FaBEbCEc0D9139bD1C8f72a23",


Do you know why the WETH address changed?

vgeddes · 2025-01-22T19:37:39Z

contracts/src/upgrades/Gateway202410.sol

        // Ensure that arbitrary users cannot initialize storage in this logic contract.
        if (ERC1967.load() == address(0)) {
            revert Unauthorized();
        }
+
+        // We expect version 0, deploying version 1.
+        CoreStorage.Layout storage $ = CoreStorage.layout();


What's this for?

For the next upgrade after this, we'll deploy a new contract like Gateway202506.sol with different initializer code.

It stops us from deploying this instance of Gateway202410 twice and hence running the same migration twice. I know this is controlled by governance, but an explicit way to do this in code is better.

Sure, we can consider adding a version check, but I don't think this PR is the right place for it.

I will remove the version check for the time being.

* do not dispatch transfer from native command * test to make sure the behavior is off * fmt * naming

alistair-singh · 2025-01-25T23:14:37Z

contracts/src/Assets.sol

+        ticket.dest = $.assetHubParaID;
+        ticket.costs = _sendTokenCosts(destinationChain, destinationChainFee, maxDestinationChainFee);
+
+        // Construct a message payload
+        if (destinationChain == $.assetHubParaID) {
+            // The funds will be minted into the receiver's account on AssetHub
+            if (destinationAddress.isAddress32()) {
+                // The receiver has a 32-byte account ID
+                ticket.payload = SubstrateTypes.SendTokenToAssetHubAddress32(
+                    token, destinationAddress.asAddress32(), $.assetHubReserveTransferFee, amount
+                );
+            } else {
+                // AssetHub does not support 20-byte account IDs
+                revert Unsupported();
+            }
+        } else {
+            if (destinationChainFee == 0) {
+                revert InvalidDestinationFee();
+            }
+            // The funds will be minted into sovereign account of the destination parachain on AssetHub,
+            // and then reserve-transferred to the receiver's account on the destination parachain.
+            if (destinationAddress.isAddress32()) {
+                // The receiver has a 32-byte account ID
+                ticket.payload = SubstrateTypes.SendTokenToAddress32(
+                    token,
+                    destinationChain,
+                    destinationAddress.asAddress32(),
+                    $.assetHubReserveTransferFee,
+                    destinationChainFee,
+                    amount
+                );
+            } else if (destinationAddress.isAddress20()) {
+                // The receiver has a 20-byte account ID
+                ticket.payload = SubstrateTypes.SendTokenToAddress20(
+                    token,
+                    destinationChain,
+                    destinationAddress.asAddress20(),
+                    $.assetHubReserveTransferFee,
+                    destinationChainFee,
+                    amount
+                );
+            } else {
+                revert Unsupported();
+            }
+        }
+
+        emit IGateway.TokenSent(token, sender, destinationChain, destinationAddress, amount);


This block can be replaced by calling _emitSendNativeTokenTicket.

alistair-singh · 2025-01-25T23:15:28Z

contracts/src/Assets.sol

@@ -144,7 +148,18 @@ library Assets {
        AssetsStorage.Layout storage $ = AssetsStorage.layout();

        // Lock the funds into AssetHub's agent contract
-        _transferToAgent($.assetHubAgent, token, sender, amount);
+        if (token != address(0)) {


It looks like you reverted the _sendEther and _sendNativeToken split and merged it again. Just out of interest why?

github-actions bot added Component: Ethereum Component: Smoke Tests Component: Web labels Dec 12, 2024

alistair-singh mentioned this pull request Dec 12, 2024

Snowbridge: Support bridging native ETH paritytech/polkadot-sdk#6855

Merged

1 task

alistair-singh force-pushed the alistair/support-eth branch 2 times, most recently from c2835dd to b75401c Compare December 17, 2024 19:22

github-actions bot removed the Component: Smoke Tests label Dec 17, 2024

alistair-singh marked this pull request as ready for review December 17, 2024 23:52

yrong reviewed Dec 18, 2024

View reviewed changes

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

vgeddes reviewed Dec 18, 2024

View reviewed changes

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

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

github-actions bot added Component: Relayer Component: Smoke Tests labels Dec 19, 2024

yrong approved these changes Jan 1, 2025

View reviewed changes

alistair-singh mentioned this pull request Jan 7, 2025

Auto Wrap/Unwrap WETH #1343

Closed

alistair-singh force-pushed the alistair/support-eth branch from c778b96 to 757bcc0 Compare January 10, 2025 09:06

vgeddes reviewed Jan 15, 2025

View reviewed changes

vgeddes reviewed Jan 17, 2025

View reviewed changes

claravanstaden approved these changes Jan 21, 2025

View reviewed changes

vgeddes self-assigned this Jan 22, 2025

vgeddes reviewed Jan 22, 2025

View reviewed changes

alistair-singh added 6 commits January 23, 2025 00:09

initial support

2c5e273

add error

3f178ef

fixed tests

5887da7

test legacy and new unlock messages

4677fe7

fix spelling

13b59fb

test registration is blocked

6d6f3fc

alistair-singh added 27 commits January 23, 2025 00:09

fix inbound messages

299c197

allow gateway proxy to receive funds

5aad78d

final tests and fixes

dfcfb6c

warnings

3c0d826

PR feedback

1212459

migrate ether on upgrade

e75906a

fix scripts

182d6ed

update bindings

61c52b0

e2e tests

db45375

fix assert

a44d093

duplicated error removal

9b67398

fix test fixture generator

2b9173e

template fixes

e209869

contract address changes

bb55b2a

contract address change

7359dba

Ignore AgentTransferFromNative bridge command (#1359)

366c457

* do not dispatch transfer from native command * test to make sure the behavior is off * fmt * naming

fmt

3ac9646

merge conflicts

aaa8139

Rename and remove parameter

a12492b

added sendEth sanity check

a149663

register ether on contruction and migration

566f2fb

remove extra branches

6ed7140

simplify math

f86a6d8

separate branches for ether and native tokens

bd1548e

fix comments

e45c9ee

factor out emitting of sendToken

2699ebe

comment

ddad89f

alistair-singh force-pushed the alistair/support-eth branch from 0921ff9 to ddad89f Compare January 22, 2025 22:17

Refactor

b972591

alistair-singh commented Jan 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Native ETH in v1 #1354

Support Native ETH in v1 #1354

alistair-singh commented Dec 12, 2024 •

edited

Loading

codecov bot commented Dec 17, 2024 •

edited

Loading

alistair-singh commented Dec 18, 2024

yrong left a comment

vgeddes Jan 15, 2025

alistair-singh Jan 16, 2025

vgeddes Jan 15, 2025

alistair-singh Jan 16, 2025

vgeddes Jan 15, 2025

alistair-singh Jan 16, 2025

alistair-singh Jan 25, 2025

vgeddes Jan 17, 2025

alistair-singh Jan 17, 2025

claravanstaden left a comment

claravanstaden Jan 21, 2025

vgeddes Jan 22, 2025

alistair-singh Jan 22, 2025

vgeddes Jan 23, 2025

alistair-singh Jan 25, 2025

alistair-singh Jan 25, 2025

	function transferNativeToGateway(address payable gateway, uint256 amount) external {
	function transferEtherToGateway(address payable gateway, uint256 amount) external {

Support Native ETH in v1 #1354

Are you sure you want to change the base?

Support Native ETH in v1 #1354

Conversation

alistair-singh commented Dec 12, 2024 • edited Loading

codecov bot commented Dec 17, 2024 • edited Loading

Codecov Report

alistair-singh commented Dec 18, 2024

yrong left a 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

Choose a reason for hiding this comment

claravanstaden left a 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

alistair-singh commented Dec 12, 2024 •

edited

Loading

codecov bot commented Dec 17, 2024 •

edited

Loading