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.

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
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
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
1 change: 1 addition & 0 deletions solidity/.gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ export.json
export/
gen/
typechain/
deployments/
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

10 changes: 5 additions & 5 deletions solidity/deploy/00_resolve_mezo_portal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,14 @@ const func: DeployFunction = async (hre: HardhatRuntimeEnvironment) => {
const { log } = deployments
const { deployer } = await getNamedAccounts()

const mezoPortal = await deployments.getOrNull("MezoPortal")
let mezoPortal = await deployments.getOrNull("MezoPortal")
if (hre.network.name === "integration") {
mezoPortal = await deployments.getArtifact("MezoPortal")
}
Copy link
Member

Choose a reason for hiding this comment

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

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")

Copy link
Member Author

Choose a reason for hiding this comment

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


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"
) {
} else if ((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.

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

throw new Error("deployed MezoPortal contract not found")
} else {
log("deploying Mezo Portal contract stub")
Expand Down
5 changes: 4 additions & 1 deletion solidity/deploy/00_resolve_tbtc_token.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@ const func: DeployFunction = async (hre: HardhatRuntimeEnvironment) => {
const { log } = deployments
const { deployer } = await getNamedAccounts()

const tbtc = await deployments.getOrNull("TBTC")
let tbtc = await deployments.getOrNull("TBTC")
if (hre.network.name === "integration") {
tbtc = await deployments.getArtifact("TBTC")
}

if (tbtc && isNonZeroAddress(tbtc.address)) {
log(`using TBTC contract at ${tbtc.address}`)
Expand Down
20 changes: 13 additions & 7 deletions solidity/deploy/00_resolve_tbtc_vault.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,11 @@ const func: DeployFunction = async (hre: HardhatRuntimeEnvironment) => {
} else {
log("deploying TBTCVault contract stub")

const tbtc = await deployments.get("TBTC")
let tbtc = await deployments.getOrNull("TBTC")
if (hre.network.name === "integration") {
tbtc = await deployments.getArtifact("TBTC")
}

const bridge = await deployments.get("Bridge")

const deployment = await deployments.deploy("TBTCVault", {
Expand All @@ -34,12 +38,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.

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

Expand Down
5 changes: 4 additions & 1 deletion solidity/deploy/01_deploy_stbtc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@ const func: DeployFunction = async (hre: HardhatRuntimeEnvironment) => {
const { treasury, governance } = await getNamedAccounts()
const { deployer: deployerSigner } = await helpers.signers.getNamedSigners()

const tbtc = await deployments.get("TBTC")
let tbtc = await deployments.getOrNull("TBTC")
if (hre.network.name === "integration") {
tbtc = await deployments.getArtifact("TBTC")
}

const [, stbtcDeployment] = await helpers.upgrades.deployProxy("stBTC", {
contractName: "stBTC",
Expand Down
8 changes: 6 additions & 2 deletions solidity/deploy/02_deploy_mezo_allocator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,13 @@ 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 mezoPortal = await deployments.get("MezoPortal")
let tbtc = await deployments.getOrNull("TBTC")
let mezoPortal = await deployments.getOrNull("MezoPortal")
if (hre.network.name === "integration") {
mezoPortal = await deployments.getArtifact("MezoPortal")
tbtc = await deployments.getArtifact("TBTC")
}

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

let tbtc = await deployments.getOrNull("TBTC")
if (hre.network.name === "integration") {
tbtc = await deployments.getArtifact("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
5 changes: 4 additions & 1 deletion solidity/deploy/04_deploy_bitcoin_redeemer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@ const func: DeployFunction = async (hre: HardhatRuntimeEnvironment) => {
const { deployments, helpers } = hre
const { deployer } = await helpers.signers.getNamedSigners()

const tbtc = await deployments.get("TBTC")
let tbtc = await deployments.getOrNull("TBTC")
if (hre.network.name === "integration") {
tbtc = await deployments.getArtifact("TBTC")
}
const stbtc = await deployments.get("stBTC")
const tbtcVault = await deployments.get("TBTCVault")

Expand Down
13 changes: 13 additions & 0 deletions solidity/hardhat.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ const config: HardhatUserConfig = {
hardhat: {
tags: ["allowStubs"],
},
integration: {
url: "http://localhost:8545",
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.

I believe we could remove this tag.

Suggested change
tags: ["allowStubs"],

Copy link
Member Author

Choose a reason for hiding this comment

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

},
sepolia: {
url: process.env.CHAIN_API_URL || "",
chainId: 11155111,
Expand All @@ -48,9 +52,18 @@ const config: HardhatUserConfig = {
},

external: {
contracts:
process.env.INTEGRATION_TEST === "true"
? [
{
artifacts: "./external/mainnet",
},
]
: undefined,
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 it if we add the --deploy-fixture flag (see comment in package.json).

Copy link
Member Author

Choose a reason for hiding this comment

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

deployments: {
sepolia: ["./external/sepolia"],
mainnet: ["./external/mainnet"],
integration: ["./external/mainnet"],
},
},

Expand Down
5 changes: 3 additions & 2 deletions solidity/package.json
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": "INTEGRATION_TEST=true hardhat test ./test/integration/*.test.ts --network integration"
Copy link
Member

Choose a reason for hiding this comment

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

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"

Copy link
Member Author

Choose a reason for hiding this comment

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

That's nice, way cleaner! 718a723

},
"devDependencies": {
"@keep-network/hardhat-helpers": "^0.7.1",
Expand Down
10 changes: 7 additions & 3 deletions solidity/test/helpers/context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,15 @@ import type {
BridgeStub,
TBTCVaultStub,
MezoAllocator,
MezoPortalStub,
IMezoPortal,
BitcoinDepositor,
BitcoinRedeemer,
TestTBTC,
} from "../../typechain"

// eslint-disable-next-line import/prefer-default-export
export async function deployment() {
const isIntegration = deployments.getNetworkName() === "integration"
await deployments.fixture()

const stbtc: stBTC = await getDeployedContract("stBTC")
Expand All @@ -22,13 +23,16 @@ export async function deployment() {
const bitcoinRedeemer: BitcoinRedeemer =
await getDeployedContract("BitcoinRedeemer")

const tbtc: TestTBTC = await getDeployedContract("TBTC")
const tbtc: TestTBTC = await getDeployedContract("TBTC", isIntegration)
const tbtcBridge: BridgeStub = await getDeployedContract("Bridge")
const tbtcVault: TBTCVaultStub = await getDeployedContract("TBTCVault")

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

return {
tbtc,
Expand Down
10 changes: 8 additions & 2 deletions solidity/test/helpers/contract.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,15 @@ const { getUnnamedSigners } = helpers.signers
// eslint-disable-next-line import/prefer-default-export
export async function getDeployedContract<T extends BaseContract>(
deploymentName: string,
isIntegrationTest: boolean = false,
): Promise<T> {
const { address, abi } = await deployments.get(deploymentName)

let address: string
let abi: string
if (isIntegrationTest) {
;({ address, abi } = await deployments.getArtifact(deploymentName))
} else {
;({ 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