Repository: Synthetixio/synthetix-v3 (ts)

Issue:
Remove v2x type Solidity grouping comments

Pull request:
Remove v2x type comments
Fix #139

Patch:
diff --git a/packages/core-js/utils/ast/comparator.js b/packages/core-js/utils/ast/comparator.js
new file mode 100644
index 00000000..30ebd1cc
--- /dev/null
+++ b/packages/core-js/utils/ast/comparator.js
@@ -0,0 +1,84 @@
+function compareStorageStructs({ previousStructsMap, currentStructsMap }) {
+  const appends = [];
+  const modifications = [];
+  const removals = [];
+  for (const previousStruct of previousStructsMap) {
+    const currentStruct = findStructInList(previousStruct, currentStructsMap);
+    if (!currentStruct) {
+      removals.push({
+        completeStruct: true,
+        contract: previousStruct.contract.name,
+        struct: previousStruct.struct.name,
+      });
+    }
+  }
+  for (const currentStruct of currentStructsMap) {
+    const previousStruct = findStructInList(currentStruct, previousStructsMap);
+    if (!previousStruct) {
+      appends.push({
+        completeStruct: true,
+        contract: currentStruct.contract.name,
+        struct: currentStruct.struct.name,
+      });
+      continue;
+    }
+    const {
+      appends: mAppends,
+      modifications: mModifications,
+      removals: mRemovals,
+    } = getMemberUpdates(previousStruct, currentStruct);
+    appends.push(...mAppends);
+    modifications.push(...mModifications);
+    removals.push(...mRemovals);
+  }
+  return { appends, modifications, removals };
+}
+
+function findStructInList(element, listOfElements) {
+  return listOfElements.find(
+    (v) => v.contract.name === element.contract.name && v.struct.name === element.struct.name
+  );
+}
+
+function getMemberUpdates(previousStruct, currentStruct) {
+  const appends = [];
+  const modifications = [];
+  const removals = [];
+  const longest =
+    currentStruct.struct.members.length > previousStruct.struct.members.length
+      ? currentStruct.struct.members.length
+      : previousStruct.struct.members.length;
+  for (let i = 0; i < longest; i++) {
+    const current = currentStruct.struct.members[i];
+    const previous = previousStruct.struct.members[i];
+    if (!current) {
+      removals.push({
+        contract: previousStruct.contract.name,
+        struct: previousStruct.struct.name,
+        old: previous,
+      });
+      continue;
+    }
+    if (!previous) {
+      appends.push({
+        contract: currentStruct.contract.name,
+        struct: currentStruct.struct.name,
+        new: current,
+      });
+      continue;
+    }
+    if (current.name !== previous.name || current.type !== previous.type) {
+      modifications.push({
+        contract: currentStruct.contract.name,
+        struct: currentStruct.struct.name,
+        old: previous,
+        new: current,
+      });
+    }
+  }
+  return { appends, modifications, removals };
+}
+
+module.exports = {
+  compareStorageStructs,
+};
diff --git a/packages/core-js/utils/ast.js b/packages/core-js/utils/ast/finders.js
similarity index 94%
rename from packages/core-js/utils/ast.js
rename to packages/core-js/utils/ast/finders.js
index e0955231..f85edc8c 100644
--- a/packages/core-js/utils/ast.js
+++ b/packages/core-js/utils/ast/finders.js
@@ -20,6 +20,10 @@ function findContractNodeVariables(contractNode) {
   return Array.from(findAll('VariableDeclaration', contractNode));
 }
 
+function findContractNodeStructs(contractNode) {
+  return Array.from(findAll('StructDefinition', contractNode));
+}
+
 function findContractStateVariables(contractName, ast) {
   return findContractNodeVariables(getContractNode(ast)).filter((n) => n.stateVariable);
 }
@@ -91,12 +95,13 @@ function findFunctionSelectors(contractName, asts) {
 
   return selectors;
 }
-
 module.exports = {
   findYulCaseValues,
   findYulStorageSlotAssignments,
   findContractNodeWithName,
   findContractNodeWithId,
+  findContractNodeVariables,
+  findContractNodeStructs,
   findContractStateVariables,
   findContractDependencies,
   findFunctionSelectors,
diff --git a/packages/core-js/utils/ast/storage-struct.js b/packages/core-js/utils/ast/storage-struct.js
new file mode 100644
index 00000000..452b362e
--- /dev/null
+++ b/packages/core-js/utils/ast/storage-struct.js
@@ -0,0 +1,46 @@
+const {
+  findContractNodeWithName,
+  findContractNodeVariables,
+  findContractNodeStructs,
+} = require('./finders');
+
+// prettier-ignore
+function orderContractsStructMap(structs) {
+  structs.sort((a, b) =>
+    a.contract.name > b.contract.name
+      ? 1
+      : a.contract.name < b.contract.name
+        ? -1
+        : a.struct.name > b.struct.name
+          ? 1
+          : a.struct.name < b.struct.name
+            ? -1
+            : 0
+  );
+}
+
+async function buildContractsStructMap(asts) {
+  const structs = [];
+  for (var [contractName, ast] of Object.entries(asts)) {
+    const contractNode = findContractNodeWithName(contractName, ast);
+    if (!contractNode) {
+      continue;
+    }
+    for (const structDefinition of findContractNodeStructs(contractNode)) {
+      const members = [];
+      for (const member of findContractNodeVariables(structDefinition)) {
+        members.push({ name: member.name, type: member.typeDescriptions.typeString });
+      }
+      structs.push({
+        contract: { name: contractName, id: contractNode.id },
+        struct: { name: structDefinition.name, members },
+      });
+    }
+  }
+  orderContractsStructMap(structs);
+  return structs;
+}
+
+module.exports = {
+  buildContractsStructMap,
+};
diff --git a/packages/core-js/utils/clone.js b/packages/core-js/utils/clone.js
new file mode 100644
index 00000000..0a3721cb
--- /dev/null
+++ b/packages/core-js/utils/clone.js
@@ -0,0 +1,6 @@
+function clone(objectToClone) {
+  return JSON.parse(JSON.stringify(objectToClone));
+}
+module.exports = {
+  clone,
+};
diff --git a/packages/deployer/internal/process-contracts.js b/packages/deployer/internal/process-contracts.js
index 0a293bdf..6f71570b 100644
--- a/packages/deployer/internal/process-contracts.js
+++ b/packages/deployer/internal/process-contracts.js
@@ -1,7 +1,7 @@
 const fs = require('fs/promises');
 const { getContractAST } = require('@synthetixio/core-js/utils/hardhat');
 const { getBytecodeHash } = require('@synthetixio/core-js/utils/contracts');
-const { findInheritedContractNames } = require('@synthetixio/core-js/utils/ast');
+const { findInheritedContractNames } = require('@synthetixio/core-js/utils/ast/finders');
 const { contractIsModule, getContractFilePath } = require('./contract-helper');
 
 /**
diff --git a/packages/deployer/internal/router-ast-validator.js b/packages/deployer/internal/router-ast-validator.js
index 0e2f65fd..eb045c11 100644
--- a/packages/deployer/internal/router-ast-validator.js
+++ b/packages/deployer/internal/router-ast-validator.js
@@ -1,4 +1,7 @@
-const { findYulCaseValues, findFunctionSelectors } = require('@synthetixio/core-js/utils/ast');
+const {
+  findYulCaseValues,
+  findFunctionSelectors,
+} = require('@synthetixio/core-js/utils/ast/finders');
 const { getModulesSelectors } = require('./contract-helper');
 const { toPrivateConstantCase } = require('./router-helper');
 const filterValues = require('filter-values');
diff --git a/packages/deployer/internal/storage-ast-validator.js b/packages/deployer/internal/storage-ast-validator.js
index 07341e82..2bd7e11a 100644
--- a/packages/deployer/internal/storage-ast-validator.js
+++ b/packages/deployer/internal/storage-ast-validator.js
@@ -2,13 +2,15 @@ const {
   findContractDependencies,
   findYulStorageSlotAssignments,
   findContractStateVariables,
-} = require('@synthetixio/core-js/utils/ast');
-const logger = require('@synthetixio/core-js/utils/logger');
+} = require('@synthetixio/core-js/utils/ast/finders');
+const { buildContractsStructMap } = require('@synthetixio/core-js/utils/ast/storage-struct');
+const { compareStorageStructs } = require('@synthetixio/core-js/utils/ast/comparator');
 const filterValues = require('filter-values');
 
 class ModuleStorageASTValidator {
-  constructor(asts) {
+  constructor(asts, previousAsts) {
     this.asts = asts;
+    this.previousAsts = previousAsts;
   }
 
   findDuplicateNamespaces(namespaces) {
@@ -89,12 +91,42 @@ class ModuleStorageASTValidator {
     return errors;
   }
 
-  findInvalidNamespaceMutations() {
-    logger.info(
-      'Unsafe storage namespace mutations are not yet validated in modules. Please only append to storage namespace structs.'
-    );
+  async findInvalidNamespaceMutations() {
+    const errors = [];
+
+    if (!this.previousAsts) {
+      return errors;
+    }
+
+    const previousStructsMap = await buildContractsStructMap(this.previousAsts);
+    const currentStructsMap = await buildContractsStructMap(this.asts);
+
+    const { modifications, removals } = compareStorageStructs({
+      previousStructsMap,
+      currentStructsMap,
+    });
+
+    modifications.forEach((m) => {
+      if (!errors.some((e) => e.contract === m.contract && e.struct === m.struct)) {
+        errors.push({
+          msg: `Invalid mutation found in namespace ${m.contract}.${m.struct}`,
+          contract: m.contract,
+          struct: m.struct,
+        });
+      }
+    });
+
+    removals.forEach((m) => {
+      if (!errors.some((e) => e.contract === m.contract && e.struct === m.struct)) {
+        errors.push({
+          msg: `Invalid mutation found in namespace ${m.contract}.${m.struct}`,
+          contract: m.contract,
+          struct: m.struct,
+        });
+      }
+    });
 
-    return [];
+    return errors;
   }
 }
 
diff --git a/packages/deployer/subtasks/cancel-deployment.js b/packages/deployer/subtasks/cancel-deployment.js
index 8812e731..11e3aaef 100644
--- a/packages/deployer/subtasks/cancel-deployment.js
+++ b/packages/deployer/subtasks/cancel-deployment.js
@@ -19,6 +19,6 @@ subtask(
 
   await del(toDelete);
 
-  logger.info('Extiting...');
+  logger.info('Exiting...');
   process.exit(0);
 });
diff --git a/packages/deployer/subtasks/prepare-deployment.js b/packages/deployer/subtasks/prepare-deployment.js
index 3a7e0137..dae0931d 100644
--- a/packages/deployer/subtasks/prepare-deployment.js
+++ b/packages/deployer/subtasks/prepare-deployment.js
@@ -60,8 +60,10 @@ subtask(
   };
 
   if (previousFile) {
+    const { sources: previousSources } = getDeploymentExtendedFiles(previousFile);
     hre.deployer.previousDeployment = {
       general: JSON.parse(fs.readFileSync(previousFile)),
+      sources: JSON.parse(fs.readFileSync(previousSources)),
     };
   }
 });
diff --git a/packages/deployer/subtasks/validate-storage.js b/packages/deployer/subtasks/validate-storage.js
index 4c3c5db4..61149f60 100644
--- a/packages/deployer/subtasks/validate-storage.js
+++ b/packages/deployer/subtasks/validate-storage.js
@@ -1,19 +1,20 @@
 const { subtask } = require('hardhat/config');
 const logger = require('@synthetixio/core-js/utils/logger');
-const { getAllContractASTs } = require('@synthetixio/core-js/utils/hardhat');
+const { getAllContractASTs, getAllPreviousContractASTs } = require('../utils/deployments');
 const ModuleStorageASTValidator = require('../internal/storage-ast-validator');
 const { SUBTASK_VALIDATE_STORAGE, SUBTASK_CANCEL_DEPLOYMENT } = require('../task-names');
 
 subtask(SUBTASK_VALIDATE_STORAGE).setAction(async (_, hre) => {
   logger.subtitle('Validating module storage usage');
 
-  const asts = await getAllContractASTs(hre);
-  const validator = new ModuleStorageASTValidator(asts);
+  const asts = getAllContractASTs(hre);
+  const previousAsts = getAllPreviousContractASTs(hre);
+  const validator = new ModuleStorageASTValidator(asts, previousAsts);
 
   let errorsFound = [];
   errorsFound.push(...validator.findNamespaceCollisions());
   errorsFound.push(...validator.findRegularVariableDeclarations());
-  errorsFound.push(...validator.findInvalidNamespaceMutations());
+  errorsFound.push(...(await validator.findInvalidNamespaceMutations()));
 
   if (errorsFound.length > 0) {
     errorsFound.forEach((error) => {
diff --git a/packages/deployer/utils/deployments.js b/packages/deployer/utils/deployments.js
index 2dac330d..6d0bbbe3 100644
--- a/packages/deployer/utils/deployments.js
+++ b/packages/deployer/utils/deployments.js
@@ -112,6 +112,25 @@ function _populateDefaults(info) {
   return { ...DeploymentInfo, ...info };
 }
 
+function _getAllContractASTs(deploymentSources) {
+  const asts = {};
+  for (const [contractName, data] of Object.entries(deploymentSources)) {
+    asts[contractName] = data.ast;
+  }
+  return asts;
+}
+
+function getAllContractASTs(hre) {
+  return _getAllContractASTs(hre.deployer.deployment.sources);
+}
+
+function getAllPreviousContractASTs(hre) {
+  if (hre.deployer.previousDeployment) {
+    return _getAllContractASTs(hre.deployer.previousDeployment.sources);
+  }
+  return null;
+}
+
 module.exports = {
   getDeploymentExtendedFiles,
   getProxyAddress,
@@ -120,4 +139,6 @@ module.exports = {
   getDeploymentFile,
   getAllDeploymentFiles,
   getDeploymentFolder,
+  getAllContractASTs,
+  getAllPreviousContractASTs,
 };


Test patch:
diff --git a/packages/core-js/test/utils/comparator.spec.js b/packages/core-js/test/utils/comparator.spec.js
new file mode 100644
index 00000000..403998e6
--- /dev/null
+++ b/packages/core-js/test/utils/comparator.spec.js
@@ -0,0 +1,206 @@
+const { equal } = require('assert/strict');
+const { clone } = require('../../utils/clone');
+const { compareStorageStructs } = require('../../utils/ast/comparator');
+
+describe('utils/ast/comparator.js compareStorageStructs', function () {
+  const previousStructsMap = [
+    {
+      contract: {
+        name: 'GlobalNamespace',
+        id: 614,
+      },
+      struct: {
+        name: 'GlobalStorage',
+        members: [
+          {
+            name: 'value',
+            type: 'uint256',
+          },
+          {
+            name: 'someValue',
+            type: 'uint256',
+          },
+        ],
+      },
+    },
+    {
+      contract: {
+        name: 'OwnerNamespace',
+        id: 630,
+      },
+      struct: {
+        name: 'OwnerStorage',
+        members: [
+          {
+            name: 'owner',
+            type: 'address',
+          },
+          {
+            name: 'nominatedOwner',
+            type: 'address',
+          },
+        ],
+      },
+    },
+    {
+      contract: {
+        name: 'ProxyNamespace',
+        id: 644,
+      },
+      struct: {
+        name: 'ProxyStorage',
+        members: [
+          {
+            name: 'implementation',
+            type: 'address',
+          },
+        ],
+      },
+    },
+    {
+      contract: {
+        name: 'SettingsNamespace',
+        id: 658,
+      },
+      struct: {
+        name: 'SettingsStorage',
+        members: [
+          {
+            name: 'aSettingValue',
+            type: 'uint256',
+          },
+        ],
+      },
+    },
+  ];
+
+  describe('compareStorageStructs no updates', function () {
+    it('detects no changes when both structMaps are equal', function () {
+      let currentStructsMap = clone(previousStructsMap);
+      const result = compareStorageStructs({
+        previousStructsMap,
+        currentStructsMap,
+      });
+      equal(result.appends.length, 0);
+      equal(result.modifications.length, 0);
+      equal(result.removals.length, 0);
+    });
+  });
+
+  describe('compareStorageStructs with appends', function () {
+    it('detects new contract added', function () {
+      let currentStructsMap = clone(previousStructsMap);
+      currentStructsMap.push({
+        contract: {
+          name: 'AnotherNamespace',
+          id: 1,
+        },
+        struct: {
+          name: 'AnotherStorage',
+          members: [
+            {
+              name: 'value',
+              type: 'uint256',
+            },
+            {
+              name: 'someValue',
+              type: 'uint256',
+            },
+          ],
+        },
+      });
+      const result = compareStorageStructs({
+        previousStructsMap,
+        currentStructsMap,
+      });
+      equal(result.appends.length, 1);
+      equal(result.appends[0].completeStruct, true);
+      equal(result.modifications.length, 0);
+      equal(result.removals.length, 0);
+    });
+
+    it('detects new member appended', function () {
+      let currentStructsMap = clone(previousStructsMap);
+      currentStructsMap[0].struct.members.push({
+        name: 'newValue',
+        type: 'uint256',
+      });
+      const result = compareStorageStructs({
+        previousStructsMap,
+        currentStructsMap,
+      });
+      equal(result.appends.length, 1);
+      equal(result.modifications.length, 0);
+      equal(result.removals.length, 0);
+    });
+
+    it('detects new member added before the last one', function () {
+      let currentStructsMap = clone(previousStructsMap);
+      currentStructsMap[0].struct.members.unshift({
+        name: 'newValue',
+        type: 'uint256',
+      });
+      const result = compareStorageStructs({
+        previousStructsMap,
+        currentStructsMap,
+      });
+      equal(result.appends.length, 1);
+      // two since two members changed the index
+      equal(result.modifications.length, 2);
+      equal(result.removals.length, 0);
+    });
+  });
+
+  describe('compareStorageStructs with removals', function () {
+    it('detects whole contract removed', function () {
+      let currentStructsMap = clone(previousStructsMap);
+      currentStructsMap.pop();
+      const result = compareStorageStructs({
+        previousStructsMap,
+        currentStructsMap,
+      });
+      equal(result.appends.length, 0);
+      equal(result.modifications.length, 0);
+      equal(result.removals.length, 1);
+    });
+
+    it('detects member removed', function () {
+      let currentStructsMap = clone(previousStructsMap);
+      currentStructsMap[0].struct.members.pop();
+      const result = compareStorageStructs({
+        previousStructsMap,
+        currentStructsMap,
+      });
+      equal(result.appends.length, 0);
+      equal(result.modifications.length, 0);
+      equal(result.removals.length, 1);
+    });
+  });
+
+  describe('compareStorageStructs with modifications', function () {
+    it('detects member name updated', function () {
+      let currentStructsMap = clone(previousStructsMap);
+      currentStructsMap[0].struct.members[0].name = 'modifiedName';
+      const result = compareStorageStructs({
+        previousStructsMap,
+        currentStructsMap,
+      });
+      equal(result.appends.length, 0);
+      equal(result.modifications.length, 1);
+      equal(result.removals.length, 0);
+    });
+
+    it('detects member type updated', function () {
+      let currentStructsMap = clone(previousStructsMap);
+      currentStructsMap[0].struct.members[0].type =
+        currentStructsMap[0].struct.members[0].type == 'uint256' ? 'address' : 'uint256';
+      const result = compareStorageStructs({
+        previousStructsMap,
+        currentStructsMap,
+      });
+      equal(result.appends.length, 0);
+      equal(result.modifications.length, 1);
+      equal(result.removals.length, 0);
+    });
+  });
+});
diff --git a/packages/deployer/test/sample-project/contracts/mixins/OwnerMixin.sol b/packages/deployer/test/sample-project/contracts/mixins/OwnerMixin.sol
index 1975ce67..2f740ce1 100644
--- a/packages/deployer/test/sample-project/contracts/mixins/OwnerMixin.sol
+++ b/packages/deployer/test/sample-project/contracts/mixins/OwnerMixin.sol
@@ -4,8 +4,6 @@ pragma solidity ^0.8.0;
 import "../storage/OwnerNamespace.sol";
 
 contract OwnerMixin is OwnerNamespace {
-    /* MODIFIERS */
-
     modifier onlyOwner() {
         require(msg.sender == _ownerStorage().owner, "Only owner allowed");
         _;
diff --git a/packages/deployer/test/sample-project/contracts/modules/OwnerModule.sol b/packages/deployer/test/sample-project/contracts/modules/OwnerModule.sol
index 1baddfee..29e14ef5 100644
--- a/packages/deployer/test/sample-project/contracts/modules/OwnerModule.sol
+++ b/packages/deployer/test/sample-project/contracts/modules/OwnerModule.sol
@@ -4,7 +4,7 @@ pragma solidity ^0.8.0;
 import "../mixins/OwnerMixin.sol";
 
 contract OwnerModule is OwnerMixin {
-    /* MUTATIVE FUNCTIONS */
+    event OwnerChanged(address newOwner);
 
     function nominateOwner(address newNominatedOwner) public onlyOwnerIfSet {
         require(newNominatedOwner != address(0), "Invalid nominated owner address");
@@ -36,8 +36,6 @@ contract OwnerModule is OwnerMixin {
         emit OwnerChanged(store.owner);
     }
 
-    /* VIEW FUNCTIONS */
-
     function getOwner() public view returns (address) {
         return _ownerStorage().owner;
     }
@@ -45,8 +43,4 @@ contract OwnerModule is OwnerMixin {
     function getNominatedOwner() public view returns (address) {
         return _ownerStorage().nominatedOwner;
     }
-
-    /* EVENTS */
-
-    event OwnerChanged(address newOwner);
 }
diff --git a/packages/deployer/test/sample-project/contracts/modules/SettingsModule.sol b/packages/deployer/test/sample-project/contracts/modules/SettingsModule.sol
index 5f3358e3..c60c0b8d 100644
--- a/packages/deployer/test/sample-project/contracts/modules/SettingsModule.sol
+++ b/packages/deployer/test/sample-project/contracts/modules/SettingsModule.sol
@@ -5,14 +5,10 @@ import "../storage/SettingsNamespace.sol";
 import "../mixins/OwnerMixin.sol";
 
 contract SettingsModule is SettingsNamespace, OwnerMixin {
-    /* MUTATIVE FUNCTIONS */
-
     function setASettingValue(uint newSettingValue) public onlyOwner {
         _settingsStorage().aSettingValue = newSettingValue;
     }
 
-    /* VIEW FUNCTIONS */
-
     function getASettingValue() public view returns (uint) {
         return _settingsStorage().aSettingValue;
     }
diff --git a/packages/deployer/test/sample-project/contracts/modules/SomeModule.sol b/packages/deployer/test/sample-project/contracts/modules/SomeModule.sol
index 289539d3..c7ef9458 100644
--- a/packages/deployer/test/sample-project/contracts/modules/SomeModule.sol
+++ b/packages/deployer/test/sample-project/contracts/modules/SomeModule.sol
@@ -4,7 +4,7 @@ pragma solidity ^0.8.0;
 import "../storage/GlobalNamespace.sol";
 
 contract SomeModule is GlobalNamespace {
-    /* MUTATIVE FUNCTIONS */
+    event ValueSet(address sender, uint value);
 
     function setValue(uint newValue) public {
         _globalStorage().value = newValue;
@@ -18,8 +18,6 @@ contract SomeModule is GlobalNamespace {
         emit ValueSet(msg.sender, newSomeValue);
     }
 
-    /* VIEW FUNCTIONS */
-
     function getValue() public view returns (uint) {
         return _globalStorage().value;
     }
@@ -27,8 +25,4 @@ contract SomeModule is GlobalNamespace {
     function getSomeValue() public view returns (uint) {
         return _globalStorage().someValue;
     }
-
-    /* EVENTS */
-
-    event ValueSet(address sender, uint value);
 }