New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

connectors-gatway: Add integration tests #1478

Merged

cdamian merged 9 commits into main from connectors-gateway-4-integration-tests

Aug 10, 2023

Contributor

cdamian commented Aug 3, 2023 •

edited

Loading

Description

Part 4 of #1453

Changes and Descriptions

Added integration tests for the Connectors Gateway pallet, routers.
Used Connectors Gateway pallet as OutboundQueue for Connectors pallet.
Adjusted integration tests for Connectors.

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

cdamian mentioned this pull request

Add pallet connectors gateway #1376

Closed

4 tasks

cdamian force-pushed the connectors-gateway-4-integration-tests branch 2 times, most recently from 78a6be1 to 19df919 Compare

August 3, 2023 16:57

cdamian marked this pull request as ready for review

August 3, 2023 16:58

cdamian requested review from mustermeiszer, NunoAlexandre, wischli, mikiquantum, branan, lemunozm and thea-leake as code owners

August 3, 2023 16:58

NunoAlexandre reviewed

View reviewed changes

Contributor

NunoAlexandre left a comment

@cdamian thanks a ton for the effort you put into this PR after all the other huges ones already 🙏

We need to work a bit on improving the structure of these new tests as well as in cleaning them up.

Namely:

I left a comment about the idea of testing setting the routers through governance and how that's adding a ton of complexity with the only added value (AFAIK) being that we are testing whether governance itself works. I really think we should drop all of this, use the root origin and focus the tests on testing our own code and setup.
We are adding a new directory to the integration-tests for the connectors_gateway with a nested directory for the routers which then hosts a new test_net and setup, artefacts of the xcm-simulator/emulator. My strong recommendation is to move the connectors_gateway directory to xcm/development/tests/connectors/gateway () and reuse the setup.rs and testnet.rs machinery we already have there for the development runtime ( this means we turn the current xcm/development/tests/connectors.rs file into xcm/development/tests/connectors/mod.rs)
Drop a lot of the indirection functions created (left in comments) and either use the real values of certain objects (like using the real multilocation of a chain or of an asset) or leave comments making it explicit that it's just dummy data; I'd prefer the former option)

runtime/integration-tests/src/connectors_gateway/gateway.rs Outdated Show resolved Hide resolved

runtime/integration-tests/src/connectors_gateway/gateway.rs Outdated

+              use crate::{
+              	chain::centrifuge::{
+              		AccountId, CouncilCollective, FastTrackVotingPeriod, MinimumDeposit, Runtime, RuntimeCall,
+              		RuntimeEvent, PARA_ID,

Contributor

NunoAlexandre Aug 4, 2023

I am confused. This should be testing the development runtime, where and only where connectors is published on. Above we import the CurrencyIdConvert from development but here we import all of these objects from the centrifuge runtime, which shouldn't even work.

We should have this now only using the development runtime but we should also think about how we will be able to reuse or cleanly copy these tests to later use them to test centrifuge, keeping in mind we may have differences in the connectors config between runtimes at a later stage.

Contributor Author

cdamian Aug 7, 2023

I think that CurrencyIdConvert is wrongly added here, currently double-checking that by re-running the whole suite.

Contributor Author

cdamian Aug 7, 2023

OK, that was an incorrect import, most likely a left over from my initial work on this while also figuring out how the re-exports in crate::chain::centrifuge as set up.

However, the weird part is that this should have been caught when compiling these integration tests or am I missing something?

Collaborator

mustermeiszer Aug 9, 2023

If it all is under tests I might not see that.

runtime/integration-tests/src/connectors_gateway/gateway.rs Outdated Show resolved Hide resolved

runtime/integration-tests/src/connectors_gateway/gateway.rs Outdated

Comment on lines 77 to 85

+              	let xcm_domain_location = MultiLocation {
+              		parents: 0,
+              		interior: Junctions::X1(Junction::Parachain(456)),
+              	};
+              	let currency_id = CurrencyId::ForeignAsset(1);
+              	let currency_location = MultiLocation {
+              		parents: 0,
+              		interior: Junctions::X1(Junction::Parachain(123)),
+              	};

Contributor

NunoAlexandre Aug 4, 2023

I understand this is just dummy data but these values, particularly the currency_location, doesn't really make much sense and can be confusing reading through this.

Contributor Author

cdamian Aug 7, 2023

I feel the same way regarding the dummy aspect and the actual values, what would you suggest as an improvement? Some different kind of Junction?

runtime/integration-tests/src/connectors_gateway/gateway.rs Outdated

+              	let ethereum_xcm_router = EthereumXCMRouter::<Runtime> {
+              		router: XCMRouter {
+              			xcm_domain,
+              			_marker: Default::default(),

Contributor

NunoAlexandre Aug 4, 2023

do we really need to specify these _marker:s?

Contributor Author

cdamian Aug 7, 2023

Yes, otherwise we'll get an error due to missing fields.

Collaborator

mustermeiszer Aug 9, 2023

I think we can do

XCMRouter {
 xcm_domain,
 ..Default::default()
}

Contributor Author

cdamian Aug 9, 2023

If we use that we'll run into:

the trait bound `XCMRouter<development_runtime::Runtime>: std::default::Default` is not satisfied

I don't see the issue with using the current approach tbh.

runtime/integration-tests/src/utils/connectors_gateway.rs Outdated Show resolved Hide resolved

runtime/integration-tests/src/xcm/development/tests/connectors.rs Outdated Show resolved Hide resolved

runtime/integration-tests/src/xcm/development/tests/connectors.rs Outdated Show resolved Hide resolved

runtime/integration-tests/src/xcm/development/tests/connectors.rs

Comment on lines +1656 to +1672

+              	pub fn get_default_moonbeam_native_token_location() -> MultiLocation {
+              		MultiLocation {
+              			parents: 1,
+              			interior: X2(Parachain(PARA_ID_MOONBEAM), general_key(&[0, 1])),
+              		}
+              	}

Contributor

NunoAlexandre Aug 4, 2023

This can also be confusing since that this is not actually GLMR's multilocation. Also, there's no "default" location of a token, as that sounds like it may have multiple but this is the default one.

Contributor Author

cdamian Aug 7, 2023

It's the same location that we use to have in the setup_pre_requirements. Given that all the other const are prepended with DEFAULT I thought it makes sense to do the same here, even though it would be clearer if they were named DEFAULT_TEST_*

runtime/integration-tests/src/connectors_gateway/routers/ethereum_xcm/router.rs Outdated Show resolved Hide resolved

cdamian force-pushed the connectors-gateway-4-integration-tests branch from 19df919 to 548f7a1 Compare

August 7, 2023 12:23

mustermeiszer previously approved these changes

View reviewed changes

Collaborator

mustermeiszer left a comment

I have no opinion on the structuring aspect. Regarding using democracy I left my opinion. But otherwise, SICK!

cdamian force-pushed the connectors-gateway-4-integration-tests branch from 1312f1a to d1d4f3f Compare

August 9, 2023 10:41

NunoAlexandre previously approved these changes

View reviewed changes

Contributor

NunoAlexandre left a comment

Looks good, @cdamian!

cdamian dismissed stale reviews from NunoAlexandre and mustermeiszer via

7985f4d

August 9, 2023 12:25

cdamian force-pushed the connectors-gateway-4-integration-tests branch 2 times, most recently from 7985f4d to da4eb31 Compare

August 9, 2023 12:59

cdamian enabled auto-merge (squash)

August 9, 2023 13:15

mustermeiszer approved these changes

View reviewed changes

NunoAlexandre approved these changes

View reviewed changes

Contributor

NunoAlexandre left a comment

👏

cdamian force-pushed the connectors-gateway-4-integration-tests branch from da4eb31 to ca0cef2 Compare

August 10, 2023 07:38

cdamian added 6 commits

August 10, 2023 12:23


          connectors-gateway: Add Ethereum Transaction pallet

886dd78


          connectors-gateway: Add gateway routers

944e960


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

6d480e3


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

c1fa05d


          gateway-routers: Change folder structure

ff92986


          gateway-routers: Re-structure routers

790521f

cdamian added 3 commits

August 10, 2023 12:23


          connectors-gateway: Add integration tests

f4f1455


          integration-tests: Drop unused imports, rename call constructor funcs…

0e30b13

…, fix rebase issues


          integration-tests: Re-structure connectors tests

069305c

cdamian force-pushed the connectors-gateway-4-integration-tests branch from ca0cef2 to 069305c Compare

August 10, 2023 09:23

cdamian merged commit 8b2b0d7 into main

cdamian deleted the connectors-gateway-4-integration-tests branch

August 10, 2023 12:33

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

NunoAlexandre NunoAlexandre approved these changes

mustermeiszer mustermeiszer approved these changes

wischli Awaiting requested review from wischli wischli is a code owner

mikiquantum Awaiting requested review from mikiquantum

branan Awaiting requested review from branan

lemunozm Awaiting requested review from lemunozm

thea-leake Awaiting requested review from thea-leake

Labels

None yet