Skip to content
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

Integration tests against Mezo Portal #372

Merged
merged 14 commits into from
Apr 25, 2024
+399 −37
Merged

Integration tests against Mezo Portal #372

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
e07a9e3
Adding MezoAllocator tests to run against mainnet fork
dimpar Apr 18, 2024
f7293bd
Fixing deployment command in CI
dimpar Apr 22, 2024
2e88d5d
Reverting ignored deployment dir
dimpar Apr 24, 2024
718a723
Simplifying artifacts fetching for integration tests
dimpar Apr 24, 2024
0a2a2d8
Adding README explaining how to run the integration tests
dimpar Apr 24, 2024
0ced354
Merge remote-tracking branch 'origin' into forking-mainnet
dimpar Apr 24, 2024
32062d6
Reverting 'deployment' command back to 'deploy'
dimpar Apr 24, 2024
962d173
Simplifying the way we check for network names in the deployment scripts
dimpar Apr 24, 2024
bfe755c
Importing ethers from HH to make eslint happy
dimpar Apr 24, 2024
e221554
Rephrasing the comment for a test
dimpar Apr 24, 2024
ce77495
Merge remote-tracking branch 'origin' into forking-mainnet
dimpar Apr 24, 2024
8c94145
Making withdrawal tests independent from each other
dimpar Apr 24, 2024
f68e51f
Covering a scenario when withdrawing more than deposited
dimpar Apr 25, 2024
90f1e64
Changing a generic error to verify against a custom error DepositNotF…
dimpar Apr 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/solidity.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ jobs:
path: solidity/

- name: Deploy
run: pnpm run deploy --no-compile
run: pnpm run deployment --no-compile

solidity-deploy-testnet:
needs: [solidity-deploy-dry-run]
Expand Down
16 changes: 16 additions & 0 deletions solidity/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,22 @@ To run the test execute:
$ pnpm test
```

### Integration testing

To run the integration tests follow these steps:

- 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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": {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#380

```

- Once the local node is running you can execute the integration tests:

```
$ pnpm test:integration
```

### Deploying

We deploy our contracts with
Expand Down
4 changes: 2 additions & 2 deletions solidity/deploy/00_resolve_mezo_portal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ const func: DeployFunction = async (hre: HardhatRuntimeEnvironment) => {
if (mezoPortal && isNonZeroAddress(mezoPortal.address)) {
log(`using MezoPortal contract at ${mezoPortal.address}`)
} else if (
(hre.network.config as HardhatNetworkConfig)?.forking?.enabled &&
hre.network.name !== "hardhat"
hre.network.name === "integration" ||
(hre.network.config as HardhatNetworkConfig)?.forking?.enabled
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We certainly can do that 962d173

) {
throw new Error("deployed MezoPortal contract not found")
} else {
Expand Down
1 change: 1 addition & 0 deletions solidity/deploy/00_resolve_tbtc_token.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const func: DeployFunction = async (hre: HardhatRuntimeEnvironment) => {
if (tbtc && isNonZeroAddress(tbtc.address)) {
log(`using TBTC contract at ${tbtc.address}`)
} else if (
hre.network.name === "integration" ||
!hre.network.tags.allowStubs ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

962d173

(hre.network.config as HardhatNetworkConfig)?.forking?.enabled
) {
Expand Down
14 changes: 8 additions & 6 deletions solidity/deploy/00_resolve_tbtc_vault.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,14 @@ const func: DeployFunction = async (hre: HardhatRuntimeEnvironment) => {
waitConfirmations: waitConfirmationsNumber(hre),
})

await deployments.execute(
"TBTC",
{ from: deployer, log: true },
"setOwner",
deployment.address,
)
if (hre.network.name === "hardhat") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

962d173

await deployments.execute(
"TBTC",
{ from: deployer, log: true },
"setOwner",
deployment.address,
)
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion solidity/deploy/02_deploy_mezo_allocator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ const func: DeployFunction = async (hre: HardhatRuntimeEnvironment) => {
const { governance } = await getNamedAccounts()
const { deployer } = await helpers.signers.getNamedSigners()

const tbtc = await deployments.get("TBTC")
const stbtc = await deployments.get("stBTC")
const tbtc = await deployments.get("TBTC")
const mezoPortal = await deployments.get("MezoPortal")

const [, deployment] = await helpers.upgrades.deployProxy("MezoAllocator", {
Expand Down
2 changes: 1 addition & 1 deletion solidity/deploy/03_deploy_bitcoin_depositor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ const func: DeployFunction = async (hre: HardhatRuntimeEnvironment) => {
const { governance } = await getNamedAccounts()
const { deployer } = await helpers.signers.getNamedSigners()

const tbtc = await deployments.get("TBTC")
const bridge = await deployments.get("Bridge")
const tbtcVault = await deployments.get("TBTCVault")
const tbtc = await deployments.get("TBTC")
const stbtc = await deployments.get("stBTC")

const [, deployment] = await helpers.upgrades.deployProxy(
Expand Down
4 changes: 4 additions & 0 deletions solidity/hardhat.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ const config: HardhatUserConfig = {
hardhat: {
tags: ["allowStubs"],
},
integration: {
url: "http://localhost:8545",
},
sepolia: {
url: process.env.CHAIN_API_URL || "",
chainId: 11155111,
Expand All @@ -51,6 +54,7 @@ const config: HardhatUserConfig = {
deployments: {
sepolia: ["./external/sepolia"],
mainnet: ["./external/mainnet"],
integration: ["./external/mainnet"],
},
},

Expand Down
5 changes: 3 additions & 2 deletions solidity/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"docs": "hardhat docgen",
"format": "npm run lint:js && npm run lint:sol && npm run lint:config",
"format:fix": "npm run lint:js:fix && npm run lint:sol:fix && npm run lint:config:fix",
Expand All @@ -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": "hardhat test --deploy-fixture ./test/integration/*.test.ts --network integration"
},
"devDependencies": {
"@keep-network/hardhat-helpers": "^0.7.1",
Expand Down
4 changes: 2 additions & 2 deletions solidity/test/helpers/context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import type {
BridgeStub,
TBTCVaultStub,
MezoAllocator,
MezoPortalStub,
IMezoPortal,
BitcoinDepositor,
BitcoinRedeemer,
TestTBTC,
Expand All @@ -28,7 +28,7 @@ export async function deployment() {

const mezoAllocator: MezoAllocator =
await getDeployedContract("MezoAllocator")
const mezoPortal: MezoPortalStub = await getDeployedContract("MezoPortal")
const mezoPortal: IMezoPortal = await getDeployedContract("MezoPortal")

return {
tbtc,
Expand Down
1 change: 0 additions & 1 deletion solidity/test/helpers/contract.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ export async function getDeployedContract<T extends BaseContract>(
deploymentName: string,
): Promise<T> {
const { address, abi } = await deployments.get(deploymentName)

// Use default unnamed signer from index 0 to initialize the contract runner.
const [defaultSigner] = await getUnnamedSigners()

Expand Down
Loading
Loading