Feat/lp v2 use gateway queue #1943

cdamian · 2024-08-01T15:21:18Z

Description

Use the new Gateway Queue pallet in LP and LP gateway.

Changes and Descriptions

Use pallet-liquidity-pools-gateway-queue for queueing inbound and outbound message in the LP gateway.
Add new handler traits for inbound and outbound messages that are implemented by LP and LP gateway, as follows:

MessageQueue trait is implemented by the LP gateway queue and used by the LP gateway to enqueue both inbound and outbound messages.
MesageProcessor trait is implemented by the LP gateway and used by the LP gateway queue when processing queued messages.
OutboundMessageHandler trait is implemented by the LP gateway and used by LP when sending an outbound message.
InboundMessageHandler trait is implemented by LP and used by the LP gateway when processing inbound messages.

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
Address the benchmark weight suggestion here
Skip the LP gateway queue in as many integration tests as possible.
Replace the fudge runtime with the normal runtime in the integration tests, where possible.

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

lemunozm · 2024-08-01T15:37:33Z

To track the history, previous Pr with the view of this can be found: #1937

runtime/integration-tests/src/cases/lp/investments.rs

cdamian · 2024-08-01T19:25:34Z

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

@@ -243,32 +204,6 @@ pub mod pallet {
 	pub type RelayerList<T: Config> =
 		StorageDoubleMap<_, Blake2_128Concat, Domain, Blake2_128Concat, DomainAddress, ()>;

-	#[pallet::storage]


Removed these.

Note: Then we need to write a migration to kill those storages. Can be done in a later PR.

OK, let's do that separately then.

Just curious, if the storage is totally empty, do we need to remove it then? I think only OutboundMessageNonceStore need to be removed

…es (wip)

…d specific inbound tests

…Weight)

codecov · 2024-08-01T21:27:21Z

Codecov Report

Attention: Patch coverage is 60.86957% with 36 lines in your changes missing coverage. Please review.

Project coverage is 47.70%. Comparing base (7070f0f) to head (e164791).

Files	Patch %	Lines
pallets/liquidity-pools-gateway/src/lib.rs	65.00%	14 Missing ⚠️
pallets/liquidity-pools/src/hooks.rs	0.00%	9 Missing ⚠️
pallets/liquidity-pools-gateway/queue/src/lib.rs	53.33%	7 Missing ⚠️
pallets/liquidity-pools-gateway/src/message.rs	0.00%	3 Missing ⚠️
libs/types/src/domain_address.rs	0.00%	2 Missing ⚠️
pallets/liquidity-pools/src/lib.rs	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1943      +/-   ##
==========================================
- Coverage   47.84%   47.70%   -0.14%     
==========================================
  Files         184      184              
  Lines       13038    12969      -69     
==========================================
- Hits         6238     6187      -51     
+ Misses       6800     6782      -18

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

lemunozm

I really think this is a super great improvement in our database in several aspects. Love it! Thanks for your work on this @cdamian

I left some minor NIT things and one major related to the weights, that IMO should be fixed before merging.

libs/types/src/domain_address.rs

lemunozm · 2024-08-02T05:34:31Z

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

@@ -28,6 +28,7 @@ mod benchmarks {
 	#[benchmark]
 	fn process_message() -> Result<(), BenchmarkError> {
 		let caller: T::AccountId = account("acc_0", 0, 0);
+		//TODO(cdamian): Add a way of retrieving the "heaviest" message.


Reminder. But I would leave it as it is in this PR. I need to know how to fit this with the batch solution. It will need another thought

BTW, being defensive means that we can avoid these benchmarks I think. So there is no longer an issue here. We can even remove the benchmarks & weights from this 🤔

I'm OK with this, @wischli?

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

lemunozm · 2024-08-02T05:46:36Z

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

+			match T::InboundMessageHandler::handle(domain_address, message) {
+				Ok(_) => (Ok(()), weight),
+				Err(e) => (Err(e), weight),
 			}


The computation of T::InboundMessageHandler::handle(domain_address, message) is not counted in the weight and it should. Note that the inner computation of T::InboundMessageHandler can be a lot. i.e. if the message is something investment-related it can imply changes in foreign investment, opening swap orders, investments, etc...

Nevertheless, there is no easy solution here. I would add manually an extra weight. I would say 200_000_000 and 4096 of PoV can be enough for all cases by now. Interesting in your thoughts @wischli

I would add that extra weight to the 2 extrinsics of the LP gateway queue that can end up processing this message when calling MessageProcessor::process. The only other place where this is called is on_idle but in that case we use the weight that's returned.

But we're still skipping the weight on the on_idle, right?

on_idle uses the weight from T::MessageProcessor::process, which only returns Weight::from_parts(0, T::Message::max_encoded_len() as u64). We're not taking into account the processed message weight. I think we should add the above weights to process_inbound_message as well. To get on_idle to compute correctly all weights

Oh, right, for inbound messages we only return the MaxEncodedLen for the message, I will add the defensive weight in there as well.

@wischli, this is the thread about the weights

The weight is something we need to discuss; I thought 5_000_000_000 was excessively high for what, in my mind, is the worst case. Although probably 200_000_000 is pretty low. But choosing 5_000_000_000 is fine for me. Just a bit worried about filling fastly the block with batches

@lemunozm IMO being defensive is desired and not an issue for now since we don't expect LP messages to explode in cardinality for the time being. As long as we don't have somewhat of more realistic overestimate via benchmarks, this value seems feasible to me, especially since it has not caused trouble so far. 5_000_000_000 computational weight equals 1% of the maximum we allow. Does not feel too defensive to me that we can easily fit processing 50+ messages into, at least, the majority of our blocks. WDYT @lemunozm?

Though knowing that we could fit 1000-1500 transfers into a block if batched (bottleneck is signature verification if submitted in isolation), I suppose we could lower it by half. Nevertheless, I cast my vote for the safer path.

It makes sense what you say above! Let's be super defensive by now 👍🏻

lemunozm

Cool! everything LSGTM (looks Super great to me!!)

wischli

Beautiful and clean PR - great job!

Just one tiny thing bothers me. The new hardcoded weight Weight::from_parts(200_000_000, 4096) seems opaque and IMO we should declare it as a const at least because it is used quite often.

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

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

wischli · 2024-08-02T12:23:31Z

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

@@ -243,32 +204,6 @@ pub mod pallet {
 	pub type RelayerList<T: Config> =
 		StorageDoubleMap<_, Blake2_128Concat, Domain, Blake2_128Concat, DomainAddress, ()>;

-	#[pallet::storage]


Note: Then we need to write a migration to kill those storages. Can be done in a later PR.

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

runtime/integration-tests/src/cases/lp/investments.rs

runtime/integration-tests/src/cases/lp/utils.rs

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

lemunozm

Awesome stuff! Love it!

wischli

LGTM! Let's merge this beauty

cdamian marked this pull request as ready for review August 1, 2024 15:24

cdamian commented Aug 1, 2024

View reviewed changes

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

cdamian commented Aug 1, 2024

View reviewed changes

runtime/integration-tests/src/cases/lp/investments.rs Show resolved Hide resolved

cdamian commented Aug 1, 2024

View reviewed changes

cdamian requested a review from wischli August 1, 2024 19:42

cdamian added 16 commits August 1, 2024 22:43

liquidity-pools: Use LP gateway queue for inbound and outbound messag…

bd54a1d

…es (wip)

tests: wip

8bfecdd

lp-gateway-queue: Add TODO for benchmarks

a7e5835

lp-gateway: Add MaxEncodedLen of inbound Message

4de208c

gateway: Move GatewayMessage to gateway pallet

f31821b

integration-tests: Adapt existing tests to LP gateway queue usage, ad…

ef008cd

…d specific inbound tests

integration-tests: Add LP gateway queue tests

3ba2f20

unit-tests: Fix

bd7f81b

integration-tests: Enable all runtimes for LP gateway queue tests

d56675e

integration-tests: Skip the gateway queue for LP gateway router tests

839f004

integration-tests: Reduce visibility for LP utils module

620e92d

integration-tests: Drop fudge from router evm tests

89c9f6f

integration-tests: Drop fudge from foreign investments tests

42a3e09

lp-gateway: Replace DispatchResultWithPostInfo with (DispatchResult, …

204f7bc

…Weight)

integration-tests: Fix LP tests (wip)

a4f5b46

lp-gateway: Drop AddAssign

967e86c

cdamian force-pushed the feat/lp-v2-use-gateway-queue branch from 9ac3c61 to 967e86c Compare August 1, 2024 19:43

integration-tests: Fix failing tests

34e4f86

lemunozm reviewed Aug 2, 2024

View reviewed changes

cdamian added 3 commits August 2, 2024 10:54

lp-gateway-queue: Add defensive weight to extrinsics

f9e4812

lp-gateway: Add defensive weight when processing inbound message

e1e458e

unit-tests: Update expected weight in LP gateway tests

9689a41

lemunozm previously approved these changes Aug 2, 2024

View reviewed changes

wischli reviewed Aug 2, 2024

View reviewed changes

lp: Add constants for LP defensive weights

60bb6e8

cdamian dismissed lemunozm’s stale review via 60bb6e8 August 2, 2024 13:24

lemunozm reviewed Aug 2, 2024

View reviewed changes

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

deps: Add cfg-primitives

8e399a6

cdamian force-pushed the feat/lp-v2-use-gateway-queue branch from 29e36c3 to 8e399a6 Compare August 2, 2024 17:48

cdamian added 2 commits August 2, 2024 21:20

LP: Add and use LP_DEFENSIVE_WEIGHT

e6625f7

lp-gateway-queue: Drop benchmarks and use defensive weights instead

e164791

lemunozm approved these changes Aug 2, 2024

View reviewed changes

wischli approved these changes Aug 5, 2024

View reviewed changes

cdamian merged commit fc2b451 into main Aug 5, 2024
12 of 14 checks passed

cdamian deleted the feat/lp-v2-use-gateway-queue branch August 5, 2024 08:47

cdamian mentioned this pull request Aug 5, 2024

Add multi-router support in LP gateway. #1893

Draft

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/lp v2 use gateway queue #1943

Feat/lp v2 use gateway queue #1943

cdamian commented Aug 1, 2024 •

edited

Loading

lemunozm commented Aug 1, 2024

cdamian Aug 1, 2024

wischli Aug 2, 2024

cdamian Aug 2, 2024

lemunozm Aug 2, 2024

codecov bot commented Aug 1, 2024 •

edited

Loading

lemunozm left a comment

lemunozm Aug 2, 2024

lemunozm Aug 2, 2024

cdamian Aug 2, 2024

lemunozm Aug 2, 2024 •

edited

Loading

cdamian Aug 2, 2024

cdamian Aug 2, 2024

lemunozm Aug 2, 2024 •

edited

Loading

cdamian Aug 2, 2024

lemunozm Aug 2, 2024

lemunozm Aug 2, 2024 •

edited

Loading

wischli Aug 2, 2024

wischli Aug 2, 2024

lemunozm Aug 2, 2024

lemunozm left a comment •

edited

Loading

wischli left a comment

wischli Aug 2, 2024

lemunozm left a comment

wischli left a comment

Feat/lp v2 use gateway queue #1943

Feat/lp v2 use gateway queue #1943

Conversation

cdamian commented Aug 1, 2024 • edited Loading

Description

Changes and Descriptions

Checklist:

lemunozm commented Aug 1, 2024

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

codecov bot commented Aug 1, 2024 • edited Loading

Codecov Report

lemunozm 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

lemunozm Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm Aug 2, 2024 • edited Loading

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

lemunozm left a comment • edited Loading

Choose a reason for hiding this comment

wischli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm left a comment

Choose a reason for hiding this comment

wischli left a comment

Choose a reason for hiding this comment

cdamian commented Aug 1, 2024 •

edited

Loading

codecov bot commented Aug 1, 2024 •

edited

Loading

lemunozm Aug 2, 2024 •

edited

Loading

lemunozm Aug 2, 2024 •

edited

Loading

lemunozm Aug 2, 2024 •

edited

Loading

lemunozm left a comment •

edited

Loading