Statemine reserve location and universal alias #841

alistair-singh · 2023-05-22T21:44:02Z

vgeddes · 2023-05-23T07:16:29Z

core/packages/test/scripts/configure-statemine.sh

+    # Allow messages to be sent from ETH to Substrate.
+    add_universal_alias
+    # Allow NativeTokens contract to ReserveAssetDeposited xcm instruction.
+    add_reserve_location $(address_for NativeTokens)


I forgot to sync with you about this. The problem is that we can't use the contract address for NativeTokens as the reserve location. If we replace NativeTokens with an upgraded contract, that will break the config on Statemint.

Instead, since the reserve location is largely symbolic, I would just use a dummy address for now.

When BridgeHub forwards XCM messages to Statemint, we can make our router say these messages originated from the dummy address too (rather than from OutboundQueue.sol). Statemint will inspect the origin of incoming messages to ensure they are the owners of the assets in ForeignAssets pallet.

At some point in the next week or two, we can replace the dummy address with the address of a real contract in our stack. This will just be a do-nothing contract, with the only property being that it was deployed by us.

contract Snowbridge {}

I was thinking that for upgrades we would just add the new NativeTokens contract address as a reserve location using the same add_reserve_location extrinsic, potentially even in the same call which sends the upgrade message across the bridge. The bridge-transfer pallet can have both versions of the contract added and can be run back to back. Once upgrade is complete we can remove the old contract address using remove_reserve_location extrinsic.

Do you see any problems with this approach?

We could also use vault contracts as a reserve location as they live forever and actually hold funds. So maybe a better choice than a dummy address.

I was thinking that for upgrades we would just add the new NativeTokens contract address as a reserve location using the same add_reserve_location extrinsic, potentially even in the same call which sends the upgrade message across the bridge. The bridge-transfer pallet can have both versions of the contract added and can be run back to back. Once upgrade is complete we can remove the old contract address using remove_reserve_location extrinsic.

Do you see any problems with this approach?

There are various problem with this approach. We can only execute add_reserve_location, etc via governance proposal voted upon by the Polkadot community. It can take weeks to execute a governance proposal.

It makes smart contract upgrades, slower, more brittle. Since it requires two governance proposals: (1) Update the ethereum side, (2) update the polkadot side. Alternatively, if you combine the proposals, then if one subtask fails, you end up with inconsistent state which requires more governance proposals to fix.

We could also use vault contracts as a reserve location as they live forever and actually hold funds. So maybe a better choice than a dummy address.

Vault contracts are an internal implementation detail. I don't want to expose them in our public API, which could cause problems if ever change our design/implementation later down the line.

Ah I see. I have created this issue to switch to a dummy address.
https://linear.app/snowfork/issue/SNO-450
I think we should defer this for now. While I am focusing on trying to get the round trip transfer done we can continue with using NativeTokens?

It won't work without a dummy address, and we need to update BridgeHub so that the XCM messages it sends to Statemint are seen as coming from a MultiLocation containing the dummy address as a junction.

Because the XCM config in Statemint/Westmint has this constraint:
https://github.com/paritytech/cumulus/pull/2013/files#diff-07287c62dd91150d975f7504bd14cc62d9889c44f7ba8ca44c30c03b05d5b071R383

https://github.com/paritytech/cumulus/blob/c167adde899a1a3818e19cf55dfe9a6b907bff63/parachains/pallets/bridge-transfer/src/impls.rs#L43

It checks that an asset has a multilocation that has the reserve location as a prefix.

Also see this comment in the bridge transfer pallet: https://github.com/paritytech/cumulus/blob/c167adde899a1a3818e19cf55dfe9a6b907bff63/parachains/pallets/bridge-transfer/src/lib.rs#L42

If we replace NativeTokens with an upgraded contract, that will break the config on Statemint.

Seems proxy pattern will fit well into this case, the proxy address of NativeTokens will not change when underlying logic contract upgraded. Another benifit with this pattern is storage isolation so no need for migration when upgrading.

Cons are as Vincent mentioned before a bit brittle and not easy to maintain. #801 (comment)

yrong · 2023-05-25T11:07:01Z

Is this file still in use now?
https://github.com/Snowfork/snowbridge/blob/alistair/statemine-reserve-location/core/packages/test/scripts/send-xcm.sh

I would assume the entry point for running E2E test is from this file
https://github.com/Snowfork/snowbridge/blob/alistair/statemine-reserve-location/smoketest/tests/lock_tokens.rs

@alistair-singh Please correct me if I'm wrong.

yrong · 2023-05-25T11:10:47Z

Before running deposit funds into Statemint's sovereign account on BridgeHub:
5Ec4AhPZk8STuex8Wsi9TwDtJQxKqzPJRCH7348Xtcs9vZLJ
This is necessary to cover rewards for "execution message" relayers. Messages will be rejected if the account is empty.

How to trigger the Initial funding, is there a script for it?

alistair-singh · 2023-05-25T11:16:56Z

https://github.com/Snowfork/snowbridge/blob/alistair/statemine-reserve-location/core/packages/test/scripts/send-xcm.sh

No I will remove.

How to trigger the Initial funding, is there a script for it?

I have added the parachain 1000 account into endowed accounts for now in my create tokens branch.

I would assume the entry point for running E2E test is from this file

That is correct however you would need to create a token first.

This reverts commit 2637a3f.

yrong · 2023-05-25T11:57:12Z

snowbridge/smoketest/tests/lock_tokens.rs

Lines 74 to 86 in f8cf0da

    
           native_tokens 
        
               .lock(weth.address(), 0, recipient.into(), value1) 
        
               .value(1000) 
        
               .send() 
        
               .await 
        
               .unwrap() 
        
               .await 
        
               .unwrap() 
        
               .unwrap(); 
        
           println!("receipt: {:#?}", receipt); 
        
           assert_eq!(receipt.status.unwrap().as_u64(), 1u64);

Nit: seems only checks receipt.status here, would it be better also connect to statemint and check token minted there as expected?

yrong

LGTM!

vgeddes

+1

Though obviously we will need to select an appropriate reserve location that works for statemint.

alistair-singh mentioned this pull request May 22, 2023

Statemine reserve location and universal alias Snowfork/cumulus#27

Merged

alistair-singh requested review from vgeddes, yrong and doubledup May 22, 2023 21:44

vgeddes reviewed May 23, 2023

View reviewed changes

alistair-singh force-pushed the alistair/statemine-reserve-location branch 2 times, most recently from a5a4751 to f8cf0da Compare May 24, 2023 19:24

alistair-singh added 7 commits May 25, 2023 13:50

add statemine parachain

a89f29a

added configuration

c79f018

update cumulus

eaf3697

all configurations added

ec1f244

Revert "add statemine parachain"

5dbbcb8

This reverts commit 2637a3f.

change chain id

64937d1

remove unused script

320a351

alistair-singh force-pushed the alistair/statemine-reserve-location branch from f8cf0da to 320a351 Compare May 25, 2023 11:51

yrong approved these changes May 25, 2023

View reviewed changes

vgeddes approved these changes May 29, 2023

View reviewed changes

alistair-singh merged commit 0467d5b into main May 30, 2023

alistair-singh deleted the alistair/statemine-reserve-location branch May 30, 2023 13:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Statemine reserve location and universal alias #841

Statemine reserve location and universal alias #841

alistair-singh commented May 22, 2023 •

edited

Loading

vgeddes May 23, 2023 •

edited

Loading

alistair-singh May 23, 2023 •

edited

Loading

alistair-singh May 23, 2023

vgeddes May 23, 2023 •

edited

Loading

vgeddes May 23, 2023 •

edited

Loading

alistair-singh May 23, 2023

vgeddes May 23, 2023

vgeddes May 23, 2023

yrong May 25, 2023

yrong commented May 25, 2023

yrong commented May 25, 2023

alistair-singh commented May 25, 2023 •

edited

Loading

yrong commented May 25, 2023 •

edited

Loading

yrong left a comment

vgeddes left a comment

Statemine reserve location and universal alias #841

Statemine reserve location and universal alias #841

Conversation

alistair-singh commented May 22, 2023 • edited Loading

vgeddes May 23, 2023 • edited Loading

Choose a reason for hiding this comment

alistair-singh May 23, 2023 • edited Loading

Choose a reason for hiding this comment

alistair-singh May 23, 2023

Choose a reason for hiding this comment

vgeddes May 23, 2023 • edited Loading

Choose a reason for hiding this comment

vgeddes May 23, 2023 • edited Loading

Choose a reason for hiding this comment

alistair-singh May 23, 2023

Choose a reason for hiding this comment

vgeddes May 23, 2023

Choose a reason for hiding this comment

vgeddes May 23, 2023

Choose a reason for hiding this comment

yrong May 25, 2023

Choose a reason for hiding this comment

yrong commented May 25, 2023

yrong commented May 25, 2023

alistair-singh commented May 25, 2023 • edited Loading

yrong commented May 25, 2023 • edited Loading

yrong left a comment

Choose a reason for hiding this comment

vgeddes left a comment

Choose a reason for hiding this comment

alistair-singh commented May 22, 2023 •

edited

Loading

vgeddes May 23, 2023 •

edited

Loading

alistair-singh May 23, 2023 •

edited

Loading

vgeddes May 23, 2023 •

edited

Loading

vgeddes May 23, 2023 •

edited

Loading

alistair-singh commented May 25, 2023 •

edited

Loading

yrong commented May 25, 2023 •

edited

Loading