From cffbfcac13fea3dcbcebefe5e38185ebbb6fe23f Mon Sep 17 00:00:00 2001 From: Trevor Porter Date: Mon, 5 Aug 2024 11:12:38 +0100 Subject: [PATCH] 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 ### Related issues ### Backward compatibility ### Testing --- Dockerfile | 1 + typescript/infra/scripts/check-deploy.ts | 2 +- .../infra/src/govern/HyperlaneAppGovernor.ts | 158 +++++++++++++----- .../infra/src/govern/HyperlaneCoreGovernor.ts | 8 +- .../infra/src/govern/ProxiedRouterGovernor.ts | 6 +- typescript/infra/src/govern/multisend.ts | 15 +- .../middleware/account/InterchainAccount.ts | 40 +++-- typescript/sdk/src/token/checker.ts | 2 +- 8 files changed, 167 insertions(+), 65 deletions(-) diff --git a/Dockerfile b/Dockerfile index 1a03f74d8..5dcd5686a 100644 --- a/Dockerfile +++ b/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 diff --git a/typescript/infra/scripts/check-deploy.ts b/typescript/infra/scripts/check-deploy.ts index 980ebd91d..5487ba2c1 100644 --- a/typescript/infra/scripts/check-deploy.ts +++ b/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); diff --git a/typescript/infra/src/govern/HyperlaneAppGovernor.ts b/typescript/infra/src/govern/HyperlaneAppGovernor.ts index 219a54cfa..437746ade 100644 --- a/typescript/infra/src/govern/HyperlaneAppGovernor.ts +++ b/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, 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 { 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) => { - 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( - chain, - call, - ); - } - call.submissionType = submissionType; - }), - ); - } catch (error) { - console.error( - `Error inferring call submission types for chain ${chain}: ${error}`, - ); + const newCalls: ChainMap = {}; + + 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 { + 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, + ); + } + 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 { + ): Promise { 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 { + additionalTxSuccessCriteria?: ( + chain: ChainName, + submitterAddress: Address, + ) => boolean, + ): Promise { 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 => { + // 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) { diff --git a/typescript/infra/src/govern/HyperlaneCoreGovernor.ts b/typescript/infra/src/govern/HyperlaneCoreGovernor.ts index 8f981380d..24b345353 100644 --- a/typescript/infra/src/govern/HyperlaneCoreGovernor.ts +++ b/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) { diff --git a/typescript/infra/src/govern/ProxiedRouterGovernor.ts b/typescript/infra/src/govern/ProxiedRouterGovernor.ts index 4837ebec6..c7424b0fc 100644 --- a/typescript/infra/src/govern/ProxiedRouterGovernor.ts +++ b/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, + )}`, + ); } } diff --git a/typescript/infra/src/govern/multisend.ts b/typescript/infra/src/govern/multisend.ts index 348762cfa..9182d3a1e 100644 --- a/typescript/infra/src/govern/multisend.ts +++ b/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({ diff --git a/typescript/sdk/src/middleware/account/InterchainAccount.ts b/typescript/sdk/src/middleware/account/InterchainAccount.ts index 7c5fe6c0a..3c22edc35 100644 --- a/typescript/sdk/src/middleware/account/InterchainAccount.ts +++ b/typescript/sdk/src/middleware/account/InterchainAccount.ts @@ -50,7 +50,7 @@ export class InterchainAccount extends RouterApp { } async deployAccount( - chain: ChainName, + destinationChain: ChainName, config: AccountConfig, routerOverride?: Address, ismOverride?: Address, @@ -61,29 +61,39 @@ export class InterchainAccount extends RouterApp { `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 diff --git a/typescript/sdk/src/token/checker.ts b/typescript/sdk/src/token/checker.ts index 48b38841f..206fc4d5b 100644 --- a/typescript/sdk/src/token/checker.ts +++ b/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,