centrifuge / centrifuge-chain

Centrifuge Chain: the layer-1 blockchain for real-world assets, built on Substrate.
https://centrifuge.io
GNU Lesser General Public License v3.0
182 stars 78 forks source link

feat: LP V2 message forwarding #1971

Closed wischli closed 3 weeks ago

wischli commented 4 weeks ago

Description

Adds feature to handle forwarding inbound and outbound messages

In order to set up forwarding for some Domain::Evm(123)

  1. In pallet-liquidity-pools-forwarder storage RouterForwarding: Store the source_domain and forwarding contract address for some router_id
  2. In pallet-liquidity-pools-gateway storage Allowlist: Store the (source_domain, DomainAddress::Evm(source_somain, forwarding_contract_address)

Sending messages

Before this PR

Gateway -> RouterDispatcher -> Router

After this PR

Gateway -> Forwarder -> Serializer -> RouterDispatcher -> Router

Receiving messages

Before this PR*

Router -> Gateway

After this PR

Router -> Serializer -> Forwarder -> Gateway

TODO

Checklist:

lemunozm commented 4 weeks ago

Just a NIT thought. Seems like an entity called forwarder is not the entity in charge of serializing / deserializing, could be weird to find those methods here? πŸ€”

Could make sense a super thing wrapper as a type runtime that makes that serialization?

// To send
gateway -> forwarder -> serializer -> router-dispatcher -> router 

// To receive
router -> serializer -> forwarder -> gateway

The serializer could be just a MessageSerializer implementing MessageSender and MessageReceiver type who calls msg.serialize() and deserialize().

Just a thought, and maybe being a paranoic organizer πŸ˜†

wischli commented 3 weeks ago

Just a NIT thought. Seems like an entity called forwarder is not the entity in charge of serializing / deserializing, could be weird to find those methods here? πŸ€”

Could make sense a super thing wrapper as a type runtime that makes that serialization?


// To send

gateway -> forwarder -> serializer -> router-dispatcher -> router 

// To receive

router -> serializer -> forwarder -> gateway

The serializer could be just a MessageSerializer implementing MessageSender and MessageReceiver type who calls msg.serialize() and deserialize().

Just a thought, and maybe being a paranoic organizer πŸ˜†

I agree that your proposal would be the preferred design. Will look into it later after finishing the mandatory tasks for this feature.

lemunozm commented 3 weeks ago

Great! but please, it is not mandatory at all for Monday πŸ™πŸ»

wischli commented 3 weeks ago

Great! but please, it is not mandatory at all for Monday πŸ™πŸ»

Much cleaner now: b39fa47

cdamian commented 3 weeks ago

Only a couple of minor nits, I'm happy we got to slay this one as well, thank you @wischli!

codecov[bot] commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 56.00000% with 33 lines in your changes missing coverage. Please review.

Project coverage is 49.01%. Comparing base (03262c5) to head (5cf3752).

Files Patch % Lines
pallets/liquidity-pools/src/message.rs 0.00% 21 Missing :warning:
runtime/common/src/routing.rs 0.00% 6 Missing :warning:
pallets/liquidity-pools-forwarder/src/lib.rs 92.30% 3 Missing :warning:
runtime/altair/src/lib.rs 0.00% 1 Missing :warning:
runtime/centrifuge/src/lib.rs 0.00% 1 Missing :warning:
runtime/development/src/lib.rs 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1971 +/- ## ========================================== + Coverage 48.92% 49.01% +0.09% ========================================== Files 183 184 +1 Lines 13133 13200 +67 ========================================== + Hits 6425 6470 +45 - Misses 6708 6730 +22 ```

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

wischli commented 3 weeks ago

Thanks @wischli for bringing this super PR along with documentation chains so fast!! πŸš€ I really like the result.

Just few NITs, some questions, and just one think that I would simplify about mocking the Message in both pallets.

But the code looks super great! Thanks

Absolutely agree! Sorry I didn't initiate like that but prio was to get it done. If you think, you can still fit something in, great, but entirely optional. Whatever we change might have to be updated/added to the docs.