Merge pull request #207 from lidofinance/feat/review-fixes

.env.example

@@ -1,4 +1,6 @@
+# RPC URL for a locally running node (Ganache, Anvil, Hardhat Network, etc.), used for scratch deployment and tests
 LOCAL_RPC_URL=http://localhost:8555
+
 LOCAL_LOCATOR_ADDRESS=
 LOCAL_AGENT_ADDRESS=
 LOCAL_VOTING_ADDRESS=
@@ -21,8 +23,13 @@ LOCAL_VALIDATORS_EXIT_BUS_ORACLE_ADDRESS=
 LOCAL_WITHDRAWAL_QUEUE_ADDRESS=
 LOCAL_WITHDRAWAL_VAULT_ADDRESS=
 
-# https://docs.lido.fi/deployed-contracts
+# RPC URL for a separate, non Hardhat Network node (Anvil, Infura, Alchemy, etc.)
 MAINNET_RPC_URL=http://localhost:8545
+# RPC URL for Hardhat Network forking, required for running tests on mainnet fork with tracing (Infura, Alchemy, etc.)
+# https://hardhat.org/hardhat-network/docs/guides/forking-other-networks#forking-other-networks
+MAINNET_FORKING_URL=
+
+# https://docs.lido.fi/deployed-contracts
 MAINNET_LOCATOR_ADDRESS=0xC1d0b3DE6792Bf6b4b37EccdcC24e45978Cfd2Eb
 MAINNET_AGENT_ADDRESS=0x3e40D73EB977Dc6a537aF587D48316feE66E9C8c
 MAINNET_VOTING_ADDRESS=0x2e59A20f205bB85a89C53f1936454680651E618e
@@ -45,10 +52,7 @@ MAINNET_VALIDATORS_EXIT_BUS_ORACLE_ADDRESS=
 MAINNET_WITHDRAWAL_QUEUE_ADDRESS=
 MAINNET_WITHDRAWAL_VAULT_ADDRESS=
 
-; Forking URL for mainnet hardhat fork
-HARDHAT_FORKING_URL=
-
-; Scratch deployment via hardhat variables
+# Scratch deployment via hardhat variables
 DEPLOYER=0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266
 GENESIS_TIME=1639659600
 GAS_PRIORITY_FEE=1

.github/workflows/tests-integration-scratch.yml

@@ -24,9 +24,21 @@ jobs:
         run: cp .env.example .env
 
       - name: Run scratch deployment
-        run: ./scripts/dao-ci-deploy.sh
+        run: ./scripts/dao-deploy.sh
+        env:
+          NETWORK: "local"
+          RPC_URL: "http://localhost:8555"
+          GENESIS_TIME: 1639659600 # just a random time
+          DEPLOYER: "0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266" # first acc of default mnemonic "test test ..."
+          GAS_PRIORITY_FEE: 1
+          GAS_MAX_FEE: 100
+          NETWORK_STATE_FILE: "deployed-local.json"
+          NETWORK_STATE_DEFAULTS_FILE: "scripts/scratch/deployed-testnet-defaults.json"
+
+      - name: Finalize scratch deployment
+        run: yarn hardhat --network local run --no-compile scripts/scratch/send-hardhat-mine.ts
 
       - name: Run integration tests
-        run: yarn test:integration:fork:scratch
+        run: yarn test:integration:fork:local
         env:
           LOG_LEVEL: debug

CONTRIBUTING.md

@@ -193,9 +193,9 @@ the project (you can use `.env.example` as a template).
 
 ###### Hardhat Mainnet Fork
 
-This is the most common way to run integration tests. It uses the Hardhat mainnet fork to simulate the mainnet
-environment. Requires `HARDHAT_FORKING_URL` and `HARDHAT_FORKING_BLOCK_NUMBER` (optional) to be set in the `.env` file
-along with some `MAINNET_*` env variables (see `.env.example`).
+This is the most common way to run integration tests. It uses instance of Hardhat Network that forks mainnet
+environment. Requires `MAINNET_FORKING_URL` to be set in the `.env` file along with some `MAINNET_*` env variables (see
+`.env.example`).
 
 ```bash
 yarn test:integration             # Run all integration tests
@@ -231,7 +231,7 @@ Requires a local deployment to be running on port `8555` and `deployed-local.jso
 (automatically generated during the scratch deployment).
 
 ```bash
-yarn test:integration:fork:scratch
+yarn test:integration:fork:local
 ```
 
 #### Foundry tests

contracts/testnets/sepolia/SepoliaDepositAdapter.sol

@@ -1,5 +1,5 @@
-// SPDX-FileCopyrightText: 2024 Lido <[email protected]>
-// SPDX-License-Identifier: GPL-3.0
+// SPDX-License-Identifier: UNLICENSED
+// for testing purposes only
 
 /* See contracts/COMPILERS.md */
 pragma solidity 0.8.9;
@@ -71,7 +71,7 @@ contract SepoliaDepositAdapter is IDepositContract, Ownable, Versioned {
     function recoverEth() external onlyOwner {
         uint256 balance = address(this).balance;
         // solhint-disable-next-line avoid-low-level-calls
-        (bool success, ) = owner().call{value: balance}("");
+        (bool success,) = owner().call{value: balance}("");
         if (!success) {
             revert EthRecoverFailed();
         }
@@ -95,7 +95,7 @@ contract SepoliaDepositAdapter is IDepositContract, Ownable, Versioned {
     ) external payable override {
         originalContract.deposit{value: msg.value}(pubkey, withdrawal_credentials, signature, deposit_data_root);
         // solhint-disable-next-line avoid-low-level-calls
-        (bool success, ) = owner().call{value: msg.value}("");
+        (bool success,) = owner().call{value: msg.value}("");
         if (!success) {
             revert DepositFailed();
         }

docs/scratch-deploy.md

@@ -46,7 +46,7 @@ These are the deployment setups, supported currently:
 Each is described in the details in the sections below.
 
 > NB: Aragon UI for Lido DAO is to be deprecated and replaced by a custom solution, thus not included in the deployment
-> script.
+> script, see https://research.lido.fi/t/discontinuation-of-aragon-ui-use/7992.
 
 ### Deploy steps
 

globals.d.ts

@@ -1,14 +1,19 @@
 declare namespace NodeJS {
   export interface ProcessEnv {
-    /* forking url for hardhat internal node, required for tracing e.g. */
-    HARDHAT_FORKING_URL?: string;
+    /* RPC URL for Hardhat Network forking, required for running tests on mainnet fork with tracing */
+    MAINNET_FORKING_URL?: string;
 
     /* logging verbosity */
     LOG_LEVEL?: "all" | "debug" | "info" | "warn" | "error" | "none";
 
-    /* flags for changing the behavior of the integration tests */
-    INTEGRATION_SCRATCH_DEPLOY?: "on" | "off"; // if "on" test will use scratch deploy instead of forking
-    INTEGRATION_SIMPLE_DVT_MODULE?: "on" | "off"; // if "on" test will use simple DVT module instead of linking NOR
+    /**
+     * Flags for changing the behavior of the integration tests
+     */
+
+    /* if "on" the integration tests will deploy the contracts to the empty Hardhat Network node using scratch deploy */
+    INTEGRATION_SCRATCH_DEPLOY?: "on" | "off";
+    /* if "on" the integration tests will enable assertions and checks for the simple DVT module */
+    INTEGRATION_SIMPLE_DVT_MODULE?: "on" | "off";
 
     /**
      * Network configuration for the protocol discovery.

hardhat.config.ts

@@ -19,7 +19,7 @@ import { HardhatUserConfig, subtask } from "hardhat/config";
 import { mochaRootHooks } from "test/hooks";
 
 const RPC_URL: string = process.env.RPC_URL || "";
-const HARDHAT_FORKING_URL = process.env.HARDHAT_FORKING_URL || "";
+const MAINNET_FORKING_URL = process.env.MAINNET_FORKING_URL || "";
 const INTEGRATION_SCRATCH_DEPLOY = process.env.INTEGRATION_SCRATCH_DEPLOY || "off";
 const ACCOUNTS_PATH = "./accounts.json";
 
@@ -28,7 +28,7 @@ const ACCOUNTS_PATH = "./accounts.json";
  * @returns The forking configuration object or undefined.
  */
 function getHardhatForkingConfig() {
-  return INTEGRATION_SCRATCH_DEPLOY === "on" || !HARDHAT_FORKING_URL ? undefined : { url: HARDHAT_FORKING_URL };
+  return INTEGRATION_SCRATCH_DEPLOY === "on" || !MAINNET_FORKING_URL ? undefined : { url: MAINNET_FORKING_URL };
 }
 
 function loadAccounts(networkName: string) {

lib/protocol/context.ts

@@ -23,6 +23,7 @@ export const getProtocolContext = async (): Promise<ProtocolContext> => {
 
   // By default, all flags are "on"
   const flags = {
+    isScratchDeploy: process.env.INTEGRATION_SCRATCH_DEPLOY === "on",
     withSimpleDvtModule: process.env.INTEGRATION_SIMPLE_DVT_MODULE !== "off",
   } as ProtocolContextFlags;
 

lib/protocol/networks.ts

@@ -6,7 +6,6 @@ import { log } from "lib";
 
 import { ProtocolNetworkItems } from "./types";
 
-// If we are running in Hardhat without fork, we need to deploy the contracts from scratch deploy first to run integration tests
 export function isNonForkingHardhatNetwork() {
   const networkName = hre.network.name;
   if (networkName === "hardhat") {

lib/protocol/types.ts

@@ -134,6 +134,7 @@ export type ProtocolSigners = {
 export type Signer = keyof ProtocolSigners;
 
 export type ProtocolContextFlags = {
+  isScratchDeploy: boolean;
   withSimpleDvtModule: boolean;
 };
 

lib/state-file.ts

@@ -191,7 +191,6 @@ export async function resetStateFile(networkName: string = hardhatNetwork.name):
   }
 }
 
-// If network name is undefined current hardhat network will be used
 export function persistNetworkState(state: DeploymentState, networkName: string = hardhatNetwork.name): void {
   const fileName = _getFileName(networkName, NETWORK_STATE_FILE_BASENAME, NETWORK_STATE_FILE_DIR);
   const stateSorted = _sortKeysAlphabetically(state);

package.json

@@ -28,7 +28,7 @@
     "test:integration:scratch": "INTEGRATION_SCRATCH_DEPLOY=on INTEGRATION_SIMPLE_DVT_MODULE=off hardhat test test/integration/**/*.ts --bail",
     "test:integration:scratch:trace": "INTEGRATION_SCRATCH_DEPLOY=on INTEGRATION_SIMPLE_DVT_MODULE=off hardhat test test/integration/**/*.ts --trace --disabletracer --bail",
     "test:integration:scratch:fulltrace": "INTEGRATION_SCRATCH_DEPLOY=on INTEGRATION_SIMPLE_DVT_MODULE=off hardhat test test/integration/**/*.ts --fulltrace --disabletracer --bail",
-    "test:integration:fork:scratch": "INTEGRATION_SIMPLE_DVT_MODULE=off hardhat test test/integration/**/*.ts --network local --bail",
+    "test:integration:fork:local": "INTEGRATION_SIMPLE_DVT_MODULE=off hardhat test test/integration/**/*.ts --network local --bail",
     "test:integration:fork:mainnet": "hardhat test test/integration/**/*.ts --network mainnet-fork --bail",
     "typecheck": "tsc --noEmit",
     "prepare": "husky",

scripts/dao-deploy.sh

@@ -2,9 +2,6 @@
 set -e +u
 set -o pipefail
 
-# TODO: Do we still need to set these variable?
-# ARAGON_APPS_REPO_REF=import-shared-minime
-
 # Check for required environment variables
 if [[ -z "${DEPLOYER}" ]]; then
   echo "Error: Environment variable DEPLOYER must be set"

scripts/dao-local-deploy.sh

@@ -22,4 +22,4 @@ bash scripts/dao-deploy.sh
 yarn hardhat --network $NETWORK run --no-compile scripts/scratch/send-hardhat-mine.ts
 
 # Run acceptance tests
-yarn test:integration:fork:scratch
+yarn test:integration:fork:local

scripts/dao-local-upgrade.sh

@@ -21,4 +21,4 @@ bash scripts/dao-upgrade.sh
 yarn hardhat --network $NETWORK run --no-compile scripts/scratch/send-hardhat-mine.ts
 
 # Run acceptance tests
-yarn test:integration:fork:scratch
+yarn test:integration:fork:local

scripts/dao-upgrade.sh

@@ -2,9 +2,6 @@
 set -e +u
 set -o pipefail
 
-# TODO: Do we still need to set these variable?
-# ARAGON_APPS_REPO_REF=import-shared-minime
-
 # Check for required environment variables
 if [[ -z "${DEPLOYER}" ]]; then
   echo "Error: Environment variable DEPLOYER must be set"

scripts/scratch/scratch-acceptance-test.ts

@@ -36,8 +36,8 @@ const ADDRESS_2 = "0x0000000000000000000000000000000000000002";
 
 const MANAGE_MEMBERS_AND_QUORUM_ROLE = streccak("MANAGE_MEMBERS_AND_QUORUM_ROLE");
 
-if (!process.env.HARDHAT_FORKING_URL) {
-  log.error("Env variable HARDHAT_FORKING_URL must be set to run fork acceptance tests");
+if (!process.env.MAINNET_FORKING_URL) {
+  log.error("Env variable MAINNET_FORKING_URL must be set to run fork acceptance tests");
   process.exit(1);
 }
 if (!process.env.NETWORK_STATE_FILE) {

scripts/scratch/steps.json

@@ -1,20 +1,20 @@
 {
   "steps": [
-    "scratch/steps/00-populate-deploy-artifact-from-env",
-    "scratch/steps/01-deploy-deposit-contract",
-    "scratch/steps/02-deploy-aragon-env",
-    "scratch/steps/03-deploy-template-and-app-bases",
-    "scratch/steps/04-register-ens-domain",
-    "scratch/steps/05-deploy-apm",
-    "scratch/steps/06-create-app-repos",
-    "scratch/steps/07-deploy-dao",
-    "scratch/steps/08-issue-tokens",
-    "scratch/steps/09-deploy-non-aragon-contracts",
-    "scratch/steps/10-gate-seal",
-    "scratch/steps/11-finalize-dao",
-    "scratch/steps/12-initialize-non-aragon-contracts",
-    "scratch/steps/13-grant-roles",
-    "scratch/steps/14-plug-curated-staking-module",
-    "scratch/steps/15-transfer-roles"
+    "scratch/steps/0000-populate-deploy-artifact-from-env",
+    "scratch/steps/0010-deploy-deposit-contract",
+    "scratch/steps/0020-deploy-aragon-env",
+    "scratch/steps/0030-deploy-template-and-app-bases",
+    "scratch/steps/0040-register-ens-domain",
+    "scratch/steps/0050-deploy-apm",
+    "scratch/steps/0060-create-app-repos",
+    "scratch/steps/0070-deploy-dao",
+    "scratch/steps/0080-issue-tokens",
+    "scratch/steps/0090-deploy-non-aragon-contracts",
+    "scratch/steps/0100-gate-seal",
+    "scratch/steps/0110-finalize-dao",
+    "scratch/steps/0120-initialize-non-aragon-contracts",
+    "scratch/steps/0130-grant-roles",
+    "scratch/steps/0140-plug-curated-staking-module",
+    "scratch/steps/0150-transfer-roles"
   ]
 }

scripts/upgrade/steps.json

@@ -1,3 +1,3 @@
 {
-  "steps": ["upgrade/steps/00-deploy-locator"]
+  "steps": ["upgrade/steps/0000-deploy-locator"]
 }