connectors-gateway: Add gateway routers #1475

cdamian · 2023-08-01T11:42:27Z

Description

Part 3 of - #1376

The routers added in this PR will be used by the Connectors Gateway pallet when sending outgoing messages via EVM or XCM.

Currently there are 3 routers:

Axelar EVM - used to call the Axelar Gateway contract which in turn calls the Connectors contract.
Axelar XCM - used to call the Axelar Gateway contract which in turn calls the Connectors contract via Moonbeam XCM.
Ethereum XCM - used to call the Connectors contract via Moonbeam XCM.

Changes and Descriptions

Add Connectors Gateway routers

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

NunoAlexandre

First Round 🏄‍♂️ A higher level review first mostly addressing the structure we are adopting here.

connectors-gateway-routers is not an actual pallet - it has no Config, no storage, no extrinsics, etc. We can and imo should just make this a sub directory of the connectors-gateway pallet.
Having connectors-gateway-routers/routers/ hosting the "primitives" of the actual routers in the root dir (i.e, connectors-gateway-routers/axelar_evm.rs, connectors-gateway-routers/ethereum_xcm.rs, etc) is counter-intuitive. I suggest that we do:

./connectors-gateway/
  |- ./routers/
     |- ./mod.rs # contains all primitives used by the actual routers; if useful, can group them in `mod`s
     |- axelar_evm.rs
     |- axelar_xcm.rs
     |- ethereum_xcm.rs

the do_init pattern is something that I personally have never seen in Substate. Happy to change my mind, but as of now I'd rather drop that and write a migration instead that sets whatever state we may want to set once we are ready to do that (we know the right weights, chain ids, etc)

pallets/connectors-gateway/connectors-gateway-routers/Cargo.toml

pallets/connectors-gateway/connectors-gateway-routers/src/axelar_evm.rs

pallets/connectors-gateway/connectors-gateway-routers/src/lib.rs

cdamian · 2023-08-02T08:49:26Z

First Round surfing_man A higher level review first mostly addressing the structure we are adopting here.

1. `connectors-gateway-routers` is not an actual pallet - it has no `Config`, no storage, no extrinsics, etc. We can and imo should just make this a sub directory of the `connectors-gateway` pallet.

2. Having `connectors-gateway-routers/routers/` hosting the "primitives" of the actual routers in the root dir (i.e, `connectors-gateway-routers/axelar_evm.rs`, `connectors-gateway-routers/ethereum_xcm.rs`, etc) is counter-intuitive. I suggest that we do:

./connectors-gateway/
  |- ./routers/
     |- ./mod.rs # contains all primitives used by the actual routers; if useful, can group them in `mod`s
     |- axelar_evm.rs
     |- axelar_xcm.rs
     |- ethereum_xcm.rs

4. the `do_init` pattern is something that I personally have never seen in Substate. Happy to change my mind, but as of now I'd rather drop that and write a migration instead that sets whatever state we may want to set once we are ready to do that (we know the right weights, chain ids, etc)

Regarding the last point - there are some pallets with init logic based on the genesis config, the session-info pallet would be one example IIRC.

As for the second part regarding migration/setting of required states, I think it makes sense to have it like this since we would only need one governance step for adding a router, otherwise, in the case of XCM, we would need 2 calls to set the storages + 1 call to set the router, and I don't see a good reason for that.

lemunozm

Just some random comments, not a review at all!

Thanks again for spliting the PRs, much easier to understand everything!

runtime/development/Cargo.toml

pallets/connectors-gateway/connectors-gateway-routers/src/mock.rs

NunoAlexandre

Looking better already, thanks for taking the comments in consideration 🙏

A few more comments. I will do another round later going through the details of the messages encoding since it's important to check those outs more carefully

pallets/connectors-gateway/routers/src/axelar_evm.rs

pallets/connectors-gateway/routers/src/routers.rs

pallets/connectors-gateway/routers/src/axelar_xcm.rs

pallets/connectors-gateway/routers/src/routers.rs

NunoAlexandre

LGTM! We will probably find some quirks when testing this on Moonbase Alpha but since it's all in development and appears correct we are good merging this imo.

The only thing I would still say is whether there really is a need for a separate crate for these routers over having them as a sub-dir of connectors-gateway. Not a blocker for me!

Thanks a lot for all the work and all the positivity in taking in all the review comments 🙏

cdamian · 2023-08-03T10:14:38Z

LGTM! We will probably find some quirks when testing this on Moonbase Alpha but since it's all in development and appears correct we are good merging this imo.

The only thing I would still say is whether there really is a need for a separate crate for these routers over having them as a sub-dir of connectors-gateway. Not a blocker for me!

Thanks a lot for all the work and all the positivity in taking in all the review comments pray

@mustermeiszer came with the idea of having routers in a separate crate, and given the xcm/evm dependencies that we need there I think it makes sense to have it like that and not to bloat the actual gateway.

NunoAlexandre · 2023-08-03T13:11:21Z

LGTM! We will probably find some quirks when testing this on Moonbase Alpha but since it's all in development and appears correct we are good merging this imo.
The only thing I would still say is whether there really is a need for a separate crate for these routers over having them as a sub-dir of connectors-gateway. Not a blocker for me!
Thanks a lot for all the work and all the positivity in taking in all the review comments pray

@mustermeiszer came with the idea of having routers in a separate crate, and given the xcm/evm dependencies that we need there I think it makes sense to have it like that and not to bloat the actual gateway.

Fair points, this is fine 👍

mustermeiszer

Nice setup! Pretty flexible!

mustermeiszer · 2023-08-09T07:25:51Z

pallets/connectors-gateway/routers/src/lib.rs

+		pallet_xcm_transactor::Pallet::<T>::set_transact_info(
+			<T as frame_system::Config>::RuntimeOrigin::root(),
+			self.xcm_domain.location.clone(),
+			self.xcm_domain.transact_info.transact_extra_weight,
+			self.xcm_domain.transact_info.max_weight,
+			self.xcm_domain.transact_info.transact_extra_weight_signed,
+		)?;


Could we run into troubles if

RouterA sets these values

time passes and we add a new RouterB

RouterB overwrites values with a new value?

I don't think so. In theory, we should only have one router for a specific location, so if at some point we overwrite the router I think it makes sense to overwrite the xcm transactor info that is stored.

mustermeiszer · 2023-08-09T07:28:17Z

pallets/connectors-gateway/routers/src/lib.rs

+			// The currency in which we want to pay fees.
+			CurrencyPayment {
+				currency: Currency::AsCurrencyId(self.xcm_domain.fee_currency.clone()),
+				fee_amount: None,


Why is this None?

This was as per the original connectors implementation found here -

centrifuge-chain/pallets/connectors/src/lib.rs

Line 541 in ac7ee49

fee_amount: None,

Maybe @NunoAlexandre can provide more specific info?

cdamian changed the title ~~Connectors gateway 3 gateway routers~~ connectors-gateway: Add gateway routers Aug 1, 2023

cdamian mentioned this pull request Aug 1, 2023

Add pallet connectors gateway #1376

Closed

4 tasks

cdamian force-pushed the connectors-gateway-3-gateway-routers branch 2 times, most recently from 1a05237 to 35dfaa1 Compare August 1, 2023 13:43

cdamian marked this pull request as ready for review August 1, 2023 14:03

cdamian requested review from mikiquantum, mustermeiszer, branan, NunoAlexandre, lemunozm, wischli and thea-leake as code owners August 1, 2023 14:03

NunoAlexandre reviewed Aug 2, 2023

View reviewed changes

lemunozm reviewed Aug 2, 2023

View reviewed changes

cdamian force-pushed the connectors-gateway-3-gateway-routers branch from 35dfaa1 to afb5f32 Compare August 2, 2023 11:02

NunoAlexandre reviewed Aug 2, 2023

View reviewed changes

cdamian added 9 commits August 3, 2023 12:31

connectors-gateway: Add Ethereum Transaction pallet

5022131

connectors-gateway: Add gateway routers

d42f195

gateway-routers: Add generic routers, Axelar XCM router, update tests

5097ee2

runtime: Remove duplicate dep entries

bf41472

gateway-routers: Use specific types when getting Axelar encoded contract

2d2ddb3

gateway-routers: Change folder structure

8f9e096

deps: Update dependencies

5a1921b

gateway-routers: Re-structure routers

1a3436f

axelar-evm: Add Goerli to EVMChain

cdb99c0

cdamian force-pushed the connectors-gateway-3-gateway-routers branch from 7505ca8 to cdb99c0 Compare August 3, 2023 09:32

NunoAlexandre approved these changes Aug 3, 2023

View reviewed changes

cdamian merged commit 637ec22 into main Aug 3, 2023

cdamian deleted the connectors-gateway-3-gateway-routers branch August 3, 2023 14:00

mustermeiszer reviewed Aug 9, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

connectors-gateway: Add gateway routers #1475

connectors-gateway: Add gateway routers #1475

cdamian commented Aug 1, 2023 •

edited

Loading

NunoAlexandre left a comment

cdamian commented Aug 2, 2023

lemunozm left a comment

NunoAlexandre left a comment

NunoAlexandre left a comment

cdamian commented Aug 3, 2023

NunoAlexandre commented Aug 3, 2023

mustermeiszer left a comment

mustermeiszer Aug 9, 2023

cdamian Aug 9, 2023

mustermeiszer Aug 9, 2023

cdamian Aug 9, 2023

connectors-gateway: Add gateway routers #1475

connectors-gateway: Add gateway routers #1475

Conversation

cdamian commented Aug 1, 2023 • edited Loading

Description

Changes and Descriptions

Checklist:

NunoAlexandre left a comment

Choose a reason for hiding this comment

cdamian commented Aug 2, 2023

lemunozm left a comment

Choose a reason for hiding this comment

NunoAlexandre left a comment

Choose a reason for hiding this comment

NunoAlexandre left a comment

Choose a reason for hiding this comment

cdamian commented Aug 3, 2023

NunoAlexandre commented Aug 3, 2023

mustermeiszer left a comment

Choose a reason for hiding this comment

mustermeiszer Aug 9, 2023

Choose a reason for hiding this comment

cdamian Aug 9, 2023

Choose a reason for hiding this comment

mustermeiszer Aug 9, 2023

Choose a reason for hiding this comment

cdamian Aug 9, 2023

Choose a reason for hiding this comment

cdamian commented Aug 1, 2023 •

edited

Loading