feat: fully testing ICA ownership with tooling (#4245)

### Description

Some tweaks to get checker tooling working with ICA ownership:
- changed the `inferCallSubmissionTypes` logic to no longer mutate
`this.calls`. Some weird side effects were occurring, especially where
the ICA logic would try to pop and push from `this.calls` while also
looping over it. This is also why I made it serial again. I ended up
moving away from mutating this.calls entirely during the iteration, so I
guess moving back to doing it concurrently can be done again. Happy to
do this if it feels good to - the only reason I haven't is this wasn't a
place where we had huge inefficiencies and it'd complicate the code a
little
- Got it working where a Safe on chain A owns an ICA on chain B

These are the flows I tested:
- sepolia is the "owner chain"
- deployed a warp route between sepolia and alfajores. Changed the
alfajores owner to an ICA owned by the sepolia deployer key. Tested
transferring ownership back from this to the deployer.
- Did the exact same as above, but now with the ICA being owned by a
Safe on sepolia
- Transferred ownership of Alfajores core contracts to and from the
sepolia-deployer-owned ICA

These are some raw notes that may be of interest of what I did
https://www.notion.so/hyperlanexyz/ICA-playground-996cf28aea1649a18051afb8bb82acb2

This doesn't auto-deploy ICAs. An ICA is expected to have been deployed
already, and then just configured in `owners.ts`. Check-deploy figures
out that it's an ICA and does all the rest. This is fine imo in the
short term as I only imagine us using ICAs on two chains (inevm and
viction) for the time being.

### Drive-by changes

<!--
Are there any minor or drive-by changes also included?
-->

### Related issues

<!--
- Fixes #[issue number here]
-->

### Backward compatibility

<!--
Are these changes backward compatible? Are there any infrastructure
implications, e.g. changes that would prohibit deploying older commits
using this infra tooling?

Yes/No
-->

### Testing

<!--
What kind of testing have these changes undergone?

None/Manual/Unit Tests
-->

Dockerfile

@@ -17,6 +17,7 @@ COPY typescript/helloworld/package.json ./typescript/helloworld/
 COPY typescript/cli/package.json ./typescript/cli/
 COPY typescript/infra/package.json ./typescript/infra/
 COPY typescript/ccip-server/package.json ./typescript/ccip-server/
+COPY typescript/widgets/package.json ./typescript/widgets/
 COPY solidity/package.json ./solidity/
 
 RUN yarn install && yarn cache clean

typescript/infra/scripts/check-deploy.ts

@@ -84,7 +84,7 @@ async function check() {
       envConfig.core,
       ismFactory,
     );
-    governor = new HyperlaneCoreGovernor(checker);
+    governor = new HyperlaneCoreGovernor(checker, ica);
   } else if (module === Modules.INTERCHAIN_GAS_PAYMASTER) {
     const igp = HyperlaneIgp.fromAddressesMap(chainAddresses, multiProvider);
     const checker = new HyperlaneIgpChecker(multiProvider, igp, envConfig.igp);

typescript/infra/src/govern/HyperlaneAppGovernor.ts

@@ -41,6 +41,12 @@ export type AnnotatedCallData = CallData & {
   description: string;
 };
 
+export type InferredCall = {
+  type: SubmissionType;
+  chain: ChainName;
+  call: AnnotatedCallData;
+};
+
 export abstract class HyperlaneAppGovernor<
   App extends HyperlaneApp<any>,
   Config extends OwnableConfig,
@@ -161,10 +167,6 @@ export abstract class HyperlaneAppGovernor<
     this.calls[chain].push(call);
   }
 
-  protected popCall(chain: ChainName): AnnotatedCallData | undefined {
-    return this.calls[chain].pop();
-  }
-
   protected async mapViolationsToCalls(): Promise<void> {
     const callObjs = await Promise.all(
       this.checker.violations.map((violation) =>
@@ -184,37 +186,48 @@ export abstract class HyperlaneAppGovernor<
   ): Promise<{ chain: string; call: AnnotatedCallData } | undefined>;
 
   protected async inferCallSubmissionTypes() {
-    await Promise.all(
-      Object.keys(this.calls).map(async (chain) => {
+    const newCalls: ChainMap<AnnotatedCallData[]> = {};
+
+    const pushNewCall = (inferredCall: InferredCall) => {
+      newCalls[inferredCall.chain] = newCalls[inferredCall.chain] || [];
+      newCalls[inferredCall.chain].push({
+        submissionType: inferredCall.type,
+        ...inferredCall.call,
+      });
+    };
+
+    for (const chain of Object.keys(this.calls)) {
       try {
-          await Promise.all(
-            this.calls[chain].map(async (call) => {
-              let submissionType = await this.inferCallSubmissionType(
-                chain,
-                call,
-              );
-              if (submissionType === SubmissionType.MANUAL) {
-                submissionType = await this.inferICAEncodedSubmissionType(
+        for (const call of this.calls[chain]) {
+          let inferredCall: InferredCall;
+
+          inferredCall = await this.inferCallSubmissionType(chain, call);
+          // If it's a manual call, it means that we're not able to make the call
+          // from a signer or Safe. In this case, we try to infer if it must be sent
+          // from an ICA controlled by a remote owner. This new inferred call will be
+          // unchanged if the call is not an ICA call after all.
+          if (inferredCall.type === SubmissionType.MANUAL) {
+            inferredCall = await this.inferICAEncodedSubmissionType(
               chain,
               call,
             );
           }
-              call.submissionType = submissionType;
-            }),
-          );
+          pushNewCall(inferredCall);
+        }
       } catch (error) {
         console.error(
           `Error inferring call submission types for chain ${chain}: ${error}`,
         );
       }
-      }),
-    );
+    }
+
+    this.calls = newCalls;
   }
 
   protected async inferICAEncodedSubmissionType(
     chain: ChainName,
     call: AnnotatedCallData,
-  ): Promise<SubmissionType> {
+  ): Promise<InferredCall> {
     const multiProvider = this.checker.multiProvider;
     const signer = multiProvider.getSigner(chain);
     if (this.interchainAccount) {
@@ -248,7 +261,11 @@ export abstract class HyperlaneAppGovernor<
           config: accountConfig,
         });
         if (!callRemote.to || !callRemote.data) {
-          return SubmissionType.MANUAL;
+          return {
+            type: SubmissionType.MANUAL,
+            chain,
+            call,
+          };
         }
         const encodedCall: AnnotatedCallData = {
           to: callRemote.to,
@@ -256,23 +273,43 @@ export abstract class HyperlaneAppGovernor<
           value: callRemote.value,
           description: `${call.description} - interchain account call from ${origin} to ${chain}`,
         };
-        const subType = await this.inferCallSubmissionType(origin, encodedCall);
+        const { type: subType } = await this.inferCallSubmissionType(
+          origin,
+          encodedCall,
+          (chain: ChainName, submitterAddress: Address) => {
+            // Require the submitter to be the owner of the ICA on the origin chain.
+            return (
+              chain === origin &&
+              eqAddress(bytes32ToAddress(accountConfig.owner), submitterAddress)
+            );
+          },
+        );
         if (subType !== SubmissionType.MANUAL) {
-          this.popCall(chain);
-          this.pushCall(origin, encodedCall);
-          return subType;
+          return {
+            type: subType,
+            chain: origin,
+            call: encodedCall,
+          };
         }
       } else {
         console.log(`Account's owner ${localOwner} is not ICA router`);
       }
     }
-    return SubmissionType.MANUAL;
+    return {
+      type: SubmissionType.MANUAL,
+      chain,
+      call,
+    };
   }
 
   protected async inferCallSubmissionType(
     chain: ChainName,
     call: AnnotatedCallData,
-  ): Promise<SubmissionType> {
+    additionalTxSuccessCriteria?: (
+      chain: ChainName,
+      submitterAddress: Address,
+    ) => boolean,
+  ): Promise<InferredCall> {
     const multiProvider = this.checker.multiProvider;
     const signer = multiProvider.getSigner(chain);
     const signerAddress = await signer.getAddress();
@@ -281,7 +318,32 @@ export abstract class HyperlaneAppGovernor<
       chain: ChainName,
       submitterAddress: Address,
     ): Promise<boolean> => {
+      // The submitter needs to have enough balance to pay for the call.
+      // Surface a warning if the submitter's balance is insufficient, as this
+      // can result in fooling the tooling into thinking otherwise valid submission
+      // types are invalid.
+      if (call.value !== undefined) {
+        const submitterBalance = await multiProvider
+          .getProvider(chain)
+          .getBalance(submitterAddress);
+        if (submitterBalance.lt(call.value)) {
+          console.warn(
+            `Submitter ${submitterAddress} has an insufficient balance for the call and is likely to fail. Balance:`,
+            submitterBalance,
+            'Balance required:',
+            call.value,
+          );
+        }
+      }
+
       try {
+        if (
+          additionalTxSuccessCriteria &&
+          !additionalTxSuccessCriteria(chain, submitterAddress)
+        ) {
+          return false;
+        }
+        // Will throw if the transaction fails
         await multiProvider.estimateGas(chain, call, submitterAddress);
         return true;
       } catch (e) {} // eslint-disable-line no-empty
@@ -289,7 +351,11 @@ export abstract class HyperlaneAppGovernor<
     };
 
     if (await transactionSucceedsFromSender(chain, signerAddress)) {
-      return SubmissionType.SIGNER;
+      return {
+        type: SubmissionType.SIGNER,
+        chain,
+        call,
+      };
     }
 
     // 2. Check if the call will succeed via Gnosis Safe.
@@ -319,7 +385,11 @@ export abstract class HyperlaneAppGovernor<
               ))
           ) {
             console.warn(`${error.message}: Setting submission type to MANUAL`);
-            return SubmissionType.MANUAL;
+            return {
+              type: SubmissionType.MANUAL,
+              chain,
+              call,
+            };
           } else {
             console.error(
               `Failed to determine if signer can propose safe transactions: ${error}`,
@@ -333,11 +403,19 @@ export abstract class HyperlaneAppGovernor<
         this.canPropose[chain].get(safeAddress) &&
         (await transactionSucceedsFromSender(chain, safeAddress))
       ) {
-        return SubmissionType.SAFE;
+        return {
+          type: SubmissionType.SAFE,
+          chain,
+          call,
+        };
       }
     }
 
-    return SubmissionType.MANUAL;
+    return {
+      type: SubmissionType.MANUAL,
+      chain,
+      call,
+    };
   }
 
   handleOwnerViolation(violation: OwnerViolation) {

typescript/infra/src/govern/HyperlaneCoreGovernor.ts

@@ -7,6 +7,7 @@ import {
   HyperlaneCore,
   HyperlaneCoreChecker,
   HyperlaneCoreDeployer,
+  InterchainAccount,
   MailboxViolation,
   MailboxViolationType,
   OwnerViolation,
@@ -19,8 +20,11 @@ export class HyperlaneCoreGovernor extends HyperlaneAppGovernor<
   HyperlaneCore,
   CoreConfig
 > {
-  constructor(readonly checker: HyperlaneCoreChecker) {
-    super(checker);
+  constructor(
+    readonly checker: HyperlaneCoreChecker,
+    readonly ica?: InterchainAccount,
+  ) {
+    super(checker, ica);
   }
 
   protected async handleMailboxViolation(violation: MailboxViolation) {

typescript/infra/src/govern/ProxiedRouterGovernor.ts

@@ -27,7 +27,11 @@ export class ProxiedRouterGovernor<
       case ViolationType.Owner:
         return this.handleOwnerViolation(violation as OwnerViolation);
       default:
-        throw new Error(`Unsupported violation type ${violation.type}`);
+        throw new Error(
+          `Unsupported violation type ${violation.type}: ${JSON.stringify(
+            violation,
+          )}`,
+        );
     }
   }
 

typescript/infra/src/govern/multisend.ts

@@ -17,10 +17,11 @@ export class SignerMultiSend extends MultiSend {
 
   async sendTransactions(calls: CallData[]) {
     for (const call of calls) {
-      const receipt = await this.multiProvider.sendTransaction(
-        this.chain,
-        call,
-      );
+      const estimate = await this.multiProvider.estimateGas(this.chain, call);
+      const receipt = await this.multiProvider.sendTransaction(this.chain, {
+        gasLimit: estimate.mul(11).div(10), // 10% buffer
+        ...call,
+      });
       console.log(`confirmed tx ${receipt.transactionHash}`);
     }
   }
@@ -58,7 +59,11 @@ export class SafeMultiSend extends MultiSend {
     const safeService = getSafeService(this.chain, this.multiProvider);
 
     const safeTransactionData = calls.map((call) => {
-      return { to: call.to, data: call.data.toString(), value: '0' };
+      return {
+        to: call.to,
+        data: call.data.toString(),
+        value: call.value?.toString() || '0',
+      };
     });
     const nextNonce = await safeService.getNextNonce(this.safeAddress);
     const safeTransaction = await safeSdk.createTransaction({

typescript/sdk/src/middleware/account/InterchainAccount.ts

@@ -50,7 +50,7 @@ export class InterchainAccount extends RouterApp<InterchainAccountFactories> {
   }
 
   async deployAccount(
-    chain: ChainName,
+    destinationChain: ChainName,
     config: AccountConfig,
     routerOverride?: Address,
     ismOverride?: Address,
@@ -61,29 +61,39 @@ export class InterchainAccount extends RouterApp<InterchainAccountFactories> {
         `Origin chain (${config.origin}) metadata needed for deploying ICAs ...`,
       );
     }
-    const localRouter = this.router(this.contractsMap[chain]);
-    const routerAddress =
+    const destinationRouter = this.router(this.contractsMap[destinationChain]);
+    const originRouterAddress =
       routerOverride ??
-      bytes32ToAddress(await localRouter.routers(originDomain));
-    const ismAddress =
-      ismOverride ?? bytes32ToAddress(await localRouter.isms(originDomain));
-    const account = await localRouter[
+      bytes32ToAddress(await destinationRouter.routers(originDomain));
+    const destinationIsmAddress =
+      ismOverride ??
+      bytes32ToAddress(await destinationRouter.isms(originDomain));
+    const destinationAccount = await destinationRouter[
       'getLocalInterchainAccount(uint32,address,address,address)'
-    ](originDomain, config.owner, routerAddress, ismAddress);
+    ](originDomain, config.owner, originRouterAddress, destinationIsmAddress);
     if (
-      (await this.multiProvider.getProvider(chain).getCode(account)) === '0x'
+      (await this.multiProvider
+        .getProvider(destinationChain)
+        .getCode(destinationAccount)) === '0x'
     ) {
       await this.multiProvider.handleTx(
-        chain,
-        localRouter[
+        destinationChain,
+        destinationRouter[
           'getDeployedInterchainAccount(uint32,address,address,address)'
-        ](originDomain, config.owner, routerAddress, ismAddress),
+        ](
+          originDomain,
+          config.owner,
+          originRouterAddress,
+          destinationIsmAddress,
+        ),
       );
-      this.logger.debug(`Interchain account deployed at ${account}`);
+      this.logger.debug(`Interchain account deployed at ${destinationAccount}`);
     } else {
-      this.logger.debug(`Interchain account recovered at ${account}`);
+      this.logger.debug(
+        `Interchain account recovered at ${destinationAccount}`,
+      );
     }
-    return account;
+    return destinationAccount;
   }
 
   // meant for ICA governance to return the populatedTx

typescript/sdk/src/token/checker.ts

@@ -44,7 +44,7 @@ export class HypERC20Checker extends HyperlaneRouterChecker<
       for (const check of checks) {
         const actual = await token[check.method]();
         const expected = config[check.method];
-        if (actual !== expected) {
+        if (expected !== undefined && actual !== expected) {
           const violation: TokenMismatchViolation = {
             type: check.violationType,
             chain,