refactor: handle command and helper errors at interpreter level

.changeset/friendly-geese-give.md

@@ -0,0 +1,6 @@
+---
+'@1hive/evmcrispr': patch
+---
+
+- Fix implicit string-to-bytes conversion
+- Refactor interpreter error handling

packages/evmcrispr/src/EVMcrispr.ts

@@ -6,6 +6,7 @@ import {
   ErrorException,
   ExpressionError,
   HelperFunctionError,
+  NodeError,
 } from './errors';
 import { timeUnits, toDecimals } from './utils';
 import type {
@@ -308,7 +309,7 @@ export class EVMcrispr {
     return res;
   };
 
-  #interpretCommand: NodeInterpreter<CommandExpressionNode> = (c) => {
+  #interpretCommand: NodeInterpreter<CommandExpressionNode> = async (c) => {
     let module: Module | undefined = this.#std;
     const moduleName =
       c.module ??
@@ -329,10 +330,25 @@ export class EVMcrispr {
       }
     }
 
-    return module.interpretCommand(c, {
-      interpretNode: this.interpretNode,
-      interpretNodes: this.interpretNodes,
-    });
+    let res: Awaited<ReturnType<typeof module.interpretCommand>>;
+
+    try {
+      res = await module.interpretCommand(c, {
+        interpretNode: this.interpretNode,
+        interpretNodes: this.interpretNodes,
+      });
+    } catch (err) {
+      // Avoid wrapping a node error insde another node error
+      if (err instanceof NodeError) {
+        throw err;
+      }
+
+      const err_ = err as Error;
+
+      EVMcrispr.panic(c, err_.message);
+    }
+
+    return res;
   };
 
   #interpretHelperFunction: NodeInterpreter<HelperFunctionNode> = async (h) => {
@@ -355,10 +371,25 @@ export class EVMcrispr {
 
     const m = filteredModules[0];
 
-    return m.interpretHelper(h, {
-      interpretNode: this.interpretNode,
-      interpretNodes: this.interpretNodes,
-    });
+    let res: Awaited<ReturnType<typeof m.interpretHelper>>;
+
+    try {
+      res = await m.interpretHelper(h, {
+        interpretNode: this.interpretNode,
+        interpretNodes: this.interpretNodes,
+      });
+    } catch (err) {
+      // Avoid wrapping a node error insde another node error
+      if (err instanceof NodeError) {
+        throw err;
+      }
+
+      const err_ = err as Error;
+
+      EVMcrispr.panic(h, err_.message);
+    }
+
+    return res;
   };
 
   #interpretLiteral: NodeInterpreter<LiteralExpressionNode> = async (n) => {

packages/evmcrispr/src/modules/Module.ts

@@ -13,7 +13,7 @@ import type {
 
 import type { BindingsManager } from '../BindingsManager';
 import { BindingsSpace } from '../BindingsManager';
-import { EVMcrispr } from '../EVMcrispr';
+import { ErrorException } from '..';
 
 export abstract class Module {
   constructor(
@@ -41,7 +41,9 @@ export abstract class Module {
     const command = this.commands[c.name];
 
     if (!command) {
-      EVMcrispr.panic(c, `command not found on module ${this.contextualName}`);
+      throw new ErrorException(
+        `command not found on module ${this.contextualName}`,
+      );
     }
 
     return command(this, c, interpreters);

packages/evmcrispr/src/modules/aragonos/commands/act.ts

@@ -1,15 +1,14 @@
 import { utils } from 'ethers';
 
-import type { Action, CommandFunction } from '../../../types';
-
+import type { CommandFunction } from '../../../types';
 import { batchForwarderActions } from '../utils/forwarders';
+import { ErrorException } from '../../../errors';
 import {
   ComparisonType,
   SIGNATURE_REGEX,
   checkArgsLength,
   encodeAction,
 } from '../../../utils';
-import { EVMcrispr } from '../../../EVMcrispr';
 import type { AragonOS } from '../AragonOS';
 
 export const act: CommandFunction<AragonOS> = async (
@@ -33,30 +32,23 @@ export const act: CommandFunction<AragonOS> = async (
   );
 
   if (!utils.isAddress(agentAddress)) {
-    EVMcrispr.panic(
-      c,
+    throw new ErrorException(
       `expected a valid agent address, but got ${agentAddress}`,
     );
   }
   if (!utils.isAddress(targetAddress)) {
-    EVMcrispr.panic(
-      c,
+    throw new ErrorException(
       `expected a valid target address, but got ${targetAddress}`,
     );
   }
 
   if (!SIGNATURE_REGEX.test(signature)) {
-    EVMcrispr.panic(c, `expected a valid signature, but got ${signature}`);
+    throw new ErrorException(
+      `expected a valid signature, but got ${signature}`,
+    );
   }
 
-  let execAction: Action;
-
-  try {
-    execAction = encodeAction(targetAddress, signature, params);
-  } catch (err) {
-    const err_ = err as Error;
-    EVMcrispr.panic(c, err_.message);
-  }
+  const execAction = encodeAction(targetAddress, signature, params);
 
   return batchForwarderActions(module.signer, [execAction], [agentAddress]);
 };

packages/evmcrispr/src/modules/aragonos/commands/connect.ts

@@ -1,11 +1,16 @@
 import { ethers, utils } from 'ethers';
 
-import type { Action, Address } from '../../..';
-import { ANY_ENTITY, BURN_ENTITY, NO_ENTITY } from '../utils';
-import type { CommandFunction, TransactionAction } from '../../../types';
+import { ErrorException } from '../../../errors';
+import type {
+  Action,
+  Address,
+  CommandFunction,
+  TransactionAction,
+} from '../../../types';
 import { NodeType, isProviderAction } from '../../../types';
 import { AragonDAO } from '../AragonDAO';
 import type { AragonOS } from '../AragonOS';
+import { ANY_ENTITY, BURN_ENTITY, NO_ENTITY } from '../utils';
 import { BindingsSpace } from '../../../BindingsManager';
 import type { BindingsManager } from '../../../BindingsManager';
 import {
@@ -17,7 +22,6 @@ import {
 } from '../../../utils';
 import { batchForwarderActions } from '../utils/forwarders';
 import { _aragonEns } from '../helpers/aragonEns';
-import { EVMcrispr } from '../../../EVMcrispr';
 
 const { BlockExpression } = NodeType;
 const { ABI, ADDR } = BindingsSpace;
@@ -111,7 +115,7 @@ export const connect: CommandFunction<AragonOS> = async (
   const forwarderApps = await interpretNodes(rest);
 
   if (!blockExpressionNode || blockExpressionNode.type !== BlockExpression) {
-    EVMcrispr.panic(c, 'last argument should be a set of commands');
+    throw new ErrorException('last argument should be a set of commands');
   }
 
   const daoAddressOrName = await interpretNode(daoNameNode);
@@ -124,7 +128,9 @@ export const connect: CommandFunction<AragonOS> = async (
     const res = await _aragonEns(daoENSName, module);
 
     if (!res) {
-      EVMcrispr.panic(c, `ENS DAO name ${daoENSName} couldn't be resolved`);
+      throw new ErrorException(
+        `ENS DAO name ${daoENSName} couldn't be resolved`,
+      );
     }
 
     daoAddress = res;
@@ -133,29 +139,22 @@ export const connect: CommandFunction<AragonOS> = async (
   const currentDao = module.currentDAO;
 
   if (currentDao && addressesEqual(currentDao.kernel.address, daoAddress)) {
-    EVMcrispr.panic(
-      c,
+    throw new ErrorException(
       `trying to connect to an already connected DAO (${daoAddress})`,
     );
   }
 
   // Allow us to keep track of connected DAOs inside nested 'connect' commands
   const nextNestingIndex = currentDao ? currentDao.nestingIndex + 1 : 1;
 
-  let dao: AragonDAO;
-  try {
-    dao = await AragonDAO.create(
-      daoAddress,
-      module.getConfigBinding('subgraphUrl'),
-      module.signer.provider ??
-        ethers.getDefaultProvider(await module.signer.getChainId()),
-      module.ipfsResolver,
-      nextNestingIndex,
-    );
-  } catch (err) {
-    const err_ = err as Error;
-    EVMcrispr.panic(c, err_.message);
-  }
+  const dao = await AragonDAO.create(
+    daoAddress,
+    module.getConfigBinding('subgraphUrl'),
+    module.signer.provider ??
+      ethers.getDefaultProvider(await module.signer.getChainId()),
+    module.ipfsResolver,
+    nextNestingIndex,
+  );
 
   module.connectedDAOs.push(dao);
 
@@ -173,7 +172,7 @@ export const connect: CommandFunction<AragonOS> = async (
   })) as Action[];
 
   if (actions.find((a) => isProviderAction(a))) {
-    EVMcrispr.panic(c, `can't switch networks inside a connect command`);
+    throw new ErrorException(`can't switch networks inside a connect command`);
   }
 
   const invalidApps: any[] = [];
@@ -185,7 +184,7 @@ export const connect: CommandFunction<AragonOS> = async (
       : dao.resolveApp(appOrAddress)?.address;
 
     if (!appAddress) {
-      EVMcrispr.panic(c, `${appOrAddress} is not a DAO's forwarder app`);
+      throw new ErrorException(`${appOrAddress} is not a DAO's forwarder app`);
     }
 
     !utils.isAddress(appAddress)
@@ -194,8 +193,7 @@ export const connect: CommandFunction<AragonOS> = async (
   });
 
   if (invalidApps.length) {
-    EVMcrispr.panic(
-      c,
+    throw new ErrorException(
       `invalid forwarder addresses found for the following: ${invalidApps.join(
         ', ',
       )}`,
@@ -204,19 +202,10 @@ export const connect: CommandFunction<AragonOS> = async (
 
   const context = await getOptValue(c, 'context', interpretNode);
 
-  let forwarderActions: Action[];
-
-  try {
-    forwarderActions = await batchForwarderActions(
-      module.signer,
-      actions as TransactionAction[],
-      forwarderAppAddresses.reverse(),
-      context,
-    );
-  } catch (err) {
-    const err_ = err as Error;
-    EVMcrispr.panic(c, err_.message);
-  }
-
-  return forwarderActions;
+  return batchForwarderActions(
+    module.signer,
+    actions as TransactionAction[],
+    forwarderAppAddresses.reverse(),
+    context,
+  );
 };

packages/evmcrispr/src/modules/aragonos/commands/forward.ts

@@ -1,22 +1,20 @@
 import { utils } from 'ethers';
 
+import { ErrorException } from '../../../errors';
 import type {
   Action,
   CommandFunction,
   TransactionAction,
 } from '../../../types';
 import { isProviderAction } from '../../../types';
-import type { ErrorException } from '../../../errors';
 
 import { batchForwarderActions } from '../utils/forwarders';
-import { EVMcrispr } from '../../../EVMcrispr';
 import {
   ComparisonType,
   checkArgsLength,
   commaListItems,
 } from '../../../utils';
 import type { AragonOS } from '../AragonOS';
-
 export const forward: CommandFunction<AragonOS> = async (
   module,
   c,
@@ -40,8 +38,7 @@ export const forward: CommandFunction<AragonOS> = async (
   );
 
   if (invalidForwarderApps.length) {
-    EVMcrispr.panic(
-      c,
+    throw new ErrorException(
       `${commaListItems(invalidForwarderApps)} are not valid forwarder address`,
     );
   }
@@ -50,22 +47,13 @@ export const forward: CommandFunction<AragonOS> = async (
     blockModule: module.contextualName,
   })) as Action[];
 
-  let forwarderActions: Action[] = [];
-
   if (blockActions.find((a) => isProviderAction(a))) {
-    EVMcrispr.panic(c, `can't switch networks inside a connect command`);
-  }
-
-  try {
-    forwarderActions = await batchForwarderActions(
-      module.signer,
-      blockActions as TransactionAction[],
-      forwarderAppAddresses.reverse(),
-    );
-  } catch (err) {
-    const err_ = err as ErrorException;
-    EVMcrispr.panic(c, err_.message);
+    throw new ErrorException(`can't switch networks inside a connect command`);
   }
 
-  return forwarderActions;
+  return batchForwarderActions(
+    module.signer,
+    blockActions as TransactionAction[],
+    forwarderAppAddresses.reverse(),
+  );
 };