Improve solidity code for PNA by vgeddes · Pull Request #1275 · Snowfork/snowbridge

vgeddes · 2024-08-25T16:10:09Z

Various improvements and cleanups

Changes:

More consistent and descriptive naming style
Simplified token registry
AgentExecutor.sol: Remove deprecated execute method
Foreign tokens are now owned by the gateway rather than the assethub agent. This simplifies code without sacrificing any security & robustness
Moved more asset-specific code to Assets.sol

FIXES: SNO-1133

acatangiu · 2024-08-26T06:08:10Z

Foreign tokens are now owned by the gateway rather than the assethub agent.

What about already registered foreign assets? Are they still using the agent as admin account? Or do you plan to switch the registered admin on AH for them (need to check if that is possible)?

yrong · 2024-08-26T08:10:25Z

Cool, much simple!

Btw: It seems we want to support multi-hop for Ethereum->AH->Para direction. So is the fee token on Para still be relay token? If so will destination chain accept that? Can we always assume it's reserve-backed transfers beween AH and Para(more discussion in https://snowfork.slack.com/archives/G01BGMGLAC9/p1724456977726639thread_ts=1724432403.659139&cid=G01BGMGLAC9)?

My concern here is seems not easy to find a common pattern to support multi-hop which applies to all parachains.

yrong · 2024-08-26T11:20:04Z

-    address public immutable OWNER;
-
-    uint8 public immutable decimals;
+    TokenLib.Token token;


Would suggest to move the token slots to the very end after decimals.

Moved fields around a bit

yrong · 2024-08-26T11:33:01Z

+
+        Token(token).burn(sender, amount);
+
+        ticket.dest = destinationChain;


Is the dest here supposed to be assetHubParaID?

Yeah good catch. I've reimplemented _sendForeignToken to mirror _sendNativeToken. So that PNA assets can be sent to assethub or a further hop from assethub.

yrong · 2024-08-26T11:34:17Z

+        if (destinationAddress.isAddress32()) {
+            // The receiver has a 32-byte account ID
+            ticket.payload = SubstrateTypes.SendForeignTokenToAddress32(
+                foreignID, destinationChain, destinationAddress.asAddress32(), destinationChainFee, amount
+            );
+        } else if (destinationAddress.isAddress20()) {
+            // The receiver has a 20-byte account ID
+            ticket.payload = SubstrateTypes.SendForeignTokenToAddress20(
+                foreignID, destinationChain, destinationAddress.asAddress20(), destinationChainFee, amount
+            );
+        } else {
+            revert Unsupported();
+        }


Need to double check the execution cost on AH(i.e. assetHubReserveTransferFee) is included.

For now I'm assuming assetHubReserveTransferFee (which is configured to have considerable buffer) will cover costs for xcm execution on AH for both ENA and PNA flows.

We should probably rename the field to something like assetHubXcmFee

vgeddes · 2024-08-26T21:29:04Z

Foreign tokens are now owned by the gateway rather than the assethub agent.

What about already registered foreign assets? Are they still using the agent as admin account? Or do you plan to switch the registered admin on AH for them (need to check if that is possible)?

This is an Ethereum-specific addition that does not involve AH. Currently there are only native tokens (aka Ethereum-native ERC20s) registered in the Gateway contract, and on AH too.

This change gives our gateway contract the ability to be aware of foreign assets on AH for example the Hydra or Kilt token in the ForeignAssets pallets.

So it does affect the code paths that involve ethereum-native assets such as WETH or WBTC.

acatangiu · 2024-08-27T09:16:10Z

Foreign tokens are now owned by the gateway rather than the assethub agent.

What about already registered foreign assets? Are they still using the agent as admin account? Or do you plan to switch the registered admin on AH for them (need to check if that is possible)?

This is an Ethereum-specific addition that does not involve AH. Currently there are only native tokens (aka Ethereum-native ERC20s) registered in the Gateway contract, and on AH too.

This change gives our gateway contract the ability to be aware of foreign assets on AH for example the Hydra or Kilt token in the ForeignAssets pallets.

So it does affect the code paths that involve ethereum-native assets such as WETH or WBTC.

I see. Sounds good! Thanks for the explanation 👍🏻

vgeddes · 2024-08-27T17:59:18Z

Cool, much simple!

Btw: It seems we want to support multi-hop for Ethereum->AH->Para direction. So is the fee token on Para still be relay token? If so will destination chain accept that? Can we always assume it's reserve-backed transfers beween AH and Para(more discussion in https://snowfork.slack.com/archives/G01BGMGLAC9/p1724456977726639thread_ts=1724432403.659139&cid=G01BGMGLAC9)?

My concern here is seems not easy to find a common pattern to support multi-hop which applies to all parachains.

One idea I had for the Ethereum->AH->Parachain flow regarding fees on the final destination parachain.

We already take DOT from AH's SA on BH to pay for XCM execution on AH. So we could go on step further and on AH we swap some of that DOT for the native token of the destination parachain.

yrong · 2024-08-28T00:12:19Z

go on step further and on AH we swap some of that DOT for the native token of the destination parachain.

Sounds good. Can we make the fee asset of destination Para configurable when register token here?

So for chains does allow AH-DOT as fee asset, swap is not required.

Updates: Seems swap heavily depends on these PRs which are not ready yet, also not exist in our forked repo.

So I would prefer to make things easy just use AH-DOT as fee for PNA and move the swap improvement to a seperate PR in future when it's included in https://github.com/polkadot-fellows/runtimes, I would assume it applies also to the legacy Ethereum native asset like WETH.

yrong · 2024-08-28T00:48:47Z

+                ticket.payload = SubstrateTypes.SendTokenToAssetHubAddress32(
+                    token, destinationAddress.asAddress32(), $.assetHubReserveTransferFee, amount
+                );


On substrate side we always expect foreignID for PNA, not the token address.

https://github.com/Snowfork/polkadot-sdk/blob/f1155b124813eb535e19e0ad5d2b31f0135b6a8b/bridges/snowbridge/primitives/router/src/inbound/mod.rs#L63

Fixed in 57fa08c

* Asset from polkadot * Remove params unnecessary * Experiment: return calldata from Agent instead of call Gateway directly * To store registries also on agent * Send polkadot tokens back * Cleanup for not exceed the size limit * Add smoke test for register polkadot token * Transfer relay token to Ethereum * Reuse IGateway.sendToken for polkadot native asset * More cleanup * Fix encode substrate types * Add smoke test send relay token back * Rename to sendForeignToken * Improve ERC20.sol * Revert the change double register the token * Migration for AssetsStorage * Rename to _sendForeignTokenCosts * Use storage for less gas * Rename to tokenAddressOf * Rename as _burnToken * _ensureAgent in Gateway * Remove unused * Clean up smoke test * Terminate register foreign token in Gateway * Mint foreign token without the callback * Burn token without the callback * constructor ERC20 with the agent as owner * Move handler to Assets.sol reduce contract size * Refactoring to move transferToken to Assets * Remove TokenMinted&TokenBurned event * Move Command.RegisterToken to top level * Fix smoke tests * Move the check before interactions * Polish * Introduce ERC20Lib.sol * Remove unused * More test * Remove AgentExecuteCommand * More contract tests * Update contract address * Add AgentExecuteCommand back for compatibility * Cleanup * Add rfc * Update polkadot-native-assets.md * Update rfc * Update rfc with fee section * Fix _calculateFee for polkadot native token * Improve solidity code for PNA (#1275) * Improve PNA implementation * More improvents to ERC20 token * Fix implementation of Assets._sendForeignToken * Fix build payload for foreign token --------- Co-authored-by: ron <yrong1997@gmail.com> * Remove storage migration * Smoke test for register relay token * Fix fee estimation * Fix binding * Remove outdated doc * Fork upgrade test with sanity checks * Fix test --------- Co-authored-by: Vincent Geddes <117534+vgeddes@users.noreply.github.com>

Improve PNA implementation

c513cbb

vgeddes requested a review from yrong August 25, 2024 16:10

yrong reviewed Aug 26, 2024

View reviewed changes

vgeddes added 2 commits August 27, 2024 19:04

More improvents to ERC20 token

53608da

Fix implementation of Assets._sendForeignToken

e1bd6c5

yrong reviewed Aug 28, 2024

View reviewed changes

Fix build payload for foreign token

57fa08c

yrong approved these changes Aug 28, 2024

View reviewed changes

yrong merged commit 84bb52c into ron/polkadot-assets-on-ethereum Aug 28, 2024

yrong deleted the vincent/polkadot-assets-on-ethereum branch August 28, 2024 11:24


		Token(token).burn(sender, amount);

		ticket.dest = destinationChain;

Conversation

vgeddes commented Aug 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acatangiu commented Aug 26, 2024

Uh oh!

yrong commented Aug 26, 2024

Uh oh!

yrong Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vgeddes Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

yrong Aug 26, 2024

Choose a reason for hiding this comment

Uh oh!

vgeddes Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

yrong Aug 26, 2024

Choose a reason for hiding this comment

Uh oh!

vgeddes Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

vgeddes commented Aug 26, 2024

Uh oh!

acatangiu commented Aug 27, 2024

Uh oh!

vgeddes commented Aug 27, 2024

Uh oh!

yrong commented Aug 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yrong Aug 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yrong Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vgeddes commented Aug 25, 2024 •

edited

Loading

yrong Aug 26, 2024 •

edited

Loading

yrong commented Aug 28, 2024 •

edited

Loading

yrong Aug 28, 2024 •

edited

Loading