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

wischli · 2023-05-25T13:59:54Z

Description

Extends Connectors v2 messages added in feat: Connectors v2 message PoC #1292 based on the technical spec
- Adds extrinsics for outbound msgs
- Adds inbound msg handlers

Please note that the spec is not finalized. I marked the corresponding sections in the code with TODO(@review) and added a couple of other ones which were not crystal clear to me. Of course, this does not mean, that you reviewers should solve this issue here. It should merely raise awareness as this PR is quite large such that there's a lot of noise. In order to simplify the reviewing process, I have left a couple of notes.

As discussed, we want to solve issues such as handling Executed* messages in subsequent PRs. Thus, I have divided open tasks related to this PR into three sections: Now, either now or later and later.

Changes and Descriptions

Adds InvestmentCollector trait required for {Collect | Redeem}For inbound messages
- Could also be part of Investment
Adds ForeignInvestment trait required to verify a given currency...
- Is the matching currency of the given pool
- Has pool_currency metadata enabled
Adds CurrencyInspect trait to enable checking whether a currency is a tranche token
Adds thorough integration (rather unit) tests for outgoing and incoming connectors messages
Fixes bug in do_collect_redeem, see commit 6158fca and this DAO Slack message for details
Temporarily: Disables CI for draft PRs

Open tasks connected to this PR and Connectors V2

TODOs in this PR

Write tests (preferably integration, otherwise using the mock-builder)
Rebase to main after Polkadot v0.9.38 #1332 is merged
Solve open questions marked by TODO(@review)

TODOs in this PR or later

Add trait PoolMetadata that allows retrieving the necessary data and the registry implements it (details) --> feat: trait PoolMetadata #1432
Fix outbound domain derivation from CurrencyId after feat: Extend CustomMetadata with transferability #1357
Derive sending domain via Gateway InboundQueue for inbound tranche token transfer and redemptions (blocked by Add pallet connectors gateway #1376)

TODO in other PRs

Changing PoolInspect trait based on this proposal
valid_until rework
Handling and tests for Executed*: Write pallet-foreign-investments which should handle the storage and dispatch of Executed* connectors messages and pallet-connectors relays the storage relevant data to pallet-foreign-investments when it executes inbound messages Decrease{Invest, Redeem}Order and Collect{Invest, Redeem}
Remove hard-coupling with pallet-xcm-transactor and remove/rework handle
E2E Tests

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

This reverts commit 2f22dae.

…trinsics

This reverts commit 9c3947d.

wischli · 2023-06-29T12:42:03Z

@NunoAlexandre @mustermeiszer I have applied all requested changes and linked the corresponding commits. I would love to get a re-review soonish so we can finally get this PR merged.

mustermeiszer · 2023-06-29T13:01:07Z

Will do tonight or tomorrow!

mustermeiszer

Two changes we need to do IMO. Otherwise, good to go.

libs/traits/src/connectors.rs

pallets/connectors/src/inbound.rs

pallets/connectors/src/lib.rs

libs/traits/src/lib.rs

NunoAlexandre

GH tells me I left 16 comments but I think this is pretty much ready apart from Frederik's last review which are important points.

Amazing work, @wischli 💯

libs/traits/src/connectors.rs

libs/traits/src/lib.rs

NunoAlexandre · 2023-06-30T09:14:06Z

pallets/connectors/src/inbound.rs

@mustermeiszer should we emit an event in such a case so that we can more easily monitor and debug those scenarios?

NunoAlexandre · 2023-06-30T09:23:21Z

pallets/investments/src/lib.rs

+	) -> Result<Self::Result, Self::Error> {
+		Pallet::<T>::do_collect_invest(who, investment_id)
+			.map_err(|e| e.error)
+			.map(|_| ())


@mustermeiszer shouldn't this return how much was actually collected? 🤔

Right now, do_collect_invest would return the post dispatch weight. We might need to add the collected amount in a subsequent PR which handles the Executed* messages.

pallets/investments/src/lib.rs

pallets/pool-system/src/tests/mod.rs

runtime/common/src/lib.rs

runtime/integration-tests/src/xcm/development/tests/connectors.rs

mustermeiszer · 2023-06-30T10:47:42Z

@mustermeiszer should we emit an event in such a case so that we can more easily monitor and debug those scenarios?

GH not showing what this is related to. Could you list it here again @NunoAlexandre ^^

wischli · 2023-06-30T12:05:10Z

GH not showing what this is related to. Could you list it here again @NunoAlexandre ^^

I also had to dig: #1364 (comment)

…trinsics

NunoAlexandre

Ready to 🚢

Kudus for your work, perseverance, and patience, @wischli 🎖️

mustermeiszer

Approve for now. Although, we got inconsistent now in naming and handling actions after hitting pallet investments...

But we will handle this in the subsequent PR so I am fine. Just noteing here for visibility.

mustermeiszer · 2023-06-30T13:51:24Z

pallets/connectors/src/inbound.rs

+	/// decreased amount on the source domain. The dispatch of this message is
+	/// delayed until the execution of the investment, e.g. at least until the
+	/// next epoch transition.
+	pub fn do_decrease_invest_order(


We got really inconsistent here now. Haha.

Oh boy, I fully relied on search and replace without double checking 🥶 Will fix. This should not be merged.

I normalized all inbound handlers with the handle prefix as some prefix was required for the transfer functions.

mustermeiszer · 2023-06-30T13:54:22Z

pallets/connectors/src/inbound.rs

+		T::ForeignInvestment::update_investment(&investor, invest_id, post_amount)?;
+
+		// Burn decreased amount
+		T::Tokens::burn_from(currency, &investor, amount)?;


Here we keep the burn, below we removed the transfer.

Sorry, this slipped through. Will handle this.

mustermeiszer

❤️

mustermeiszer

Re-approve

NunoAlexandre

🤟🏻

* refactor: move connectors codec to traits * refactor: move TypeId for DomainAddress * Revert "fix: remove new extrinsics" This reverts commit 2f22dae. * refactor: move TypeId impl for DomainLocator * chore: add sanity checks to transfer_tranche_tokens * wip: invest & redeem handlers * feat: add foreign investment traits * feat: add u128 conv for GeneralCurrencyPrefix * refactor: normalize message codec vars * feat: add v2 extrinsics + inbound handlers * docs: minor inv improvements * tests: apply add_member changes * ci: disable on draft PRs * feat: add CurrencyInspect trait * feat: apply CurrencyInspect * tests: add transfer_non_tranche_tokens * tests: add add_currency, allow_pool_currency * chore: finish tests, fix inbound handlers * fix: order id storage in do_collect_redeem * chore: various fixes and cleanups * tests: add inbound tranche token transfer * tests: add non tranche transfers to local * chore: apply clippy * Revert "ci: disable on draft PRs" This reverts commit 9c3947d. * fix: revert creep changes * refactor: apply suggestions from review * fix: apply evm meta derivation from asset * refactor: normalize token, currency * docs: apply suggestion from code review * fix: cleanup * tests: extend allow_pool_currency fails * clippy: fix * docs: add pallets config types * refactor: apply suggestions from review * refactor: rename to ConnectorsWrappedToken * docs: improve update_asset in con tests * fix: derive tranche decimals from registry * refactor: investments trait * fix: use DomainLocator * refactor: apply suggestions from code review * refactor: extend try_get_wrapped_token * refactor: remove wrapped token conversions * docs: add subsequent pr todos * feat: add connectors queue traits * refactor: cleanup * refactor: rm local transfer checks * chore: apply clippy to connectors tests * fix: issues after rebasing * refactor: apply suggestions from code review * tests: fix after rebase * fix: remove burn, normalize inbound handlers

* feat: trait PoolMetadata * fmt * refactor: change get return type to result * tests: add PoolMetadata * fmt: taplo * feat: add connectors v2 extrinsics & inbound msg handlers (#1364) * refactor: move connectors codec to traits * refactor: move TypeId for DomainAddress * Revert "fix: remove new extrinsics" This reverts commit 2f22dae. * refactor: move TypeId impl for DomainLocator * chore: add sanity checks to transfer_tranche_tokens * wip: invest & redeem handlers * feat: add foreign investment traits * feat: add u128 conv for GeneralCurrencyPrefix * refactor: normalize message codec vars * feat: add v2 extrinsics + inbound handlers * docs: minor inv improvements * tests: apply add_member changes * ci: disable on draft PRs * feat: add CurrencyInspect trait * feat: apply CurrencyInspect * tests: add transfer_non_tranche_tokens * tests: add add_currency, allow_pool_currency * chore: finish tests, fix inbound handlers * fix: order id storage in do_collect_redeem * chore: various fixes and cleanups * tests: add inbound tranche token transfer * tests: add non tranche transfers to local * chore: apply clippy * Revert "ci: disable on draft PRs" This reverts commit 9c3947d. * fix: revert creep changes * refactor: apply suggestions from review * fix: apply evm meta derivation from asset * refactor: normalize token, currency * docs: apply suggestion from code review * fix: cleanup * tests: extend allow_pool_currency fails * clippy: fix * docs: add pallets config types * refactor: apply suggestions from review * refactor: rename to ConnectorsWrappedToken * docs: improve update_asset in con tests * fix: derive tranche decimals from registry * refactor: investments trait * fix: use DomainLocator * refactor: apply suggestions from code review * refactor: extend try_get_wrapped_token * refactor: remove wrapped token conversions * docs: add subsequent pr todos * feat: add connectors queue traits * refactor: cleanup * refactor: rm local transfer checks * chore: apply clippy to connectors tests * fix: issues after rebasing * refactor: apply suggestions from code review * tests: fix after rebase * fix: remove burn, normalize inbound handlers * refactor: simplify trait imports

wischli added 13 commits May 4, 2023 17:21

refactor: move connectors codec to traits

333baa1

refactor: move TypeId for DomainAddress

c1c07d0

Revert "fix: remove new extrinsics"

d825bf3

This reverts commit 2f22dae.

refactor: move TypeId impl for DomainLocator

a444162

chore: add sanity checks to transfer_tranche_tokens

f7d7bed

wip: invest & redeem handlers

2c67d1d

feat: add foreign investment traits

ffc8522

feat: add u128 conv for GeneralCurrencyPrefix

f1246d7

refactor: normalize message codec vars

0cb33b1

feat: add v2 extrinsics + inbound handlers

78badd0

docs: minor inv improvements

9e9013a

tests: apply add_member changes

21587ef

ci: disable on draft PRs

9c3947d

wischli self-assigned this May 25, 2023

wischli added the I8-enhancement An additional feature. label May 25, 2023

wischli added 4 commits June 6, 2023 16:15

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

ac05f8b

…trinsics

feat: add CurrencyInspect trait

20c6af2

feat: apply CurrencyInspect

6630451

tests: add transfer_non_tranche_tokens

04ade36

wischli mentioned this pull request Jun 7, 2023

feat: Extend CustomMetadata with Connectors-specific metadata #1350

Closed

wischli added 10 commits June 9, 2023 15:26

tests: add add_currency, allow_pool_currency

61209a4

chore: finish tests, fix inbound handlers

740067b

fix: order id storage in do_collect_redeem

6158fca

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

4c99a90

…trinsics

chore: various fixes and cleanups

c3c9d0a

tests: add inbound tranche token transfer

fa64b45

tests: add non tranche transfers to local

ccb88f9

chore: apply clippy

46cafd4

Revert "ci: disable on draft PRs"

3c52de6

This reverts commit 9c3947d.

fix: revert creep changes

4f56811

wischli requested review from NunoAlexandre and mustermeiszer June 29, 2023 12:42

fix: issues after rebasing

2165341

mustermeiszer requested changes Jun 30, 2023

View reviewed changes

NunoAlexandre reviewed Jun 30, 2023

View reviewed changes

wischli added 3 commits June 30, 2023 14:10

refactor: apply suggestions from code review

de64a54

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

a588972

…trinsics

tests: fix after rebase

a8d8ad7

wischli requested review from mustermeiszer and NunoAlexandre June 30, 2023 12:30

NunoAlexandre previously approved these changes Jun 30, 2023

View reviewed changes

mustermeiszer previously approved these changes Jun 30, 2023

View reviewed changes

fix: remove burn, normalize inbound handlers

fe841c0

wischli dismissed stale reviews from mustermeiszer and NunoAlexandre via fe841c0 June 30, 2023 14:20

wischli requested review from mustermeiszer and NunoAlexandre June 30, 2023 14:20

mustermeiszer approved these changes Jun 30, 2023

View reviewed changes

wischli enabled auto-merge (squash) June 30, 2023 14:49

Merge branch 'main' into feat/connectors-v2-extrinsics

036d16a

mustermeiszer approved these changes Jun 30, 2023

View reviewed changes

NunoAlexandre approved these changes Jul 3, 2023

View reviewed changes

wischli merged commit 86e6544 into main Jul 3, 2023
11 checks passed

NunoAlexandre deleted the feat/connectors-v2-extrinsics branch July 3, 2023 09:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

wischli commented May 25, 2023 •

edited

wischli commented Jun 29, 2023

mustermeiszer commented Jun 29, 2023

mustermeiszer left a comment

NunoAlexandre left a comment

NunoAlexandre Jun 30, 2023

NunoAlexandre Jun 30, 2023

wischli Jun 30, 2023

mustermeiszer commented Jun 30, 2023

wischli commented Jun 30, 2023

NunoAlexandre left a comment

mustermeiszer left a comment

mustermeiszer Jun 30, 2023

wischli Jun 30, 2023

wischli Jun 30, 2023

mustermeiszer Jun 30, 2023

wischli Jun 30, 2023

mustermeiszer left a comment

mustermeiszer left a comment

NunoAlexandre left a comment

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

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

Conversation

wischli commented May 25, 2023 • edited

Description

Changes and Descriptions

Open tasks connected to this PR and Connectors V2

TODOs in this PR

TODOs in this PR or later

TODO in other PRs

Checklist:

wischli commented Jun 29, 2023

mustermeiszer commented Jun 29, 2023

mustermeiszer left a comment

Choose a reason for hiding this comment

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

mustermeiszer commented Jun 30, 2023

wischli commented Jun 30, 2023

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

mustermeiszer left a comment

Choose a reason for hiding this comment

mustermeiszer left a comment

Choose a reason for hiding this comment

NunoAlexandre left a comment

Choose a reason for hiding this comment

wischli commented May 25, 2023 •

edited