Don't use domain when storing routers #1962

cdamian · 2024-08-13T11:52:07Z

No description provided.

cdamian · 2024-08-13T11:55:45Z

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

-			ensure!(domain != Domain::Centrifuge, Error::<T>::DomainNotSupported);
-
-			//TODO(cdamian): Outbound -  Call router.init() for each router?
+			<Routers<T>>::set(router_ids.clone());


Still a bit confused by this setup since the routers that we're storing are not the ones that we are using when processing...

Anyway, here I'm storing all the available routers right @lemunozm?

I think when processing, you should check the router to process is on that Routers list. We do not want to process a message if the user removes that router. But just that 👍🏻

So if a message comes from a not configured router, then the gateway will discard it

cdamian · 2024-08-13T11:56:43Z

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

-	fn get_expected_proof_count(domain: &Domain) -> Result<u32, DispatchError> {
-		let routers = Routers::<T>::get(domain).ok_or(Error::<T>::RoutersNotFound)?;
+	fn get_expected_proof_count() -> Result<u32, DispatchError> {
+		let routers = Routers::<T>::get();


And here, for example, I should instead use T::RouterId::for_domain...?

Yes, because we want to get only the valid routers for the domain where the message comes

cdamian · 2024-08-13T11:58:06Z

Main question would be, when to use Routers::<T>::get() and when to use T::RouterId::for_domain().

lemunozm · 2024-08-13T12:04:48Z

Routers::::get() and when to use T::RouterId::for_domain()

Routers::<T>::get() the list of all configured routers, something like a whitelist for the gateway.
T::RouterId::for_domain() the list of all ~~configured~~ static routers for a specific domain. Used each time you need to know the routers for a domain.
- More accurate will be to always filter this list with the Router::<T>::get(). I mean, each router returned by the T::RouterId::for_domain() that is not in Router::<T>::get() must be discarded
  - Maybe an auxiliary method that does that or similar, replacing for_domain() for that auxiliary method

cdamian · 2024-08-13T12:08:31Z

Routers::::get() and when to use T::RouterId::for_domain()

Routers::<T>::get() the list of all configured routers, something like a whitelist for the gateway.

T::RouterId::for_domain() the list of all configured routers for a specific domain. Used each time you need to know the routers for a domain.

More accurate will be to always filter this list with the Router::<T>::get(). I mean, each router returned by the T::RouterId::for_domain() that is not in Router::<T>::get() must be discarded

Maybe an auxiliary method that does that or similar, replacing for_domain() for that auxiliary method

Still not sure what the benefit is from doing all of this, instead of keeping track which router IDs are set for each domain, in storage, like the current approach.

lemunozm · 2024-08-13T12:10:53Z

Because basically, T::RouterId::for_domain() is representing the static relation between routers and domains, it's something that the gateway doesn't know (and something it can not ensure either). The gateway knows configured routers.

This list: T::RouterId::for_domain(), exists independently of the routers configured in the gateway

T::RouterId::for_domain() is something hardcoded at runtime type level and not configurable for the user

cdamian · 2024-08-13T12:13:28Z

Because basically, T::RouterId::for_domain() is representing the static relation between routers and domains, it's something that the gateway doesn't know (and something it can not ensure either). The gateway knows configured routers.

This list: T::RouterId::for_domain(), exists independently of the routers configured in the gateway

But what's wrong about having this relation in storage?

If this changes at any time, we'll have to do a RU right? If we store this, we just update the storage with whatever we need.

cdamian · 2024-08-13T12:15:35Z

Because basically, T::RouterId::for_domain() is representing the static relation between routers and domains, it's something that the gateway doesn't know (and something it can not ensure either). The gateway knows configured routers.

In the current implementation, the gateway knows about this relation. And I would also argue that the gateway should know, going forward.

lemunozm · 2024-08-13T12:24:30Z

The gateway doesn't know because not all Domain can be mapped to all Routers

lemunozm · 2024-08-13T12:25:34Z

The relation is not something the operator can choose, is something inherent from how the chain is coded, statically

lemunozm · 2024-08-13T12:27:18Z

Suppose, you use that storage. The user operator can add an entry Domain::Centrifuge with router AxelarEvm, which could not exists. Also the gateway can not check the relation is wrong

cdamian · 2024-08-13T12:36:10Z

The gateway doesn't know because not all Domain can be mapped to all Routers
Suppose, you use that storage. The user operator can add an entry Domain::Centrifuge with router AxelarEvm, which could not exists. Also the gateway can not check the relation is wrong

OK, this is the case in the current implementation and one can definitely screw up the domain to router relationship.

The relation is not something the operator can choose, is something inherent from how the chain is coded, statically

^ fixes that.

Thank you for clarifying @lemunozm!

cdamian · 2024-08-13T12:41:22Z

Will adjust some things here then merge so I can continue working on the initial branch for multi-routers.

* lp-gateway: Unit tests WIP * lp-gateway: Don't store routers under domain * wip

* wip * lp-gateway: Add router hash for inbound messages, extrinsic to set inbound routers, session ID storage * lp-gateway: Add router hash for inbound messages, extrinsic to set inbound routers, session ID storage * lp-gateway: Use router hashes for inbound, use session ID, update inbound message processing logic * lp-gateway: Add and use InboundProcessingInfo * lp-gateway: Unit tests WIP * lp-gateway: Unit tests WIP 2 * docs: Improve comments * lp-gateway: Move message processing logic to a new file * lp-gateway: Merge inbound/outbound routers extrinsics into one, add logic for removing invalid session IDs on idle * lp-gateway: Unit tests WIP * lp-gateway: Add extrinsic for executing message recovery * rebase: WIP * Don't use domain when storing routers (#1962) * lp-gateway: Unit tests WIP * lp-gateway: Don't store routers under domain * wip * wip * lp-gateway: Unit test WIP * lp-gateway: Rename RouterSupport to RouterProvider, add separate entity that implements it * lp-gateway: Add more asserts in unit tests * lp-gateway: Attempt to execute message during recovery * lp-gateway: Remove extra constraints from SessionId * lp-gateway: Drop session ID invalidation (#1965) * lp-gateway: Drop session ID invalidation * lp-gateway: Add test for invalid router * lp-gateway: Add more unit tests * lp-gateway: Move InboundEntry logic to implementation * lp-gateway: Use safe math * lp-gateway: Add more unit tests * checks: Fix clippy, taplo, formatting * review: Remove mock-builder dep, rename LP encoding proof methods * lp-gateway: Remove Default impls for RouterId and GatewayMessage * lp-gateway: Allow empty list of routers * lp-gateway: Drop InboundProcessingInfo * lp-gateway: Rename LP encoding methods to get_proof and to_proof_message * lp-gateway: Move outbound processing logic back to pallet * review: Small fixes * lp-gateway: Add comment for post voting dispatch error case * lp-gatway: Drop session ID check in create_post_voting_entry * lp: Remove unused import * integration-tests: Register routers during setup (#1968) * integration-tests: Set routers in all setup funcs and tests

cdamian added 2 commits August 13, 2024 14:31

lp-gateway: Unit tests WIP

89a5034

lp-gateway: Don't store routers under domain

c12b4db

cdamian changed the title ~~Dont use domain when storing routers~~ Don't use domain when storing routers Aug 13, 2024

cdamian commented Aug 13, 2024

View reviewed changes

cdamian requested review from wischli and lemunozm August 13, 2024 11:59

wip

004e7c1

cdamian marked this pull request as ready for review August 13, 2024 12:59

cdamian merged commit 317631c into feat/lp-v2-gateway-multi-router Aug 13, 2024
4 of 12 checks passed

cdamian deleted the dont-use-domain-when-storing-routers branch August 13, 2024 13:00

cdamian added a commit that referenced this pull request Aug 13, 2024

Don't use domain when storing routers (#1962)

b3fbd20

* lp-gateway: Unit tests WIP * lp-gateway: Don't store routers under domain * wip

cdamian added a commit that referenced this pull request Aug 13, 2024

Don't use domain when storing routers (#1962)

29c453f

* lp-gateway: Unit tests WIP * lp-gateway: Don't store routers under domain * wip

cdamian added a commit that referenced this pull request Aug 13, 2024

Don't use domain when storing routers (#1962)

ee3604f

* lp-gateway: Unit tests WIP * lp-gateway: Don't store routers under domain * wip

cdamian added a commit that referenced this pull request Aug 14, 2024

Don't use domain when storing routers (#1962)

6a60fdd

* lp-gateway: Unit tests WIP * lp-gateway: Don't store routers under domain * wip

cdamian added a commit that referenced this pull request Aug 14, 2024

Don't use domain when storing routers (#1962)

0fa56ed

* lp-gateway: Unit tests WIP * lp-gateway: Don't store routers under domain * wip

cdamian added a commit that referenced this pull request Aug 15, 2024

Don't use domain when storing routers (#1962)

e04c300

* lp-gateway: Unit tests WIP * lp-gateway: Don't store routers under domain * wip

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't use domain when storing routers #1962

Don't use domain when storing routers #1962

cdamian commented Aug 13, 2024

cdamian Aug 13, 2024 •

edited

Loading

lemunozm Aug 13, 2024

lemunozm Aug 13, 2024

cdamian Aug 13, 2024

lemunozm Aug 13, 2024

cdamian commented Aug 13, 2024

lemunozm commented Aug 13, 2024 •

edited

Loading

cdamian commented Aug 13, 2024

lemunozm commented Aug 13, 2024 •

edited

Loading

cdamian commented Aug 13, 2024 •

edited

Loading

cdamian commented Aug 13, 2024 •

edited

Loading

lemunozm commented Aug 13, 2024

lemunozm commented Aug 13, 2024

lemunozm commented Aug 13, 2024

cdamian commented Aug 13, 2024

cdamian commented Aug 13, 2024 •

edited

Loading

Don't use domain when storing routers #1962

Don't use domain when storing routers #1962

Conversation

cdamian commented Aug 13, 2024

cdamian Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

lemunozm Aug 13, 2024

Choose a reason for hiding this comment

lemunozm Aug 13, 2024

Choose a reason for hiding this comment

cdamian Aug 13, 2024

Choose a reason for hiding this comment

lemunozm Aug 13, 2024

Choose a reason for hiding this comment

cdamian commented Aug 13, 2024

lemunozm commented Aug 13, 2024 • edited Loading

cdamian commented Aug 13, 2024

lemunozm commented Aug 13, 2024 • edited Loading

cdamian commented Aug 13, 2024 • edited Loading

cdamian commented Aug 13, 2024 • edited Loading

lemunozm commented Aug 13, 2024

lemunozm commented Aug 13, 2024

lemunozm commented Aug 13, 2024

cdamian commented Aug 13, 2024

cdamian commented Aug 13, 2024 • edited Loading

cdamian Aug 13, 2024 •

edited

Loading

lemunozm commented Aug 13, 2024 •

edited

Loading

lemunozm commented Aug 13, 2024 •

edited

Loading

cdamian commented Aug 13, 2024 •

edited

Loading

cdamian commented Aug 13, 2024 •

edited

Loading

cdamian commented Aug 13, 2024 •

edited

Loading