cdamian commented 1 month ago

Description

This PR adds support for multiple routers in the LP gateway.

LP Gateway Pallet Changes

Extrinsics

Added set_routers extrinsic that sets the set of valid routers for both inbound and outbound messages.
- Apart from populating the according Routers storage, a new session ID is created to keep track of incoming messages or proofs.
Added execute_message_recovery extrinsic that is used to manually increase the proof count for a particular message proof and router ID, and execute the message if the required proof count is met.

Message Processing

Inbound

For every inbound message that is received via the MessageReceiver implementation, we check the Allowlist storage to confirm that the origin is allowed to submit message, if successful, we queue an inbound gateway message that contains the necessary information for processing i.e. origin address, message and router identifier.

For every inbound gateway message we create an InboundEntry that specifies whether the message is a message type or proof type.

After validation, this InboundEntry is then added to storage, if a previous entry of this type is found, the expected counts are updated accordingly - for a message type, the total expected proof count is increased by the value required for that domain, similarly, for a proof type, the proof count is increased by 1.

If the PendingInboundEntries storage contains one message entry from the first router, and the required proof entries from each of the remaining routers, the counts are decreased and the message is forwarded to the InboundMessageHandler for execution.

Outbound

For every outbound message received from the called of the OutboundMessageHandler::handle we retrieve all routers for the destination Domain.

The queueing is done using the LP Gateway Queue as follows:

the message is queued as-is using the first router.
the proof for the message is queued using the remaining routers.

Upon eventual receipt of the queued messages/proofs from the LP Gateway Queue, they are sent for further processing to the MessageSender.

Other Notable Changes

The LPEncoding trait now has 2 more methods for retrieving a message proof and converting a message type to a proof type.
The RouterSupport trait was slightly adjusted to better fit with the requirement of providing a statically defined list of RouterIds for a specific domain.

Checklist:

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

lemunozm commented 4 weeks ago

Regarding the tests, I think we should simplify them and/or factorize them. 5k lines are too many lines to refactor tomorrow because something has changed 😅.

I think by using two messages and 2 routers in the test cases, we can have all the safety we're looking for.

At least, at this stage, where we actually only support 1 router 😆

cdamian commented 4 weeks ago

Regarding the tests, I think we should simplify them and/or factorize them. 5k lines are too many lines to refactor tomorrow because something has changed 😅.

I think by using two messages and 2 routers in the test cases, we can have all the safety we're looking for.

At least, at this stage, where we actually only support 1 router 😆

Might seem a bit of an overkill but when I initially dealt with the message processing logic, these tests helped me stay on the correct path. I would definitely keep the ones for two_routers all the way up to the four_messages ones since those confirm that multiple identical entries are processed accordingly. For the ones with the three_routers, I would only keep the three_message ones, since that covers the case where we process maximum one message.

I'm all for simplifying these and hopefully have some logic that generates messages + expected test results, but then again, that logic would also have to be tested and I'm not sure if we have time to deal with that given the remaining things that we have to do for the audit.

Maybe we can live with having ultra-verbose tests for the time being since IMO, it's a bit clear of what's tested and what's expected.

lemunozm commented 4 weeks ago

Maybe we can live with having ultra-verbose tests for the time being since IMO, it's a bit clear of what's tested and what's expected.

Agree @cdamian, let's keep it as tech-debt. Testing is not under audit.

cdamian commented 4 weeks ago

Gonna add the fixes for ITs on a different branch to reduce the noise here.

codecov[bot] commented 4 weeks ago

Codecov Report

Attention: Patch coverage is 76.73469% with 57 lines in your changes missing coverage. Please review.

Project coverage is 48.87%. Comparing base (e8f1efd) to head (89ffd0e). Report is 1 commits behind head on main.

Files	Patch %	Lines
.../liquidity-pools-gateway/src/message_processing.rs	79.67%	38 Missing :warning:
pallets/liquidity-pools-gateway/src/lib.rs	76.59%	11 Missing :warning:
pallets/liquidity-pools/src/message.rs	0.00%	6 Missing :warning:
pallets/axelar-router/src/lib.rs	0.00%	1 Missing :warning:
runtime/common/src/routing.rs	0.00%	1 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #1961 +/- ## ========================================== + Coverage 47.76% 48.87% +1.10% ========================================== Files 183 183 Lines 12929 13143 +214 ========================================== + Hits 6176 6424 +248 + Misses 6753 6719 -34 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

centrifuge / centrifuge-chain

Feat/lp v2 gateway multi router #1961