Feat: LP integration tests #1720

mustermeiszer · 2024-02-06T09:25:24Z

Description

Currently, we can only test EVM <> Substrate interactions in the UI/testnetworks. This is cumbersome and not reliable for testing changes both on the Substrate as also on the EVM side.
This PR enables to write real e2e tests for both codebases.

Changes and Descriptions

builds https:/centrifuge/liquidity-pools and returns all needed *.json files for creating contracts
provides an additional trait EvmEnv that provides interaction with EVM in the test codebase

How to test

Install Foundry Forge CLI
Run Tests

cargo test -p runtime-integration-tests

If you encounter an error with an unset LP_SOL_SOURCES env var, the script did not build the solidity contracts in target/debug/build/runtime-integration-tests-*/out/liquidity-pools properly. Try removing all runtime-integration-tests directories in target/debug/build/ and runtime_integration_tests ones in target/debug/debs/. If that doesn't help, clear the entire target directory.

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

lemunozm

Really cool testing feature and added very clean! 🚀

Left some minor questions, suggestions and NITs. But everything is minor. Good job!

runtime/integration-tests/src/generic/cases/example.rs

lemunozm · 2024-02-12T07:53:51Z

runtime/integration-tests/build.rs

+ /*
+ match Command::new("git")
+ .args(&["fetch", "--all", "--recurse-submodules=yes"])
+ .output()
+ {
+ Ok(o) if o.status.success() => {}
+ Ok(o) => {
+ println!(
+ "cargo:warning=Git fetch failed with: \n - status: {}\n -stderr: {}",
+ o.status,
+ String::from_utf8(o.stderr).expect("stderr is utf-8 encoded. qed.")
+ );
+ }
+ Err(err) => {
+ println!("cargo:warning=Failed to execute git command: {}", err);
+ }
+ }
+ */


Q: Should this always be done, or is it expected the developer does this manually before running these tests?

+1. AFAICT, we should solve this via CI which regularly pulls the latest contracts

we should solve this via CI which regularly pulls the latest contracts

One concern regarding this is that an update in liquidity-pools can break our CI. I suppose it is centrifuge-chain who should choose the commit used from liquidity-pools. When liquidity-pools is updated, then we should create a commit in centrifuge-chain just updating the dependency.

I am not 100% sure on the process here, tbh. @hieronx wdyt?

When liquidity-pools is updated, then we should create a commit in centrifuge-chain just updating the dependency.

I think this makes sense. Also because we probably want to be able to run the main tests against the release commit from liquidity pools (what is live now), but also want to be able to run the tests in a branch against a more recent development commit. Having the commit be chosen manually would make this possible.

IMO, the Liquidity Pools repo should include a CI which runs LP integration tests from Centrifuge Chain and notify Protocol / Centrifuge Chain repo when it is failing such that we know a breaking change has been introduced on the EVM side.

runtime/integration-tests/src/generic/envs/runtime_env.rs

runtime/integration-tests/src/generic/cases/lp/pool_management.rs

runtime/integration-tests/src/generic/cases/lp/mod.rs

runtime/integration-tests/src/generic/cases/lp/pool_management.rs

runtime/integration-tests/src/generic/envs/runtime_env.rs

cdamian · 2024-02-12T09:21:58Z

.gitmodules

+[submodule "runtime/integration-tests/submodules/liquidity-pools"]
+ path = runtime/integration-tests/submodules/liquidity-pools
+ url = [email protected]:centrifuge/liquidity-pools.git
+ branch = feat/local-router


Eventually, we want to use the main branch here right?

Good point! Curious too about this

Feature branch for the local router we need for routing to local evm. Will be master once this is merged

Main branch can be used now. cc @wischli

Update: Main branch cannot be used as the LocalRouter feat is required. We have been using the centrifuge-chain/release-v1.0 branch: centrifuge/liquidity-pools#260

cdamian · 2024-02-12T09:23:05Z

Looks awesome so far @mustermeiszer! Thanks for taking care!

…ests

runtime/integration-tests/src/generic/cases/liquidity_pools_transfers.rs

runtime/integration-tests/src/generic/cases/lp/transfers.rs

cdamian · 2024-06-11T08:55:15Z

runtime/integration-tests/build.rs

+ .to_str()
+ .expect("OsStr is utf-8. qed");
+
+ match Command::new("forge")


Should we add a small script for downloading a specific version of this or at least mention the version that is known to work. I guess we want to avoid having failures here by using a version that might behave differently.

IMO, there definitely needs to be some improvement to this. Maybe it is sufficient to add forge install to our init.sh script for local development. For CI, forge is already installed manually. Eventually, there should be one codepath setting up requirements both for local development. as well as CI.

I am unsure how to proceed here. I think for now, erroring out if forge is not installed is fine, or should we install it during the build script - i mean locally in the build directory?

or should we install it during the build script - i mean locally in the build directory?

I think this is the best option. The script should fetch locally everything it needs to work properly.

Although compiling this implies compiling 900 crates 😅

runtime/integration-tests/src/generic/cases/lp/mod.rs

cdamian · 2024-06-11T10:00:27Z

runtime/integration-tests/build.rs

+ let source = match parent {
+ "liquidity-pools" => {
+ println!(
+ "cargo:info=Build liquidity-pools Solidity contracts. Stored at {} ",


cargo:info doesn't seem to be supported.

using warning now. Maybe confusing, but I think helpful to know where they are stored, wdyt?

Yea, let's keep the warning for now, I wanna re-run everything on this branch later on and I think I'll need these ^^

Maybe executing echo and printing the info message can work to avoid the warning?

runtime/integration-tests/build.rs

cdamian · 2024-06-11T10:01:55Z

runtime/integration-tests/build.rs

+
+ /*
+ match Command::new("git")
+ .args(&["pull", "--all", "--recurse-submodules=yes"])


Note - Locally, I had to run a git submodule update --init --recursive first.

Do we want that in or should everybody pull the deps themselves?

Hmm, probably not. But I think we should adapt some of the comments/error messages to in this part.

Agree with @cdamian that we should modify the message.

runtime/integration-tests/build.rs

lemunozm

First review pass. Looks pretty clean despite the amount of code in the PR 💯

Just some Nits and question. Nothing important. In my second round I wanted to review the cases. Hopefully today or tomorrow

libs/utils/src/lib.rs

runtime/integration-tests/src/generic/cases/ethereum_transaction.rs

runtime/integration-tests/src/generic/env.rs

runtime/integration-tests/src/generic/cases/ethereum_transaction.rs

runtime/integration-tests/src/generic/envs/evm_env.rs

runtime/integration-tests/src/generic/envs/runtime_env.rs

runtime/integration-tests/src/generic/mod.rs

lemunozm

LGTM! Let's merge this beast!

runtime/integration-tests/src/generic/cases/lp/mod.rs

lemunozm · 2024-06-13T09:30:30Z

@cdamian I think we need your approval to get this in 💪🏻

mustermeiszer added 5 commits February 5, 2024 17:33

feat: wip build script

f731286

wip: intermediate

9d15786

feat: update lp submodule

eac27ea

feat: update lp submodule

1f418e9

feat: intermedaite

0e3bb0a

mustermeiszer force-pushed the feat/lp-integration-tests branch from 82aaa1a to 0e3bb0a Compare February 6, 2024 16:29

mustermeiszer added 16 commits February 6, 2024 17:34

feat: deploy first code

22abc57

feat: add raw

fd6f41b

feat: add raw

59fa031

feat: first run for deploying on sol side

d6e5817

feat: add notes for tomorrow

1f621e2

feat: evm side setup

518d5d9

update remote lp

2c23682

feat: try using local script

2e5021e

feat: update lp deps

2c69b18

fix: latest sol version does not have reference anymore.

78f536f

feat: erc20 deploys

fd113f6

feat: set router

c9ac15a

feat: register assets

53c8b86

feat: add currencies to local lp deployment

e1b0fb8

feat: test AddCurrency msg

08ffae4

chore: reorg

6b4f005

lemunozm reviewed Feb 12, 2024

View reviewed changes

cdamian reviewed Feb 12, 2024

View reviewed changes

runtime/integration-tests/src/generic/envs/runtime_env.rs Outdated Show resolved Hide resolved

cdamian reviewed Feb 12, 2024

View reviewed changes

runtime/integration-tests/src/generic/envs/runtime_env.rs Outdated Show resolved Hide resolved

cdamian reviewed Feb 12, 2024

View reviewed changes

runtime/integration-tests/src/generic/envs/runtime_env.rs Outdated Show resolved Hide resolved

cdamian reviewed Feb 12, 2024

View reviewed changes

wischli added the I4-tests Test needs fixing or improving. label Feb 12, 2024

wischli assigned mustermeiszer Feb 12, 2024

mustermeiszer added 2 commits June 7, 2024 23:11

feat: enable precompile for all runtimes

598610e

Merge remote-tracking branch 'origin/main' into feat/lp-integration-t…

f55a91e

…ests

mustermeiszer commented Jun 8, 2024

View reviewed changes

mustermeiszer added 5 commits June 8, 2024 21:09

fix: transfers

950242a

fix: after merge main

eb84385

fix: fmt with nightly

6ac4563

fix: import

c6198a1

fix: unneeded import

990deb9

cdamian reviewed Jun 11, 2024

View reviewed changes

runtime/integration-tests/src/generic/cases/lp/mod.rs Show resolved Hide resolved

cdamian reviewed Jun 11, 2024

View reviewed changes

runtime/integration-tests/build.rs Outdated Show resolved Hide resolved

cdamian reviewed Jun 11, 2024

View reviewed changes

fix: env and block

9b15d12

cdamian reviewed Jun 11, 2024

View reviewed changes

runtime/integration-tests/build.rs Outdated Show resolved Hide resolved

mustermeiszer added 2 commits June 12, 2024 10:05

fix: ethereum transact inte test

0d90280

Merge branch 'main' into feat/lp-integration-tests

1ef229e

lemunozm reviewed Jun 12, 2024

View reviewed changes

mustermeiszer added 6 commits June 12, 2024 14:47

chore: review suggestion use parachain_state_mut

ce7823e

fix: part 1 build script

aa3e416

chore: mv naming for Luis

12bedc7

chore: make precompile associated type

857d618

fix: wrong imports of Precompiles from runtimes

699a4ca

chore: remove unneeded conversion H160

e4e048a

lemunozm approved these changes Jun 13, 2024

View reviewed changes

runtime/integration-tests/src/generic/cases/lp/mod.rs Show resolved Hide resolved

mustermeiszer enabled auto-merge (squash) June 13, 2024 07:51

gpmayorga approved these changes Jun 13, 2024

View reviewed changes

cdamian approved these changes Jun 13, 2024

View reviewed changes

mustermeiszer merged commit 3b6a8c9 into main Jun 13, 2024
11 of 12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: LP integration tests #1720

Feat: LP integration tests #1720

mustermeiszer commented Feb 6, 2024 •

edited

Loading

lemunozm left a comment •

edited

Loading

lemunozm Feb 12, 2024

wischli Feb 12, 2024

lemunozm Feb 12, 2024 •

edited

Loading

mustermeiszer Feb 13, 2024

hieronx Feb 14, 2024

wischli Apr 9, 2024

cdamian Feb 12, 2024

lemunozm Feb 12, 2024

mustermeiszer Feb 12, 2024

mustermeiszer Mar 1, 2024

wischli Apr 9, 2024

cdamian commented Feb 12, 2024

cdamian Jun 11, 2024

wischli Jun 11, 2024

mustermeiszer Jun 12, 2024

lemunozm Jun 13, 2024

lemunozm Jun 13, 2024

cdamian Jun 11, 2024

mustermeiszer Jun 12, 2024

cdamian Jun 12, 2024

lemunozm Jun 13, 2024

cdamian Jun 11, 2024

mustermeiszer Jun 12, 2024

cdamian Jun 12, 2024

lemunozm Jun 13, 2024

lemunozm left a comment

lemunozm left a comment

lemunozm commented Jun 13, 2024

Feat: LP integration tests #1720

Feat: LP integration tests #1720

Conversation

mustermeiszer commented Feb 6, 2024 • edited Loading

Description

Changes and Descriptions

How to test

Checklist:

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

lemunozm Feb 12, 2024 • 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

cdamian commented Feb 12, 2024

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

lemunozm left a comment

Choose a reason for hiding this comment

lemunozm left a comment

Choose a reason for hiding this comment

lemunozm commented Jun 13, 2024

mustermeiszer commented Feb 6, 2024 •

edited

Loading

lemunozm left a comment •

edited

Loading

lemunozm Feb 12, 2024 •

edited

Loading