From 3a05a45e3ab62f29ad113330c3ee8b36bf827dd8 Mon Sep 17 00:00:00 2001 From: Jakub Nowakowski Date: Fri, 8 Dec 2023 13:32:35 +0100 Subject: [PATCH 01/13] Replace realpath with another resolution As per https://github.com/thesis/acre/pull/58#discussion_r1418754955 realpath is not available out of the box on OSX. Instead of installing additional package, we changed the way the path is resolved. --- core/scripts/fetch_external_artifacts.sh | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/core/scripts/fetch_external_artifacts.sh b/core/scripts/fetch_external_artifacts.sh index 5c043568e..c75c98122 100755 --- a/core/scripts/fetch_external_artifacts.sh +++ b/core/scripts/fetch_external_artifacts.sh @@ -1,7 +1,10 @@ #! /bin/bash set -eou pipefail -ROOT_DIR="$(realpath "$(dirname $0)/../")" +ROOT_DIR=$( + cd "$(dirname $0)/../" + pwd -P +) TMP_DIR=${ROOT_DIR}/tmp/external-artifacts EXTERNAL_ARTIFACTS_DIR=${ROOT_DIR}/external From aa133deee055d45fee2659f66052c50a19b83e5a Mon Sep 17 00:00:00 2001 From: Dmitry Date: Mon, 11 Dec 2023 13:31:09 +0100 Subject: [PATCH 02/13] Removing a rule that already exist in parent config --- core/.solhint.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/.solhint.json b/core/.solhint.json index 435aa7886..7afe6405c 100644 --- a/core/.solhint.json +++ b/core/.solhint.json @@ -1,5 +1,5 @@ { "extends": "thesis", "plugins": [], - "rules": { "func-visibility": ["error", { "ignoreConstructors": true }] } + "rules": {} } From 9e0ffd9959bca489a23f44f710f0fda236d7bf4b Mon Sep 17 00:00:00 2001 From: Dmitry Date: Mon, 11 Dec 2023 13:36:23 +0100 Subject: [PATCH 03/13] Renaming AcreRouter -> Dispatcher --- .../{AcreRouter.sol => Dispatcher.sol} | 6 +- core/deploy/02_deploy_acre_router.ts | 4 +- .../22_transfer_ownership_acre_router.ts | 4 +- ...{AcreRouter.test.ts => Dispatcher.test.ts} | 76 +++++++++---------- core/test/helpers/context.ts | 6 +- 5 files changed, 48 insertions(+), 48 deletions(-) rename core/contracts/{AcreRouter.sol => Dispatcher.sol} (94%) rename core/test/{AcreRouter.test.ts => Dispatcher.test.ts} (57%) diff --git a/core/contracts/AcreRouter.sol b/core/contracts/Dispatcher.sol similarity index 94% rename from core/contracts/AcreRouter.sol rename to core/contracts/Dispatcher.sol index c7224e324..02801b743 100644 --- a/core/contracts/AcreRouter.sol +++ b/core/contracts/Dispatcher.sol @@ -3,11 +3,11 @@ pragma solidity ^0.8.21; import "@openzeppelin/contracts/access/Ownable.sol"; -/// @title AcreRouter -/// @notice AcreRouter is a contract that routes TBTC from stBTC (Acre) to +/// @title Dispatcher +/// @notice Dispatcher is a contract that routes TBTC from stBTC (Acre) to /// a given vault and back. Vaults supply yield strategies with TBTC that /// generate yield for Bitcoin holders. -contract AcreRouter is Ownable { +contract Dispatcher is Ownable { struct Vault { bool approved; } diff --git a/core/deploy/02_deploy_acre_router.ts b/core/deploy/02_deploy_acre_router.ts index be2e45b11..bf99d4d73 100644 --- a/core/deploy/02_deploy_acre_router.ts +++ b/core/deploy/02_deploy_acre_router.ts @@ -5,7 +5,7 @@ const func: DeployFunction = async (hre: HardhatRuntimeEnvironment) => { const { getNamedAccounts, deployments } = hre const { deployer } = await getNamedAccounts() - await deployments.deploy("AcreRouter", { + await deployments.deploy("Dispatcher", { from: deployer, args: [], log: true, @@ -18,5 +18,5 @@ const func: DeployFunction = async (hre: HardhatRuntimeEnvironment) => { export default func -func.tags = ["AcreRouter"] +func.tags = ["Dispatcher"] func.dependencies = ["Acre"] diff --git a/core/deploy/22_transfer_ownership_acre_router.ts b/core/deploy/22_transfer_ownership_acre_router.ts index 7652e04d2..2d7748580 100644 --- a/core/deploy/22_transfer_ownership_acre_router.ts +++ b/core/deploy/22_transfer_ownership_acre_router.ts @@ -6,10 +6,10 @@ const func: DeployFunction = async (hre: HardhatRuntimeEnvironment) => { const { deployer, governance } = await getNamedAccounts() const { log } = deployments - log(`transferring ownership of AcreRouter contract to ${governance}`) + log(`transferring ownership of Dispatcher contract to ${governance}`) await deployments.execute( - "AcreRouter", + "Dispatcher", { from: deployer, log: true, waitConfirmations: 1 }, "transferOwnership", governance, diff --git a/core/test/AcreRouter.test.ts b/core/test/Dispatcher.test.ts similarity index 57% rename from core/test/AcreRouter.test.ts rename to core/test/Dispatcher.test.ts index e19b68f07..c9037b850 100644 --- a/core/test/AcreRouter.test.ts +++ b/core/test/Dispatcher.test.ts @@ -6,14 +6,14 @@ import { SnapshotRestorer, takeSnapshot, } from "@nomicfoundation/hardhat-toolbox/network-helpers" -import type { AcreRouter } from "../typechain" +import type { Dispatcher } from "../typechain" import { deployment } from "./helpers/context" import { getNamedSigner, getUnnamedSigner } from "./helpers/signer" -describe("AcreRouter", () => { +describe("Dispatcher", () => { let snapshot: SnapshotRestorer - let acreRouter: AcreRouter + let dispatcher: Dispatcher let governance: HardhatEthersSigner let thirdParty: HardhatEthersSigner let vault1: Address @@ -28,7 +28,7 @@ describe("AcreRouter", () => { governance = (await getNamedSigner()).governance ;[thirdParty] = await getUnnamedSigner() - acreRouter = (await deployment()).acreRouter + dispatcher = (await deployment()).dispatcher }) beforeEach(async () => { @@ -43,9 +43,9 @@ describe("AcreRouter", () => { context("when caller is not a governance account", () => { it("should revert when adding a vault", async () => { await expect( - acreRouter.connect(thirdParty).addVault(vault1), + dispatcher.connect(thirdParty).addVault(vault1), ).to.be.revertedWithCustomError( - acreRouter, + dispatcher, "OwnableUnauthorizedAccount", ) }) @@ -53,33 +53,33 @@ describe("AcreRouter", () => { context("when caller is a governance account", () => { it("should be able to add vaults", async () => { - await acreRouter.connect(governance).addVault(vault1) - await acreRouter.connect(governance).addVault(vault2) - await acreRouter.connect(governance).addVault(vault3) + await dispatcher.connect(governance).addVault(vault1) + await dispatcher.connect(governance).addVault(vault2) + await dispatcher.connect(governance).addVault(vault3) - expect(await acreRouter.vaults(0)).to.equal(vault1) - const isVault1Approved = await acreRouter.vaultsInfo(vault1) + expect(await dispatcher.vaults(0)).to.equal(vault1) + const isVault1Approved = await dispatcher.vaultsInfo(vault1) expect(isVault1Approved).to.equal(true) - expect(await acreRouter.vaults(1)).to.equal(vault2) - const isVault2Approved = await acreRouter.vaultsInfo(vault2) + expect(await dispatcher.vaults(1)).to.equal(vault2) + const isVault2Approved = await dispatcher.vaultsInfo(vault2) expect(isVault2Approved).to.equal(true) - expect(await acreRouter.vaults(2)).to.equal(vault3) - const isVault3Approved = await acreRouter.vaultsInfo(vault3) + expect(await dispatcher.vaults(2)).to.equal(vault3) + const isVault3Approved = await dispatcher.vaultsInfo(vault3) expect(isVault3Approved).to.equal(true) }) it("should not be able to add the same vault twice", async () => { - await acreRouter.connect(governance).addVault(vault1) + await dispatcher.connect(governance).addVault(vault1) await expect( - acreRouter.connect(governance).addVault(vault1), + dispatcher.connect(governance).addVault(vault1), ).to.be.revertedWith("Vault already approved") }) it("should emit an event when adding a vault", async () => { - await expect(acreRouter.connect(governance).addVault(vault1)) - .to.emit(acreRouter, "VaultAdded") + await expect(dispatcher.connect(governance).addVault(vault1)) + .to.emit(dispatcher, "VaultAdded") .withArgs(vault1) }) }) @@ -87,17 +87,17 @@ describe("AcreRouter", () => { describe("removeVault", () => { beforeEach(async () => { - await acreRouter.connect(governance).addVault(vault1) - await acreRouter.connect(governance).addVault(vault2) - await acreRouter.connect(governance).addVault(vault3) + await dispatcher.connect(governance).addVault(vault1) + await dispatcher.connect(governance).addVault(vault2) + await dispatcher.connect(governance).addVault(vault3) }) context("when caller is not a governance account", () => { it("should revert when adding a vault", async () => { await expect( - acreRouter.connect(thirdParty).removeVault(vault1), + dispatcher.connect(thirdParty).removeVault(vault1), ).to.be.revertedWithCustomError( - acreRouter, + dispatcher, "OwnableUnauthorizedAccount", ) }) @@ -105,37 +105,37 @@ describe("AcreRouter", () => { context("when caller is a governance account", () => { it("should be able to remove vaults", async () => { - await acreRouter.connect(governance).removeVault(vault1) + await dispatcher.connect(governance).removeVault(vault1) // Last vault replaced the first vault in the 'vaults' array - expect(await acreRouter.vaults(0)).to.equal(vault3) - const isVault1Approved = await acreRouter.vaultsInfo(vault1) + expect(await dispatcher.vaults(0)).to.equal(vault3) + const isVault1Approved = await dispatcher.vaultsInfo(vault1) expect(isVault1Approved).to.equal(false) - expect(await acreRouter.vaultsLength()).to.equal(2) + expect(await dispatcher.vaultsLength()).to.equal(2) - await acreRouter.connect(governance).removeVault(vault2) + await dispatcher.connect(governance).removeVault(vault2) // Last vault (vault2) was removed from the 'vaults' array - expect(await acreRouter.vaults(0)).to.equal(vault3) - expect(await acreRouter.vaultsLength()).to.equal(1) - const isVault2Approved = await acreRouter.vaultsInfo(vault2) + expect(await dispatcher.vaults(0)).to.equal(vault3) + expect(await dispatcher.vaultsLength()).to.equal(1) + const isVault2Approved = await dispatcher.vaultsInfo(vault2) expect(isVault2Approved).to.equal(false) - await acreRouter.connect(governance).removeVault(vault3) - expect(await acreRouter.vaultsLength()).to.equal(0) - const isVault3Approved = await acreRouter.vaultsInfo(vault3) + await dispatcher.connect(governance).removeVault(vault3) + expect(await dispatcher.vaultsLength()).to.equal(0) + const isVault3Approved = await dispatcher.vaultsInfo(vault3) expect(isVault3Approved).to.equal(false) }) it("should not be able to remove a vault that is not approved", async () => { await expect( - acreRouter.connect(governance).removeVault(vault4), + dispatcher.connect(governance).removeVault(vault4), ).to.be.revertedWith("Not a vault") }) it("should emit an event when removing a vault", async () => { - await expect(acreRouter.connect(governance).removeVault(vault1)) - .to.emit(acreRouter, "VaultRemoved") + await expect(dispatcher.connect(governance).removeVault(vault1)) + .to.emit(dispatcher, "VaultRemoved") .withArgs(vault1) }) }) diff --git a/core/test/helpers/context.ts b/core/test/helpers/context.ts index ec4c56cbd..34875e43d 100644 --- a/core/test/helpers/context.ts +++ b/core/test/helpers/context.ts @@ -2,7 +2,7 @@ import { deployments } from "hardhat" import { getDeployedContract } from "./contract" -import type { Acre, AcreRouter, TestERC20 } from "../../typechain" +import type { Acre, Dispatcher, TestERC20 } from "../../typechain" // eslint-disable-next-line import/prefer-default-export export async function deployment() { @@ -10,7 +10,7 @@ export async function deployment() { const tbtc: TestERC20 = await getDeployedContract("TBTC") const acre: Acre = await getDeployedContract("Acre") - const acreRouter: AcreRouter = await getDeployedContract("AcreRouter") + const dispatcher: Dispatcher = await getDeployedContract("Dispatcher") - return { tbtc, acre, acreRouter } + return { tbtc, acre, dispatcher } } From b56c0ec251325348d9be63e746480e3ca4d0df4b Mon Sep 17 00:00:00 2001 From: Dmitry Date: Mon, 11 Dec 2023 13:50:27 +0100 Subject: [PATCH 04/13] Changing a comment for vaults array --- core/contracts/Dispatcher.sol | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/core/contracts/Dispatcher.sol b/core/contracts/Dispatcher.sol index 02801b743..ff2bddd18 100644 --- a/core/contracts/Dispatcher.sol +++ b/core/contracts/Dispatcher.sol @@ -12,12 +12,12 @@ contract Dispatcher is Ownable { bool approved; } - /// @notice Approved vaults within the Yiern Modules that implement ERC4626 - /// standard. These vaults deposit assets to yield strategies, e.g. - /// Uniswap V3 WBTC/TBTC pool. Vault can be a part of Acre ecosystem - /// or can be implemented externally. As long as it complies with - /// ERC4626 standard and is approved by the owner it can be - /// plugged into Acre. + /// @notice Approved Yield Vaults that implement ERC4626 standard. These + /// vaults deposit assets to yield strategies, e.g. Uniswap V3 + /// WBTC/TBTC pool. Vault can be a part of Acre ecosystem or can be + /// implemented externally. As long as it complies with ERC4626 + /// standard and is approved by the owner it can be plugged into + /// Acre. address[] public vaults; mapping(address => Vault) public vaultsInfo; From d7e8ac65a3a2bb5e9d42062ff5001e0690dfb75b Mon Sep 17 00:00:00 2001 From: Dmitry Date: Mon, 11 Dec 2023 13:53:09 +0100 Subject: [PATCH 05/13] Renaming Vault -> VaultInfo struct --- core/contracts/Dispatcher.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/contracts/Dispatcher.sol b/core/contracts/Dispatcher.sol index ff2bddd18..fb9dfdca0 100644 --- a/core/contracts/Dispatcher.sol +++ b/core/contracts/Dispatcher.sol @@ -8,7 +8,7 @@ import "@openzeppelin/contracts/access/Ownable.sol"; /// a given vault and back. Vaults supply yield strategies with TBTC that /// generate yield for Bitcoin holders. contract Dispatcher is Ownable { - struct Vault { + struct VaultInfo { bool approved; } @@ -19,7 +19,7 @@ contract Dispatcher is Ownable { /// standard and is approved by the owner it can be plugged into /// Acre. address[] public vaults; - mapping(address => Vault) public vaultsInfo; + mapping(address => VaultInfo) public vaultsInfo; event VaultAdded(address indexed vault); event VaultRemoved(address indexed vault); From 01086fce2f4abda3c67f5a1d01502a5fc9ea7711 Mon Sep 17 00:00:00 2001 From: Dmitry Date: Mon, 11 Dec 2023 14:09:02 +0100 Subject: [PATCH 06/13] Approve->authorize for vaults and functions around it --- core/contracts/Dispatcher.sol | 22 ++++++------ core/test/Dispatcher.test.ts | 64 +++++++++++++++++------------------ 2 files changed, 43 insertions(+), 43 deletions(-) diff --git a/core/contracts/Dispatcher.sol b/core/contracts/Dispatcher.sol index fb9dfdca0..ed5a0b4d6 100644 --- a/core/contracts/Dispatcher.sol +++ b/core/contracts/Dispatcher.sol @@ -9,14 +9,14 @@ import "@openzeppelin/contracts/access/Ownable.sol"; /// generate yield for Bitcoin holders. contract Dispatcher is Ownable { struct VaultInfo { - bool approved; + bool authorized; } - /// @notice Approved Yield Vaults that implement ERC4626 standard. These + /// @notice Authorized Yield Vaults that implement ERC4626 standard. These /// vaults deposit assets to yield strategies, e.g. Uniswap V3 /// WBTC/TBTC pool. Vault can be a part of Acre ecosystem or can be /// implemented externally. As long as it complies with ERC4626 - /// standard and is approved by the owner it can be plugged into + /// standard and is authorized by the owner it can be plugged into /// Acre. address[] public vaults; mapping(address => VaultInfo) public vaultsInfo; @@ -26,23 +26,23 @@ contract Dispatcher is Ownable { constructor() Ownable(msg.sender) {} - /// @notice Adds a vault to the list of approved vaults. + /// @notice Adds a vault to the list of authorized vaults. /// @param vault Address of the vault to add. - function addVault(address vault) external onlyOwner { - require(!vaultsInfo[vault].approved, "Vault already approved"); + function authorizeVault(address vault) external onlyOwner { + require(!vaultsInfo[vault].authorized, "Vault already authorized"); vaults.push(vault); - vaultsInfo[vault].approved = true; + vaultsInfo[vault].authorized = true; emit VaultAdded(vault); } - /// @notice Removes a vault from the list of approved vaults. + /// @notice Removes a vault from the list of authorized vaults. /// @param vault Address of the vault to remove. - function removeVault(address vault) external onlyOwner { - require(vaultsInfo[vault].approved, "Not a vault"); + function deauthorizeVault(address vault) external onlyOwner { + require(vaultsInfo[vault].authorized, "Not a vault"); - vaultsInfo[vault].approved = false; + vaultsInfo[vault].authorized = false; for (uint256 i = 0; i < vaults.length; i++) { if (vaults[i] == vault) { diff --git a/core/test/Dispatcher.test.ts b/core/test/Dispatcher.test.ts index c9037b850..5c29d54e8 100644 --- a/core/test/Dispatcher.test.ts +++ b/core/test/Dispatcher.test.ts @@ -39,11 +39,11 @@ describe("Dispatcher", () => { await snapshot.restore() }) - describe("addVault", () => { + describe("authorizeVault", () => { context("when caller is not a governance account", () => { it("should revert when adding a vault", async () => { await expect( - dispatcher.connect(thirdParty).addVault(vault1), + dispatcher.connect(thirdParty).authorizeVault(vault1), ).to.be.revertedWithCustomError( dispatcher, "OwnableUnauthorizedAccount", @@ -53,49 +53,49 @@ describe("Dispatcher", () => { context("when caller is a governance account", () => { it("should be able to add vaults", async () => { - await dispatcher.connect(governance).addVault(vault1) - await dispatcher.connect(governance).addVault(vault2) - await dispatcher.connect(governance).addVault(vault3) + await dispatcher.connect(governance).authorizeVault(vault1) + await dispatcher.connect(governance).authorizeVault(vault2) + await dispatcher.connect(governance).authorizeVault(vault3) expect(await dispatcher.vaults(0)).to.equal(vault1) - const isVault1Approved = await dispatcher.vaultsInfo(vault1) - expect(isVault1Approved).to.equal(true) + const isVault1Authorized = await dispatcher.vaultsInfo(vault1) + expect(isVault1Authorized).to.equal(true) expect(await dispatcher.vaults(1)).to.equal(vault2) - const isVault2Approved = await dispatcher.vaultsInfo(vault2) - expect(isVault2Approved).to.equal(true) + const isVault2Authorized = await dispatcher.vaultsInfo(vault2) + expect(isVault2Authorized).to.equal(true) expect(await dispatcher.vaults(2)).to.equal(vault3) - const isVault3Approved = await dispatcher.vaultsInfo(vault3) - expect(isVault3Approved).to.equal(true) + const isVault3Authorized = await dispatcher.vaultsInfo(vault3) + expect(isVault3Authorized).to.equal(true) }) it("should not be able to add the same vault twice", async () => { - await dispatcher.connect(governance).addVault(vault1) + await dispatcher.connect(governance).authorizeVault(vault1) await expect( - dispatcher.connect(governance).addVault(vault1), - ).to.be.revertedWith("Vault already approved") + dispatcher.connect(governance).authorizeVault(vault1), + ).to.be.revertedWith("Vault already authorized") }) it("should emit an event when adding a vault", async () => { - await expect(dispatcher.connect(governance).addVault(vault1)) + await expect(dispatcher.connect(governance).authorizeVault(vault1)) .to.emit(dispatcher, "VaultAdded") .withArgs(vault1) }) }) }) - describe("removeVault", () => { + describe("deauthorizeVault", () => { beforeEach(async () => { - await dispatcher.connect(governance).addVault(vault1) - await dispatcher.connect(governance).addVault(vault2) - await dispatcher.connect(governance).addVault(vault3) + await dispatcher.connect(governance).authorizeVault(vault1) + await dispatcher.connect(governance).authorizeVault(vault2) + await dispatcher.connect(governance).authorizeVault(vault3) }) context("when caller is not a governance account", () => { it("should revert when adding a vault", async () => { await expect( - dispatcher.connect(thirdParty).removeVault(vault1), + dispatcher.connect(thirdParty).deauthorizeVault(vault1), ).to.be.revertedWithCustomError( dispatcher, "OwnableUnauthorizedAccount", @@ -105,36 +105,36 @@ describe("Dispatcher", () => { context("when caller is a governance account", () => { it("should be able to remove vaults", async () => { - await dispatcher.connect(governance).removeVault(vault1) + await dispatcher.connect(governance).deauthorizeVault(vault1) // Last vault replaced the first vault in the 'vaults' array expect(await dispatcher.vaults(0)).to.equal(vault3) - const isVault1Approved = await dispatcher.vaultsInfo(vault1) - expect(isVault1Approved).to.equal(false) + const isVault1Authorized = await dispatcher.vaultsInfo(vault1) + expect(isVault1Authorized).to.equal(false) expect(await dispatcher.vaultsLength()).to.equal(2) - await dispatcher.connect(governance).removeVault(vault2) + await dispatcher.connect(governance).deauthorizeVault(vault2) // Last vault (vault2) was removed from the 'vaults' array expect(await dispatcher.vaults(0)).to.equal(vault3) expect(await dispatcher.vaultsLength()).to.equal(1) - const isVault2Approved = await dispatcher.vaultsInfo(vault2) - expect(isVault2Approved).to.equal(false) + const isVault2Authorized = await dispatcher.vaultsInfo(vault2) + expect(isVault2Authorized).to.equal(false) - await dispatcher.connect(governance).removeVault(vault3) + await dispatcher.connect(governance).deauthorizeVault(vault3) expect(await dispatcher.vaultsLength()).to.equal(0) - const isVault3Approved = await dispatcher.vaultsInfo(vault3) - expect(isVault3Approved).to.equal(false) + const isVault3Authorized = await dispatcher.vaultsInfo(vault3) + expect(isVault3Authorized).to.equal(false) }) - it("should not be able to remove a vault that is not approved", async () => { + it("should not be able to remove a vault that is not authorized", async () => { await expect( - dispatcher.connect(governance).removeVault(vault4), + dispatcher.connect(governance).deauthorizeVault(vault4), ).to.be.revertedWith("Not a vault") }) it("should emit an event when removing a vault", async () => { - await expect(dispatcher.connect(governance).removeVault(vault1)) + await expect(dispatcher.connect(governance).deauthorizeVault(vault1)) .to.emit(dispatcher, "VaultRemoved") .withArgs(vault1) }) From f477e4500a7fffdb1082f666b711be4c92de70c4 Mon Sep 17 00:00:00 2001 From: Dmitry Date: Mon, 11 Dec 2023 15:17:27 +0100 Subject: [PATCH 07/13] Replacing require err messages with custom errors --- core/contracts/Dispatcher.sol | 11 +++++++++-- core/test/Dispatcher.test.ts | 4 ++-- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/core/contracts/Dispatcher.sol b/core/contracts/Dispatcher.sol index ed5a0b4d6..88e14f103 100644 --- a/core/contracts/Dispatcher.sol +++ b/core/contracts/Dispatcher.sol @@ -8,6 +8,9 @@ import "@openzeppelin/contracts/access/Ownable.sol"; /// a given vault and back. Vaults supply yield strategies with TBTC that /// generate yield for Bitcoin holders. contract Dispatcher is Ownable { + error VaultAlreadyAuthorized(); + error VaultUnauthorized(); + struct VaultInfo { bool authorized; } @@ -29,7 +32,9 @@ contract Dispatcher is Ownable { /// @notice Adds a vault to the list of authorized vaults. /// @param vault Address of the vault to add. function authorizeVault(address vault) external onlyOwner { - require(!vaultsInfo[vault].authorized, "Vault already authorized"); + if (vaultsInfo[vault].authorized) { + revert VaultAlreadyAuthorized(); + } vaults.push(vault); vaultsInfo[vault].authorized = true; @@ -40,7 +45,9 @@ contract Dispatcher is Ownable { /// @notice Removes a vault from the list of authorized vaults. /// @param vault Address of the vault to remove. function deauthorizeVault(address vault) external onlyOwner { - require(vaultsInfo[vault].authorized, "Not a vault"); + if (!vaultsInfo[vault].authorized) { + revert VaultUnauthorized(); + } vaultsInfo[vault].authorized = false; diff --git a/core/test/Dispatcher.test.ts b/core/test/Dispatcher.test.ts index 5c29d54e8..2305d34e3 100644 --- a/core/test/Dispatcher.test.ts +++ b/core/test/Dispatcher.test.ts @@ -74,7 +74,7 @@ describe("Dispatcher", () => { await dispatcher.connect(governance).authorizeVault(vault1) await expect( dispatcher.connect(governance).authorizeVault(vault1), - ).to.be.revertedWith("Vault already authorized") + ).to.be.revertedWithCustomError(dispatcher, "VaultAlreadyAuthorized") }) it("should emit an event when adding a vault", async () => { @@ -130,7 +130,7 @@ describe("Dispatcher", () => { it("should not be able to remove a vault that is not authorized", async () => { await expect( dispatcher.connect(governance).deauthorizeVault(vault4), - ).to.be.revertedWith("Not a vault") + ).to.be.revertedWithCustomError(dispatcher, "VaultUnauthorized") }) it("should emit an event when removing a vault", async () => { From d0bd5dd79dc4c8d7fcb1aad6121d0657fcf7ae13 Mon Sep 17 00:00:00 2001 From: Dmitry Date: Mon, 11 Dec 2023 15:28:49 +0100 Subject: [PATCH 08/13] Renaming events and returning the entire vaults array - Renamed VauldAdded->VaultAuthorized and VaultRemoved->VaultDeauthorized events - Returning vaults array instead of vaults array length --- core/contracts/Dispatcher.sol | 12 ++++++------ core/test/Dispatcher.test.ts | 10 +++++----- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/core/contracts/Dispatcher.sol b/core/contracts/Dispatcher.sol index 88e14f103..309da2ae8 100644 --- a/core/contracts/Dispatcher.sol +++ b/core/contracts/Dispatcher.sol @@ -24,8 +24,8 @@ contract Dispatcher is Ownable { address[] public vaults; mapping(address => VaultInfo) public vaultsInfo; - event VaultAdded(address indexed vault); - event VaultRemoved(address indexed vault); + event VaultAuthorized(address indexed vault); + event VaultDeauthorized(address indexed vault); constructor() Ownable(msg.sender) {} @@ -39,7 +39,7 @@ contract Dispatcher is Ownable { vaults.push(vault); vaultsInfo[vault].authorized = true; - emit VaultAdded(vault); + emit VaultAuthorized(vault); } /// @notice Removes a vault from the list of authorized vaults. @@ -60,10 +60,10 @@ contract Dispatcher is Ownable { } } - emit VaultRemoved(vault); + emit VaultDeauthorized(vault); } - function vaultsLength() external view returns (uint256) { - return vaults.length; + function getVaults() external view returns (address[] memory) { + return vaults; } } diff --git a/core/test/Dispatcher.test.ts b/core/test/Dispatcher.test.ts index 2305d34e3..6aeea01fc 100644 --- a/core/test/Dispatcher.test.ts +++ b/core/test/Dispatcher.test.ts @@ -79,7 +79,7 @@ describe("Dispatcher", () => { it("should emit an event when adding a vault", async () => { await expect(dispatcher.connect(governance).authorizeVault(vault1)) - .to.emit(dispatcher, "VaultAdded") + .to.emit(dispatcher, "VaultAuthorized") .withArgs(vault1) }) }) @@ -111,18 +111,18 @@ describe("Dispatcher", () => { expect(await dispatcher.vaults(0)).to.equal(vault3) const isVault1Authorized = await dispatcher.vaultsInfo(vault1) expect(isVault1Authorized).to.equal(false) - expect(await dispatcher.vaultsLength()).to.equal(2) + expect((await dispatcher.getVaults()).length).to.equal(2) await dispatcher.connect(governance).deauthorizeVault(vault2) // Last vault (vault2) was removed from the 'vaults' array expect(await dispatcher.vaults(0)).to.equal(vault3) - expect(await dispatcher.vaultsLength()).to.equal(1) + expect((await dispatcher.getVaults()).length).to.equal(1) const isVault2Authorized = await dispatcher.vaultsInfo(vault2) expect(isVault2Authorized).to.equal(false) await dispatcher.connect(governance).deauthorizeVault(vault3) - expect(await dispatcher.vaultsLength()).to.equal(0) + expect((await dispatcher.getVaults()).length).to.equal(0) const isVault3Authorized = await dispatcher.vaultsInfo(vault3) expect(isVault3Authorized).to.equal(false) }) @@ -135,7 +135,7 @@ describe("Dispatcher", () => { it("should emit an event when removing a vault", async () => { await expect(dispatcher.connect(governance).deauthorizeVault(vault1)) - .to.emit(dispatcher, "VaultRemoved") + .to.emit(dispatcher, "VaultDeauthorized") .withArgs(vault1) }) }) From cf0d47d893f5795308b3f88337274fc35fead4e8 Mon Sep 17 00:00:00 2001 From: Dmitry Date: Mon, 11 Dec 2023 15:31:57 +0100 Subject: [PATCH 09/13] Adding func.dependencies for Dispatcher ownership transfer --- core/deploy/22_transfer_ownership_acre_router.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/core/deploy/22_transfer_ownership_acre_router.ts b/core/deploy/22_transfer_ownership_acre_router.ts index 2d7748580..686d378c4 100644 --- a/core/deploy/22_transfer_ownership_acre_router.ts +++ b/core/deploy/22_transfer_ownership_acre_router.ts @@ -19,3 +19,4 @@ const func: DeployFunction = async (hre: HardhatRuntimeEnvironment) => { export default func func.tags = ["TransferOwnershipAcreRouter"] +func.dependencies = ["Dispatcher"] From b131f8206a9a79535a10f423f6612fd41f3ce90a Mon Sep 17 00:00:00 2001 From: Dmitry Date: Mon, 11 Dec 2023 16:06:18 +0100 Subject: [PATCH 10/13] Refactoring Dispatcher tests: - Replaced Address type with a String type - Added test fixture just like we have in Acre.test.ts - Various renames --- core/test/Dispatcher.test.ts | 119 ++++++++++++++++++++--------------- 1 file changed, 68 insertions(+), 51 deletions(-) diff --git a/core/test/Dispatcher.test.ts b/core/test/Dispatcher.test.ts index 6aeea01fc..5307dcc27 100644 --- a/core/test/Dispatcher.test.ts +++ b/core/test/Dispatcher.test.ts @@ -1,34 +1,41 @@ -import { getUnnamedAccounts } from "hardhat" +import { ethers } from "hardhat" import { HardhatEthersSigner } from "@nomicfoundation/hardhat-ethers/signers" -import { Address } from "hardhat-deploy/types" import { expect } from "chai" import { SnapshotRestorer, takeSnapshot, + loadFixture, } from "@nomicfoundation/hardhat-toolbox/network-helpers" import type { Dispatcher } from "../typechain" import { deployment } from "./helpers/context" import { getNamedSigner, getUnnamedSigner } from "./helpers/signer" +async function fixture() { + const { dispatcher } = await deployment() + const { governance } = await getNamedSigner() + const [thirdParty] = await getUnnamedSigner() + + return { dispatcher, governance, thirdParty } +} + describe("Dispatcher", () => { let snapshot: SnapshotRestorer let dispatcher: Dispatcher let governance: HardhatEthersSigner let thirdParty: HardhatEthersSigner - let vault1: Address - let vault2: Address - let vault3: Address - let vault4: Address + let vaultAddress1: string + let vaultAddress2: string + let vaultAddress3: string + let vaultAddress4: string before(async () => { - const accounts = await getUnnamedAccounts() - ;[vault1, vault2, vault3, vault4] = accounts - - governance = (await getNamedSigner()).governance - ;[thirdParty] = await getUnnamedSigner() + ;({ dispatcher, governance, thirdParty } = await loadFixture(fixture)) - dispatcher = (await deployment()).dispatcher + vaultAddress1 = await ethers.Wallet.createRandom().getAddress() + vaultAddress2 = await ethers.Wallet.createRandom().getAddress() + vaultAddress3 = await ethers.Wallet.createRandom().getAddress() + vaultAddress4 = await ethers.Wallet.createRandom().getAddress() }) beforeEach(async () => { @@ -43,7 +50,7 @@ describe("Dispatcher", () => { context("when caller is not a governance account", () => { it("should revert when adding a vault", async () => { await expect( - dispatcher.connect(thirdParty).authorizeVault(vault1), + dispatcher.connect(thirdParty).authorizeVault(vaultAddress1), ).to.be.revertedWithCustomError( dispatcher, "OwnableUnauthorizedAccount", @@ -53,49 +60,54 @@ describe("Dispatcher", () => { context("when caller is a governance account", () => { it("should be able to add vaults", async () => { - await dispatcher.connect(governance).authorizeVault(vault1) - await dispatcher.connect(governance).authorizeVault(vault2) - await dispatcher.connect(governance).authorizeVault(vault3) - - expect(await dispatcher.vaults(0)).to.equal(vault1) - const isVault1Authorized = await dispatcher.vaultsInfo(vault1) - expect(isVault1Authorized).to.equal(true) - - expect(await dispatcher.vaults(1)).to.equal(vault2) - const isVault2Authorized = await dispatcher.vaultsInfo(vault2) - expect(isVault2Authorized).to.equal(true) - - expect(await dispatcher.vaults(2)).to.equal(vault3) - const isVault3Authorized = await dispatcher.vaultsInfo(vault3) - expect(isVault3Authorized).to.equal(true) + await dispatcher.connect(governance).authorizeVault(vaultAddress1) + await dispatcher.connect(governance).authorizeVault(vaultAddress2) + await dispatcher.connect(governance).authorizeVault(vaultAddress3) + + expect(await dispatcher.vaults(0)).to.equal(vaultAddress1) + const isVaultAddress1Authorized = + await dispatcher.vaultsInfo(vaultAddress1) + expect(isVaultAddress1Authorized).to.equal(true) + + expect(await dispatcher.vaults(1)).to.equal(vaultAddress2) + const isVaultAddress2Authorized = + await dispatcher.vaultsInfo(vaultAddress2) + expect(isVaultAddress2Authorized).to.equal(true) + + expect(await dispatcher.vaults(2)).to.equal(vaultAddress3) + const isVaultAddress3Authorized = + await dispatcher.vaultsInfo(vaultAddress3) + expect(isVaultAddress3Authorized).to.equal(true) }) it("should not be able to add the same vault twice", async () => { - await dispatcher.connect(governance).authorizeVault(vault1) + await dispatcher.connect(governance).authorizeVault(vaultAddress1) await expect( - dispatcher.connect(governance).authorizeVault(vault1), + dispatcher.connect(governance).authorizeVault(vaultAddress1), ).to.be.revertedWithCustomError(dispatcher, "VaultAlreadyAuthorized") }) it("should emit an event when adding a vault", async () => { - await expect(dispatcher.connect(governance).authorizeVault(vault1)) + await expect( + dispatcher.connect(governance).authorizeVault(vaultAddress1), + ) .to.emit(dispatcher, "VaultAuthorized") - .withArgs(vault1) + .withArgs(vaultAddress1) }) }) }) describe("deauthorizeVault", () => { beforeEach(async () => { - await dispatcher.connect(governance).authorizeVault(vault1) - await dispatcher.connect(governance).authorizeVault(vault2) - await dispatcher.connect(governance).authorizeVault(vault3) + await dispatcher.connect(governance).authorizeVault(vaultAddress1) + await dispatcher.connect(governance).authorizeVault(vaultAddress2) + await dispatcher.connect(governance).authorizeVault(vaultAddress3) }) context("when caller is not a governance account", () => { it("should revert when adding a vault", async () => { await expect( - dispatcher.connect(thirdParty).deauthorizeVault(vault1), + dispatcher.connect(thirdParty).deauthorizeVault(vaultAddress1), ).to.be.revertedWithCustomError( dispatcher, "OwnableUnauthorizedAccount", @@ -105,38 +117,43 @@ describe("Dispatcher", () => { context("when caller is a governance account", () => { it("should be able to remove vaults", async () => { - await dispatcher.connect(governance).deauthorizeVault(vault1) + await dispatcher.connect(governance).deauthorizeVault(vaultAddress1) // Last vault replaced the first vault in the 'vaults' array - expect(await dispatcher.vaults(0)).to.equal(vault3) - const isVault1Authorized = await dispatcher.vaultsInfo(vault1) - expect(isVault1Authorized).to.equal(false) + expect(await dispatcher.vaults(0)).to.equal(vaultAddress3) + const isVaultAddress1Authorized = + await dispatcher.vaultsInfo(vaultAddress1) + expect(isVaultAddress1Authorized).to.equal(false) expect((await dispatcher.getVaults()).length).to.equal(2) - await dispatcher.connect(governance).deauthorizeVault(vault2) + await dispatcher.connect(governance).deauthorizeVault(vaultAddress2) - // Last vault (vault2) was removed from the 'vaults' array - expect(await dispatcher.vaults(0)).to.equal(vault3) + // Last vault (vaultAddress2) was removed from the 'vaults' array + expect(await dispatcher.vaults(0)).to.equal(vaultAddress3) expect((await dispatcher.getVaults()).length).to.equal(1) - const isVault2Authorized = await dispatcher.vaultsInfo(vault2) - expect(isVault2Authorized).to.equal(false) + const isVaultAddress2Authorized = + await dispatcher.vaultsInfo(vaultAddress2) + expect(isVaultAddress2Authorized).to.equal(false) - await dispatcher.connect(governance).deauthorizeVault(vault3) + await dispatcher.connect(governance).deauthorizeVault(vaultAddress3) expect((await dispatcher.getVaults()).length).to.equal(0) - const isVault3Authorized = await dispatcher.vaultsInfo(vault3) - expect(isVault3Authorized).to.equal(false) + const isVaultAddress3Authorized = + await dispatcher.vaultsInfo(vaultAddress3) + expect(isVaultAddress3Authorized).to.equal(false) }) it("should not be able to remove a vault that is not authorized", async () => { await expect( - dispatcher.connect(governance).deauthorizeVault(vault4), + dispatcher.connect(governance).deauthorizeVault(vaultAddress4), ).to.be.revertedWithCustomError(dispatcher, "VaultUnauthorized") }) it("should emit an event when removing a vault", async () => { - await expect(dispatcher.connect(governance).deauthorizeVault(vault1)) + await expect( + dispatcher.connect(governance).deauthorizeVault(vaultAddress1), + ) .to.emit(dispatcher, "VaultDeauthorized") - .withArgs(vault1) + .withArgs(vaultAddress1) }) }) }) From e94c5fc0f4dd7f232b5c67261ebf84991ba9e0ee Mon Sep 17 00:00:00 2001 From: Dmitry Date: Mon, 11 Dec 2023 16:32:21 +0100 Subject: [PATCH 11/13] Simplifying tests --- core/test/Dispatcher.test.ts | 40 ++++++++++++++++-------------------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/core/test/Dispatcher.test.ts b/core/test/Dispatcher.test.ts index 5307dcc27..1fdecc5d9 100644 --- a/core/test/Dispatcher.test.ts +++ b/core/test/Dispatcher.test.ts @@ -59,28 +59,22 @@ describe("Dispatcher", () => { }) context("when caller is a governance account", () => { - it("should be able to add vaults", async () => { + it("should be able to authorize vaults", async () => { await dispatcher.connect(governance).authorizeVault(vaultAddress1) await dispatcher.connect(governance).authorizeVault(vaultAddress2) await dispatcher.connect(governance).authorizeVault(vaultAddress3) expect(await dispatcher.vaults(0)).to.equal(vaultAddress1) - const isVaultAddress1Authorized = - await dispatcher.vaultsInfo(vaultAddress1) - expect(isVaultAddress1Authorized).to.equal(true) + expect(await dispatcher.vaultsInfo(vaultAddress1)).to.be.equal(true) expect(await dispatcher.vaults(1)).to.equal(vaultAddress2) - const isVaultAddress2Authorized = - await dispatcher.vaultsInfo(vaultAddress2) - expect(isVaultAddress2Authorized).to.equal(true) + expect(await dispatcher.vaultsInfo(vaultAddress2)).to.be.equal(true) expect(await dispatcher.vaults(2)).to.equal(vaultAddress3) - const isVaultAddress3Authorized = - await dispatcher.vaultsInfo(vaultAddress3) - expect(isVaultAddress3Authorized).to.equal(true) + expect(await dispatcher.vaultsInfo(vaultAddress3)).to.be.equal(true) }) - it("should not be able to add the same vault twice", async () => { + it("should not be able to authorize the same vault twice", async () => { await dispatcher.connect(governance).authorizeVault(vaultAddress1) await expect( dispatcher.connect(governance).authorizeVault(vaultAddress1), @@ -116,14 +110,12 @@ describe("Dispatcher", () => { }) context("when caller is a governance account", () => { - it("should be able to remove vaults", async () => { + it("should be able to authorize vaults", async () => { await dispatcher.connect(governance).deauthorizeVault(vaultAddress1) // Last vault replaced the first vault in the 'vaults' array expect(await dispatcher.vaults(0)).to.equal(vaultAddress3) - const isVaultAddress1Authorized = - await dispatcher.vaultsInfo(vaultAddress1) - expect(isVaultAddress1Authorized).to.equal(false) + expect(await dispatcher.vaultsInfo(vaultAddress1)).to.be.equal(false) expect((await dispatcher.getVaults()).length).to.equal(2) await dispatcher.connect(governance).deauthorizeVault(vaultAddress2) @@ -131,18 +123,22 @@ describe("Dispatcher", () => { // Last vault (vaultAddress2) was removed from the 'vaults' array expect(await dispatcher.vaults(0)).to.equal(vaultAddress3) expect((await dispatcher.getVaults()).length).to.equal(1) - const isVaultAddress2Authorized = - await dispatcher.vaultsInfo(vaultAddress2) - expect(isVaultAddress2Authorized).to.equal(false) + expect(await dispatcher.vaultsInfo(vaultAddress2)).to.be.equal(false) await dispatcher.connect(governance).deauthorizeVault(vaultAddress3) expect((await dispatcher.getVaults()).length).to.equal(0) - const isVaultAddress3Authorized = - await dispatcher.vaultsInfo(vaultAddress3) - expect(isVaultAddress3Authorized).to.equal(false) + expect(await dispatcher.vaultsInfo(vaultAddress3)).to.be.equal(false) }) - it("should not be able to remove a vault that is not authorized", async () => { + it("should be able to deauthorize a vault and authorize it again", async () => { + await dispatcher.connect(governance).deauthorizeVault(vaultAddress1) + expect(await dispatcher.vaultsInfo(vaultAddress1)).to.be.equal(false) + + await dispatcher.connect(governance).authorizeVault(vaultAddress1) + expect(await dispatcher.vaultsInfo(vaultAddress1)).to.be.equal(true) + }) + + it("should not be able to deauthorize a vault that is not authorized", async () => { await expect( dispatcher.connect(governance).deauthorizeVault(vaultAddress4), ).to.be.revertedWithCustomError(dispatcher, "VaultUnauthorized") From e21a50b8fe3268e34c5e5b9da22daecf930a048c Mon Sep 17 00:00:00 2001 From: Jakub Nowakowski Date: Wed, 13 Dec 2023 08:45:28 +0100 Subject: [PATCH 12/13] Improve signers mapping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Rafał Czajkowski <57687279+r-czajkowski@users.noreply.github.com> --- core/test/helpers/signer.ts | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/core/test/helpers/signer.ts b/core/test/helpers/signer.ts index c6fea1c0e..22dcda331 100644 --- a/core/test/helpers/signer.ts +++ b/core/test/helpers/signer.ts @@ -25,13 +25,7 @@ export async function getNamedSigner(): Promise<{ * @returns Array of unnamed Hardhat Ethers Signers. */ export async function getUnnamedSigner(): Promise { - const unnamedSigners: HardhatEthersSigner[] = [] + const accounts = await getUnnamedAccounts() - await Promise.all( - (await getUnnamedAccounts()).map(async (address) => { - unnamedSigners.push(await ethers.getSigner(address)) - }), - ) - - return unnamedSigners + return await Promise.all(accounts.map(ethers.getSigner)) } From 2831ef6791b974690550a78e386e184dcd9f3190 Mon Sep 17 00:00:00 2001 From: Jakub Nowakowski Date: Wed, 13 Dec 2023 09:22:12 +0100 Subject: [PATCH 13/13] Don't return awaited promise in getUnnamedSigner Fix `@typescript-eslint/return-await` error returned by eslint. --- core/test/helpers/signer.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/test/helpers/signer.ts b/core/test/helpers/signer.ts index 22dcda331..0ae57f35e 100644 --- a/core/test/helpers/signer.ts +++ b/core/test/helpers/signer.ts @@ -27,5 +27,5 @@ export async function getNamedSigner(): Promise<{ export async function getUnnamedSigner(): Promise { const accounts = await getUnnamedAccounts() - return await Promise.all(accounts.map(ethers.getSigner)) + return Promise.all(accounts.map(ethers.getSigner)) }