Add multi-router support in LP gateway. #1893

cdamian · 2024-06-28T10:25:57Z

Description

The LP gateway queue logic was moved to a new pallet which is used for handling both inbound/outbound messages.

Outbound messages

Changed processing of outbound messages by sending the actual message once, using one router, and sending message proofs using the remaining routers.

Inbound messages

Inbound messages are sent for further processing (i.e. to the LP pallet) only when the required number of message proofs is received.

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

cdamian · 2024-06-28T10:26:52Z

pallets/liquidity-pools-gateway/src/lib.rs

+	/// This can only be set by an admin.
+	#[pallet::storage]
+	#[pallet::getter(fn domain_multi_routers)]
+	pub type DomainMultiRouters<T: Config> =


Added a separate storage and extrinsic since I'm not sure how to proceed in this case - do we eventually remove the old storage after a migration or can we nuke/replace it now?

You can replace it now. If needed, the migration can go along with this PR, which I think is quite straightforward: just adding the current router, if it exists, into the vector and purge the old storage

cdamian · 2024-06-28T11:59:51Z

Re-basing on main.

codecov · 2024-06-28T14:22:00Z

Codecov Report

Attention: Patch coverage is 69.44444% with 11 lines in your changes missing coverage. Please review.

Project coverage is 47.38%. Comparing base (d034784) to head (22a8f03).

Files	Patch %	Lines
pallets/liquidity-pools-gateway/src/lib.rs	69.44%	11 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1893      +/-   ##
==========================================
+ Coverage   47.36%   47.38%   +0.01%     
==========================================
  Files         176      176              
  Lines       13305    13325      +20     
==========================================
+ Hits         6302     6314      +12     
- Misses       7003     7011       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mustermeiszer

Pretty straightforward change. Wanted to propose a few things if we are anyways working on this:

Add a dedicated Queue for Inbound and Outbound that is currently served by on idle, so that we can easily switch to Tasks in the future.
(NOT SURE IF GOOD IDEA) Instead of iterating over all routers we could have a DoubleMap and then serving the message in a queue does take care of
- 1. Ensuring that a message can be send over each Router only once

Furthermore
We will need to have the same for inbound logic. So a the Gateway must take care of a few things

Outgoing
- Each Message is sent only once over each existing Router
Incoming
- Each Message is only forwarded to the LiquidityPools pallet once some information has been verified.
  NOTE: It is to be determined how this will happen, but we should already take care right now that messages are "queued" until this verification has happened.

mustermeiszer · 2024-07-01T08:39:46Z

I think https://github.com/paritytech/polkadot-sdk/blame/master/substrate/frame/message-queue/src/lib.rs could be a solution, but I think the queue itself is overly complicated and in general very opaque an raw.

trait ProcessMessage is using a &[u8] as its message type for some reason.

cdamian · 2024-07-01T08:46:28Z

Pretty straightforward change. Wanted to propose a few things if we are anyways working on this:

Add a dedicated Queue for Inbound and Outbound that is currently served by on idle, so that we can easily switch to Tasks in the future.

(NOT SURE IF GOOD IDEA) Instead of iterating over all routers we could have a DoubleMap and then serving the message in a queue does take care of

Ensuring that a message can be send over each Router only once

Furthermore We will need to have the same for inbound logic. So a the Gateway must take care of a few things

Outgoing

Each Message is sent only once over each existing Router

Incoming

Each Message is only forwarded to the LiquidityPools pallet once some information has been verified.
NOTE: It is to be determined how this will happen, but we should already take care right now that messages are "queued" until this verification has happened.

@mustermeiszer we already have a queue in place for outbound messages - do you mean that we should replace that with a queue implementation that will serve both the LP gateway and the main LP pallet?

mustermeiszer · 2024-07-01T09:01:31Z

we already have a queue in place for outbound messages - do you mean that we should replace that with a queue implementation that will serve both the LP gateway and the main LP pallet?

I think now it makes sense, because there will be much more processing needed. Having a general Inbound/Outbound queue would make sense.

The general idea of the multi router logic is.

Sending:

1. Send full message over one Router
1. Send MessageProofs over the other Routers

Receiving
NOTE: That proofs CAN be received prior to the message itself

Receive Message/MessageProof
Check whether message can be already forwarded to the pallet-liquidity-pools

I really, really strongly am against exposing the MessageProofs and any Multi-Router aggregation to the pallet-liquidity-pools. The pallet itself should always simply assume to process the next message wrt. to business logic.

I would have split the the enum into an enum LpProtocol containing messages related to ensuring the transport and also the batching, and one enum LpMessages containing only the business logic related parts. For now, we could ensure that LpMessage is a subset of LpProtocol so that pallet-liquidity-pools only handles business parts.

For sending batches I would also say we have an fn batch() in pallet-liquidity-pools and the gatway takes care of creating such a batch. Easiest for all purposes would be to have an Gateway::start_pack_batch() on the gateway and an Gateway::end_pack_batch(). All calls to Gateway::submit() that happen between those two will then be boundled into a batch by the gateway. This way we can keep the current non-batch extrinsic logic and just iterate over the Vec<LiquidityPools::Call> that we receive in the fn batch()

pallets/liquidity-pools-gateway/src/lib.rs

…g logic

lemunozm

First fast look to this. Will do another one more deeply soon!

lemunozm · 2024-07-24T10:25:43Z

pallets/liquidity-pools/src/message.rs

+	//TODO(cdamian) - do we need to add the deprecated fields?
+	//
+	//
+	// 25 - Freeze tranche tokens
+	// DEPRECATED_Freeze,
+	// 26 - Unfreeze tranche tokens
+	// DEPRECATED_Unfreeze,
+	/// Trigger a redeem request.
+	///
+	/// Note - placeholder that should be updated.
+	TriggerRedeemRequest,
+	/// A multi-directional message used for verifying
+	/// both inbound and outbound messages.
+	MessageProof {
+		proof: [u8; 32],
+	},


Sorry @cdamian I didn't know, your feature came with new messages from LPv2. In that case, maybe it makes more sense to add those to lpv2 branch.

Maybe this PR can be split into something which is idempotent over how main works with LPv1 (it means, not sending any proof, just the original message by one router). And a new very small PR over LPv2 sending the proof.

That way most changes are done over main avoiding future git conflicts

Yea, I was afraid we might have to do this. Will look into doing a smooth transition.

OK, am currently thinking about splitting this in 3 parts, and (roughly) do the following on top of the LPv2 branch:
1 - add the new queue pallet;
2 - use the new queue pallet in the gateway;
3 - add proof processing for inbound/outbound messages;

libs/traits/src/liquidity_pools.rs

pallets/liquidity-pools-gateway/queue/src/lib.rs

cdamian · 2024-07-25T11:04:05Z

This PR will eventually be closed in favor of separate PRs on top of the LPv2 branch:
1 - Add gateway queue - #1930
2 - Use gateway queue - #1943

mustermeiszer

Just a brief scan. Nothing to complain, sounds and looks like a solid plan.

cdamian commented Jun 28, 2024

View reviewed changes

cdamian marked this pull request as ready for review June 28, 2024 10:28

cdamian requested review from lemunozm, wischli and mustermeiszer June 28, 2024 10:30

cdamian force-pushed the feat/add-lp-gateway-multi-router-support branch from cdc56a2 to a5e30b1 Compare June 28, 2024 12:06

cdamian force-pushed the feat/add-lp-gateway-multi-router-support branch from c145538 to 22a8f03 Compare June 28, 2024 14:39

mustermeiszer reviewed Jul 1, 2024

View reviewed changes

pallets/liquidity-pools-gateway/src/lib.rs Outdated Show resolved Hide resolved

cdamian added 12 commits July 23, 2024 16:42

lp-gateway: Add multi-router storage, update outbound queue processin…

6493d9a

…g logic

tests: Fix and add more tests

b360d05

fmt: Fix

4a0f59c

clippy: Fix

29811d1

tests: Add more tests

dd8faaa

fmt: Fix

09a6abf

runtime-common: Fix test config

282e89d

it: Fix LP test

3c6476d

wip

790f427

lp-gateway: Handle message proofs for inbound and outbound messages

b37b4b3

gateway: Update PostDispatchInfo weight calculations

11a9fbd

lp-gateway: Use gateway queue pallet

7e40381

cdamian force-pushed the feat/add-lp-gateway-multi-router-support branch from 22a8f03 to 7e40381 Compare July 23, 2024 14:52

lemunozm reviewed Jul 24, 2024

View reviewed changes

cdamian marked this pull request as draft July 25, 2024 11:03

mustermeiszer reviewed Jul 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add multi-router support in LP gateway. #1893

Add multi-router support in LP gateway. #1893

cdamian commented Jun 28, 2024 •

edited

Loading

cdamian Jun 28, 2024

lemunozm Jul 1, 2024 •

edited

Loading

cdamian commented Jun 28, 2024

codecov bot commented Jun 28, 2024 •

edited

Loading

mustermeiszer left a comment

mustermeiszer commented Jul 1, 2024

cdamian commented Jul 1, 2024

mustermeiszer commented Jul 1, 2024

lemunozm left a comment

lemunozm Jul 24, 2024 •

edited

Loading

lemunozm Jul 24, 2024

cdamian Jul 24, 2024

cdamian Jul 25, 2024

cdamian commented Jul 25, 2024 •

edited

Loading

mustermeiszer left a comment

Add multi-router support in LP gateway. #1893

Are you sure you want to change the base?

Add multi-router support in LP gateway. #1893

Conversation

cdamian commented Jun 28, 2024 • edited Loading

Description

Outbound messages

Inbound messages

Checklist:

cdamian Jun 28, 2024

Choose a reason for hiding this comment

lemunozm Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

cdamian commented Jun 28, 2024

codecov bot commented Jun 28, 2024 • edited Loading

Codecov Report

mustermeiszer left a comment

Choose a reason for hiding this comment

mustermeiszer commented Jul 1, 2024

cdamian commented Jul 1, 2024

mustermeiszer commented Jul 1, 2024

lemunozm left a comment

Choose a reason for hiding this comment

lemunozm Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

lemunozm Jul 24, 2024

Choose a reason for hiding this comment

cdamian Jul 24, 2024

Choose a reason for hiding this comment

cdamian Jul 25, 2024

Choose a reason for hiding this comment

cdamian commented Jul 25, 2024 • edited Loading

mustermeiszer left a comment

Choose a reason for hiding this comment

cdamian commented Jun 28, 2024 •

edited

Loading

lemunozm Jul 1, 2024 •

edited

Loading

codecov bot commented Jun 28, 2024 •

edited

Loading

lemunozm Jul 24, 2024 •

edited

Loading

cdamian commented Jul 25, 2024 •

edited

Loading