Skip to content
This repository has been archived by the owner on Oct 22, 2024. It is now read-only.

Polkadot assets on ethereum #128

Merged
merged 77 commits into from
Sep 2, 2024
Merged

Conversation

yrong
Copy link

@yrong yrong commented Mar 21, 2024

No description provided.

@yrong yrong marked this pull request as ready for review March 26, 2024 17:04
bridges/snowbridge/pallets/system/src/lib.rs Outdated Show resolved Hide resolved
@@ -243,6 +253,10 @@ pub mod pallet {
pub type PricingParameters<T: Config> =
StorageValue<_, PricingParametersOf<T>, ValueQuery, T::DefaultPricingParameters>;

#[pallet::storage]
#[pallet::getter(fn tokens)]
pub type Tokens<T: Config> = StorageMap<_, Twox64Concat, TokenId, Location, OptionQuery>;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto. Location version is implicitly v4.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was rolled back.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alistair-singh We should not use VersionedLocation for fields in storage. There is other commentary in the PR about this.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is stopping someone from rolling over xcm versions prelude without us knowing and breaking the lookup of Tokens?

Copy link
Author

@yrong yrong Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With paritytech@5ef32c8 will just not compile.

bridges/snowbridge/primitives/core/src/lib.rs Outdated Show resolved Hide resolved
bridges/snowbridge/primitives/core/src/outbound.rs Outdated Show resolved Hide resolved
bridges/snowbridge/primitives/router/src/inbound/mod.rs Outdated Show resolved Hide resolved
@yrong yrong requested a review from vgeddes April 9, 2024 00:39
@yrong yrong changed the base branch from snowbridge to bridge-next-gen April 9, 2024 12:07
Copy link
Collaborator

@claravanstaden claravanstaden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Such a cool feature. 🌟

bridges/snowbridge/pallets/system/src/lib.rs Outdated Show resolved Hide resolved
bridges/snowbridge/pallets/system/src/lib.rs Outdated Show resolved Hide resolved
bridges/snowbridge/pallets/system/src/lib.rs Outdated Show resolved Hide resolved
bridges/snowbridge/pallets/system/src/lib.rs Outdated Show resolved Hide resolved
bridges/snowbridge/primitives/core/src/lib.rs Outdated Show resolved Hide resolved
bridges/snowbridge/primitives/core/src/outbound.rs Outdated Show resolved Hide resolved
bridges/snowbridge/pallets/system/src/lib.rs Outdated Show resolved Hide resolved
bridges/snowbridge/pallets/system/src/lib.rs Outdated Show resolved Hide resolved
bridges/snowbridge/pallets/system/src/lib.rs Outdated Show resolved Hide resolved
bridges/snowbridge/primitives/core/src/lib.rs Outdated Show resolved Hide resolved
bridges/snowbridge/primitives/core/src/lib.rs Outdated Show resolved Hide resolved
bridges/snowbridge/primitives/core/src/outbound.rs Outdated Show resolved Hide resolved
bridges/snowbridge/primitives/core/src/outbound.rs Outdated Show resolved Hide resolved
bridges/snowbridge/primitives/core/src/outbound.rs Outdated Show resolved Hide resolved
bridges/snowbridge/primitives/core/src/outbound.rs Outdated Show resolved Hide resolved
@yrong yrong merged commit b19c1e1 into snowbridge Sep 2, 2024
8 checks passed
@yrong yrong deleted the ron/polkadot-assets-on-ethereum branch September 2, 2024 09:00

T::Token::mint_into(&caller, amount)?;

let relay_token_asset_id: Location = Location::new(1, [GlobalConsensus(Westend)]);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like we accepting this in Location relative to the Ethereum address.
I think we should accept an asset relative to bridge hub as input, and then re-achoring it onto the bridge location.

Copy link
Author

@yrong yrong Sep 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is the Location relative to the Ethereum after reanchored on AH in

On BH outbound router we check the asset location registered as expected in

let token_id = TokenIdOf::convert_location(&asset_id).ok_or(InvalidAsset)?;
let expected_asset_id = ConvertAssetId::convert(&token_id).ok_or(InvalidAsset)?;
ensure!(asset_id == expected_asset_id, InvalidAsset);

I prefer to register PNA with the reanchored location directly without adding reanchor logic in outbound router every time when receiving the xcm from AH.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am suggesting doing the re-anchoring once inside the registration extrinsic.

All other XCM related functions, when taking locations as input, are relative to the current chain. See pallet-xcm for example. So its not obvious to users that this is the required way to call it.

The other thing is that Locations are not distinct:
(2, [GlobalConsensus(Westend)]) and (1, [Here]) are the same asset and if re-anchoring from bridge hub will both collapse down to the unique (1, [GlobalConsensus(Westend)]). So they get the same id.

Copy link
Author

@yrong yrong Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All other XCM related functions, when taking locations as input, are relative to the current chain

Yeah, that's true. But it's a bit different here actually the asset location is reanchored in pallet-xcm relative to Ethereum in the view of AH. Then routed to BH with SovereignPaidRemoteExporter without reanchoring.

TBH I don't think the reanchor in pallet-xcm is nessessary at all for our use case. But it's another story.

So I'd prefer to make things easy for now(Permissionless register token is not even enabled yet) and improve it later in another PR.

@alistair-singh WDYT?

bridges/snowbridge/pallets/system/src/lib.rs Show resolved Hide resolved
yrong added a commit that referenced this pull request Sep 2, 2024
* Register token from polkadot

* Extract AssetRegistrarMetadata

* ReserveTransfer from AssetHub

* Transfer DOT back to AssetHub

* Fix breaking tests

* Fix register token

* Increase dispatch_gas to cover the actual cost for register token

* Add ConvertAssetId to outbound router

* Rename to SendForeignToken

* Update cost

* Move Command.RegisterToken to top level

* Use VersionedLocation for storage

* Use versioned location

* TokenIdOf follow the same pattern as AgentIdOf

* Rename MintToken to TransferNativeToken

* Rename as SendNativeToken

* More refactoring

* Remove AgentExecuteCommand

* Remove TokenExists check

* Update cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/tests/snowbridge.rs

Co-authored-by: Clara van Staden <[email protected]>

* Benchmark register_token

* More checks for the events

* More tests for send native token

* More tests describe tokenID

* More asset identifiers

* Add LocationToToken & Always use VersionedLocation

* Fix breaking tests

* Update gas cost for registering polkadot token

* Add AgentExecuteCommand back for compatibility

* Split into 2 tests each covers one direction

* Revert the change on AH

* Switch to native token on penpal for the integration

* Make fee asset as configuration parameter of the Channel

* Revert fee_asset_id as channel property

* Short epoch

* Fix integration tests

* Fix format

* Cleanup

* Register relay token

* Use relay token as fee asset

* Fix clippy

* Fix missing dependency

* Remove reanchored prefix from asset_id

* Fix test

* Fix the instruction

* Fix test

* Multi hop transfer

* Fix tests

* allow bridge hub assets

* Register token from BH directly and remove force_register_token

* Decrease gas estimation for PNA

* Cleanup

* Store Location rather than VersionedLocation

* Fix test

* Add more tests

* Revamp reanchor logic

* Improve reanchor & Fix tests

* Use secondary governance channel to register PNA

* Rename as asset location

* Use BoundVec limit size of name&symbol

* Describe location of PNA & more tests

* Add test

* Fix taplo

* More tests

* Cleanup

* Format code

* Batch rename token command

* Remove agent_id

* Rename as AssetMetadata

* Add test for penpal native token

---------

Co-authored-by: Clara van Staden <[email protected]>
Co-authored-by: Alistair Singh <[email protected]>
github-merge-queue bot pushed a commit to paritytech/polkadot-sdk that referenced this pull request Sep 13, 2024
# Description

Adding support for send polkadot native assets(PNA) to Ethereum network
through snowbridge. Asset with location in view of AH Including:

- Relay token `(1,Here)`
- Native asset `(0,[PalletInstance(instance),GenereIndex(index)])`
managed by Assets Pallet
- Native asset of Parachain `(1,[Parachain(paraId)])` managed by Foreign
Assets Pallet

The original PR in Snowfork#128
which has been internally reviewed by Snowbridge team.

# Notes

- This feature depends on the companion solidity change in
Snowfork/snowbridge#1155. Currently register PNA
is only allowed from
[sudo](https://github.com/Snowfork/polkadot-sdk/blob/46cb3528cd8cd1394af2335a6907d7ab8647717a/bridges/snowbridge/pallets/system/src/lib.rs#L621),
so it's actually not enabled. Will require another runtime upgrade to
make the call permissionless together with upgrading the Gateway
contract.

- To make things easy multi-hop transfer(i.e. sending PNA from Ethereum
through AH to Destination chain) is not support ed in this PR. For this
case user can switch to 2-phases transfer instead.

---------

Co-authored-by: Clara van Staden <[email protected]>
Co-authored-by: Alistair Singh <[email protected]>
Co-authored-by: Vincent Geddes <[email protected]>
Co-authored-by: Francisco Aguirre <[email protected]>
Co-authored-by: Adrian Catangiu <[email protected]>
vgeddes added a commit that referenced this pull request Sep 13, 2024
# Description

Adding support for send polkadot native assets(PNA) to Ethereum network
through snowbridge. Asset with location in view of AH Including:

- Relay token `(1,Here)`
- Native asset `(0,[PalletInstance(instance),GenereIndex(index)])`
managed by Assets Pallet
- Native asset of Parachain `(1,[Parachain(paraId)])` managed by Foreign
Assets Pallet

The original PR in #128
which has been internally reviewed by Snowbridge team.

# Notes

- This feature depends on the companion solidity change in
Snowfork/snowbridge#1155. Currently register PNA
is only allowed from
[sudo](https://github.com/Snowfork/polkadot-sdk/blob/46cb3528cd8cd1394af2335a6907d7ab8647717a/bridges/snowbridge/pallets/system/src/lib.rs#L621),
so it's actually not enabled. Will require another runtime upgrade to
make the call permissionless together with upgrading the Gateway
contract.

- To make things easy multi-hop transfer(i.e. sending PNA from Ethereum
through AH to Destination chain) is not support ed in this PR. For this
case user can switch to 2-phases transfer instead.

---------

Co-authored-by: Clara van Staden <[email protected]>
Co-authored-by: Alistair Singh <[email protected]>
Co-authored-by: Vincent Geddes <[email protected]>
Co-authored-by: Francisco Aguirre <[email protected]>
Co-authored-by: Adrian Catangiu <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants