feat: Connectors v2 message PoC #1292

wischli · 2023-03-28T08:16:00Z

Description

Applies ConnectorsV2 message format based on the spec

TODO

Discuss requirement of CurrencyId to be representable by u128
Fix unit tests
Add mapping from CurrencyId to u128 via the new GeneralCurrencyIndex trait
Verify encoding/decoding updates in Solidity

Changes and Descriptions

Updates connectors Message enum based on the spec
- Updates existing messages
- Adds new messages
Updates existing Centrifuge chain extrinsics
Adds conversion for CurrencyId enum to u128 via two GeneralCurrencyIndex impls
- First one for CurrencyId
- Second one for the connectors Pallet
Does not add flow/extrinsics for new messages, will be done in a subsequent PR

Checklist:

I have added Rust doc comments to structs, enums, traits and functions
I have made corresponding changes to the documentation
I have performed a self-review of my code
I have added tests that prove my fix is effective or that my feature works

pallets/connectors/src/message.rs

NunoAlexandre · 2023-03-29T13:50:03Z

Discuss requirement of CurrencyId to be representable by u128

I have been thinking about this. The spec sort of presumes that every currency we will be identifying as a u128 has an inner GeneralIndex value within its MultiLocation representation. But that's not always the case, neither can/should we rely on that.

I'd suggest that we have a storage mapping from CurrencyId -> u128 that we set for every CurrencyId we want to support for these transfers. This grants us some benefits:

we can support new currencies to be transferred through Connectors without a runtime upgrade
we don't have a setup where EVERY currency registered in the asset registry is automatically connector-transferrable
other dAPPs or our own can query this state to know how this mapping is taking place
The community can have a saying on what tokens they find favourable to be connector-transferrable or not

On the Solidity side we will also need a similar mapping but from uint128 -> address.

What do you think? cc @offerijns @mustermeiszer

pallets/connectors/src/message.rs

NunoAlexandre

First round 🚀

Things will still change a bit until the messages v2 spec is finalised so hang in there 🙏

libs/types/src/tokens.rs

pallets/connectors/src/lib.rs

pallets/connectors/src/message.rs

…ssage-poc

pallets/connectors/src/lib.rs

pallets/connectors/src/message.rs

Co-authored-by: Jeroen Offerijns <Offerijns@users.noreply.github.com>

pallets/connectors/src/lib.rs

pallets/connectors/src/message.rs

NunoAlexandre · 2023-05-02T11:45:16Z

runtime/development/src/lib.rs

+parameter_types! {
+	#[derive(scale_info::TypeInfo)]
+	pub const GeneralCurrencyPrefix: [u8; 12] = GENERAL_CURRENCY_INDEX_PREFIX;
+}
+


I think we can define this parameter_types in runtime-common, no?

Since this is a constant, I feel like it's better suited in the current place, e.g the constants module of libs/primitives/lib.rs. WDYT @mustermeiszer?

Agreeing with William here. All constants should stay in the primitives or the respective const of libs

NunoAlexandre

🚀

mustermeiszer

Looks good already.

We should note that we can remove the hard coupling with pallet_xcm_transactor once the pallet-connectors-gateway takes care of routing messages.

Thinks we can remove then

hard coupling
fn handle
fn add_connector
fn set_domain_router

Few changes I think are necessary:

Move impl TypeId for DomainAddress into id module of crate types
Move trait Codec to traits
fn update_member
The adding of members should always go through the pallet-permissions interface. As we also coded the member list bot to use this API. So the extrinsic must only check if the permissions is existing. If yes, then submit message. Furthermore, we must block if the domain is local -- i.e. Centrifuge
fn transfer_tranche_tokens & fn transfer
→ for both of these messages to the local domain should be forbidden. I think it is debatable if the gateway logic @cdamian will implement will ever allow to send messages to our own domain. I would argue no, but this is debatable. SO we should either agree on blocking this on the gateway side or already on the connectors side. WDYT?

Missing:

fn allow_pool_currency
→ This is needed. The restriction is currently not 100% clear. I would say we restrict calling this only from democracy/AdminOrigin for now. Maybe we find a way to make it clear in the asset-registry or in the pallet-foreign-investments whether another currency can be used for investing.

mustermeiszer · 2023-05-03T09:03:05Z

libs/primitives/src/lib.rs

@@ -249,6 +249,9 @@ pub mod constants {
 	pub const fn deposit(items: u32, bytes: u32) -> Balance {
 		items as Balance * 15 * CENTI_CFG + (bytes as Balance) * 6 * CENTI_CFG
 	}
+
+	/// The prefix for tokens managed by Connectors.
+	pub const GENERAL_CURRENCY_INDEX_PREFIX: [u8; 12] = *b"CfgCnctCurId";


In order to not have clashes with external currencies and their representation in our CurrencyId::ForeignAssets(u128) I would like to have this prefix be a trimmed hash of the given bytes. Given that the alphabet is at most 52 values and u8 has room for 255 this will increase the probability of not having colissions. But maybe I am too worried there.

It might be overkill but I am always supporting safer approaches. Unfortunately we lose out on the const property due to using blake2_128 and splitting. WDYT? d774926

mustermeiszer · 2023-05-03T18:42:16Z

pallets/connectors/src/lib.rs

@@ -393,7 +416,7 @@ pub mod pallet {
 		/// Transfer tranche tokens to a given address
 		#[pallet::weight(< T as Config >::WeightInfo::transfer())]
 		#[pallet::call_index(6)]
-		pub fn transfer(
+		pub fn transfer_tranche_tokens(


Block local domain!

Great catch! 05dd755

mustermeiszer · 2023-05-03T18:42:42Z

pallets/connectors/src/lib.rs

+		pub fn transfer(
+			origin: OriginFor<T>,
+			asset_id: CurrencyIdOf<T>,
+			domain_address: DomainAddress,
+			amount: <T as pallet::Config>::Balance,
+		) -> DispatchResult {


Block local domain!

Great catch! 05dd755

pallets/connectors/src/lib.rs

mustermeiszer · 2023-05-03T20:10:20Z

pallets/connectors/src/lib.rs

+		// TODO: Replace weight after benchmarking
+		#[pallet::weight(< T as Config >::WeightInfo::add_connector())]
+		#[pallet::call_index(8)]
+		pub fn add_currency(


Do we have an issue for tracking that. This would be really important!

I mean the FIXME

We do not have an existing Github issue but this is part of the asset metadata update which is owned by @NunoAlexandre

Improved message in 05dd755

mustermeiszer · 2023-05-03T20:21:50Z

runtime/development/src/lib.rs

+parameter_types! {
+	#[derive(scale_info::TypeInfo)]
+	pub const GeneralCurrencyPrefix: [u8; 12] = GENERAL_CURRENCY_INDEX_PREFIX;
+}
+


Agreeing with William here. All constants should stay in the primitives or the respective const of libs

mustermeiszer

Approval under the constraint that we do a follow-up PR. This is not production code so we can go with this temporary inconsistency in main.

…ssage-poc

mustermeiszer

LGTM! Thanks for adapting to my comments.

wischli added 2 commits March 28, 2023 10:09

feat: update Message enum

65a3d17

feat: add (dummy) Into<u128> for CurrencyId

690430e

NunoAlexandre reviewed Mar 29, 2023

View reviewed changes

pallets/connectors/src/message.rs Outdated Show resolved Hide resolved

NunoAlexandre reviewed Mar 29, 2023

View reviewed changes

pallets/connectors/src/message.rs Show resolved Hide resolved

wischli added 4 commits March 29, 2023 16:38

tests: apply changes to existing

b5eb58c

refactor: add consts in tests

4588b91

tests: apply new messages

0839851

refactor: apply suggestions from soft review

394fd4e

wischli self-assigned this Mar 30, 2023

wischli added D5-breaks-api Pull request changes public api. Document changes publicly. crcl-cross-chain Circle cross-chain related. labels Mar 30, 2023

wischli added 2 commits March 30, 2023 16:01

fix: clippy

6a86151

tests: fix integration

3ba9230

wischli marked this pull request as ready for review April 3, 2023 07:34

lemunozm reviewed Apr 3, 2023

View reviewed changes

pallets/connectors/src/message.rs Outdated Show resolved Hide resolved

NunoAlexandre reviewed Apr 3, 2023

View reviewed changes

wischli added 5 commits April 3, 2023 15:27

refactor: add LocalCurrencyId storage

9c84f3a

Merge remote-tracking branch 'origin/main' into feat/connectors-v2-me…

ccc22b0

…ssage-poc

feat: add general index mapping for CurrencyId

af67315

tests: fix integration for connectors

492fdb3

fix: clippy warning

7726985

wischli force-pushed the feat/connectors-v2-message-poc branch from 06ef965 to 7726985 Compare April 19, 2023 15:32

refactor: apply suggestions from review

93662a1

wischli requested review from NunoAlexandre, lemunozm and mustermeiszer April 20, 2023 08:34

wischli added 2 commits April 20, 2023 10:43

refactor: remove unused code

3db1233

refactor: apply latest spec changes

6cc8863

wischli added the I8-enhancement An additional feature. label Apr 20, 2023

hieronx reviewed May 1, 2023

View reviewed changes

pallets/connectors/src/lib.rs Outdated Show resolved Hide resolved

pallets/connectors/src/message.rs Outdated Show resolved Hide resolved

wischli added 3 commits May 2, 2023 10:46

Apply suggestion from code review by @NunoAlexandre

572fb34

chore: add message docs, fix order

4ecddf9

Apply suggestion from review by @offerijns

2ab8593

wischli requested a review from NunoAlexandre May 2, 2023 09:08

wischli marked this pull request as ready for review May 2, 2023 09:08

hieronx reviewed May 2, 2023

View reviewed changes

pallets/connectors/src/message.rs Outdated Show resolved Hide resolved

Update pallets/connectors/src/message.rs

618f7c2

Co-authored-by: Jeroen Offerijns <Offerijns@users.noreply.github.com>

NunoAlexandre reviewed May 2, 2023

View reviewed changes

wischli added 2 commits May 2, 2023 16:22

docs: improve AssetNotFound

bdbf65d

docs: apply normalized directionality to Message

14b3f09

NunoAlexandre previously approved these changes May 2, 2023

View reviewed changes

Merge branch 'main' into feat/connectors-v2-message-poc

6a06ba4

mustermeiszer requested changes May 3, 2023

View reviewed changes

wischli added 2 commits May 4, 2023 10:51

refactor: hashed GeneralCurrencyPrefix

d774926

fix: block local domain transfers

05dd755

mustermeiszer previously approved these changes May 4, 2023

View reviewed changes

wischli added 3 commits May 4, 2023 10:57

refactor: rm unneeded trait bounding

6b06a9d

Merge remote-tracking branch 'origin/main' into feat/connectors-v2-me…

e045d0e

…ssage-poc

fmt: apply comment wrapping

787cc53

wischli dismissed stale reviews from mustermeiszer and NunoAlexandre via 787cc53 May 4, 2023 08:59

wischli requested a review from mustermeiszer May 4, 2023 09:10

fix: remove new extrinsics

2f22dae

wischli requested a review from NunoAlexandre May 4, 2023 12:56

mustermeiszer approved these changes May 4, 2023

View reviewed changes

wischli merged commit bd4a96b into main May 4, 2023
11 checks passed

wischli deleted the feat/connectors-v2-message-poc branch May 4, 2023 13:15

wischli mentioned this pull request May 25, 2023

feat: add connectors v2 extrinsics & inbound msg handlers #1364

Merged

15 tasks

wischli mentioned this pull request Sep 19, 2023

lp-gateway: Emit more events when processing an incoming message #1550

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Connectors v2 message PoC #1292

feat: Connectors v2 message PoC #1292

wischli commented Mar 28, 2023 •

edited

NunoAlexandre commented Mar 29, 2023

NunoAlexandre left a comment

NunoAlexandre May 2, 2023

wischli May 2, 2023

mustermeiszer May 3, 2023

NunoAlexandre left a comment

mustermeiszer left a comment

mustermeiszer May 3, 2023

wischli May 4, 2023

mustermeiszer May 3, 2023

wischli May 4, 2023

mustermeiszer May 3, 2023

wischli May 4, 2023

mustermeiszer May 3, 2023

mustermeiszer May 3, 2023

wischli May 4, 2023

wischli May 4, 2023 •

edited

mustermeiszer May 3, 2023

mustermeiszer left a comment

mustermeiszer left a comment

feat: Connectors v2 message PoC #1292

feat: Connectors v2 message PoC #1292

Conversation

wischli commented Mar 28, 2023 • edited

Description

TODO

Changes and Descriptions

Checklist:

NunoAlexandre commented Mar 29, 2023

NunoAlexandre 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

NunoAlexandre left a comment

Choose a reason for hiding this comment

mustermeiszer 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

wischli May 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mustermeiszer left a comment

Choose a reason for hiding this comment

mustermeiszer left a comment

Choose a reason for hiding this comment

wischli commented Mar 28, 2023 •

edited

wischli May 4, 2023 •

edited