MultisigValidatorManager tests

solidity/core/contracts/test/TestMultisigValidatorManager.sol

@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: MIT OR Apache-2.0
+pragma solidity >=0.6.11;
+pragma abicoder v2;
+
+import {MultisigValidatorManager} from "../validator-manager/MultisigValidatorManager.sol";
+
+/**
+ * This contract exists to test MultisigValidatorManager.sol, which is abstract
+ * and cannot be deployed directly.
+ */
+contract TestMultisigValidatorManager is MultisigValidatorManager {
+    // solhint-disable-next-line no-empty-blocks
+    constructor(
+        uint32 _outboxDomain,
+        address[] memory _validatorSet,
+        uint256 _quorumThreshold
+    )
+        MultisigValidatorManager(_outboxDomain, _validatorSet, _quorumThreshold)
+    {}
+}

solidity/core/contracts/validator-manager/InboxMultisigValidatorManager.sol

@@ -13,7 +13,13 @@ contract InboxMultisigValidatorManager is MultisigValidatorManager {
      * @param _remoteDomain The remote domain of the outbox chain.
      */
     // solhint-disable-next-line no-empty-blocks
-    constructor(uint32 _remoteDomain) MultisigValidatorManager(_remoteDomain) {}
+    constructor(
+        uint32 _remoteDomain,
+        address[] memory _validatorSet,
+        uint256 _quorumThreshold
+    )
+        MultisigValidatorManager(_remoteDomain, _validatorSet, _quorumThreshold)
+    {}
 
     // ============ External Functions ============
 

solidity/core/contracts/validator-manager/MultisigValidatorManager.sol

@@ -58,9 +58,22 @@ abstract contract MultisigValidatorManager is Ownable {
      * @param _outboxDomain The domain of the outbox this validator manager
      * tracks the validator set for.
      */
-    constructor(uint32 _outboxDomain) Ownable() {
+    constructor(
+        uint32 _outboxDomain,
+        address[] memory _validatorSet,
+        uint256 _quorumThreshold
+    ) Ownable() {
+        // Set immutables.
         outboxDomain = _outboxDomain;
         outboxDomainHash = keccak256(abi.encodePacked(_outboxDomain, "ABACUS"));
+
+        // Enroll validators. Reverts if there are any duplicates.
+        uint256 _validatorSetLength = _validatorSet.length;
+        for (uint256 i = 0; i < _validatorSetLength; i++) {
+            _enrollValidator(_validatorSet[i]);
+        }
+
+        _setQuorumThreshold(_quorumThreshold);
     }
 
     // ============ External Functions ============
@@ -71,8 +84,7 @@ abstract contract MultisigValidatorManager is Ownable {
      * @param _validator The validator to add to the validator set.
      */
     function enrollValidator(address _validator) external onlyOwner {
-        require(validators.add(_validator), "enrolled");
+        _enrollValidator(_validator);
-        emit EnrollValidator(_validator);
     }
 
     /**
@@ -81,8 +93,7 @@ abstract contract MultisigValidatorManager is Ownable {
      * @param _validator The validator to remove from the validator set.
      */
     function unenrollValidator(address _validator) external onlyOwner {
-        require(validators.remove(_validator), "!enrolled");
+        _unenrollValidator(_validator);
-        emit UnenrollValidator(_validator);
     }
 
     /**
@@ -90,12 +101,21 @@ abstract contract MultisigValidatorManager is Ownable {
      * @param _quorumThreshold The new quorum threshold.
      */
     function setQuorumThreshold(uint256 _quorumThreshold) external onlyOwner {
-        require(
+        _setQuorumThreshold(_quorumThreshold);
-            _quorumThreshold > 0 && _quorumThreshold <= validators.length(),
+    }
-            "!range"
+
-        );
+    /**
-        quorumThreshold = _quorumThreshold;
+     * @notice Gets the addresses of the current validator set.
-        emit SetQuorumThreshold(_quorumThreshold);
+     * @dev There is no ordering guarantee.
+     * @return The addresses of the validator set.
+     */
+    function validatorSet() external view returns (address[] memory) {
+        uint256 _length = validators.length();
+        address[] memory _validatorSet = new address[](_length);
+        for (uint256 i = 0; i < _length; i++) {
+            _validatorSet[i] = validators.at(i);
+        }
+        return _validatorSet;
     }
 
     // ============ Public Functions ============
@@ -130,7 +150,7 @@ abstract contract MultisigValidatorManager is Ownable {
         address _previousSigner = address(0);
         uint256 _validatorSignatureCount = 0;
         for (uint256 i = 0; i < _signaturesLength; i++) {
-            address _signer = recoverCheckpointSigner(
+            address _signer = _recoverCheckpointSigner(
                 _root,
                 _index,
                 _signatures[i]
@@ -155,7 +175,7 @@ abstract contract MultisigValidatorManager is Ownable {
      * @param _signature Signature on the the checkpoint.
      * @return The signer of the checkpoint signature.
      **/
-    function recoverCheckpointSigner(
+    function _recoverCheckpointSigner(
         bytes32 _root,
         uint256 _index,
         bytes calldata _signature
@@ -166,4 +186,37 @@ abstract contract MultisigValidatorManager is Ownable {
         _digest = ECDSA.toEthSignedMessageHash(_digest);
         return ECDSA.recover(_digest, _signature);
     }
+
+    /**
+     * @notice Enrolls a validator into the validator set.
+     * @dev Reverts if _validator is already in the validator set.
+     * @param _validator The validator to add to the validator set.
+     */
+    function _enrollValidator(address _validator) internal {
+        require(validators.add(_validator), "enrolled");
+        emit EnrollValidator(_validator);
+    }
+
+    /**
+     * @notice Unenrolls a validator from the validator set.
+     * @dev Reverts if _validator is not in the validator set.
+     * @param _validator The validator to remove from the validator set.
+     */
+    function _unenrollValidator(address _validator) internal {
+        require(validators.remove(_validator), "!enrolled");
+        emit UnenrollValidator(_validator);
+    }
+
+    /**
+     * @notice Sets the quorum threshold.
+     * @param _quorumThreshold The new quorum threshold.
+     */
+    function _setQuorumThreshold(uint256 _quorumThreshold) internal {
+        require(
+            _quorumThreshold > 0 && _quorumThreshold <= validators.length(),
+            "!range"
+        );
+        quorumThreshold = _quorumThreshold;
+        emit SetQuorumThreshold(_quorumThreshold);
+    }
 }

solidity/core/contracts/validator-manager/OutboxMultisigValidatorManager.sol

@@ -28,7 +28,11 @@ contract OutboxMultisigValidatorManager is MultisigValidatorManager {
      * @param _localDomain The local domain.
      */
     // solhint-disable-next-line no-empty-blocks
-    constructor(uint32 _localDomain) MultisigValidatorManager(_localDomain) {}
+    constructor(
+        uint32 _localDomain,
+        address[] memory _validatorSet,
+        uint256 _quorumThreshold
+    ) MultisigValidatorManager(_localDomain, _validatorSet, _quorumThreshold) {}
 
     // ============ External Functions ============
 

solidity/core/test/inbox.test.ts

@@ -21,7 +21,7 @@ const proveAndProcessTestCases = require('../../../vectors/proveAndProcess.json'
 const localDomain = 2000;
 const remoteDomain = 1000;
 
-describe.only('Inbox', async () => {
+describe('Inbox', async () => {
   const badRecipientFactories = [
     BadRecipient1__factory,
     BadRecipient3__factory,
@@ -39,7 +39,9 @@ describe.only('Inbox', async () => {
     // Inbox.initialize will ensure the validator manager is a contract.
     // TestValidatorManager doesn't have any special logic, it just submits
     // checkpoints without any signature verification.
-    const testValidatorManagerFactory = new TestValidatorManager__factory(signer);
+    const testValidatorManagerFactory = new TestValidatorManager__factory(
+      signer,
+    );
     validatorManager = await testValidatorManagerFactory.deploy();
   });
 

solidity/core/test/validator-manager/multisigValidatorManager.test.ts

@@ -0,0 +1,265 @@
+import { ethers } from 'hardhat';
+import { expect } from 'chai';
+import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers';
+import { types } from '@abacus-network/utils';
+
+import {
+  TestMultisigValidatorManager,
+  TestMultisigValidatorManager__factory,
+} from '../../types';
+import { Validator } from '../lib/core';
+
+const OUTBOX_DOMAIN = 1234;
+const OUTBOX_DOMAIN_HASH = ethers.utils.keccak256(
+  ethers.utils.solidityPack(['uint32', 'string'], [OUTBOX_DOMAIN, 'ABACUS']),
+);
+const QUORUM_THRESHOLD = 1;
+
+// const VALIDATOR = '0xdeadbeef00000000000000000000000000000000';
+
+// Signs a checkpoint with the provided validators,
+// sorts
+async function getCheckpointSignatures(
+  root: types.HexString,
+  index: number,
+  unsortedValidators: Validator[],
+): Promise<string[]> {
+  const validators = unsortedValidators.sort((a, b) => {
+    const aAddress = a.address.toLowerCase();
+    const bAddress = b.address.toLowerCase();
+
+    if (aAddress < bAddress) {
+      return -1;
+    } else if (aAddress > bAddress) {
+      return 1;
+    } else {
+      return 0;
+    }
+  });
+
+  const signedCheckpoints = await Promise.all(
+    validators.map((validator) => validator.signCheckpoint(root, index)),
+  );
+  return signedCheckpoints.map(
+    (signedCheckpoint) => signedCheckpoint.signature,
+  );
+}
+
+describe.only('MultisigValidatorManager', async () => {
+  let validatorManager: TestMultisigValidatorManager,
+    signer: SignerWithAddress,
+    nonOwner: SignerWithAddress,
+    validator0: Validator,
+    validator1: Validator,
+    validator2: Validator,
+    validator3: Validator;
+
+  before(async () => {
+    const signers = await ethers.getSigners();
+    [signer, nonOwner] = signers;
+    const [
+      ,
+      ,
+      validatorSigner0,
+      validatorSigner1,
+      validatorSigner2,
+      validatorSigner3,
+    ] = signers;
+    validator0 = await Validator.fromSigner(validatorSigner0, OUTBOX_DOMAIN);
+    validator1 = await Validator.fromSigner(validatorSigner1, OUTBOX_DOMAIN);
+    validator2 = await Validator.fromSigner(validatorSigner2, OUTBOX_DOMAIN);
+    validator3 = await Validator.fromSigner(validatorSigner3, OUTBOX_DOMAIN);
+  });
+
+  beforeEach(async () => {
+    const validatorManagerFactory = new TestMultisigValidatorManager__factory(
+      signer,
+    );
+    validatorManager = await validatorManagerFactory.deploy(
+      OUTBOX_DOMAIN,
+      [validator0.address],
+      QUORUM_THRESHOLD,
+    );
+  });
+
+  describe('#constructor', () => {
+    it('sets the outboxDomain', async () => {
+      expect(await validatorManager.outboxDomain()).to.equal(OUTBOX_DOMAIN);
+    });
+
+    it('sets the outboxDomainHash', async () => {
+      expect(await validatorManager.outboxDomainHash()).to.equal(
+        OUTBOX_DOMAIN_HASH,
+      );
+    });
+
+    it('enrolls the validator set', async () => {
+      expect(await validatorManager.validatorSet()).to.deep.equal([
+        validator0.address,
+      ]);
+    });
+
+    it('sets the quorum threshold', async () => {
+      expect(await validatorManager.quorumThreshold()).to.equal([
+        QUORUM_THRESHOLD,
+      ]);
+    });
+  });
+
+  describe('#enrollValidator', () => {
+    it('enrolls a validator into the validator set', async () => {
+      await validatorManager.enrollValidator(validator1.address);
+
+      expect(await validatorManager.validatorSet()).to.deep.equal([
+        validator0.address,
+        validator1.address,
+      ]);
+    });
+
+    it('emits the EnrollValidator event', async () => {
+      expect(await validatorManager.enrollValidator(validator1.address))
+        .to.emit(validatorManager, 'EnrollValidator')
+        .withArgs(validator1.address);
+    });
+
+    it('reverts if the validator is already enrolled', async () => {
+      await expect(
+        validatorManager.enrollValidator(validator0.address),
+      ).to.be.revertedWith('enrolled');
+    });
+
+    it('reverts when called by a non-owner', async () => {
+      await expect(
+        validatorManager.connect(nonOwner).enrollValidator(validator1.address),
+      ).to.be.revertedWith('Ownable: caller is not the owner');
+    });
+  });
+
+  describe('#unenrollValidator', () => {
+    it('unenrolls a validator from the validator set', async () => {
+      await validatorManager.unenrollValidator(validator0.address);
+
+      expect(await validatorManager.validatorSet()).to.deep.equal([]);
+    });
+
+    it('emits the UnenrollValidator event', async () => {
+      expect(await validatorManager.unenrollValidator(validator0.address))
+        .to.emit(validatorManager, 'UnenrollValidator')
+        .withArgs(validator0.address);
+    });
+
+    it('reverts if the validator is not already enrolled', async () => {
+      await expect(
+        validatorManager.unenrollValidator(validator1.address),
+      ).to.be.revertedWith('!enrolled');
+    });
+
+    it('reverts when called by a non-owner', async () => {
+      await expect(
+        validatorManager
+          .connect(nonOwner)
+          .unenrollValidator(validator0.address),
+      ).to.be.revertedWith('Ownable: caller is not the owner');
+    });
+  });
+
+  describe('#setQuorumThreshold', () => {
+    beforeEach(async () => {
+      // Have 2 validators to allow us to have more than 1 valid
+      // quorum threshold
+      await validatorManager.enrollValidator(validator1.address);
+    });
+
+    it('sets the quorum threshold', async () => {
+      await validatorManager.setQuorumThreshold(2);
+
+      expect(await validatorManager.quorumThreshold()).to.equal(2);
+    });
+
+    it('emits the SetQuorumThreshold event', async () => {
+      expect(await validatorManager.setQuorumThreshold(2))
+        .to.emit(validatorManager, 'SetQuorumThreshold')
+        .withArgs(2);
+    });
+
+    it('reverts if the new quorum threshold is zero', async () => {
+      await expect(validatorManager.setQuorumThreshold(0)).to.be.revertedWith(
+        '!range',
+      );
+    });
+
+    it('reverts if the new quorum threshold is > the validator set size', async () => {
+      await expect(validatorManager.setQuorumThreshold(3)).to.be.revertedWith(
+        '!range',
+      );
+    });
+
+    it('reverts when called by a non-owner', async () => {
+      await expect(
+        validatorManager.connect(nonOwner).setQuorumThreshold(2),
+      ).to.be.revertedWith('Ownable: caller is not the owner');
+    });
+  });
+
+  describe('#isQuorum', () => {
+    const root = ethers.utils.formatBytes32String('test root');
+    const index = 1;
+
+    beforeEach(async () => {
+      // Have 3 validators and a quorum of 2
+      await validatorManager.enrollValidator(validator1.address);
+      await validatorManager.enrollValidator(validator2.address);
+
+      await validatorManager.setQuorumThreshold(2);
+    });
+
+    it('returns true when there is a quorum', async () => {
+      const signatures = await getCheckpointSignatures(root, index, [
+        validator0,
+        validator1,
+      ]);
+      expect(await validatorManager.isQuorum(root, index, signatures)).to.be
+        .true;
+    });
+
+    it('returns true when a quorum exists even if provided with non-validator signatures', async () => {
+      const signatures = await getCheckpointSignatures(
+        root,
+        index,
+        [validator0, validator1, validator3], // validator 3 is not enrolled
+      );
+      expect(await validatorManager.isQuorum(root, index, signatures)).to.be
+        .true;
+    });
+
+    it('returns false when the signature count is < the quorum threshold', async () => {
+      const signatures = await getCheckpointSignatures(root, index, [
+        validator0,
+      ]);
+      expect(await validatorManager.isQuorum(root, index, signatures)).to.be
+        .false;
+    });
+
+    it('returns false when some signatures are not from enrolled validators', async () => {
+      const signatures = await getCheckpointSignatures(
+        root,
+        index,
+        [validator0, validator3], // validator 3 is not enrolled
+      );
+      expect(await validatorManager.isQuorum(root, index, signatures)).to.be
+        .false;
+    });
+
+    it('reverts when signatures are not ordered by their signer', async () => {
+      // Reverse the signature order, purposely messing up the
+      // ascending sort
+      const signatures = (
+        await getCheckpointSignatures(root, index, [validator0, validator1])
+      ).reverse();
+
+      await expect(
+        validatorManager.isQuorum(root, index, signatures),
+      ).to.be.revertedWith('!sorted signers');
+    });
+  });
+});