feat: logic for new functions config

defi-wonderland · Mar 4, 2024 · 406735d · 406735d
1 parent 3c14067
commit 406735d
Show file tree

Hide file tree

Showing 9 changed files with 298 additions and 52 deletions.
diff --git a/src/constants.ts b/src/constants.ts
@@ -5,5 +5,4 @@ export const defaultFunctions: Functions = {
   external: { tags: { dev: false, notice: true, return: true, param: true } },
   public: { tags: { dev: false, notice: true, return: true, param: true } },
   private: { tags: { dev: false, notice: true, return: true, param: true } },
-  constructor: false as unknown as Function & boolean,
 } as const;
diff --git a/src/main.ts b/src/main.ts
@@ -76,6 +76,11 @@ async function getConfig(configPath: string): Promise<Config> {
         description: 'If set to true, all external and public functions must have @inheritdoc.',
         default: true,
       },
+      constructorNatspec: {
+        type: 'boolean',
+        description: 'If set to true, all contracts must have a natspec for the constructor.',
+        default: false,
+      },
     })
     .parseSync();
 

diff --git a/src/types.ts b/src/types.ts
@@ -10,48 +10,24 @@ import {
 import { Static, Type } from '@sinclair/typebox';
 
 // NOTE: For params like `return` if its set to true we will only force it if the function does return something
+
+export const functionSchema = Type.Object({
+  tags: Type.Object({
+    dev: Type.Boolean({ default: false }),
+    notice: Type.Boolean({ default: true }),
+    return: Type.Boolean({ default: true }),
+    param: Type.Boolean({ default: true }),
+  }),
+});
+
 export const functionConfigSchema = Type.Object({
-  internal: Type.Optional(
-    Type.Object({
-      tags: Type.Object({
-        dev: Type.Boolean({ default: false }),
-        notice: Type.Boolean({ default: true }),
-        return: Type.Boolean({ default: true }),
-        param: Type.Boolean({ default: true }),
-      }),
-    })
-  ),
-  external: Type.Optional(
-    Type.Object({
-      tags: Type.Object({
-        dev: Type.Boolean({ default: false }),
-        notice: Type.Boolean({ default: true }),
-        return: Type.Boolean({ default: true }),
-        param: Type.Boolean({ default: true }),
-      }),
-    })
-  ),
-  public: Type.Optional(
-    Type.Object({
-      tags: Type.Object({
-        dev: Type.Boolean({ default: false }),
-        notice: Type.Boolean({ default: true }),
-        return: Type.Boolean({ default: true }),
-        param: Type.Boolean({ default: true }),
-      }),
-    })
-  ),
-  private: Type.Optional(
-    Type.Object({
-      tags: Type.Object({
-        dev: Type.Boolean({ default: false }),
-        notice: Type.Boolean({ default: true }),
-        return: Type.Boolean({ default: true }),
-        param: Type.Boolean({ default: true }),
-      }),
-    })
-  ),
-  constructor: Type.Boolean({ default: false }),
+  internal: functionSchema,
+
+  external: functionSchema,
+
+  public: functionSchema,
+
+  private: functionSchema,
 });
 
 export const configSchema = Type.Object({
@@ -60,8 +36,10 @@ export const configSchema = Type.Object({
   root: Type.String({ default: './' }),
   functions: functionConfigSchema,
   inheritdoc: Type.Boolean({ default: true }),
+  constructorNatspec: Type.Boolean({ default: false }),
 });
 
+export type FunctionConfig = Static<typeof functionSchema>;
 export type Config = Static<typeof configSchema>;
 export type Functions = Static<typeof functionConfigSchema>;
 

diff --git a/src/utils.ts b/src/utils.ts
@@ -237,6 +237,7 @@ export async function processConfig(filePath: string): Promise<Config> {
     root: detectedConfig.root ?? './',
     functions: detectedConfig.functions,
     inheritdoc: detectedConfig.inheritdoc ?? true,
+    constructorNatspec: detectedConfig.constructorNatspec ?? false,
   };
 
   // Validate the received config matches our expected type

diff --git a/src/validator.ts b/src/validator.ts
@@ -1,4 +1,4 @@
-import { Config, Natspec, NodeToProcess } from './types';
+import { Config, FunctionConfig, Functions, Natspec, NatspecDefinition, NodeToProcess } from './types';
 import { matchesFunctionKind, getElementFrequency } from './utils';
 import {
   EnumDefinition,
@@ -46,11 +46,29 @@ export class Validator {
 
     // Validate natspec for the constructor only if configured
     if (matchesFunctionKind(node, 'constructor')) {
-      return this.config.functions?.constructor ? this.validateParameters(node as FunctionDefinition, natspecParams) : [];
+      return this.config.constructorNatspec ? this.validateParameters(node as FunctionDefinition, natspecParams) : [];
     }
 
     // Inheritdoc is not enforced nor present, and there is no other documentation, returning error
-    if (!natspec.tags.length) return [`Natspec is missing`];
+    if (!natspec.tags.length) {
+      // If node is a function, check the user defined config
+      if (node instanceof FunctionDefinition) {
+        let needsWarning = false;
+
+        Object.keys(this.config.functions).forEach((key) => {
+          Object.keys(this.config.functions[key as keyof Functions].tags).forEach((tag) => {
+            if (this.config.functions[key as keyof Functions][tag as keyof FunctionConfig]) {
+              needsWarning = true;
+            }
+          });
+        });
+
+        if (needsWarning) return [`Natspec is missing`];
+      } else {
+        // TODO: Change this logic when we have more config options for events, structs, enums etc.
+        return [`Natspec is missing`];
+      }
+    }
 
     // Validate the completeness of the documentation
     let alerts: string[] = [];
@@ -63,7 +81,12 @@ export class Validator {
       alerts = [...alerts, ...this.validateParameters(node, natspecParams)];
     } else if (node instanceof FunctionDefinition) {
       const natspecReturns = natspec.returns.map((p) => p.name);
-      alerts = [...alerts, ...this.validateParameters(node, natspecParams), ...this.validateReturnParameters(node, natspecReturns)];
+      alerts = [
+        ...alerts,
+        ...this.validateParameters(node, natspecParams),
+        ...this.validateReturnParameters(node, natspecReturns),
+        ...this.validateTags(node, natspec.tags),
+      ];
     } else if (node instanceof ModifierDefinition) {
       alerts = [...alerts, ...this.validateParameters(node, natspecParams)];
     } else if (node instanceof StructDefinition) {
@@ -87,6 +110,12 @@ export class Validator {
     let alerts: string[] = [];
     const counter = getElementFrequency(natspecParams);
 
+    if (node instanceof FunctionDefinition) {
+      if (!this.config.functions[node.visibility as keyof Functions]?.tags.param) {
+        return [];
+      }
+    }
+
     for (let paramName of definedParameters) {
       if (!natspecParams.includes(paramName)) {
         alerts.push(`@param ${paramName} is missing`);
@@ -128,6 +157,11 @@ export class Validator {
    * @returns {string[]} - The list of alerts
    */
   private validateReturnParameters(node: FunctionDefinition, natspecReturns: (string | undefined)[]): string[] {
+    // If return tags are not enforced, return no warnings
+    if (!this.config.functions[node.visibility as keyof Functions]?.tags.return) {
+      return [];
+    }
+
     let alerts: string[] = [];
     let functionReturns = node.vReturnParameters.vParameters.map((p) => p.name);
 
@@ -145,6 +179,49 @@ export class Validator {
     return alerts;
   }
 
+  private validateTags(node: FunctionDefinition, natspecTags: NatspecDefinition[]): string[] {
+    const isDevTagForced = this.config.functions[node.visibility as keyof Functions]?.tags.dev;
+    const isNoticeTagForced = this.config.functions[node.visibility as keyof Functions]?.tags.notice;
+
+    // If both are disabled no warnings should emit so we dont need to check anything
+    if (!isDevTagForced && !isNoticeTagForced) {
+      return [];
+    }
+
+    let alerts: string[] = [];
+
+    let devCounter = 0;
+    let noticeCounter = 0;
+
+    for (const tag of natspecTags) {
+      if (tag.name === 'dev') {
+        devCounter++;
+      } else if (tag.name === 'notice') {
+        noticeCounter++;
+      }
+    }
+
+    // Needs a dev tag
+    // More then one dev tag is ok
+    if (isDevTagForced && devCounter === 0) {
+      alerts.push(`@dev is missing`);
+    }
+
+    if (isNoticeTagForced) {
+      // Needs one notice tag
+      if (noticeCounter === 0) {
+        alerts.push(`@notice is missing`);
+      }
+
+      // Cant have more then one notice tag
+      if (noticeCounter > 1) {
+        alerts.push(`@notice is duplicated`);
+      }
+    }
+
+    return alerts;
+  }
+
   /**
    * Checks if the node requires inheritdoc
    * @param {NodeToProcess} node - The node to process

diff --git a/test/contracts/BasicSample.sol b/test/contracts/BasicSample.sol
@@ -49,6 +49,13 @@ contract BasicSample is AbstractBasic {
         return true;
     }
 
+    /**
+    * @notice External function that returns a bool
+    */
+    function externalNoReturn() external pure returns (bool) {
+        return true;
+    }
+
     /**
      * @notice Private test function
      * @param _magicNumber A parameter description

diff --git a/test/utils.test.ts b/test/utils.test.ts
@@ -205,6 +205,7 @@ describe('Utils', () => {
         exclude: '',
         inheritdoc: true,
         functions: defaultFunctions,
+        constructorNatspec: false,
       });
     });
 
@@ -235,6 +236,7 @@ describe('Utils', () => {
         exclude: './contracts/ignored.sol',
         inheritdoc: false,
         functions: defaultFunctions,
+        constructorNatspec: false,
       });
     });
 
@@ -251,8 +253,8 @@ describe('Utils', () => {
                 param: true,
               },
             },
-            constructor: true,
           },
+          constructorNatspec: true,
         })
       );
       const config = await utils.processConfig(path.join(__dirname, './valid.config.json'));
@@ -295,8 +297,8 @@ describe('Utils', () => {
               param: true,
             },
           },
-          constructor: true,
         },
+        constructorNatspec: true,
       });
     });
 

diff --git a/test/utils/helpers.ts b/test/utils/helpers.ts
@@ -1,6 +1,6 @@
 import { ASTKind, ASTReader, SourceUnit, compileSol } from 'solc-typed-ast';
 import path from 'path';
-import { NodeToProcess } from '../../src/types';
+import { Functions, NodeToProcess } from '../../src/types';
 
 export async function getFileCompiledSource(filePath: string): Promise<SourceUnit> {
   const compiledFile = await compileSol(filePath, 'auto');
@@ -15,3 +15,10 @@ export function expectWarning(warnArray: string[], expectedWarn: string, numberO
 export function findNode(nodes: readonly NodeToProcess[], name: string): any {
   return nodes.find((x) => x.name === name);
 }
+
+export const defaultFunctions: Functions = {
+  internal: { tags: { dev: false, notice: true, return: true, param: true } },
+  external: { tags: { dev: false, notice: true, return: true, param: true } },
+  public: { tags: { dev: false, notice: true, return: true, param: true } },
+  private: { tags: { dev: false, notice: true, return: true, param: true } },
+};