lp-gateway: Add multi-router support #1948

cdamian · 2024-08-06T09:10:55Z

Description

LP Gateway:

Multi-router

Added new extrinsic for adding multi-routers, and 2 new storages for multi routers:

one storage that maps a router hash to a router.
- the Router trait now has a hash() method that allows us to create a hash using certain router fields.
one storage that maps a domain to a bounded vec of router hashes.

Inbound/outbound message handing:

Message proofs

The LPEnconding trait has 2 new methods used for:

retrieving a message proof from a message proof message, if any.
converting a message to a message proof message.

Outbound

The OutboundMessageHandler implementation is now queueing one message using one router and message proofs using the remaining routers.

Inbound

Added 2 new storages for keeping track of inbound messages and message proofs.

The inbound message processing logic is checking that the required number of proofs are received before submitting the message to the InboundMessageHandler.

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
Invalidate all message proofs if the routers for a domain change

cdamian · 2024-08-06T09:15:40Z

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

+				return (Err(Error::<T>::NotEnoughRouters.into()), weight);
+			}
+
+			let expected_proof_count: u32 = (routers_count - 1) as u32;


Hey @hieronx, was wondering if the solidity side already has (testing) support for sending these message proofs. This will be necessary for our integration tests.

Yes Solidity has full implementation of the message proofs already. As well as initiate/dispute message recovery.

lemunozm

I left some comments but I think the idea is the correct one!

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

libs/traits/src/liquidity_pools.rs

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

lemunozm · 2024-08-06T09:31:16Z

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

+						match InboundMessageProofCount::<T>::try_mutate(message_proof, |count| {
+							*count += 1;


Could the same router send the proof 3 times and we will count 3?

In this current state, yes. Especially given that we can get the same message more than once.

Added some tests with different nonproof/proof messages to ensure that these storages remain sane, please let me know what you think.

I'll check in the next review pass!

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

lemunozm

I think the overall flow is the correct one, ~~but I think there are some math issues on it~~ <- it was ok!

lemunozm · 2024-08-07T11:47:00Z

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

+			let _ = InboundMessages::<T>::clear(u32::MAX, None);
+			let _ = InboundMessageProofCount::<T>::clear(u32::MAX, None);


We can exceed the block weight here

True, should we maybe add a flag that marks this storage for clearing and then attempt to clear this on_idle?

I think this is when the session_id has to the rescue. on_idle should be as small as possible.

OK, I will wrap up the normal inbound message processing logic to make sure I get it right then I can do another pass to improve storages etc.

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

libs/traits/src/liquidity_pools.rs

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

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

lemunozm · 2024-08-07T13:23:30Z

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

+				return (Ok(()), weight);
+			}
+
+			if current_message_proof_count == total_expected_proof_count {


@cdamian, just one important thing. How here do we know that the counts belong to different routers and not from the same router? I think this is where the concept of "vote" comes in

That's the fun part, we do not...

Not sure if we can use the domain_address though. If it comes via EVM it's most likely the same sender - the gateway router contract deployed on evm. Or maybe I'm mixing things up...

It appears to me that we cannot rely on the domain_address based on our Slack conversation. We need to assume that in the future each domain has more than one configured router. So I would derive it "special origin" for the router, e.g. in case of Axelar axelar_gateway_precompile::Pallet::<T>::GatewayContract (see my slack message).

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

wischli

Overall looks good! Of course we need to iron out open questions and apply our current Slack conversation. Really nice work with the UTs!

wischli · 2024-08-07T12:49:19Z

pallets/liquidity-pools-gateway/routers/src/routers/axelar_evm.rs

+	pub fn hash(&self) -> T::Hash {
+		BlakeTwo256::hash(self.evm_chain.encode().as_slice())
+	}


Just to be sure: The router hash is solely used locally and not as part of any dispatched message right? Asking because Solidity usually uses keccak256.

Yes, this is mainly for the LP gateway to map a router hash to a router in storage.

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

wischli · 2024-08-07T13:01:28Z

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

+			let mut router_hashes = Vec::new();
+
+			for router in &routers {
+				router.init().map_err(|_| Error::<T>::RouterInitFailed)?;


Nit: It would be great to extend the RouterInitFailed error to have some payload such as the router hash. Otherwise it will be hard to know which router caused a failure when running into issues during mutli router setup. E.g.

pub enum Error<T> { /// Router initialization failed. RouterInitFailed { router: T::Hash }, // snip }

Awesome suggestion. thank you!

I didn't know substrate supported custom content in the error; if it works, amazing!

Substrate did not support this from Genesis but IIRC it was added early last year. However, since we have lacked behind Substrate versions so much, we haven't utilised it yet.

It seems we can add other PalletErrors in there - https://github.com/paritytech/polkadot-sdk/blob/149c70938f2b29f8d92ba1cc952aeb63d4084e27/bridges/snowbridge/pallets/inbound-queue/src/lib.rs#L185.

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

wischli · 2024-08-08T07:26:50Z

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

+		}
+
+		fn get_expected_message_proof_count() -> u32 {
+			T::MultiRouterCount::get() - 1


Again, we must use safe math as long as we cannot restrict MultiRouterCount to be at least one.

Suggested change

T::MultiRouterCount::get() - 1

T::MultiRouterCount::get().saturating_sub(1)

Yes, most of the logic for inbound messages is super-dirty, will change all of these once I figure out proof processing accordingly.

Makes absolute sense. Better have something working with some dirt than have a shining non-functional beauty 😅

wischli · 2024-08-08T07:30:06Z

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

+						match InboundMessageProofCount::<T>::try_mutate(message_proof, |count| {
+							count.ensure_add_assign(1)?;
+
+							Ok(*count)
+						}) {
+							Ok(r) => r,
+							Err(e) => return Err(e),
+						};


Am I missing something or can we simplify readability by utilising ??:

Suggested change

match InboundMessageProofCount::<T>::try_mutate(message_proof, |count| {

count.ensure_add_assign(1)?;

Ok(*count)

}) {

Ok(r) => r,

Err(e) => return Err(e),

};

match InboundMessageProofCount::<T>::try_mutate(message_proof, |count| {

count.ensure_add_assign(1)?;

Ok(*count)

})?;

Might drop these fugly try_mutates.

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

wischli · 2024-08-08T07:41:38Z

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

+				return (Ok(()), weight);
+			}
+
+			if current_message_proof_count == total_expected_proof_count {


It appears to me that we cannot rely on the domain_address based on our Slack conversation. We need to assume that in the future each domain has more than one configured router. So I would derive it "special origin" for the router, e.g. in case of Axelar axelar_gateway_precompile::Pallet::<T>::GatewayContract (see my slack message).

wischli · 2024-08-08T07:46:38Z

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

@@ -544,12 +735,12 @@ pub mod pallet {
 		/// weight for these operations in the `DispatchResultWithPostInfo`.
 		fn process_outbound_message(
 			sender: T::AccountId,
-			domain: Domain,


Placeholder (might answer itself further down below): I am curios how or where we determine the destination domain instead now?

cdamian · 2024-08-08T10:04:00Z

Too many conflicts, gonna start fresh from main.

…bound routers, session ID storage

…ound message processing logic

…ogic for removing invalid session IDs on idle

lemunozm · 2024-08-13T06:46:38Z

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

+					}
+				}
+
+				mod four_messages {


If it works for 3, can we ensure it will always work for 4?

In this case, we are testing with 2 routers, and the minimum number of messages needed for a successful message submission is 2. So, if it works with 2, it should work with more messages.

Initially, I added tests for up to 4 messages to confirm that duplicate messages are processed accordingly. Still thinking about generating correct expected results so we can slim these tests down a bit.

cdamian · 2024-08-13T08:50:21Z

Will close this and rebase on #1958 soon.

cdamian commented Aug 6, 2024

View reviewed changes

lemunozm reviewed Aug 6, 2024

View reviewed changes

cdamian commented Aug 6, 2024

View reviewed changes

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

cdamian commented Aug 7, 2024

View reviewed changes

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

lemunozm requested changes Aug 7, 2024

View reviewed changes

lemunozm reviewed Aug 7, 2024

View reviewed changes

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

lemunozm reviewed Aug 7, 2024

View reviewed changes

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

wischli reviewed Aug 8, 2024

View reviewed changes

cdamian force-pushed the feat/lp-v2-gateway-multi-router branch 5 times, most recently from 3a23458 to 80dbb03 Compare August 9, 2024 17:28

cdamian added 6 commits August 10, 2024 12:09

wip

15aaedc

lp-gateway: Add router hash for inbound messages, extrinsic to set in…

e605716

…bound routers, session ID storage

lp-gateway: Use router hashes for inbound, use session ID, update inb…

a9830aa

…ound message processing logic

lp-gateway: Add and use InboundProcessingInfo

9d8e562

lp-gateway: Unit tests WIP

51c2af5

lp-gateway: Unit tests WIP 2

9be4c58

cdamian force-pushed the feat/lp-v2-gateway-multi-router branch from 80dbb03 to 9be4c58 Compare August 10, 2024 09:09

cdamian added 3 commits August 10, 2024 13:34

docs: Improve comments

2535664

lp-gateway: Move message processing logic to a new file

0a6482d

lp-gateway: Merge inbound/outbound routers extrinsics into one, add l…

128bc9f

…ogic for removing invalid session IDs on idle

wischli mentioned this pull request Aug 12, 2024

Support for bidirectional routers #1958

Merged

cdamian added 2 commits August 12, 2024 21:01

lp-gateway: Unit tests WIP

54ee3cb

lp-gateway: Add extrinsic for executing message recovery

121b52f

lemunozm reviewed Aug 13, 2024

View reviewed changes

cdamian closed this Aug 13, 2024

		match InboundMessageProofCount::<T>::try_mutate(message_proof, \|count\| {
		*count += 1;

		let _ = InboundMessages::<T>::clear(u32::MAX, None);
		let _ = InboundMessageProofCount::<T>::clear(u32::MAX, None);

	T::MultiRouterCount::get() - 1
	T::MultiRouterCount::get().saturating_sub(1)

lp-gateway: Add multi-router support #1948

lp-gateway: Add multi-router support #1948

Conversation

cdamian commented Aug 6, 2024 • edited Loading

Description

LP Gateway:

Multi-router

Inbound/outbound message handing:

Message proofs

Outbound

Inbound

Checklist:

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

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

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

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

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

cdamian commented Aug 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdamian commented Aug 13, 2024

cdamian commented Aug 6, 2024 •

edited

Loading

lemunozm left a comment •

edited

Loading