Integration tests against Mezo Portal #372

dimpar · 2024-04-18T13:44:06Z

Depends on #350

This configuration is aimed to run MezoAllocator tests against mainnet fork and check the flow with the real Mezo Portal contract deployed on mainnet. This also sets up a ground for other tests that can also be moved and checked against the real contracts on mainnet.

The network is called "integration". A local hardhat node should be run to execute these tests.
E.g.

 npx hardhat node --no-deploy --fork https://eth-mainnet.g.alchemy.com/v2/<key> --fork-block-number 19680873

19680873 the current block number was used as of writing these tests.

Once the local node is running, then the tests can be executed by running:

 pnpm test:integration

This configuration is aimed to run MezoAllocator tests against mainnet fork and check the flow with the real Mezo Portal contract deployed on mainnet. This also sets up a ground for other tests that can also be moved and check against the real contracts on mainnet. The network is called "integration". A local hardhat node should be run to execute these tests.

Changed from "pnpm run deploy" to "pnpm run deployment". The original change was driven by the conflict between hardhat and pnpm "deploy" commands.

nkuba

Please include instructions in README on how to run the tests.

nkuba · 2024-04-23T11:55:30Z

solidity/package.json

@@ -16,7 +16,7 @@
  "scripts": {
    "clean": "hardhat clean && rm -rf cache/ export/ gen/ export.json",
    "build": "hardhat compile",
-    "deploy": "hardhat deploy --export export.json",
+    "deployment": "hardhat deploy --export export.json",


I'd keep deploy as we're used to it across the projects. I know it may overlap with pnpm deploy but we should execute the deployment script with pnpm run deploy as we do in CI.

It's not me not liking the deploy name, it is that it's conflicting with the pnpm deploy build-in command and it's not working as is. See here. If you run pnpm deploy you will get

ERR_PNPM_NOTHING_TO_DEPLOY No project was selected for deployment

Having said that, I figured I change it to deployment (you can suggest some other name) so at least we have a working command. Otherwise it's not useful.

It will work if you execute it with pnpm run deploy. pnpm run <script> is the command we use to run the scripts.

Ah, I see, I didn't know that and assumed all the commands are triggered as pnpm <command> b08c47f

nkuba · 2024-04-23T11:57:05Z

solidity/.gitignore

@@ -5,3 +5,4 @@ export.json
 export/
 gen/
 typechain/
+deployments/


Let's ignore only the specific network artifacts (deployments/integration/), as we don't want to ignore deployments/mainnet/ and deployments/sepolia/ directories.

After I ran the tests locally I noticed there was no output under the deployments/ directory. Maybe we don't need this entry added to .gitignore at all?

You're right, turns out it's not needed 2e88d5d. Must've been a leftover after some changes during implementation. Removed.

nkuba · 2024-04-23T12:35:28Z

solidity/package.json

@@ -26,7 +26,8 @@
    "lint:sol:fix": "solhint 'contracts/**/*.sol' --fix && prettier --write 'contracts/**/*.sol'",
    "lint:config": "prettier --check '**/*.@(json)'",
    "lint:config:fix": "prettier --write '**/*.@(json)'",
-    "test": "hardhat test"
+    "test": "hardhat test ./test/*.test.ts",
+    "test:integration": "INTEGRATION_TEST=true hardhat test ./test/integration/*.test.ts --network integration"


If we add --deploy-fixture flag the external deployment artifacts will be read from the ./external/mainnet/ directory and we won't need to deal with the alternative deployments.getArtifact path in deployment scripts.

Suggested change

"test:integration": "INTEGRATION_TEST=true hardhat test ./test/integration/*.test.ts --network integration"

"test:integration": "hardhat test --deploy-fixture ./test/integration/*.test.ts --network integration"

That's nice, way cleaner! 718a723

nkuba · 2024-04-23T12:36:29Z

solidity/hardhat.config.ts

@@ -29,6 +29,10 @@ const config: HardhatUserConfig = {
    hardhat: {
      tags: ["allowStubs"],
    },
+    integration: {
+      url: "http://localhost:8545",
+      tags: ["allowStubs"],


I believe we could remove this tag.

Suggested change

tags: ["allowStubs"],

nkuba · 2024-04-23T12:37:34Z

solidity/deploy/00_resolve_mezo_portal.ts

+  let mezoPortal = await deployments.getOrNull("MezoPortal")
+  if (hre.network.name === "integration") {
+    mezoPortal = await deployments.getArtifact("MezoPortal")
+  }


If we use --deploy-fixture flag for tests running we could revert the getArtifact change across the deployment scripts.

Suggested change

let mezoPortal = await deployments.getOrNull("MezoPortal")

if (hre.network.name === "integration") {

mezoPortal = await deployments.getArtifact("MezoPortal")

}

const mezoPortal = await deployments.getOrNull("MezoPortal")

nkuba · 2024-04-23T12:39:49Z

solidity/deploy/00_resolve_mezo_portal.ts

-    (hre.network.config as HardhatNetworkConfig)?.forking?.enabled &&
-    hre.network.name !== "hardhat"
-  ) {
+  } else if ((hre.network.config as HardhatNetworkConfig)?.forking?.enabled) {


For integration tests we want the script to fail if the deployment artifact is not found, we need to make sure it doesn't deploy the stub.

Suggested change

} else if ((hre.network.config as HardhatNetworkConfig)?.forking?.enabled) {

} else if (

hre.network.name === "integration" ||

(hre.network.config as HardhatNetworkConfig)?.forking?.enabled

) {

We should also introduce a similar change in other scripts.

nkuba · 2024-04-23T12:41:41Z

solidity/hardhat.config.ts

+    contracts:
+      process.env.INTEGRATION_TEST === "true"
+        ? [
+            {
+              artifacts: "./external/mainnet",
+            },
+          ]
+        : undefined,


We could remove it if we add the --deploy-fixture flag (see comment in package.json).

We can use the '--deploy-fixture' flag to fetch the mainnet artifacts for integration tests purposes. This can replace dealing with the 'integration' flag and using 'getArtifacts' function.

netlify · 2024-04-24T08:46:35Z

‼️ Deploy request for acre-dapp-testnet rejected.

Learn more in the Netlify docs →

Name	Link
🔨 Latest commit	`ce77495`

dimpar · 2024-04-24T08:50:52Z

Please include instructions in README on how to run the tests.

Added 0a2a2d8

nkuba · 2024-04-24T10:26:21Z

solidity/deploy/00_resolve_mezo_portal.ts

+    hre.network.name === "integration" ||
+    (hre.network.config as HardhatNetworkConfig)?.forking?.enabled


Since we're not using hardhat forking network and we want the stubs to be deployed only for unit tests maybe we could modify this check in all deployment scripts and throw the error when hre.network.name !== "hardhat"?

Suggested change

hre.network.name === "integration" ||

(hre.network.config as HardhatNetworkConfig)?.forking?.enabled

hre.network.name !== "hardhat"

This way the stub will be deployed only for hardhat network (which we use for unit tests), and for any other case (integration, mainnet, sepolia) when the actual contract is not found it will throw an error.

We certainly can do that 962d173

nkuba · 2024-04-24T10:27:15Z

solidity/deploy/00_resolve_tbtc_token.ts

+    hre.network.name === "integration" ||
    !hre.network.tags.allowStubs ||


We could remove the allowStubs tag from the network configuration and deployment scripts as it is used only for hardhat network (see other comment).

nkuba · 2024-04-24T10:28:17Z

solidity/deploy/00_resolve_tbtc_vault.ts

-      "setOwner",
-      deployment.address,
-    )
+    if (hre.network.name === "hardhat") {


We're in the else block, the stub is being deployed so maybe we don't need this network check here?

nkuba · 2024-04-24T10:30:13Z

solidity/test/integration/MezoAllocator.test.ts

+    } = await loadFixture(fixture))
+
+    await impersonateAccount(whaleAddress)
+    // eslint-disable-next-line


We could fix the eslint problem if we import ethers from hardhat:

import { helpers, ethers } from "hardhat"

Suggested change

// eslint-disable-next-line

nkuba · 2024-04-24T10:33:01Z

solidity/test/integration/MezoAllocator.test.ts

+
+        it("should increment the deposit id", async () => {
+          const actualDepositId = await mezoAllocator.depositId()
+          // As of writing this test, the deposit id was 2272 before the new


Suggested change

// As of writing this test, the deposit id was 2272 before the new

// As of forked block 19680873, the deposit id was 2272 before the new

solidity/test/integration/MezoAllocator.test.ts

Instead of running 'pnpm deploy', you should run 'pnpm run deploy' to bypass the conflict between a custom hardhat command and a build-in pnpm command.

We use stubs only in hardhat network. For all other networks we'd use actual contracts. This way the deployment scripts can be simplified.

Verified that MezoPortal will throw an InsufficientDepositAmount error when a depositor wants to withdraw more than was initially deposited.

nkuba · 2024-04-25T09:32:11Z

solidity/README.md

+- Run the Hardhat Node locally forking Mainnet at block `19680873`:
+
+```
+$ npx hardhat node --no-deploy --fork https://eth-mainnet.g.alchemy.com/v2/<key> --fork-block-number 19680873


Another idea to simplify setup is to include the node configuration in the hardhat config file and define a script, so to run the node for testing we will only need to run pnpm run test:node-forking.

diff --git a/solidity/hardhat.config.ts b/solidity/hardhat.config.ts index ac6a1492..e1e2fae2 100644 --- a/solidity/hardhat.config.ts +++ b/solidity/hardhat.config.ts @@ -26,6 +26,12 @@ const config: HardhatUserConfig = { }, networks: { + hardhat: { + forking: + process.env.FORKING === "true" + ? { url: process.env.CHAIN_API_URL ?? "", blockNumber: 19680873 } + : undefined, + }, integration: { url: "http://localhost:8545", }, diff --git a/solidity/package.json b/solidity/package.json index 8cb5969e..4e200da4 100644 --- a/solidity/package.json +++ b/solidity/package.json @@ -27,6 +27,7 @@ "lint:config": "prettier --check '**/*.@(json)'", "lint:config:fix": "prettier --write '**/*.@(json)'", "test": "hardhat test ./test/*.test.ts", + "test:node-forking": "FORKING=true hardhat node --no-deploy", "test:integration": "hardhat test --deploy-fixture ./test/integration/*.test.ts --network integration" }, "devDependencies": {

It's not blocking this PR. I'll create a follow-up PR.

Yea, that might work. We'll need to provide a key for CHAIN_API_URL because we probably don't want to make it public. Or even assemble everything into one bash script to run the tests like ./scripts/run-integration-tests.sh

nkuba · 2024-04-25T09:44:32Z

solidity/test/integration/MezoAllocator.test.ts

+          // It is reverted because deposit Id is 0 and there is no deposit
+          // with id 0 in Mezo Portal for Acre. Mezo Portal reverts with the
+          // "unrecognized custom error" that is why we verify only against
+          // a generic revert.
+          await expect(stbtc.withdraw(1n, depositor, depositor)).to.be.reverted


We should be able to verify the exact custom error now similar to the InsufficientDepositAmount check.

Done 90f1e64

…ound

Depends on #372 Adding CI config `solidity-integration-test` to run integration tests against forked mainnet by the local hardhat node.

dimpar requested a review from nkuba April 18, 2024 13:44

dimpar self-assigned this Apr 18, 2024

Fixing deployment command in CI

f7293bd

Changed from "pnpm run deploy" to "pnpm run deployment". The original change was driven by the conflict between hardhat and pnpm "deploy" commands.

dimpar mentioned this pull request Apr 22, 2024

CI integration test #375

Merged

Base automatically changed from mo-tests to main April 23, 2024 11:41

nkuba reviewed Apr 23, 2024

View reviewed changes

dimpar added 3 commits April 24, 2024 10:38

Reverting ignored deployment dir

2e88d5d

Simplifying artifacts fetching for integration tests

718a723

We can use the '--deploy-fixture' flag to fetch the mainnet artifacts for integration tests purposes. This can replace dealing with the 'integration' flag and using 'getArtifacts' function.

Adding README explaining how to run the integration tests

0a2a2d8

Merge remote-tracking branch 'origin' into forking-mainnet

0ced354

dimpar marked this pull request as ready for review April 24, 2024 08:51

nkuba reviewed Apr 24, 2024

View reviewed changes

Reverting 'deployment' command back to 'deploy'

32062d6

Instead of running 'pnpm deploy', you should run 'pnpm run deploy' to bypass the conflict between a custom hardhat command and a build-in pnpm command.

dimpar force-pushed the forking-mainnet branch from b08c47f to 32062d6 Compare April 24, 2024 10:39

dimpar added 6 commits April 24, 2024 14:05

Simplifying the way we check for network names in the deployment scripts

962d173

We use stubs only in hardhat network. For all other networks we'd use actual contracts. This way the deployment scripts can be simplified.

Importing ethers from HH to make eslint happy

bfe755c

Rephrasing the comment for a test

e221554

Merge remote-tracking branch 'origin' into forking-mainnet

ce77495

Making withdrawal tests independent from each other

8c94145

Covering a scenario when withdrawing more than deposited

f68e51f

Verified that MezoPortal will throw an InsufficientDepositAmount error when a depositor wants to withdraw more than was initially deposited.

nkuba reviewed Apr 25, 2024

View reviewed changes

Changing a generic error to verify against a custom error DepositNotF…

90f1e64

…ound

nkuba mentioned this pull request Apr 25, 2024

Improve stBTC tests coverage for MezoAllocator #345

Closed

nkuba approved these changes Apr 25, 2024

View reviewed changes

nkuba merged commit 9beb294 into main Apr 25, 2024
20 checks passed

nkuba deleted the forking-mainnet branch April 25, 2024 10:40

nkuba added a commit that referenced this pull request Apr 26, 2024

CI integration test (#375)

d83ad31

Depends on #372 Adding CI config `solidity-integration-test` to run integration tests against forked mainnet by the local hardhat node.

	"test:integration": "INTEGRATION_TEST=true hardhat test ./test/integration/*.test.ts --network integration"
	"test:integration": "hardhat test --deploy-fixture ./test/integration/*.test.ts --network integration"

		hre.network.name === "integration" \|\|
		(hre.network.config as HardhatNetworkConfig)?.forking?.enabled

	hre.network.name === "integration" \|\|
	(hre.network.config as HardhatNetworkConfig)?.forking?.enabled
	hre.network.name !== "hardhat"

		hre.network.name === "integration" \|\|
		!hre.network.tags.allowStubs \|\|

	// As of writing this test, the deposit id was 2272 before the new
	// As of forked block 19680873, the deposit id was 2272 before the new

Integration tests against Mezo Portal #372

Integration tests against Mezo Portal #372

Conversation

dimpar commented Apr 18, 2024

nkuba 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Apr 24, 2024 • edited Loading

‼️ Deploy request for acre-dapp-testnet rejected.

dimpar commented Apr 24, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Apr 24, 2024 •

edited

Loading