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)

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

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

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

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 added 12 commits January 10, 2025 11:04

initial support

77397c1

add error

f6084c6

fixed tests

77bf8fa

test legacy and new unlock messages

0cc7131

fix spelling

49a7672

test registration is blocked

ec3d473

fix inbound messages

4ee66a9

allow gateway proxy to receive funds

82b0a07

final tests and fixes

2ddb7c6

warnings

f6cbde3

PR feedback

57b1d1a

migrate ether on upgrade

0257217

alistair-singh added 10 commits January 10, 2025 11:06

fix scripts

21e9221

update bindings

af3f8fa

e2e tests

1ce5903

fix assert

72be4de

duplicated error removal

f46da12

fix test fixture generator

98bcf13

template fixes

8f28178

contract address changes

e64b9a8

contract address change

f962ed6

Ignore AgentTransferFromNative bridge command (#1359)

757bcc0

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

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

alistair-singh added 2 commits January 10, 2025 11:18

fmt

694c2f4

merge conflicts

b5a553f

vgeddes reviewed Jan 15, 2025

View reviewed changes

alistair-singh added 7 commits January 16, 2025 15:06

Rename and remove parameter

aca3846

added sendEth sanity check

fe702f0

register ether on contruction and migration

a3e5aa4

remove extra branches

d7b6b40

simplify math

1a5b119

separate branches for ether and native tokens

d8a9a58

fix comments

abc1d95

vgeddes reviewed Jan 17, 2025

View reviewed changes

factor out emitting of sendToken

0921ff9

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

vgeddes Jan 17, 2025

alistair-singh Jan 17, 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

alistair-singh commented Dec 12, 2024 •

edited

Loading

codecov bot commented Dec 17, 2024 •

edited

Loading