fix(common): include only errors defined in the contract (#2194)

latticexyz · Jan 30, 2024 · c162ad5 · c162ad5
1 parent 570456d
commit c162ad5
Show file tree

Hide file tree

Showing 3 changed files with 66 additions and 41 deletions.
diff --git a/.changeset/nine-plants-carry.md b/.changeset/nine-plants-carry.md
@@ -0,0 +1,5 @@
+---
+"@latticexyz/common": patch
+---
+
+Prevented errors not included in the contract (but present in the file) from being included in the interface by `contractToInterface`
diff --git a/e2e/packages/contracts/src/systems/CustomErrorsSystem.sol b/e2e/packages/contracts/src/systems/CustomErrorsSystem.sol
@@ -4,6 +4,8 @@ pragma solidity >=0.8.21;
 import { System } from "@latticexyz/world/src/System.sol";
 import { Position } from "../CustomTypes.sol";
 
+error ThisErrorShouldBeAbsentFromTheGeneratedSystemInterface();
+
 contract CustomErrorsSystem is System {
   error TestError1();
   error TestError2(Position position, uint256 value, string name, bool flag);

diff --git a/packages/common/src/codegen/utils/contractToInterface.ts b/packages/common/src/codegen/utils/contractToInterface.ts
@@ -1,5 +1,10 @@
 import { parse, visit } from "@solidity-parser/parser";
-import type { SourceUnit, TypeName, VariableDeclaration } from "@solidity-parser/parser/dist/src/ast-types";
+import type {
+  ContractDefinition,
+  SourceUnit,
+  TypeName,
+  VariableDeclaration,
+} from "@solidity-parser/parser/dist/src/ast-types";
 import { MUDError } from "../../errors";
 
 export interface ContractInterfaceFunction {
@@ -36,52 +41,55 @@ export function contractToInterface(
 } {
   const ast = parse(data);
 
-  let withContract = false;
+  const contractNode = findContractNode(parse(data), contractName);
   let symbolImports: SymbolImport[] = [];
   const functions: ContractInterfaceFunction[] = [];
   const errors: ContractInterfaceError[] = [];
 
-  visit(ast, {
-    ContractDefinition({ name }) {
-      if (name === contractName) {
-        withContract = true;
-      }
-    },
-    FunctionDefinition(
-      { name, visibility, parameters, stateMutability, returnParameters, isConstructor, isFallback, isReceiveEther },
-      parent
-    ) {
-      if (parent !== undefined && parent.type === "ContractDefinition" && parent.name === contractName) {
-        try {
-          // skip constructor and fallbacks
-          if (isConstructor || isFallback || isReceiveEther) return;
-          // forbid default visibility (this check might be unnecessary, modern solidity already disallows this)
-          if (visibility === "default") throw new MUDError(`Visibility is not specified`);
-
-          if (visibility === "external" || visibility === "public") {
-            functions.push({
-              name: name === null ? "" : name,
-              parameters: parameters.map(parseParameter),
-              stateMutability: stateMutability || "",
-              returnParameters: returnParameters === null ? [] : returnParameters.map(parseParameter),
-            });
-
-            for (const { typeName } of parameters.concat(returnParameters ?? [])) {
-              const symbols = typeNameToSymbols(typeName);
-              symbolImports = symbolImports.concat(symbolsToImports(ast, symbols));
-            }
-          }
-        } catch (error: unknown) {
-          if (error instanceof MUDError) {
-            error.message = `Function "${name}" in contract "${contractName}": ${error.message}`;
+  if (!contractNode) {
+    throw new MUDError(`Contract not found: ${contractName}`);
+  }
+
+  visit(contractNode, {
+    FunctionDefinition({
+      name,
+      visibility,
+      parameters,
+      stateMutability,
+      returnParameters,
+      isConstructor,
+      isFallback,
+      isReceiveEther,
+    }) {
+      try {
+        // skip constructor and fallbacks
+        if (isConstructor || isFallback || isReceiveEther) return;
+        // forbid default visibility (this check might be unnecessary, modern solidity already disallows this)
+        if (visibility === "default") throw new MUDError(`Visibility is not specified`);
+
+        if (visibility === "external" || visibility === "public") {
+          functions.push({
+            name: name === null ? "" : name,
+            parameters: parameters.map(parseParameter),
+            stateMutability: stateMutability || "",
+            returnParameters: returnParameters === null ? [] : returnParameters.map(parseParameter),
+          });
+
+          for (const { typeName } of parameters.concat(returnParameters ?? [])) {
+            const symbols = typeNameToSymbols(typeName);
+            symbolImports = symbolImports.concat(symbolsToImports(ast, symbols));
           }
-          throw error;
         }
+      } catch (error: unknown) {
+        if (error instanceof MUDError) {
+          error.message = `Function "${name}" in contract "${contractName}": ${error.message}`;
+        }
+        throw error;
       }
     },
     CustomErrorDefinition({ name, parameters }) {
       errors.push({
-        name: name === null ? "" : name,
+        name,
         parameters: parameters.map(parseParameter),
       });
 
@@ -92,17 +100,27 @@ export function contractToInterface(
     },
   });
 
-  if (!withContract) {
-    throw new MUDError(`Contract not found: ${contractName}`);
-  }
-
   return {
     functions,
     errors,
     symbolImports,
   };
 }
 
+function findContractNode(ast: SourceUnit, contractName: string): ContractDefinition | undefined {
+  let contract = undefined;
+
+  visit(ast, {
+    ContractDefinition(node) {
+      if (node.name === contractName) {
+        contract = node;
+      }
+    },
+  });
+
+  return contract;
+}
+
 function parseParameter({ name, typeName, storageLocation }: VariableDeclaration): string {
   let typedNameWithLocation = "";