self nits

solidity/core/contracts/Common.sol

@@ -51,7 +51,7 @@ abstract contract Common is ICommon, OwnableUpgradeable {
     // ============ Modifiers ============
 
     /**
-     * @notice Ensures that function is called by the ValidatorManager contract
+     * @notice Ensures that a function is called by the ValidatorManager contract.
      */
     modifier onlyValidatorManager() {
         require(msg.sender == validatorManager, "!validatorManager");

solidity/core/contracts/Inbox.sol

@@ -82,7 +82,7 @@ contract Inbox is IInbox, Version0, Common {
 
     /**
      * @notice Checkpoints the provided root and index.
-     * @dev Called by the ValidatorManager, which is responsible for verifying
+     * @dev Called by the ValidatorManager, which is responsible for verifying a
      * quorum of validator signatures on the checkpoint.
      * @dev Reverts if checkpoints's index is not greater than our latest index.
      * @param _root Checkpoint's merkle root.

solidity/core/contracts/test/TestValidatorManager.sol

@@ -1,11 +1,15 @@
 // SPDX-License-Identifier: MIT OR Apache-2.0
 pragma solidity >=0.6.11;
 
-import {Inbox} from "../Inbox.sol";
+import {IInbox} from "../../interfaces/IInbox.sol";
 
+/**
+ * Intended for testing Inbox.sol, which requires its validator manager
+ * to be a contract.
+ */
 contract TestValidatorManager {
     function checkpoint(
-        Inbox _inbox,
+        IInbox _inbox,
         bytes32 _root,
         uint256 _index
     ) external {

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

@@ -6,6 +6,11 @@ pragma abicoder v2;
 import {IInbox} from "../../interfaces/IInbox.sol";
 import {MultisigValidatorManager} from "./MultisigValidatorManager.sol";
 
+/**
+ * @title InboxMultisigValidatorManager
+ * @notice Verifies checkpoints are signed by a quorum of validators and submits
+ * them to an Inbox.
+ */
 contract InboxMultisigValidatorManager is MultisigValidatorManager {
     // ============ Constructor ============
 

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

@@ -8,8 +8,9 @@ import {ECDSA} from "@openzeppelin/contracts/cryptography/ECDSA.sol";
 import {EnumerableSet} from "@openzeppelin/contracts/utils/EnumerableSet.sol";
 
 /**
- * @notice Manages an ownable validator set that sign checkpoints with
- * a basic ECDSA multi-signature.
+ * @title MultisigValidatorManager
+ * @notice Manages an ownable set of validators that ECDSA sign checkpoints to
+ * reach a quorum.
  */
 abstract contract MultisigValidatorManager is Ownable {
     // ============ Libraries ============
@@ -41,22 +42,24 @@ abstract contract MultisigValidatorManager is Ownable {
     event EnrollValidator(address indexed validator);
 
     /**
-     * @notice Emitted when a validator is unenrolled in the validator set.
+     * @notice Emitted when a validator is unenrolled from the validator set.
      * @param validator The address of the validator.
      */
     event UnenrollValidator(address indexed validator);
 
     /**
      * @notice Emitted when the quorum threshold is set.
-     * @param quorumThreshold The quorum threshold.
+     * @param quorumThreshold The new quorum threshold.
      */
     event SetQuorumThreshold(uint256 quorumThreshold);
 
     // ============ Constructor ============
 
     /**
-     * @param _outboxDomain The domain of the outbox this validator manager
-     * tracks the validator set for.
+     * @param _outboxDomain The domain of the outbox the validator set is for.
+     * @param _validatorSet The set of validator addresses.
+     * @param _quorumThreshold The quorum threshold. Must be greater than or equal
+     * to the length of _validatorSet.
      */
     constructor(
         uint32 _outboxDomain,
@@ -106,7 +109,7 @@ abstract contract MultisigValidatorManager is Ownable {
 
     /**
      * @notice Gets the addresses of the current validator set.
-     * @dev There is no ordering guarantee.
+     * @dev There ordering guarantee due to the semantics of EnumerableSet.AddressSet.
      * @return The addresses of the validator set.
      */
     function validatorSet() external view returns (address[] memory) {
@@ -207,6 +210,8 @@ abstract contract MultisigValidatorManager is Ownable {
 
     /**
      * @notice Unenrolls a validator from the validator set.
+     * @dev Reverts if the resulting validator set length is less than
+     * the quorum threshold.
      * @dev Reverts if _validator is not in the validator set.
      * @param _validator The validator to remove from the validator set.
      */

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

@@ -6,14 +6,22 @@ pragma abicoder v2;
 import {IOutbox} from "../../interfaces/IOutbox.sol";
 import {MultisigValidatorManager} from "./MultisigValidatorManager.sol";
 
+/**
+ * @title OutboxMultisigValidatorManager
+ * @notice Verifies if an improper checkpoint has been signed by a quorum of
+ * validators and reports it to an Outbox.
+ */
 contract OutboxMultisigValidatorManager is MultisigValidatorManager {
     // ============ Events ============
 
     /**
      * @notice Emitted when proof of an improper checkpoint is submitted.
+     * @dev Observers of this event should filter by the outbox address.
+     * @param outbox The outbox.
      * @param root Root of the improper checkpoint.
      * @param index Index of the improper checkpoint.
      * @param signatures A quorum of signatures on the improper checkpoint.
+     * May include non-validator signatures.
      */
     event ImproperCheckpoint(
         address indexed outbox,
@@ -25,7 +33,10 @@ contract OutboxMultisigValidatorManager is MultisigValidatorManager {
     // ============ Constructor ============
 
     /**
-     * @param _localDomain The local domain.
+     * @param _outboxDomain The local domain.
+     * @param _validatorSet The set of validator addresses.
+     * @param _quorumThreshold The quorum threshold. Must be greater than or equal
+     * to the length of _validatorSet.
      */
     // solhint-disable-next-line no-empty-blocks
     constructor(
@@ -36,16 +47,17 @@ contract OutboxMultisigValidatorManager is MultisigValidatorManager {
 
     // ============ External Functions ============
 
-    // Determines if a quorum of signers have signed an improper checkpoint,
-    // and fails the Outbox if so.
-    // If staking / slashing existed, we'd want to check this for individual validator
-    // signatures. Because we don't care about that and we don't want a single byzantine
-    // validator to be able to fail the outbox, we require a quorum.
-    //
-    // Gets the domain from IOutbox(_outbox).localDomain(), then
-    // requires isQuorum(domain, _root, _index, _signatures),
-    // requires that the checkpoint is an improper checkpoint,
-    // and calls IOutbox(_outbox).fail(). (Similar behavior as existing improperCheckpoint)
+    /**
+     * @notice Determines if a quorum of validators have signed an improper checkpoint,
+     * failing the Outbox if so.
+     * @dev Improper checkpoints signed by individual validators are not handled to prevent
+     * a single byzantine validator from failing the Outbox.
+     * @param _outbox The outbox.
+     * @param _root The merkle root of the checkpoint.
+     * @param _index The index of the checkpoint.
+     * @param _signatures Signatures over the checkpoint to be checked for a validator
+     * quorum. Must be sorted in ascending order by signer address.
+     */
     function improperCheckpoint(
         IOutbox _outbox,
         bytes32 _root,

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

@@ -8,14 +8,14 @@ import {
   InboxMultisigValidatorManager,
   InboxMultisigValidatorManager__factory,
 } from '../../types';
-import { getCheckpointSignatures } from './utils';
+import { signCheckpoint } from './utils';
 import { expect } from 'chai';
 
 const OUTBOX_DOMAIN = 1234;
 const INBOX_DOMAIN = 4321;
 const QUORUM_THRESHOLD = 2;
 
-describe.only('InboxMultisigValidatorManager', () => {
+describe('InboxMultisigValidatorManager', () => {
   let validatorManager: InboxMultisigValidatorManager,
     inbox: Inbox,
     signer: SignerWithAddress,
@@ -55,7 +55,7 @@ describe.only('InboxMultisigValidatorManager', () => {
     const index = 1;
 
     it('submits a checkpoint to the Inbox if there is a quorum', async () => {
-      const signatures = await getCheckpointSignatures(
+      const signatures = await signCheckpoint(
         root,
         index,
         [validator0, validator1], // 2/2 signers, making a quorum
@@ -67,7 +67,7 @@ describe.only('InboxMultisigValidatorManager', () => {
     });
 
     it('reverts if there is not a quorum', async () => {
-      const signatures = await getCheckpointSignatures(
+      const signatures = await signCheckpoint(
         root,
         index,
         [validator0], // 1/2 signers is not a quorum

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

@@ -7,12 +7,9 @@ import {
   TestMultisigValidatorManager,
   TestMultisigValidatorManager__factory,
 } from '../../types';
-import { getCheckpointSignatures } from './utils';
+import { signCheckpoint } from './utils';
 
 const OUTBOX_DOMAIN = 1234;
-const OUTBOX_DOMAIN_HASH = ethers.utils.keccak256(
-  ethers.utils.solidityPack(['uint32', 'string'], [OUTBOX_DOMAIN, 'ABACUS']),
-);
 const QUORUM_THRESHOLD = 1;
 
 const domainHashTestCases = require('../../../../vectors/domainHash.json');
@@ -60,8 +57,9 @@ describe('MultisigValidatorManager', async () => {
     });
 
     it('sets the outboxDomainHash', async () => {
+      const outboxDomainHash = await validatorManager.domainHash(OUTBOX_DOMAIN);
       expect(await validatorManager.outboxDomainHash()).to.equal(
-        OUTBOX_DOMAIN_HASH,
+        outboxDomainHash,
       );
     });
 
@@ -175,7 +173,7 @@ describe('MultisigValidatorManager', async () => {
       );
     });
 
-    it('reverts if the new quorum threshold is > the validator set size', async () => {
+    it('reverts if the new quorum threshold is greater than the validator set size', async () => {
       await expect(validatorManager.setQuorumThreshold(3)).to.be.revertedWith(
         '!range',
       );
@@ -201,7 +199,7 @@ describe('MultisigValidatorManager', async () => {
     });
 
     it('returns true when there is a quorum', async () => {
-      const signatures = await getCheckpointSignatures(root, index, [
+      const signatures = await signCheckpoint(root, index, [
         validator0,
         validator1,
       ]);
@@ -210,7 +208,7 @@ describe('MultisigValidatorManager', async () => {
     });
 
     it('returns true when a quorum exists even if provided with non-validator signatures', async () => {
-      const signatures = await getCheckpointSignatures(
+      const signatures = await signCheckpoint(
         root,
         index,
         [validator0, validator1, validator3], // validator 3 is not enrolled
@@ -219,8 +217,8 @@ describe('MultisigValidatorManager', async () => {
         .true;
     });
 
-    it('returns false when the signature count is < the quorum threshold', async () => {
-      const signatures = await getCheckpointSignatures(root, index, [
+    it('returns false when the signature count is less than the quorum threshold', async () => {
+      const signatures = await signCheckpoint(root, index, [
         validator0,
       ]);
       expect(await validatorManager.isQuorum(root, index, signatures)).to.be
@@ -228,7 +226,7 @@ describe('MultisigValidatorManager', async () => {
     });
 
     it('returns false when some signatures are not from enrolled validators', async () => {
-      const signatures = await getCheckpointSignatures(
+      const signatures = await signCheckpoint(
         root,
         index,
         [validator0, validator3], // validator 3 is not enrolled
@@ -241,7 +239,7 @@ describe('MultisigValidatorManager', async () => {
       // Reverse the signature order, purposely messing up the
       // ascending sort
       const signatures = (
-        await getCheckpointSignatures(root, index, [validator0, validator1])
+        await signCheckpoint(root, index, [validator0, validator1])
       ).reverse();
 
       await expect(

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

@@ -9,7 +9,7 @@ import {
   OutboxMultisigValidatorManager,
   OutboxMultisigValidatorManager__factory,
 } from '../../types';
-import { getCheckpointSignatures } from './utils';
+import { signCheckpoint } from './utils';
 
 const OUTBOX_DOMAIN = 1234;
 const INBOX_DOMAIN = 4321;
@@ -50,7 +50,7 @@ describe('OutboxMultisigValidatorManager', () => {
     const index = 1;
 
     it('accepts an improper checkpoint if there is a quorum', async () => {
-      const signatures = await getCheckpointSignatures(
+      const signatures = await signCheckpoint(
         root,
         index,
         [validator0, validator1], // 2/2 signers, making a quorum
@@ -71,7 +71,7 @@ describe('OutboxMultisigValidatorManager', () => {
     });
 
     it('reverts if there is not a quorum', async () => {
-      const signatures = await getCheckpointSignatures(
+      const signatures = await signCheckpoint(
         root,
         index,
         [validator0], // 1/2 signers is not a quorum
@@ -97,7 +97,7 @@ describe('OutboxMultisigValidatorManager', () => {
       await outbox.checkpoint();
       const [root, index] = await outbox.latestCheckpoint();
 
-      const signatures = await getCheckpointSignatures(
+      const signatures = await signCheckpoint(
         root,
         index.toNumber(),
         [validator0, validator1], // 2/2 signers, making a quorum

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

@@ -2,7 +2,7 @@ import { types, Validator } from '@abacus-network/utils';
 
 // Signs a checkpoint with the provided validators and returns
 // the signatures sorted by validator addresses in ascending order
-export async function getCheckpointSignatures(
+export async function signCheckpoint(
   root: types.HexString,
   index: number,
   unsortedValidators: Validator[],

typescript/hardhat/src/TestAbacusDeploy.ts

@@ -1,5 +1,5 @@
 import { ethers } from "ethers";
-import { types } from "@abacus-network/utils";
+import { types, Validator } from "@abacus-network/utils";
 import {
   Outbox,
   Outbox__factory,
@@ -63,7 +63,7 @@ export class TestAbacusDeploy extends TestDeploy<
       await outboxMultisigValidatorManagerFactory.deploy(
         domain,
         [signerAddress],
-        1
+        1,
       );
 
     const inboxMultisigValidatorManagerFactory =
@@ -80,7 +80,7 @@ export class TestAbacusDeploy extends TestDeploy<
         await inboxMultisigValidatorManagerFactory.deploy(
           remote,
           [signerAddress],
-          1
+          1,
         );
       inboxMultisigValidatorManagers[remote] = inboxMultisigValidatorManager;
     });
@@ -112,8 +112,6 @@ export class TestAbacusDeploy extends TestDeploy<
 
     const inboxFactory = new TestInbox__factory(signer);
     const inboxes: Record<types.Domain, TestInbox> = {};
-    // this.remotes reads this.instances which has not yet been set.
-    // const remotes = Object.keys(this.config.signer).map((d) => parseInt(d));
     const inboxDeploys = remotes.map(async (remote) => {
       const inboxMultisigValidatorManager =
         inboxMultisigValidatorManagers[remote];
@@ -217,23 +215,27 @@ export class TestAbacusDeploy extends TestDeploy<
     ) {
       return;
     }
-    // TODO come back to this
-
     // Update the Outbox and Inboxes to the latest roots.
     // This is technically not necessary given that we are not proving against
     // a root in the TestInbox.
-    // const validator = await Validator.fromSigner(
-    //   this.config.signer[domain],
-    //   domain
-    // );
-    // const { signature } = await validator.signCheckpoint(
-    //   root,
-    //   index.toNumber()
-    // );
+    const validator = await Validator.fromSigner(
+      this.config.signer[domain],
+      domain
+    );
+    const { signature } = await validator.signCheckpoint(
+      root,
+      index.toNumber()
+    );
 
     for (const remote of this.remotes(domain)) {
       const inbox = this.inbox(remote, domain);
-      await inbox.checkpoint(root, index /*, signature*/);
+      const inboxMultisigValidatorManager = this.inboxMultisigValidatorManager(remote, domain);
+      await inboxMultisigValidatorManager.checkpoint(
+        inbox.address,
+        root,
+        index,
+        [signature],
+      );
     }
 
     // Find all messages dispatched on the outbox since the previous checkpoint.

typescript/infra/src/core/deploy.ts

@@ -1,4 +1,3 @@
-// @ts-nocheck
 import path from 'path';
 import { ethers } from 'ethers';
 import { types } from '@abacus-network/utils';
@@ -30,18 +29,6 @@ export class AbacusCoreDeployer extends AbacusAppDeployer<
   CoreContractAddresses,
   CoreConfig
 > {
-  multisigValidatorManagerConfig(
-    config: CoreConfig,
-    domain: types.Domain,
-  ): MultisigValidatorManagerConfig {
-    const domainName = this.mustResolveDomainName(domain);
-    const validatorManagerConfig = config.multisigValidatorManagers[domainName];
-    if (!validatorManagerConfig) {
-      throw new Error(`No validator manager config for ${domainName}`);
-    }
-    return validatorManagerConfig;
-  }
-
   async deployContracts(
     domain: types.Domain,
     config: CoreConfig,
@@ -67,16 +54,6 @@ export class AbacusCoreDeployer extends AbacusAppDeployer<
         outboxMultisigValidatorManagerConfig.quorumThreshold,
       );
 
-    // for (const name of this.domainNames) {
-    //   const validator = config.validators[name];
-    //   if (!validator) throw new Error(`No validator for ${name}`);
-    //   await validatorManager.enrollValidator(
-    //     this.resolveDomain(name),
-    //     validator,
-    //     overrides,
-    //   );
-    // }
-
     const outbox = await this.deployProxiedContract(
       domain,
       'Outbox',
@@ -249,15 +226,28 @@ export class AbacusCoreDeployer extends AbacusAppDeployer<
   ): Promise<ethers.ContractReceipt> {
     const contracts = core.mustGetContracts(domain);
     const overrides = core.getOverrides(domain);
-    await contracts.validatorManager.transferOwnership(owner, overrides);
+    await contracts.outboxMultisigValidatorManager.transferOwnership(owner, overrides);
     await contracts.xAppConnectionManager.transferOwnership(owner, overrides);
     await contracts.upgradeBeaconController.transferOwnership(owner, overrides);
     for (const chain of Object.keys(
       contracts.addresses.inboxes,
     ) as ChainName[]) {
+      await contracts.inboxMultisigValidatorManager(chain).transferOwnership(owner, overrides);
       await contracts.inbox(chain).transferOwnership(owner, overrides);
     }
     const tx = await contracts.outbox.transferOwnership(owner, overrides);
     return tx.wait(core.getConfirmations(domain));
   }
+
+  multisigValidatorManagerConfig(
+    config: CoreConfig,
+    domain: types.Domain,
+  ): MultisigValidatorManagerConfig {
+    const domainName = this.mustResolveDomainName(domain);
+    const validatorManagerConfig = config.multisigValidatorManagers[domainName];
+    if (!validatorManagerConfig) {
+      throw new Error(`No validator manager config for ${domainName}`);
+    }
+    return validatorManagerConfig;
+  }
 }