Unit Tests - NomadBase, Replica, CI (nomad-xyz#247)

.github/workflows/tests.yml

@@ -10,12 +10,24 @@ jobs:
         uses: onbjerg/foundry-toolchain@v1
         with:
           version: nightly
-
       - run: |
           yarn install
       - run: |
           yarn lint
+      - run: |
+          yarn build:accumulator-cli
       - run: |
           yarn build
       - run: |
           yarn test
+      - name: "Static Analysis: Core"
+        uses: crytic/[email protected]
+        id: slither
+        continue-on-error: true
+        with:
+          target: "packages/contracts-core"
+      - name: "Check Gas snapshot"
+        continue-on-error: true
+        id: snapshot-check
+        run: |
+          yarn snapshot:check

.gitignore

@@ -3,6 +3,7 @@ yarn-error.log
 **/tsconfig.tsbuildinfo
 dist/
 cache/
+scripts/accumulator-cli
 
 .pnp.*
 .yarn/*

.gitmodules

@@ -0,0 +1,3 @@
+[submodule "lib/forge-std"]
+	path = lib/forge-std
+	url = https://github.com/foundry-rs/forge-std

foundry.toml

@@ -1,16 +1,35 @@
 [default]
-solc_version = '0.7.6'
-optimizer_runs = 999999
-bytecodeHash = 'none'
+solc_version = '0.7.6'        # Version of solc that we use
+remappings = [                # Libraries that we use from node_modules and are used by the smart contracts
+"@openzeppelin/=node_modules/@openzeppelin/",
+"@summa-tx/=node_modules/@summa-tx/",
+"@nomad-xyz/=packages/"
+]
+offline = true                # Disable downloading of missing solc version(s)
+optimizer = true              # Enable or disable the solc optimizer
+optimizer_runs = 999999       # The number of optimizer runs
+verbosity = 3                 # The verbosity of tests
+bytecode_hash = "none"        # For deterministic code
+block_timestamp = 1622400000  # Timestamp for tests (non-zero)
 
 [core]
 src = 'packages/contracts-core/contracts'
+test = 'packages/contracts-core/contracts/foundry-tests'
 out = 'packages/contracts-core/foundry_artifacts'
+ffi = true
+
+[core-ci]
+src = 'packages/contracts-core/contracts'
+test = 'packages/contracts-core/contracts/foundry-tests'
+out = 'packages/contracts-core/foundry_artifacts'
+fuzz-runs = 10_000
 
 [router]
 src = 'packages/contracts-router/contracts'
 out = 'packages/contracts-router/foundry_artifacts'
 
 [bridge]
 src = 'packages/contracts-bridge/contracts'
-out = 'packages/contracts-bridge/foundry_artifacts'
+out = 'packages/contracts-bridge/foundry_artifacts'
+
+

package.json

@@ -15,9 +15,10 @@
     "packages/indexer"
   ],
   "scripts": {
-    "bootstrap": "yarn workspaces foreach -Apv --exclude @nomad-xyz/local-environment --exclude @nomad-xyz/monitor run bootstrap",
-    "build": "yarn workspaces foreach -Atv  --exclude @nomad-xyz/local-environment --exclude @nomad-xyz/monitor run build",
-    "compile": "yarn workspaces foreach -Apv --exclude @nomad-xyz/local-environment --exclude @nomad-xyz/monitor run compile",
+    "compile": "yarn workspaces foreach -Apv --exclude @nomad-xyz/deploy --exclude @nomad-xyz/local-environment --exclude @nomad-xyz/monitor run compile",
+    "bootstrap": "yarn build:accumulator-cli && yarn workspaces foreach -Apv --exclude @nomad-xyz/deploy --exclude @nomad-xyz/local-environment --exclude @nomad-xyz/monitor run bootstrap",
+    "build": "yarn build:accumulator-cli && yarn workspaces foreach -Apv --exclude @nomad-xyz/deploy --exclude @nomad-xyz/local-environment --exclude @nomad-xyz/monitor run build",
+    "build:accumulator-cli": "cd scripts && ./build-acli.sh",
     "contracts-bridge": "yarn workspace @nomad-xyz/contracts-bridge",
     "contracts-core": "yarn workspace @nomad-xyz/contracts-core",
     "contracts-router": "yarn workspace @nomad-xyz/contracts-router",
@@ -31,7 +32,10 @@
     "sdk": "yarn workspace @nomad-xyz/sdk",
     "sdk-bridge": "yarn workspace @nomad-xyz/sdk-bridge",
     "sdk-govern": "yarn workspace @nomad-xyz/sdk-govern",
-    "test": "yarn workspaces foreach -Apv --exclude @nomad-xyz/local-environment run test"
+    "test": "yarn workspaces foreach -Apv --exclude @nomad-xyz/deploy  --exclude @nomad-xyz/local-environment run test",
+    "gen-proof": "scripts/accumulator-cli",
+    "snapshot": "yarn workspaces foreach -Apv --include @nomad-xyz/contracts-core run snapshot",
+    "snapshot:check": "yarn workspaces foreach -Apv --include @nomad-xyz/contracts-core run snapshot:check"
   },
   "packageManager": "[email protected]"
 }

packages/contracts-core/.env.example

@@ -1,2 +1,3 @@
 ETHERSCAN_API_KEY=
-INFURA_API_KEY=
+FOUNDRY_PROFILE=core
+

packages/contracts-core/.gas-snapshot

@@ -0,0 +1,38 @@
+HomeTest:test_committedRoot() (gas: 13674)
+HomeTest:test_dispatchRejectBigMessage() (gas: 29181)
+HomeTest:test_homeDomain() (gas: 8010)
+HomeTest:test_improperUpdate() (gas: 46103)
+HomeTest:test_nonUpdaterManagerCannotSetUpdater() (gas: 11839)
+HomeTest:test_onlyUpdaterManagerSetUpdater() (gas: 19666)
+HomeTest:test_succesfulDispatch() (gas: 216412)
+HomeTest:test_successfulDispatchAndUpdate() (gas: 228379)
+NomadBaseTest:test_acceptUpdaterSignature() (gas: 27469)
+NomadBaseTest:test_failInitializeTwice() (gas: 13452)
+NomadBaseTest:test_homeDomainHash() (gas: 10078)
+NomadBaseTest:test_ownerIsContractCreator() (gas: 7631)
+NomadBaseTest:test_rejectNonUpdaterSignature() (gas: 27434)
+NomadBaseTest:test_stateIsActiveAfterInit() (gas: 7679)
+ReplicaTest:test_acceptLeafCorrectProof() (gas: 141654)
+ReplicaTest:test_acceptReplicaUpdate() (gas: 66564)
+ReplicaTest:test_acceptableRootLegacyRejectStatus() (gas: 10263)
+ReplicaTest:test_acceptableRootLegacySuccess() (gas: 6361)
+ReplicaTest:test_acceptableRootRejectNotCommited() (gas: 7941)
+ReplicaTest:test_acceptableRootRejectNotTimedOut() (gas: 96705)
+ReplicaTest:test_acceptableRootSuccess() (gas: 9985)
+ReplicaTest:test_notProcessLegacyProvenMessageEmptyAddress() (gas: 46553)
+ReplicaTest:test_notProcessLegacyProvenMessageRevertingHandlers1() (gas: 48959)
+ReplicaTest:test_notProcessLegacyProvenMessageRevertingHandlers2() (gas: 49086)
+ReplicaTest:test_notProcessLegacyProvenMessageRevertingHandlers3() (gas: 49018)
+ReplicaTest:test_notProcessLegacyProvenMessageRevertingHandlers4() (gas: 48958)
+ReplicaTest:test_notProcessLegacyWrongDestination() (gas: 68756)
+ReplicaTest:test_notProcessUnprovenMessage() (gas: 48486)
+ReplicaTest:test_processLegacyProvenMessageReturnZeroHandler() (gas: 55453)
+ReplicaTest:test_processProvenMessage() (gas: 127782)
+ReplicaTest:test_proveAndProcess() (gas: 130139)
+ReplicaTest:test_rejectLeafWrongProof() (gas: 126638)
+ReplicaTest:test_rejectReplicaNonCurrentUpdate() (gas: 17008)
+ReplicaTest:test_rejectReplicaUpdateInvalidSig() (gas: 27613)
+ReplicaTest:test_setConfirmationOnlyOwner() (gas: 39727)
+ReplicaTest:test_setOptimisticTimeoutOnlyOwner() (gas: 18541)
+ReplicaTest:test_setUpdaterOnlyOwner() (gas: 25159)
+ReplicaTest:test_updateProveAndProcessMessage() (gas: 149999)

packages/contracts-core/.gitignore

@@ -1,4 +1,5 @@
-node_modules/
+/node_modules
+!/node_modules/
 cache/
 artifacts/
 coverage/
@@ -7,4 +8,5 @@ coverage.json
 src.ts/
 src/
 dist/
-foundry_artifacts/
+foundry_artifacts/
+scripts/accumulator-cli

packages/contracts-core/README.md

@@ -1,12 +1,44 @@
-## Nomad Core
+## Nomad Core 𓀃
 
 Solidity implementations of the core Nomad protocol.
 
 ### Setup
 
-- See repo setup
-- `brew install jq`   OR   `sudo apt-get install jq`
+- `yarn bootstrap`: `yarn clean` and `yarn build`
 
 ### Build
 
-- `yarn build`
+- `yarn build`: compile smart contracts and create definitions for the SDK
+
+### Test
+
+For testing, we use [Foundry](https://getfoundry.sh/).
+
+- Run `yarn build:accumulator-cli` from the root directory of the monorepo. It will build a rust-based cli tool that creates Sparse Merkle Tree proofs for arbitrary data. It's used in our testing suite via the `--ffi` flag for Forge. The binary is built in there `/scripts` top-level directory of the monorepo
+- `--ffi` means that Forge will run arbitrary shell commands as part of the testing suite. You should never run `forge --ffi` without knowing what exactly are the shell commands that will be executed, as the testing suite could be malicious and execute malicious commands. This is why the feature is disabled by default and must be explicitly enabled.
+- `yarn test` will run all tests. Note that `--ffi` is enabled by default,
+- `yarn snapshot --check` will run the test suite and verify that it doesn't produce a different gas snapshot from the existing (`.gas-snapshot`)
+- `yarn snapshot` will create a new `.gas-snapshot`. You can inspect the different gas usage via `git diff`
+- 'yarn snapshot:check' will run the test suite and check gas consumption against the **existing** `.gas-snapshot`. It will `pass` only if there is no change in the gas consumption
+- `yarn gen-proof` will execute the `accumulator-cli` binary
+
+### Suggested workflow
+
+- Define feature
+- Write tests based on [Foundry best practices](https://book.getfoundry.sh) and the existing test structure
+- Run test suite with `FOUNDRY_PROFILE=core forge test --ffi -vvv` and verify that your new tests `FAIL`
+- Write the new feature
+- Run again the test suite and verify that the tests `PASS`
+- Run `FOUNDRY_PROFILE=core forge snapshot` to produce the new snapshot. You can't use `forge snapshot --check`, since you added new tests that are not present in the current `.gas-snapshot`
+- Using `git diff .gas-snapshot`, you can easily verify if some change you made resulted to a gas consumption change for the tests that already existed
+
+**Tip**: It is advised to run the forge commands on their own and not via `yarn` or `npm` for faster development cycle. `yarn` will add a few seconds of lag, due to the fact that it has to spin up a `Node` runtime and the interpret the `yarn` source code.
+
+### Static Analysis
+
+The monorepo is configured to run [slither](https://github.com/crytic/slither) with every PR. We suggest all contributors to use slither while developing, to avoid common mistakes.
+
+- Install Slither
+- Run `yarn test:static-analyze`
+
+We use a `yarn command` because we need to link the top-level `node_modules` directory in the `core-contracts` package. It's a known [issue](https://github.com/crytic/slither/issues/852) for which the workaround is to link the directory.

packages/contracts-core/contracts/Home.sol

@@ -78,7 +78,7 @@ contract Home is Version0, QueueManager, MerkleTreeManager, NomadBase {
     event ImproperUpdate(bytes32 oldRoot, bytes32 newRoot, bytes signature);
 
     /**
-    * @notice Emitted when proof of a double update is submitted,
+     * @notice Emitted when proof of a double update is submitted,
      * which sets the contract to FAILED state
      * @param oldRoot Old root shared between two conflicting updates
      * @param newRoot Array containing two conflicting new roots
@@ -130,7 +130,7 @@ contract Home is Version0, QueueManager, MerkleTreeManager, NomadBase {
     }
 
     /**
-    * @notice Ensures that contract state != FAILED when the function is called
+     * @notice Ensures that contract state != FAILED when the function is called
      */
     modifier notFailed() {
         require(state != States.Failed, "failed state");
@@ -255,7 +255,7 @@ contract Home is Version0, QueueManager, MerkleTreeManager, NomadBase {
     }
 
     /**
-    * @notice Called by external agent. Checks that signatures on two sets of
+     * @notice Called by external agent. Checks that signatures on two sets of
      * roots are valid and that the new roots conflict with each other. If both
      * cases hold true, the contract is failed and a `DoubleUpdate` event is
      * emitted.

packages/contracts-core/contracts/Replica.sol

@@ -83,9 +83,7 @@ contract Replica is Version0, NomadBase {
 
     // ============ Constructor ============
 
-    constructor(
-        uint32 _localDomain
-    ) NomadBase(_localDomain) {}
+    constructor(uint32 _localDomain) NomadBase(_localDomain) {}
 
     // ============ Initializer ============
 
@@ -286,7 +284,10 @@ contract Replica is Version0, NomadBase {
     ) public returns (bool) {
         // ensure that message has not been processed
         // Note that this allows re-proving under a new root.
-        require(messages[_leaf] != LEGACY_STATUS_PROCESSED, "already processed");
+        require(
+            messages[_leaf] != LEGACY_STATUS_PROCESSED,
+            "already processed"
+        );
         // calculate the expected root based on the proof
         bytes32 _calculatedRoot = MerkleLib.branchRoot(_leaf, _proof, _index);
         // if the root is valid, change status to Proven
@@ -316,7 +317,8 @@ contract Replica is Version0, NomadBase {
         // but does not allow governance action to lower a production env below
         // the safe value
         uint256 _current = optimisticSeconds;
-        if (_current != 0 && _current > 1500) require(_optimisticSeconds >= 1500, "optimistic timeout too low");
+        if (_current != 0 && _current > 1500)
+            require(_optimisticSeconds >= 1500, "optimistic timeout too low");
         // ensure the optimistic timeout is less than 1 year
         // (prevents overflow when adding block.timestamp)
         require(_optimisticSeconds < 31536000, "optimistic timeout too high");

packages/contracts-core/contracts/UpdaterManager.sol

@@ -103,7 +103,7 @@ contract UpdaterManager is IUpdaterManager, Ownable {
     }
 
     /**
-    * @dev should be impossible to renounce ownership;
+     * @dev should be impossible to renounce ownership;
      * we override OpenZeppelin Ownable implementation
      * of renounceOwnership to make it a no-op
      */

packages/contracts-core/contracts/XAppConnectionManager.sol

@@ -213,7 +213,7 @@ contract XAppConnectionManager is Ownable {
     }
 
     /**
-    * @dev should be impossible to renounce ownership;
+     * @dev should be impossible to renounce ownership;
      * we override OpenZeppelin Ownable implementation
      * of renounceOwnership to make it a no-op
      */