WIP: Liquidity Pools v2 #1909

lemunozm · 2024-07-12T13:48:51Z

Description

LP-V2 base branch where we've merged the individual PRs. For more information, check the individual PRs merged in this branch

* wip: add new msgs + reorder * refactor: apply renaming * fix: ITs * feat: UpdateRestriction message * fix: cancel_unprocessed_investment IT * fix: fmt * fix: clippy * tests: reanchor solidity to reorder branch * feat: apply hook to AddTranche msg * tests: fix pool_management ITs * wip: fix lp investments * fmt * fix: Tranche namespace * refactor: remove fulfilled from FulfilledRedeemRequest * chore: update lp submodule * minor cleanup * chore: update lp submodule * chore: add missing cleanup * fixes + ignore failing tests * fmt * tests: fix message indices

* add uts (#1905) * main changes for FI * fix correlation precission * minor renames * update investment UTs * update redemption UTs * miscelaneous UTs * minor renames * simplify correlation and embed to the only needed place * doc change * remove unused bound * swapping calls into entities.rs file * merge SwapDone methods into FulfilledSwapHook * fix events * working without pallet-swaps * remove boilerplate for removing entries * minor msg change * minor simplification * correct fulfilled sum after last collect * check OrderIdToSwapId storage * sending the message inside Info closure is not really a problem * send msgs from the entities * remove same currency check in impl.rs * unify hooks * remove pallet-swaps * minor fmt * add docs * add architecture diagram * return cancelled foreign amount from FI interface * update liquidity-pools * add messages to diagram * implement hooks * fix runtimes * adapt integration-tests * fix docs * fix clippy

lemunozm · 2024-07-16T10:21:07Z

Modified this PR to ready to review because we want to be sure this branch has always CI in green

codecov · 2024-07-16T12:51:43Z

Codecov Report

Attention: Patch coverage is 69.78131% with 152 lines in your changes missing coverage. Please review.

Project coverage is 47.81%. Comparing base (f647a49) to head (a38c4f9).
Report is 1 commits behind head on main.

Files	Patch %	Lines
pallets/liquidity-pools/src/hooks.rs	0.00%	34 Missing ⚠️
pallets/liquidity-pools-gateway/queue/src/lib.rs	53.22%	29 Missing ⚠️
pallets/foreign-investments/src/entities.rs	77.87%	25 Missing ⚠️
pallets/liquidity-pools/src/message.rs	54.71%	24 Missing ⚠️
pallets/foreign-investments/src/swaps.rs	75.40%	15 Missing ⚠️
libs/mocks/src/liquidity_pools_gateway_queue.rs	0.00%	4 Missing ⚠️
pallets/liquidity-pools-gateway/src/lib.rs	66.66%	4 Missing ⚠️
pallets/liquidity-pools/src/lib.rs	82.60%	4 Missing ⚠️
pallets/foreign-investments/src/impls.rs	95.38%	3 Missing ⚠️
pallets/foreign-investments/src/lib.rs	77.77%	2 Missing ⚠️
... and 5 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1909      +/-   ##
==========================================
- Coverage   47.92%   47.81%   -0.12%     
==========================================
  Files         181      184       +3     
  Lines       13159    13036     -123     
==========================================
- Hits         6307     6233      -74     
+ Misses       6852     6803      -49

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

lemunozm · 2024-07-16T21:39:06Z

@wischli are the current failing tests already fixed in #1915 or should I fix them (I think I should fix two and ignore the rest)?

wischli · 2024-07-16T22:04:43Z

@wischli are the current failing tests already fixed in #1915 or should I fix them (I think I should fix two and ignore the rest)?

They are not part of #1915 yet but I can definitely include that! Apologies that I didn't check those.

lemunozm · 2024-07-17T06:15:13Z

No worries at all! It's more. I think the two tests from cases::liquidity_pools::foreign_investments::mismatching_currencies have been probably broken by me after the foreign changes.

I will try to get this green, and then in your PR, fix whatever remains

* chore: update submodule to latest `main` 6d7f242c0dd83b1b5a4f6d506370a1f3ecbef9ce * wip: fix ITs * chore: update submodule * fix: remove sender param from `Transfer*` messages * chore: cleanup * docs: setup evm * fix: msg tests * fix: more ITs * fix: missing refactor after rebase * chore: update submodule to 223a0f36edabc675f8c74c47b20e366178df7ca3 * chore: improvements * fmt * Apply suggestions from code review * chore: bump spec_version * fmt: taplo

* custo serialize/deserialize for batch * compiling solution * serialization/deserialization working for batch message * remove gmpf changes * add batch nested protection * faster serialization for submessages * correct termination * add tests for deserialize batch of batch * add into_iter * remove unused Box and add pack methods * fix non_std compilation * non_std * fix max_encoded_len recursion issue * fix submodules * reduce batch limit

* refactor: add domain hook address storage * tests: add gateway lp admin account checks * refactor: use GetByKey * fix: outboundq mock * refactor: hook 20 bytes instead of 32

lemunozm · 2024-07-25T11:47:42Z

I think the conflicts are due to #1926 will and I'll fix on Monday when I'm back

lemunozm · 2024-07-29T10:41:59Z

Rebased! CI should pass 🤞🏻

lemunozm

Done my self-review. Some comments/remainders below

lemunozm · 2024-07-30T09:24:00Z

pallets/liquidity-pools/src/tests.rs

+				Permissions::mock_has(|_, _, _| true);
+				Pools::mock_pool_exists(|_| true);
+				Pools::mock_tranche_exists(|_, _| true);
+				AssetRegistry::mock_metadata(|_| Some(util::default_metadata()));


NIT (I think it comes from a merge where does not applied the config_mocks() call)

Suggested change

Permissions::mock_has(|_, _, _| true);

Pools::mock_pool_exists(|_| true);

Pools::mock_tranche_exists(|_, _| true);

AssetRegistry::mock_metadata(|_| Some(util::default_metadata()));

config_mocks();

In config_mocks(), we mock a hook address for the AddTranche message. This test requires this hook to not have been set. If we execute config_mocks, then we need to revert https://github.com/centrifuge/centrifuge-chain/pull/1909/files/5a63633955ba13282a2448dd2a15262e5173c9ba#diff-57577a9a8bc154a3e3b81af63e11d2b863c6716120411b1480117c8c44ee02d5R446-R452

The idea is to overwrite the mock make it fail, so:

config_mocks(); Gateway::mock_get(|_| None);

make the tests work as expected.

The idea is not to copy-past all initialization for all these tests and not create an order in how the mocks are defined. I did that refactor, but I think this test was added later and was skipped by me in the merge.

I will add this to my not yet published cleanup PR! Sorry for not following the status-quo here 🥲

I changed it in a commit after for your work on this, so you indeed followed the status-quo!!

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

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

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

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

* pallets: Add LP Gateway queue pallet * lp-gateway-queue: Add benchmarks * integration-tests: Add LP gateway tests * docs: Update LP gateway queue entry * lp-gateway-queue: Use default weight for PostDispatchInfo * lp-gateway-queue: Add DEFAULT_WEIGHT_REF_TIME const, extract message processing logic, use BaseArithmetic for nonce * runtime: Add defensive weights for LP gateway queue * lint: Obey * taplo: Obey * pallet: Use DispatchResult for extrinsics * runtime: Update benchmarks and weight info * benchmarks: Add default for Message type (wip) * pallet: Add Default bound to Message type

lemunozm · 2024-07-30T11:37:03Z

BTW @wischli I see 6 tests ignored. Did you say 5 in the sync?

* fix: add remark call to borrow proxy * fix: add missing message fields * chore: bump to v0.13.3 * chore: update submodule * chore: enable fixed tests --------- Co-authored-by: Frederik Gartenmeister <[email protected]>

wischli

Looks super clean except for the leftovers from me which @lemunozm already pointed out. Working on a cleanup PR which will also fix the last failing test transfer_tranche_tokens_domain_to_local_to_domain.

wischli · 2024-07-31T09:14:57Z

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

+	#[benchmark]
+	fn process_message() -> Result<(), BenchmarkError> {
+		let caller: T::AccountId = account("acc_0", 0, 0);
+		let message = T::Message::default();


Not sure if this was raised before but eventually we need to benchmark the heaviest message such that the benchmark covers the worst case weight of processing messages. For now, this is fine and a good improvement over not having any benchmarking at all!

That will be tricky since the message itself is irrelevant given that it gets passed to a mock.

Open to suggestions nevertheless, there's plenty of room for improvement here.

it gets passed to a mock

Note that when the benchmark is executed for the real runtimes, the mock is no longer used.

@wischli we already talked about adding a PoV for the MaxEncodedLen to the resulting weights

Note that when the benchmark is executed for the real runtimes, the mock is no longer used.

Oh, that explains a lot then. Where does this magical switch from mock runtime to actual runtime happen?

Yes, we should definitely add that there before merging this.

The magic happens here: https://github.com/centrifuge/centrifuge-chain/blob/main/runtime/centrifuge/src/lib.rs#L2775

That macro calls the benchmarks of those pallets using the real runtime instead of the mock. That's why sometimes the benchmarking is so difficult to achieve. You should be able to create a benchmark that works fine for any runtime configuration.

Right now, the maximum message is no more than 256 bytes. So we're "safe", maybe just adding to the extrinsic is enough:

#[pallet::weight(T::WeightInfo::process_message().saturating_add(Weights::from_parts(0, T::Message::max_encoded_len())))]

I would not go with a huge change because the batch process will need some special treatment regarding this. I'll do it in the batch PR once I rebase over this.

Thank you for the pointer, I'll expand that macro and take a look.

As for the process_message weight, with the changes that I'm currently working on, this will be a bit easier since that extrinsic will now submit the message to the queue. It will be then processed on_idle and return a DispatchResultWithPostInfo that can have more granular weights, as per this.

If you're able to return directly the weight there, then I would say we're fine

BTW (NIT), if you're only interested in the weight, maybe it's interesting to return a tuple (DispatchResult, Weight) instead. That way, you can get rid of the unused pay_fees, and you have the weight for both successful processing and failures. I was experimenting with that in the batch PR; maybe it's useful in this part too.

Awesome idea, it should make our lives easier.

wischli · 2024-07-31T09:41:46Z

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

+							// Extra weight breakdown:
+							//
+							// 1 read for the message
+							// 1 write for the event
+							// 1 write for the message removal


Love this! On a note, dispatching events does not count as a write but we can keep the weight increment here.

wischli · 2024-07-31T09:47:16Z

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

+	/// Storage for messages that will be processed during the `on_idle` hook.
+	#[pallet::storage]
+	#[pallet::getter(fn message_queue)]
+	pub type MessageQueue<T: Config> = StorageMap<_, Blake2_128Concat, T::MessageNonce, T::Message>;
+
+	/// Storage for messages that failed during processing.
+	#[pallet::storage]
+	#[pallet::getter(fn failed_message_queue)]
+	pub type FailedMessageQueue<T: Config> =
+		StorageMap<_, Blake2_128Concat, T::MessageNonce, (T::Message, DispatchError)>;


I am wondering whether we need another storage which maps a message (hash) to its nonce? Has this been discussed already?

cc @cdamian

Yes, the 3rd part of the multi-router work has logic that involves hashes, see a potential solution here.

wischli · 2024-07-31T09:56:28Z

pallets/liquidity-pools/src/tests.rs

+				Permissions::mock_has(|_, _, _| true);
+				Pools::mock_pool_exists(|_| true);
+				Pools::mock_tranche_exists(|_, _| true);
+				AssetRegistry::mock_metadata(|_| Some(util::default_metadata()));


I will add this to my not yet published cleanup PR! Sorry for not following the status-quo here 🥲

wischli · 2024-07-31T11:25:51Z

runtime/integration-tests/src/cases/liquidity_pools_gateway_queue.rs

+};
+
+#[test_runtimes(all)]
+fn submit_and_process<T: Runtime + FudgeSupport>() {


Curios question for @cdamian: Are there any other scenarios you have in mind for ITs or does this suffice for now?

For now this suffices as the LP gateway doesn't actually process anything. In the 2nd part that I'm tackling right now I will extend this whole suite.

From my experience, testing the biggest happy path in IT is enough. Anything else can be tested with UTs in a much simpler way.

* increase version for foreign investments * fix investment issue * fix solidity call for transfers_tokens * fix tests * minimize required tranche investors for IT * fix docs * fix docs * docs: fix hyperlink * refactor: enable ITs for centrifuge + dev runtimes (#1938) * fix: enable ITs for centrifuge + dev runtimes * fmt * fix: revert some centrifuge ITs * fmt * revert centrifuge addition to ITs --------- Co-authored-by: William Freudenberger <[email protected]>

lemunozm · 2024-08-01T13:17:25Z

Finally, this is green! I think we can just rebase and merge

wischli

Approving our great teamwork here! Once CI is green, we can merge this!

wischli and others added 2 commits July 12, 2024 14:09

Merge remote-tracking branch 'origin/main' into feat/lp-v2

426ae56

lemunozm assigned lemunozm, cdamian and wischli Jul 12, 2024

lemunozm changed the title ~~WIP: LPv2 work~~ WIP: Liquidity Pools v2 Jul 12, 2024

lemunozm added 4 commits July 15, 2024 09:59

ignore failing tests (#1910)

fec46b1

fix clippy

8066e95

Merge remote-tracking branch 'origin/main' into feat/lp-v2

dd9f57d

lemunozm marked this pull request as ready for review July 16, 2024 10:21

lemunozm requested review from mustermeiszer, wischli, cdamian and hieronx as code owners July 16, 2024 10:21

fix tests after merge

6f46ba2

lemunozm and others added 8 commits July 17, 2024 16:38

fix foreign investment tests (#1918)

ceda10d

ignore failing tests (#1919)

103b27e

Merge remote-tracking branch 'origin/main' into feat/lp-v2

63d7f05

fix previous merge

e7eb61c

Merge remote-tracking branch 'origin/main' into feat/lp-v2

25bfde7

feat: add domain hook storage (#1928)

020cf59

* refactor: add domain hook address storage * tests: add gateway lp admin account checks * refactor: use GetByKey * fix: outboundq mock * refactor: hook 20 bytes instead of 32

Merge remote-tracking branch 'origin/main' into feat/lp-v2

ecd8821

fix cargo fmt

5a63633

lemunozm commented Jul 30, 2024

View reviewed changes

lp-v2: fix message fields (#1933)

d2118cb

* fix: add remark call to borrow proxy * fix: add missing message fields * chore: bump to v0.13.3 * chore: update submodule * chore: enable fixed tests --------- Co-authored-by: Frederik Gartenmeister <[email protected]>

wischli reviewed Jul 31, 2024

View reviewed changes

refactor: cleanup my leftovers (#1935)

a38c4f9

cdamian mentioned this pull request Aug 1, 2024

Feat/lp v2 use gateway queue #1937

Closed

7 tasks

lemunozm and others added 2 commits August 1, 2024 13:36

Merge remote-tracking branch 'origin/main' into feat/lp-v2

02a4af3

Merge remote-tracking branch 'origin/main' into feat/lp-v2

e9a47a5

lemunozm enabled auto-merge (squash) August 1, 2024 13:18

chore: bump spec version to v0.14.0

0764a1d

wischli temporarily deployed to production August 1, 2024 14:18 — with GitHub Actions Inactive

wischli approved these changes Aug 1, 2024

View reviewed changes

lemunozm merged commit b682138 into main Aug 1, 2024
14 of 15 checks passed

cdamian mentioned this pull request Aug 1, 2024

Feat/lp v2 use gateway queue #1943

Merged

7 tasks

WIP: Liquidity Pools v2 #1909

WIP: Liquidity Pools v2 #1909

Conversation

lemunozm commented Jul 12, 2024 • edited Loading

Description

lemunozm commented Jul 16, 2024 • edited Loading

codecov bot commented Jul 16, 2024 • edited Loading

Codecov Report

lemunozm commented Jul 16, 2024

wischli commented Jul 16, 2024

lemunozm commented Jul 17, 2024

lemunozm commented Jul 25, 2024

lemunozm commented Jul 29, 2024

lemunozm left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm commented Jul 30, 2024

wischli 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

lemunozm Jul 31, 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 1, 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

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

lemunozm Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

lemunozm commented Aug 1, 2024

wischli left a comment

Choose a reason for hiding this comment

lemunozm commented Jul 12, 2024 •

edited

Loading

lemunozm commented Jul 16, 2024 •

edited

Loading

codecov bot commented Jul 16, 2024 •

edited

Loading

lemunozm left a comment •

edited

Loading

lemunozm Jul 31, 2024 •

edited

Loading

lemunozm Aug 1, 2024 •

edited

Loading

lemunozm Jul 31, 2024 •

edited

Loading