From d287ee0ab7fd17a67643ba2fcf417538f010831e Mon Sep 17 00:00:00 2001 From: Ryan Goulding Date: Tue, 24 Oct 2023 11:28:22 -0700 Subject: [PATCH] Fix PingPong Unit Test Resolves #119. This change includes the following: * Adds a _totalPings argument to the ping() function. This is needed to limit the Unit Test to one ping/pong. This was done to avoid re-entrancy limitations of the LZEndpointMock, which can only call send one time per chain endpoint. It is also nice to be able to limit the number of pings sent. * Adjusts unit tests to correspond with the changed behavior. * Adds documentation to `PingPong.sol`. Signed-off-by: Ryan Goulding --- README.md | 2 +- contracts/examples/PingPong.sol | 57 +++-- contracts/lzApp/NonblockingLzApp.sol | 1 - hardhat.config.js | 319 +++++++++++++-------------- tasks/index.js | 7 +- tasks/ping.js | 15 +- test/examples/PingPong.test.js | 102 +++++---- 7 files changed, 270 insertions(+), 233 deletions(-) diff --git a/README.md b/README.md index 654f8fd1..0acba8f1 100644 --- a/README.md +++ b/README.md @@ -11,7 +11,7 @@ ### Install & Run tests ```shell yarn install -npx hardhat test +yarn test ``` * The code in the `/contracts` folder demonstrates LayerZero behaviours. diff --git a/contracts/examples/PingPong.sol b/contracts/examples/PingPong.sol index e703af69..a50df307 100644 --- a/contracts/examples/PingPong.sol +++ b/contracts/examples/PingPong.sol @@ -1,10 +1,10 @@ // SPDX-License-Identifier: MIT // -// Note: you will need to fund each deployed contract with gas +// Note: You will need to fund each deployed contract with gas. // // PingPong sends a LayerZero message back and forth between chains -// until it is paused or runs out of gas! +// a predetermined number of times (or until it runs out of gas). // // Demonstrates: // 1. a recursive feature of calling send() from inside lzReceive() @@ -16,57 +16,74 @@ pragma abicoder v2; import "../lzApp/NonblockingLzApp.sol"; +/// @title PingPong +/// @notice Sends a LayerZero message back and forth between chains a predetermined number of times. contract PingPong is NonblockingLzApp { - // event emitted every ping() to keep track of consecutive pings count - event Ping(uint pings); - // constructor requires the LayerZero endpoint for this chain + /// @dev event emitted every ping() to keep track of consecutive pings count + event Ping(uint256 pingCount); + + /// @param _endpoint The LayerZero endpoint address. constructor(address _endpoint) NonblockingLzApp(_endpoint) {} - // pings the destination chain, along with the current number of pings sent + /// @notice Pings the destination chain, along with the current number of pings sent. + /// @param _dstChainId The destination chain ID. + /// @param _totalPings The total number of pings to send. function ping( - uint16 _dstChainId + uint16 _dstChainId, + uint256 _totalPings ) public { - _ping(_dstChainId, 0); + _ping(_dstChainId, 0, _totalPings); } - // pings the destination chain, along with the current number of pings sent + /// @dev Internal function to ping the destination chain, along with the current number of pings sent. + /// @param _dstChainId The destination chain ID. + /// @param _pings The current ping count. + /// @param _totalPings The total number of pings to send. function _ping( uint16 _dstChainId, - uint _ping + uint256 _pings, + uint256 _totalPings ) internal { require(address(this).balance > 0, "This contract ran out of money."); // encode the payload with the number of pings - bytes memory payload = abi.encode(_ping); + bytes memory payload = abi.encode(_pings, _totalPings); + // encode the adapter parameters uint16 version = 1; uint256 gasForDestinationLzReceive = 350000; bytes memory adapterParams = abi.encodePacked(version, gasForDestinationLzReceive); // send LayerZero message - _lzSend( // {value: messageFee} will be paid out of this contract! - _dstChainId, // destination chainId - payload, // abi.encode()'ed bytes - payable(this), // (msg.sender will be this contract) refund address (LayerZero will refund any extra gas back to caller of send() - address(0x0), // future param, unused for this example + _lzSend( // {value: messageFee} will be paid out of this contract! + _dstChainId, // destination chainId + payload, // abi.encode()'ed bytes + payable(this), // (msg.sender will be this contract) refund address (LayerZero will refund any extra gas back to caller of send()) + address(0x0), // future param, unused for this example adapterParams, // v1 adapterParams, specify custom destination gas qty address(this).balance ); } + /// @dev Internal function to handle incoming Ping messages. + /// @param _srcChainId The source chain ID from which the message originated. + /// @param _payload The payload of the incoming message. function _nonblockingLzReceive( uint16 _srcChainId, - bytes memory _srcAddress, + bytes memory, /*_srcAddress*/ uint64, /*_nonce*/ bytes memory _payload ) internal override { // decode the number of pings sent thus far - uint pings = abi.decode(_payload, (uint)) + 1; - emit Ping(pings); + (uint256 pingCount, uint256 totalPings) = abi.decode(_payload, (uint256, uint256)); + ++pingCount; + emit Ping(pingCount); // *pong* back to the other side - _ping(_srcChainId,pings); + if (pingCount < totalPings) { + _ping(_srcChainId, pingCount, totalPings); + } } // allow this contract to receive ether diff --git a/contracts/lzApp/NonblockingLzApp.sol b/contracts/lzApp/NonblockingLzApp.sol index 4b20ea73..95f74b9e 100644 --- a/contracts/lzApp/NonblockingLzApp.sol +++ b/contracts/lzApp/NonblockingLzApp.sol @@ -32,7 +32,6 @@ abstract contract NonblockingLzApp is LzApp { 150, abi.encodeWithSelector(this.nonblockingLzReceive.selector, _srcChainId, _srcAddress, _nonce, _payload) ); - // try-catch all errors/exceptions if (!success) { _storeFailedMessage(_srcChainId, _srcAddress, _nonce, _payload, reason); } diff --git a/hardhat.config.js b/hardhat.config.js index 21df4ab1..a05348e5 100644 --- a/hardhat.config.js +++ b/hardhat.config.js @@ -1,43 +1,45 @@ -require("dotenv").config(); - -require('hardhat-contract-sizer'); -require("@nomiclabs/hardhat-waffle"); -require(`@nomiclabs/hardhat-etherscan`); -require("solidity-coverage"); -require('hardhat-gas-reporter'); -require('hardhat-deploy'); -require('hardhat-deploy-ethers'); -require('@openzeppelin/hardhat-upgrades'); -require('./tasks'); +require("dotenv").config() + +// uncomment to include contract sizing in test output +// require("hardhat-contract-sizer") +require("@nomiclabs/hardhat-waffle") +require(`@nomiclabs/hardhat-etherscan`) +require("solidity-coverage") +// uncomment to include gas reporting in test output +//require('hardhat-gas-reporter') +require("hardhat-deploy") +require("hardhat-deploy-ethers") +require("@openzeppelin/hardhat-upgrades") +require("./tasks") // This is a sample Hardhat task. To learn how to create your own go to // https://hardhat.org/guides/create-task.html task("accounts", "Prints the list of accounts", async (taskArgs, hre) => { - const accounts = await hre.ethers.getSigners(); + const accounts = await hre.ethers.getSigners() - for (const account of accounts) { - console.log(account.address); - } -}); + for (const account of accounts) { + console.log(account.address) + } +}) function getMnemonic(networkName) { - if (networkName) { - const mnemonic = process.env['MNEMONIC_' + networkName.toUpperCase()] - if (mnemonic && mnemonic !== '') { - return mnemonic + if (networkName) { + const mnemonic = process.env["MNEMONIC_" + networkName.toUpperCase()] + if (mnemonic && mnemonic !== "") { + return mnemonic + } } - } - const mnemonic = process.env.MNEMONIC - if (!mnemonic || mnemonic === '') { - return 'test test test test test test test test test test test junk' - } + const mnemonic = process.env.MNEMONIC + if (!mnemonic || mnemonic === "") { + return "test test test test test test test test test test test junk" + } - return mnemonic + return mnemonic } function accounts(chainKey) { - return { mnemonic: getMnemonic(chainKey) } + return { mnemonic: getMnemonic(chainKey) } } // You need to export an object to set up your config @@ -47,137 +49,134 @@ function accounts(chainKey) { * @type import('hardhat/config').HardhatUserConfig */ module.exports = { - - solidity: { - compilers: [ - { - version: "0.8.4", - settings: { - optimizer: { - enabled: true, - runs: 200 - } - } - }, - { - version: "0.7.6", - settings: { - optimizer: { - enabled: true, - runs: 200 - } - } - }, - { - version: "0.8.12", - settings: { - optimizer: { - enabled: true, - runs: 200 - } - } - } - ] - - - }, - - // solidity: "0.8.4", - contractSizer: { - alphaSort: false, - runOnCompile: true, - disambiguatePaths: false, - }, - - namedAccounts: { - deployer: { - default: 0, // wallet address 0, of the mnemonic in .env - }, - proxyOwner: { - default: 1, - }, - }, - - mocha: { - timeout: 100000000 - }, - - networks: { - ethereum: { - url: "https://mainnet.infura.io/v3/9aa3d95b3bc440fa88ea12eaa4456161", // public infura endpoint - chainId: 1, - accounts: accounts(), + solidity: { + compilers: [ + { + version: "0.8.4", + settings: { + optimizer: { + enabled: true, + runs: 200, + }, + }, + }, + { + version: "0.7.6", + settings: { + optimizer: { + enabled: true, + runs: 200, + }, + }, + }, + { + version: "0.8.12", + settings: { + optimizer: { + enabled: true, + runs: 200, + }, + }, + }, + ], + }, + + // solidity: "0.8.4", + contractSizer: { + alphaSort: false, + runOnCompile: true, + disambiguatePaths: false, + }, + + namedAccounts: { + deployer: { + default: 0, // wallet address 0, of the mnemonic in .env + }, + proxyOwner: { + default: 1, + }, + }, + + mocha: { + timeout: 100000000, + }, + + networks: { + ethereum: { + url: "https://mainnet.infura.io/v3/9aa3d95b3bc440fa88ea12eaa4456161", // public infura endpoint + chainId: 1, + accounts: accounts(), + }, + bsc: { + url: "https://bsc-dataseed1.binance.org", + chainId: 56, + accounts: accounts(), + }, + avalanche: { + url: "https://api.avax.network/ext/bc/C/rpc", + chainId: 43114, + accounts: accounts(), + }, + polygon: { + url: "https://rpc-mainnet.maticvigil.com", + chainId: 137, + accounts: accounts(), + }, + arbitrum: { + url: `https://arb1.arbitrum.io/rpc`, + chainId: 42161, + accounts: accounts(), + }, + optimism: { + url: `https://mainnet.optimism.io`, + chainId: 10, + accounts: accounts(), + }, + fantom: { + url: `https://rpcapi.fantom.network`, + chainId: 250, + accounts: accounts(), + }, + metis: { + url: `https://andromeda.metis.io/?owner=1088`, + chainId: 1088, + accounts: accounts(), + }, + + goerli: { + url: "https://goerli.infura.io/v3/9aa3d95b3bc440fa88ea12eaa4456161", // public infura endpoint + chainId: 5, + accounts: accounts(), + }, + "bsc-testnet": { + url: "https://data-seed-prebsc-1-s1.binance.org:8545/", + chainId: 97, + accounts: accounts(), + }, + fuji: { + url: `https://api.avax-test.network/ext/bc/C/rpc`, + chainId: 43113, + accounts: accounts(), + }, + mumbai: { + url: "https://rpc-mumbai.maticvigil.com/", + chainId: 80001, + accounts: accounts(), + }, + "arbitrum-goerli": { + url: `https://goerli-rollup.arbitrum.io/rpc/`, + chainId: 421613, + accounts: accounts(), + }, + "optimism-goerli": { + url: `https://goerli.optimism.io/`, + chainId: 420, + accounts: accounts(), + }, + "fantom-testnet": { + url: `https://rpc.ankr.com/fantom_testnet`, + chainId: 4002, + accounts: accounts(), + }, }, - bsc: { - url: "https://bsc-dataseed1.binance.org", - chainId: 56, - accounts: accounts(), - }, - avalanche: { - url: "https://api.avax.network/ext/bc/C/rpc", - chainId: 43114, - accounts: accounts(), - }, - polygon: { - url: "https://rpc-mainnet.maticvigil.com", - chainId: 137, - accounts: accounts(), - }, - arbitrum: { - url: `https://arb1.arbitrum.io/rpc`, - chainId: 42161, - accounts: accounts(), - }, - optimism: { - url: `https://mainnet.optimism.io`, - chainId: 10, - accounts: accounts(), - }, - fantom: { - url: `https://rpcapi.fantom.network`, - chainId: 250, - accounts: accounts(), - }, - metis: { - url: `https://andromeda.metis.io/?owner=1088`, - chainId: 1088, - accounts: accounts(), - }, - - goerli: { - url: "https://goerli.infura.io/v3/9aa3d95b3bc440fa88ea12eaa4456161", // public infura endpoint - chainId: 5, - accounts: accounts(), - }, - 'bsc-testnet': { - url: 'https://data-seed-prebsc-1-s1.binance.org:8545/', - chainId: 97, - accounts: accounts(), - }, - fuji: { - url: `https://api.avax-test.network/ext/bc/C/rpc`, - chainId: 43113, - accounts: accounts(), - }, - mumbai: { - url: "https://rpc-mumbai.maticvigil.com/", - chainId: 80001, - accounts: accounts(), - }, - 'arbitrum-goerli': { - url: `https://goerli-rollup.arbitrum.io/rpc/`, - chainId: 421613, - accounts: accounts(), - }, - 'optimism-goerli': { - url: `https://goerli.optimism.io/`, - chainId: 420, - accounts: accounts(), - }, - 'fantom-testnet': { - url: `https://rpc.ankr.com/fantom_testnet`, - chainId: 4002, - accounts: accounts(), - } - } -}; +} diff --git a/tasks/index.js b/tasks/index.js index 78e5c9e4..a8be2840 100644 --- a/tasks/index.js +++ b/tasks/index.js @@ -42,10 +42,9 @@ task("pingPongSetTrustedRemote", "set the trusted remote", require("./pingPongSe "the targetNetwork to set as trusted" ) -task("ping", "call ping to start the pingPong with the target network", require("./ping")).addParam( - "targetNetwork", - "the targetNetwork to commence pingponging with" -) +task("ping", "call ping to start the pingPong with the target network", require("./ping")) + .addParam("targetNetwork", "the targetNetwork to commence pingponging with") + .addOptionalParam("n", "number of pings to send", 2, types.int) task("getSigners", "show the signers of the current mnemonic", require("./getSigners")).addOptionalParam("n", "how many to show", 3, types.int) diff --git a/tasks/ping.js b/tasks/ping.js index cc50c548..af08c80c 100644 --- a/tasks/ping.js +++ b/tasks/ping.js @@ -2,17 +2,18 @@ const CHAIN_ID = require("../constants/chainIds.json") const { getDeploymentAddresses } = require("../utils/readStatic") module.exports = async function (taskArgs, hre) { - const dstChainId = CHAIN_ID[taskArgs.targetNetwork] - const dstPingPongAddr = getDeploymentAddresses(taskArgs.targetNetwork)["PingPong"] + const targetNetwork = taskArgs.targetNetwork + const totalPings = taskArgs.n + + const dstChainId = CHAIN_ID[targetNetwork] + const dstPingPongAddr = getDeploymentAddresses(targetNetwork)["PingPong"] + // get local contract instance const pingPong = await ethers.getContract("PingPong") console.log(`[source] pingPong.address: ${pingPong.address}`) - let tx = await ( - await pingPong.ping( - dstChainId, - ) - ).wait() + let tx = await (await pingPong.ping(dstChainId, totalPings)).wait() + console.log(`✅ Pings started! [${hre.network.name}] pinging with target chain [${dstChainId}] @ [${dstPingPongAddr}]`) console.log(`...tx: ${tx.transactionHash}`) } diff --git a/test/examples/PingPong.test.js b/test/examples/PingPong.test.js index 2857411d..1b72a42d 100644 --- a/test/examples/PingPong.test.js +++ b/test/examples/PingPong.test.js @@ -1,49 +1,71 @@ const { expect } = require("chai") const { ethers } = require("hardhat") -describe("PingPong", function () { - beforeEach(async function () { - this.accounts = await ethers.getSigners() - this.owner = this.accounts[0] - - // use this chainId - this.chainIdSrc = 1 - this.chainIdDst = 2 - - // create a LayerZero Endpoint mock for testing - const LZEndpointMock = await ethers.getContractFactory("LZEndpointMock") - this.layerZeroEndpointMockSrc = await LZEndpointMock.deploy(this.chainIdSrc) - this.layerZeroEndpointMockDst = await LZEndpointMock.deploy(this.chainIdDst) - - // create two PingPong instances - const PingPong = await ethers.getContractFactory("PingPong") - this.pingPongA = await PingPong.deploy(this.layerZeroEndpointMockSrc.address) - this.pingPongB = await PingPong.deploy(this.layerZeroEndpointMockDst.address) - - this.layerZeroEndpointMockSrc.setDestLzEndpoint(this.pingPongB.address, this.layerZeroEndpointMockDst.address) - this.layerZeroEndpointMockDst.setDestLzEndpoint(this.pingPongA.address, this.layerZeroEndpointMockSrc.address) - - // set each contracts source address so it can send to each other - await this.pingPongA.setTrustedRemote( - this.chainIdDst, - ethers.utils.solidityPack(["address", "address"], [this.pingPongB.address, this.pingPongA.address]) - ) // for A, set B - await this.pingPongB.setTrustedRemote( - this.chainIdSrc, - ethers.utils.solidityPack(["address", "address"], [this.pingPongA.address, this.pingPongB.address]) - ) // for B, set A - - await this.pingPongA.enable(true) - await this.pingPongB.enable(true) +// fund "to" address by "value" from "signer" +const fund = async (signer, to, value) => { + ;( + await signer.sendTransaction({ + to, + value, + }) + ).wait() +} + +describe("PingPong", async function () { + const chainIdA = 1 + const chainIdB = 2 + // amount to fund each PingPong instance + const pingPongBalance = ethers.utils.parseEther(".1") + const gasForDstLzReceive = 350000 + + let LZEndpointMock, layerZeroEndpointMockA, layerZeroEndpointMockB + let PingPong, pingPongA, pingPongB + let owner + + before(async function () { + LZEndpointMock = await ethers.getContractFactory("LZEndpointMock") + PingPong = await ethers.getContractFactory("PingPong") + owner = (await ethers.getSigners())[0] }) - it("increment the counter of the destination PingPong when paused should revert", async function () { - await expect(this.pingPongA.ping(this.chainIdDst, this.pingPongB.address, 0)).to.revertedWith("Pausable: paused") + beforeEach(async function () { + layerZeroEndpointMockA = await LZEndpointMock.deploy(chainIdA) + layerZeroEndpointMockB = await LZEndpointMock.deploy(chainIdB) + + // create two PingPong contract instances and provide native token balance + pingPongA = await PingPong.deploy(layerZeroEndpointMockA.address) + await fund(owner, pingPongA.address, pingPongBalance) + pingPongB = await PingPong.deploy(layerZeroEndpointMockB.address) + await fund(owner, pingPongB.address, pingPongBalance) + + await layerZeroEndpointMockA.setDestLzEndpoint(pingPongB.address, layerZeroEndpointMockB.address) + await layerZeroEndpointMockB.setDestLzEndpoint(pingPongA.address, layerZeroEndpointMockA.address) + + // enable bidirectional communication between pingPongA and pingPongB + await pingPongA.setTrustedRemote(chainIdB, ethers.utils.solidityPack(["address", "address"], [pingPongB.address, pingPongA.address])) // for A, set B + await pingPongB.setTrustedRemote(chainIdA, ethers.utils.solidityPack(["address", "address"], [pingPongA.address, pingPongB.address])) // for B, set A }) - it("increment the counter of the destination PingPong when unpaused show not revert", async function () { - await this.pingPongA.enable(false) - await this.pingPongB.enable(false) - await this.pingPongA.ping(this.chainIdDst, this.pingPongB.address, 0, { value: ethers.utils.parseEther("0.5") }) + it("ping back and forth once between PingPong contract instances", async function () { + const startBalanceA = await ethers.provider.getBalance(pingPongA.address) + const startBalanceB = await ethers.provider.getBalance(pingPongB.address) + + // Send one ping from A->B, then one pong back from B->A. Validate B emits a ping with count=1. + await expect(pingPongA.ping(chainIdB, 2)).to.emit(pingPongB, "Ping").withArgs(1) + + // Ensure pingPongA has emitted exactly one Ping with count=2 and no MessageFailed events. + const aPings = await pingPongA.queryFilter(pingPongA.filters.Ping(), 0, "latest") + expect(aPings.length).to.equal(1) + // waffle 3 is incapable of expect'ing multiple emits. + expect(pingPongA.interface.decodeEventLog("Ping", aPings[0].data).pingCount).to.equal(2) + expect((await pingPongA.queryFilter(pingPongA.filters.MessageFailed(), 0, "latest")).length).to.equal(0) + + // Ensure pingPongB has emitted one Ping and no MessageFailed events. + expect((await pingPongB.queryFilter(pingPongB.filters.Ping(), 0, "latest")).length).to.equal(1) + expect((await pingPongB.queryFilter(pingPongB.filters.MessageFailed(), 0, "latest")).length).to.equal(0) + + // Ensure PingPong contract balances have decreased. + expect(await ethers.provider.getBalance(pingPongA.address)).to.be.lt(startBalanceA.sub(gasForDstLzReceive)) + expect(await ethers.provider.getBalance(pingPongB.address)).to.be.lt(startBalanceB.sub(gasForDstLzReceive)) }) })