Contracts V2 #1300

vgeddes · 2024-10-07T17:35:37Z

Tasks:

contracts/src/utils/SparseBitmap.sol

contracts/src/types/V2.sol

contracts/src/Gateway.sol

contracts/src/interfaces/IGateway.sol

contracts/src/types/V2.sol

codecov · 2024-10-19T11:40:27Z

Codecov Report

Attention: Patch coverage is 51.76152% with 178 lines in your changes missing coverage. Please review.

Please upload report for BASE (v2@6d11768). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
contracts/src/Gateway.sol	27.04%	88 Missing and 1 partial ⚠️
contracts/src/v2/Calls.sol	0.00%	50 Missing ⚠️
contracts/src/v2/Handlers.sol	0.00%	21 Missing ⚠️
contracts/src/Functions.sol	80.95%	8 Missing ⚠️
contracts/src/v1/Calls.sol	90.90%	5 Missing ⚠️
contracts/src/AgentExecutor.sol	0.00%	3 Missing ⚠️
contracts/src/upgrades/polkadot/Gateway202411.sol	33.33%	2 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff          @@
##             v2    #1300   +/-   ##
=====================================
  Coverage      ?   60.22%           
=====================================
  Files         ?       23           
  Lines         ?      714           
  Branches      ?      120           
=====================================
  Hits          ?      430           
  Misses        ?      275           
  Partials      ?        9

Flag	Coverage Δ
solidity	`60.22% <51.76%> (?)`

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.

yrong · 2024-10-20T03:24:01Z

contracts/src/v2/Calls.sol

+        _ensureOutboundMessagingEnabled();
+
+        // Wrap provided ether and transfer to AssetHub agent
+        address assetHubAgent = Functions.ensureAgent(Constants.ASSET_HUB_AGENT_ID);


Since we know agent of asset-hub do exists in production. Is this check nessessary?

Yeah the check is not necessary, I'll add a TODO.

contracts/src/v2/Calls.sol

contracts/src/SubstrateTypes.sol

yrong · 2024-10-20T03:45:40Z

contracts/src/v2/Calls.sol

+        bytes memory payload =
+            SubstrateTypes.encodePayloadV2(ticket.origin, ticket.assets, ticket.xcm);


Cool, A lot more simple now!

With ticket.xcm does that mean for V2 there is no need to construct xcm on chain any more?

UI will build the xcm and it's the responsibility of relayer to validate the xcm and check the relay is profitable or not.

On BH we just decode the xcm with

VersionedXcm::<()>::decode_with_depth_limit( MAX_XCM_DECODE_DEPTH, &mut data, )

and forward it to AH transparently.

Yeah, exactly.

Though for registration of native tokens, we need to control the XCM and so we need to build it on chain. See more details in 149c3e4.

Though for registration of native tokens, control the XCM and so we need to build it on chain

Still not sure fully understand why ticket.assets is required here, why not just build the xcm off chain?

@vgeddes I've removed the MessageConverter completely in yrong/polkadot-sdk#3 which I'd assume is not in use any more, correct me if I'm wrong.

Still not sure fully understand why ticket.assets is required here, why not just build the xcm off chain?

When the message gets to BH, we convert the payload.assets to instructions like ReserveAssetDeposited,etc, and then append the instructions in payload.xcm.

Or in other words, BH builds final XCM using:

finalXCM = concat( assets_to_xcm(payload.asssets), UniversalOrigin(Ethereum) DescendOrigin(payload.origin) payload.xcm )

We can't trust the user to build the entire xcm offchain as they could mint any assets they want. Rather, they can only supply a fragment of the xcm.

We can't trust the user to build the entire xcm offchain as they could mint any assets they want.

Yeah, that make sense.

Instead of convert the assets to xcm, can we just do some basic check on BH to verify the xcm is constructed correctly based upon the assets? e.g.

ensure!(xcm contains DescendOrigin(payload.origin)) ensure!(xcm contains ReserveAssetDeposited(asset)|WithdrawAsset(asset)) // Depends on the transfer type is ENA or PNA ...

So the point here is on chain we do validate rather than construct.

Btw: for transfer I'm not sure DescendOrigin(userOrigin) make sense. For PNA we need to burn the asset locked in Ethereum sovereign on AH. IIUC that's why we change origin to UniversalOrigin(GlobalConsensus(network)) before WithdrawAsset here.

So the point here is on chain we do validate rather than construct.

The issue with this approach is that the validation needs to happen on the Ethereum side in order to do this right. The actual locking/unlocking/minting/burning of tokens occurs on the Ethereum side in solidity, which must match the Assets section of the XCM. If the validation happens on BH or in a relayer, and we reject the message, it is too late because we have already changed the state on the Ethereum side. This is why it is better to build the assets section of the XCM on BH to match what was done on the Ethereum side.

Related comment here: yrong/polkadot-sdk#3 (comment)

it is too late because we have already changed the state on the Ethereum side

Just curious what will happen in this case. Is this something acceptable assuming we will implement error handing mechanism any way? The message with errored xcm will get trapped on BH and we allow end user to edit/repair it?

Something like we allow user to add fee for message pending from Substrate to Ethereum direction, assuming there is no relayer who will pick it up if it's not profitable.

Just curious what will happen in this case. Is this something acceptable assuming we will implement error handing mechanism any way?

I think if the message is queued and if the message is profitable, than once submitted to BH we should not fail for any reason (such as validating the xcm for example) and we should try to always push the assets to AH where they can get trapped if the user passes invalid xcm. And we should trust that we constrain the origin to stop users from doing things that they cannot within the xcm, instead of validating.

contracts/src/v2/Calls.sol

yrong · 2024-10-22T12:46:47Z

contracts/src/Gateway.sol

+    /// @param leafProof A message proof used to verify that the message is in the merkle tree committed by the OutboundQueue pallet
+    /// @param headerProof A proof that the commitment is included in parachain header that was finalized by BEEFY.
+    /// @param rewardAddress Account on BH to credit delivery rewards
+    function v2_submit(


should it also be in IGateway.sol? Meanwhile we may need another function isRelayed(nonce) to check if the nonce has been relayed.

Good catch!

contracts/src/Gateway.sol

yrong · 2024-10-24T00:31:01Z

contracts/src/v2/Types.sol

+
+struct Command {
+    uint8 kind;
+    uint256 gas;


Gas is uint64.

contracts/src/Constants.sol

alistair-singh · 2024-10-24T22:15:11Z

contracts/test/SparseBitmap.t.sol

+
+    function testGetSet() public {
+        bitmap.set(384);
+        assertEq(bitmap.get(384), true);


Maybe test the bits around the set bit to make sure they remain unset.

Suggested change

assertEq(bitmap.get(384), true);

assertEq(bitmap.get(383), false);

assertEq(bitmap.get(384), true);

assertEq(bitmap.get(385), false);

alistair-singh · 2024-10-24T22:27:10Z

contracts/src/v2/Calls.sol

+        bytes memory payload =
+            SubstrateTypes.encodePayloadV2(ticket.origin, ticket.assets, ticket.xcm);


So the point here is on chain we do validate rather than construct.

The issue with this approach is that the validation needs to happen on the Ethereum side in order to do this right. The actual locking/unlocking/minting/burning of tokens occurs on the Ethereum side in solidity, which must match the Assets section of the XCM. If the validation happens on BH or in a relayer, and we reject the message, it is too late because we have already changed the state on the Ethereum side. This is why it is better to build the assets section of the XCM on BH to match what was done on the Ethereum side.

Related comment here: yrong/polkadot-sdk#3 (comment)

alistair-singh · 2024-10-24T23:07:19Z

contracts/src/v2/Calls.sol

+
+    // Register Ethereum-native token on AHK, using `xcmFeeAHP` and `xcmFeeAHK`
+    // of `msg.value` to pay for execution on AHP and AHK respectively.
+    function registerTokenOnKusama(address token, uint128 xcmFeeAHP, uint128 xcmFeeAHK)


Maybe we should introduce a Network Enum here as a destination parameter, instead of having 2 registers methods. The network can simply contain the Polkadot networks.
https://github.com/paritytech/polkadot-sdk/blob/master/polkadot/xcm/src/v4/junction.rs/#L125

alistair-singh · 2024-10-24T23:24:15Z

contracts/src/v2/Calls.sol

+    error TokenNotRegistered();
+    error InvalidAsset();
+
+    address public constant WETH_ADDRESS = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2;


Hardcoded WETH_ADDRESS?

yeah, why would it need to be mutable?

Oh, I see, for Ethereum testnets, WETH will have a different address. got it. 👍

alistair-singh · 2024-10-25T22:05:01Z

contracts/src/Gateway.sol

-            revert ChannelDoesNotExist();
-        }
+    // See docs for `IGateway.sendMessage`
+    function sendMessage(bytes calldata xcm, bytes[] calldata assets) external payable {


I think we should add a MultiAddress parameter that can be the asset claimer. If provided then we can use this account with the SetClaimer XCM instruction. If it is zero/empty address then we SetClaimer to the origin(msg.sender).

yrong · 2024-10-27T16:53:46Z

contracts/src/Gateway.sol


-        emit IGateway.OutboundMessageAccepted(channelID, channel.outboundNonce, messageID, ticket.payload);
+        emit IGateway.InboundMessageDispatched(message.nonce, success, rewardAddress);


Would it be better to store the delivery status on chain? e.g:

mapping(uint64 nonce => DeliveryStatus) public nonces; struct DeliveryStatus { uint64 blockNumber; bool result; }

With the blockNumber it's easy for relayer to filter this event efficiently when submit the delivery proof.

That's a good point Ron. Though there are some simplifying assumptions which I think allow us to save 20000 gas by not storing that status on-chain.

The plan I had in mind was that the relayer who calls v2_submit is also responsible for sending delivery proof back to BH. Since this relayer witnessed the result for v2_submit it can generate a proof for InboundMessageDispatched, and there isn't a need for additional metadata on-chain.

For other cases when the delivery proof could not be sent back immediately, we could create a command in snowbridge-relay that queries for InboundMessageDispatched(rewardAddress=X) against some public ethereum node or event indexer, and sends the delivery proofs for the matching events.

I will make a rewardAddress an indexed event parameter, which allows this query to be much faster to execute.

yrong · 2024-10-28T07:56:03Z

contracts/src/v2/Types.sol

+import {OperatingMode} from "./../types/Common.sol";
+
+// Inbound message from a Polkadot parachain (via BridgeHub)
+struct InboundMessage {


would suggest to rename as InboundMessageV2 or conflict error when generate go bindings.

> mage build 2024/10/28 15:53:26 INFO: Skipped ssz generated object: relays/beacon/state/beacon_encoding.go 2024/10/28 15:53:26 INFO: Skipped ssz generated object: relays/beacon/state/beacon_deneb_encoding.go # github.com/snowfork/snowbridge/relayer/contracts contracts/gateway.go:47:6: InboundMessage redeclared in this block contracts/gateway.go:40:6: other declaration of InboundMessage Error: running "go build -o build/snowbridge-relay main.go" failed with exit code 1

af374a6 FYI

yrong · 2024-10-28T08:20:15Z

contracts/src/interfaces/IGateway.sol

+    ) external payable;
+
+    // Check if an inbound message was previously accepted and dispatched
+    function v2_isDispatched(uint64 nonce) external returns (bool);


Should add view keyword here.

2e8d519

alistair-singh · 2024-10-29T23:55:37Z

contracts/src/interfaces/IGateway.sol

+
+    // Register Ethereum-native token on AHK, using `xcmFeeAHP` and `xcmFeeAHK`
+    // of `msg.value` to pay for execution on AHP and AHK respectively.
+    function v2_registerTokenOnKusama(


Just a thought on registration while thinking about our Security model. Kusama token registration can be built as a separate contract on top of the gateway.

The implementation of a contract called KusamaRegistration, that can build the registration Transact call onchain, and then submit it to the gateway.

On BH we would decend origin to that Location so the origin would be { parents:2 interior: X2(Ethreum{chain_id:1}, KusamaRegistration)}.

This origin would be forwarded to AHK using AliasOrigin, and can be used secure the register extrinsic on the foreignAssets pallet on AHK. This is currently how we secure token registration now on AHP, using the gateway contracts origin (Snowbridge Sovereign).

Upgrade would be just deploying a new version of the KusamaRegistration contract and then updating the origin which is allowed to call register on AHK. And it would be separate from the Gateway.

claravanstaden

Nice!

I was wondering if it would be feasible to add Transact support in this release, given that all the building blocks are there?

claravanstaden · 2024-10-21T13:32:35Z

contracts/scripts/UpgradeShell.sol

@@ -59,7 +56,9 @@ contract UpgradeShell is Script {
                assetHubReserveTransferFee: mDot(100), // 0.1 DOT
                exchangeRate: ud60x18(0.0024e18),
                multiplier: ud60x18(1.33e18),
-                rescueOperator: 0x4B8a782D4F03ffcB7CE1e95C5cfe5BFCb2C8e967
+                rescueOperator: 0x4B8a782D4F03ffcB7CE1e95C5cfe5BFCb2C8e967,


Probably don't need this anymore, since we have already dropped the operator on mainnet?

claravanstaden · 2024-10-21T13:43:47Z

contracts/src/Constants.sol

+    ParaID constant ASSET_HUB_PARA_ID = ParaID.wrap(1000);
+    bytes32 constant ASSET_HUB_AGENT_ID =
+        0x81c5ab2571199e3188135178f3c2c8e2d268be1313d029b30f534fa579b69b79;
+
+    ParaID constant BRIDGE_HUB_PARA_ID = ParaID.wrap(1002);


Nice! Seems like these para IDs are the same on all the environments now anyway.

claravanstaden · 2024-10-21T14:04:28Z

contracts/src/Gateway.sol

-            try Gateway(this).createChannel{gas: maxDispatchGas}(message.params) {}
-            catch {
+        } else if (message.command == CommandV1.CreateChannel) {
+            success = false;


So we essentially disable creating new channels?

claravanstaden · 2024-10-21T14:17:44Z

contracts/src/Gateway.sol

-        UpdateChannelParams memory params = abi.decode(data, (UpdateChannelParams));
+    // @dev Get token address by tokenID
+    function tokenAddressOf(bytes32 tokenID) external view returns (address) {
+        return CallsV1.tokenAddressOf(tokenID);


How come this is constrained to v1? Would it not stay the same for v2?

claravanstaden · 2024-10-21T14:19:34Z

contracts/src/Gateway.sol

-        emit ChannelUpdated(params.channelID);
+    /// @dev Create an agent for a consensus system on Polkadot
+    function v1_handleCreateAgent(bytes calldata data) external onlySelf {
+        HandlersV1.createAgent(data);


Why is there an extra layer of functions for these commands? Wouldn't it be fine to use HandlersV1.createAgent(data); directly in https://github.com/Snowfork/snowbridge/pull/1300/files#diff-74470ba8f35b42dbc8805739e92708c0696343e0b1819feb3e7a5ebb77cc623cR168?

try / catch can only catch errors from external function calls.

claravanstaden · 2024-10-21T14:22:08Z

contracts/src/Gateway.sol

-    function _submitOutbound(Ticket memory ticket) internal {
-        ChannelID channelID = ticket.dest.into();
-        Channel storage channel = _ensureChannel(channelID);
+    uint256 public constant DISPATCH_OVERHEAD_GAS_V2 = 32_000;


How do you calculate this? Comment about it might be useful.

claravanstaden · 2024-10-31T14:00:39Z

contracts/src/upgrades/rococo/RococoGatewayV2.sol

@@ -16,16 +16,7 @@ contract RococoGatewayV2 is Gateway {
        bytes32 bridgeHubAgentID,


nit: File should probably be for Westend now that Rococo is deprecated.

claravanstaden · 2024-10-31T14:07:24Z

contracts/src/v1/Calls.sol

+    * Internal functions
+    */
+
+    // Convert foreign currency to native currency (ROC/KSM/DOT -> ETH)


Suggested change

// Convert foreign currency to native currency (ROC/KSM/DOT -> ETH)

// Convert foreign currency to native currency (WND/PAS/KSM/DOT -> ETH)

contracts/src/v2/Calls.sol

contracts/src/v2/Types.sol

claravanstaden · 2024-11-01T08:21:04Z

contracts/src/Gateway.sol

+        InboundMessageV2 calldata message,
+        bytes32[] calldata leafProof,
+        Verification.Proof calldata headerProof,
+        bytes32 rewardAddress


Won't we just just derive the account from the origin on BH?

It's different. The origin in InboundMessage is derived from account on source chain, while the rewardAddress is account of the relayer for accepting reward.

contracts/src/v2/Calls.sol

vgeddes added 2 commits October 2, 2024 14:46

v2 initial commit

e0b2371

latest changes

a7ad609

yrong reviewed Oct 8, 2024

View reviewed changes

contracts/src/utils/SparseBitmap.sol Show resolved Hide resolved

yrong reviewed Oct 8, 2024

View reviewed changes

contracts/src/types/V2.sol Outdated Show resolved Hide resolved

yrong reviewed Oct 8, 2024

View reviewed changes

contracts/src/Gateway.sol Show resolved Hide resolved

Flesh out dispatch logic for inbound messages

1be9e5c

yrong reviewed Oct 8, 2024

View reviewed changes

contracts/src/interfaces/IGateway.sol Show resolved Hide resolved

yrong reviewed Oct 9, 2024

View reviewed changes

contracts/src/types/V2.sol Outdated Show resolved Hide resolved

Add reward address

e7fecaf

yrong mentioned this pull request Oct 15, 2024

OutboundQueue V2 yrong/polkadot-sdk#2

Closed

8 tasks

vgeddes added 2 commits October 17, 2024 14:32

make code compile

08715b0

Split initializer impl into a library to reduce contract size

111f738

yrong mentioned this pull request Oct 18, 2024

Outbound queue v2 yrong/polkadot-sdk#4

Open

8 tasks

vgeddes added 2 commits October 18, 2024 21:24

Major refactor

b0867e8

Update tests

ee37e3c

vgeddes changed the title V2 Contracts V2 Oct 18, 2024

vgeddes added 3 commits October 19, 2024 11:40

add scripts back

6829f38

Update scripts

8c878ce

Merge branch 'v2' into vincent/v2

0b25864

vgeddes changed the base branch from main to v2 October 19, 2024 09:56

vgeddes marked this pull request as ready for review October 19, 2024 09:56

Finish outbound messaging

c35743d

vgeddes requested review from claravanstaden and alistair-singh October 19, 2024 14:21

yrong reviewed Oct 20, 2024

View reviewed changes

contracts/src/v2/Calls.sol Outdated Show resolved Hide resolved

yrong reviewed Oct 20, 2024

View reviewed changes

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

yrong reviewed Oct 20, 2024

View reviewed changes

contracts/src/v2/Calls.sol Outdated Show resolved Hide resolved

vgeddes added 3 commits October 20, 2024 14:44

Implement token registration for V2

149c3e4

Make functions payable

02ac263

improve docs

c7332c3

yrong reviewed Oct 22, 2024

View reviewed changes

yrong reviewed Oct 23, 2024

View reviewed changes

contracts/src/Gateway.sol Show resolved Hide resolved

yrong reviewed Oct 24, 2024

View reviewed changes

comments

c0bab8a

alistair-singh reviewed Oct 25, 2024

View reviewed changes

review feedback

eae219e

yrong reviewed Oct 27, 2024

View reviewed changes

vgeddes added 3 commits October 27, 2024 20:00

cleanups

318b7dc

Add initial tests for V2

e73125d

Make rewardAddress an indexed event parameter

fa9b8a7

yrong reviewed Oct 28, 2024

View reviewed changes

alistair-singh reviewed Oct 29, 2024

View reviewed changes

yrong mentioned this pull request Oct 30, 2024

Snowbridge V2 paritytech/polkadot-sdk#6281

Closed

claravanstaden reviewed Oct 31, 2024

View reviewed changes

claravanstaden reviewed Nov 1, 2024

View reviewed changes

contracts/src/v2/Calls.sol Show resolved Hide resolved

vgeddes added 3 commits November 3, 2024 17:28

Clean up interfaces

00016a7

Make WETH address configurable

5817542

Autowrap ether

4c3b2af

		bytes memory payload =
		SubstrateTypes.encodePayloadV2(ticket.origin, ticket.assets, ticket.xcm);


		emit IGateway.OutboundMessageAccepted(channelID, channel.outboundNonce, messageID, ticket.payload);
		emit IGateway.InboundMessageDispatched(message.nonce, success, rewardAddress);

		@@ -16,16 +16,7 @@ contract RococoGatewayV2 is Gateway {
		bytes32 bridgeHubAgentID,

	// Convert foreign currency to native currency (ROC/KSM/DOT -> ETH)
	// Convert foreign currency to native currency (WND/PAS/KSM/DOT -> ETH)

Contracts V2 #1300

Are you sure you want to change the base?

Contracts V2 #1300

Conversation

vgeddes commented Oct 7, 2024 • edited Loading

codecov bot commented Oct 19, 2024 • edited Loading

Codecov Report

yrong Oct 20, 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

yrong Oct 20, 2024 • edited Loading

Choose a reason for hiding this comment

yrong Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrong Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrong Oct 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrong Oct 28, 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

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

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 Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

vgeddes commented Oct 7, 2024 •

edited

Loading

codecov bot commented Oct 19, 2024 •

edited

Loading

yrong Oct 20, 2024 •

edited

Loading

yrong Oct 20, 2024 •

edited

Loading

yrong Oct 21, 2024 •

edited

Loading

yrong Oct 25, 2024 •

edited

Loading

yrong Oct 22, 2024 •

edited

Loading

yrong Oct 28, 2024 •

edited

Loading

yrong Nov 4, 2024 •

edited

Loading