From 4c0605dca5287813136bb6d568dd2eda01fd7348 Mon Sep 17 00:00:00 2001 From: xeno097 Date: Fri, 1 Nov 2024 11:47:22 -0400 Subject: [PATCH] fix: warp deploy apply ownership fixes (#4726) ### Description This PR adds the required logic for allowing an already existing AdminProxy contract to be used in a warp route deployment using `warp deploy` by modifying `warp init`. Additionally, `warp apply` now allows the transfer of ownership of the warp route if specified in the config. - If the user specifies the `--yes` flag the proxy admin prompt will not be shown - Updates the `ProxiedRouterDeployer` to check if a proxy admin contract has been defined in the current configuration so that it can be reused instead of deploying a new contract. ### Drive-by changes - Defines a `DeployedOwnableConfig` type to represent any deployed contract that can be owned by an address (AdminProxy in this case) - ### Related issues - Fixes https://github.com/hyperlane-xyz/hyperlane-monorepo/issues/4710 ### Backward compatibility - Yes: the new field has been added as an optional field and if not present in the config it is ignored ### Testing - Manual testing --------- Co-authored-by: Paul Balaji <10051819+paulbalaji@users.noreply.github.com> --- .changeset/pink-bats-mix.md | 6 ++ typescript/cli/src/config/warp.ts | 21 ++++++- typescript/cli/src/utils/input.ts | 59 ++++++++++++++++++- typescript/sdk/src/deploy/types.ts | 4 +- typescript/sdk/src/index.ts | 1 + .../sdk/src/router/ProxiedRouterDeployer.ts | 29 +++++++-- typescript/sdk/src/router/schemas.ts | 3 +- typescript/sdk/src/schemas.ts | 4 ++ .../token/EvmERC20WarpModule.hardhat-test.ts | 50 +++++++++++++++- .../sdk/src/token/EvmERC20WarpModule.ts | 34 +++++++++++ .../sdk/src/token/EvmERC20WarpRouteReader.ts | 20 +++++++ typescript/sdk/src/token/deploy.ts | 19 ++++-- 12 files changed, 232 insertions(+), 18 deletions(-) create mode 100644 .changeset/pink-bats-mix.md diff --git a/.changeset/pink-bats-mix.md b/.changeset/pink-bats-mix.md new file mode 100644 index 0000000000..dd3ae396ac --- /dev/null +++ b/.changeset/pink-bats-mix.md @@ -0,0 +1,6 @@ +--- +'@hyperlane-xyz/cli': minor +'@hyperlane-xyz/sdk': minor +--- + +Add optional proxy admin reuse in warp route deployments and admin proxy ownership transfer in warp apply diff --git a/typescript/cli/src/config/warp.ts b/typescript/cli/src/config/warp.ts index dd3a237139..1174d0156b 100644 --- a/typescript/cli/src/config/warp.ts +++ b/typescript/cli/src/config/warp.ts @@ -3,6 +3,7 @@ import { stringify as yamlStringify } from 'yaml'; import { ChainMap, + DeployedOwnableConfig, IsmConfig, IsmType, MailboxClientConfig, @@ -28,7 +29,10 @@ import { readYamlOrJson, writeYamlOrJson, } from '../utils/files.js'; -import { detectAndConfirmOrPrompt } from '../utils/input.js'; +import { + detectAndConfirmOrPrompt, + setProxyAdminConfig, +} from '../utils/input.js'; import { createAdvancedIsmConfig } from './ism.js'; @@ -129,7 +133,9 @@ export async function createWarpRouteDeployConfig({ chainMetadata: context.chainMetadata, message: 'Select chains to connect', requireNumber: 1, - requiresConfirmation: true, + // If the user supplied the --yes flag we skip asking selection + // confirmation + requiresConfirmation: !context.skipConfirmation, }); const result: WarpRouteDeployConfig = {}; @@ -147,6 +153,12 @@ export async function createWarpRouteDeployConfig({ message: `Could not retrieve mailbox address from the registry for chain "${chain}". Please enter a valid mailbox address:`, })); + const proxyAdmin: DeployedOwnableConfig = await setProxyAdminConfig( + context, + chain, + owner, + ); + /** * The logic from the cli is as follows: * --advanced flag is provided: the user will have to build their own configuration using the available ISM types @@ -190,6 +202,7 @@ export async function createWarpRouteDeployConfig({ mailbox, type, owner, + proxyAdmin, isNft, interchainSecurityModule, token: await input({ @@ -203,6 +216,7 @@ export async function createWarpRouteDeployConfig({ type, owner, isNft, + proxyAdmin, collateralChainName: '', // This will be derived correctly by zod.parse() below interchainSecurityModule, }; @@ -216,6 +230,7 @@ export async function createWarpRouteDeployConfig({ mailbox, type, owner, + proxyAdmin, isNft, interchainSecurityModule, token: await input({ @@ -230,6 +245,7 @@ export async function createWarpRouteDeployConfig({ mailbox, type, owner, + proxyAdmin, isNft, interchainSecurityModule, token: await input({ @@ -242,6 +258,7 @@ export async function createWarpRouteDeployConfig({ mailbox, type, owner, + proxyAdmin, isNft, interchainSecurityModule, }; diff --git a/typescript/cli/src/utils/input.ts b/typescript/cli/src/utils/input.ts index 251c6c0c51..a7295d333c 100644 --- a/typescript/cli/src/utils/input.ts +++ b/typescript/cli/src/utils/input.ts @@ -18,11 +18,15 @@ import type { PartialDeep } from '@inquirer/type'; import ansiEscapes from 'ansi-escapes'; import chalk from 'chalk'; +import { ProxyAdmin__factory } from '@hyperlane-xyz/core'; +import { ChainName, DeployedOwnableConfig } from '@hyperlane-xyz/sdk'; import { WarpCoreConfig } from '@hyperlane-xyz/sdk'; +import { Address, isAddress, rootLogger } from '@hyperlane-xyz/utils'; import { readWarpCoreConfig } from '../config/warp.js'; import { CommandContext } from '../context/types.js'; -import { logGray, logRed } from '../logger.js'; +import { logGray } from '../logger.js'; +import { logRed } from '../logger.js'; import { indentYamlOrJson } from './files.js'; import { selectRegistryWarpRoute } from './tokens.js'; @@ -77,6 +81,59 @@ export async function inputWithInfo({ return answer; } +/** + * Prompts the user to optionally set an existing ProxyAdmin contract address to be used in a WarpToken deployment. + */ +export async function setProxyAdminConfig( + context: CommandContext, + chain: ChainName, + warpRouteOwner: Address, +): Promise { + const defaultAdminConfig: DeployedOwnableConfig = { + owner: warpRouteOwner, + }; + + // default to deploying a new ProxyAdmin with `warpRouteOwner` as the owner + // if the user supplied the --yes flag + if (context.skipConfirmation) { + return defaultAdminConfig; + } + + const useExistingProxy = await confirm({ + message: `Use an existing Proxy Admin contract for the warp route deployment on chain "${chain}"?`, + }); + + if (!useExistingProxy) { + return defaultAdminConfig; + } + + const proxyAdminAddress = await input({ + message: `Please enter the address of the Proxy Admin contract to be used on chain "${chain}":`, + validate: isAddress, + }); + + const proxy = ProxyAdmin__factory.connect( + proxyAdminAddress, + context.multiProvider.getProvider(chain), + ); + + try { + const ownerAddress = await proxy.owner(); + return { + address: proxyAdminAddress, + owner: ownerAddress, + }; + } catch (error) { + rootLogger.error( + `Failed to read owner address from ProxyAdmin contract at ${proxy.address} on chain ${chain}.`, + error, + ); + throw new Error( + `Failed to read owner address from ProxyAdmin contract at ${proxy.address}. Are you sure this is a ProxyAdmin contract?`, + ); + } +} + /** * Gets a {@link WarpCoreConfig} based on the provided path or prompts the user to choose one: * - if `symbol` is provided the user will have to select one of the available warp routes. diff --git a/typescript/sdk/src/deploy/types.ts b/typescript/sdk/src/deploy/types.ts index e8afe8f7ad..c1612d10d2 100644 --- a/typescript/sdk/src/deploy/types.ts +++ b/typescript/sdk/src/deploy/types.ts @@ -8,11 +8,13 @@ import type { } from '@hyperlane-xyz/core'; import { Address } from '@hyperlane-xyz/utils'; -import { OwnableSchema } from '../schemas.js'; +import { DeployedOwnableSchema, OwnableSchema } from '../schemas.js'; import type { ChainName } from '../types.js'; export type OwnableConfig = z.infer; +export type DeployedOwnableConfig = z.infer; + export interface CheckerViolation { chain: ChainName; type: string; diff --git a/typescript/sdk/src/index.ts b/typescript/sdk/src/index.ts index efadabc449..7fb3c1729d 100644 --- a/typescript/sdk/src/index.ts +++ b/typescript/sdk/src/index.ts @@ -96,6 +96,7 @@ export { HyperlaneProxyFactoryDeployer } from './deploy/HyperlaneProxyFactoryDep export { CheckerViolation, OwnableConfig, + DeployedOwnableConfig, OwnerViolation, ProxyAdminViolation, ViolationType, diff --git a/typescript/sdk/src/router/ProxiedRouterDeployer.ts b/typescript/sdk/src/router/ProxiedRouterDeployer.ts index e938c096c4..9edcad76c6 100644 --- a/typescript/sdk/src/router/ProxiedRouterDeployer.ts +++ b/typescript/sdk/src/router/ProxiedRouterDeployer.ts @@ -1,6 +1,8 @@ import { constants } from 'ethers'; import { + ProxyAdmin, + ProxyAdmin__factory, Router, TimelockController, TimelockController__factory, @@ -82,12 +84,27 @@ export abstract class ProxiedRouterDeployer< chain: ChainName, config: Config, ): Promise> { - const proxyAdmin = await this.deployContractFromFactory( - chain, - this.factories.proxyAdmin, - 'proxyAdmin', - [], - ); + let proxyAdmin: ProxyAdmin; + if (config.proxyAdmin?.address) { + this.logger.debug( + `Reusing existing ProxyAdmin at ${config.proxyAdmin.address} for chain ${chain}`, + ); + proxyAdmin = ProxyAdmin__factory.connect( + config.proxyAdmin.address, + this.multiProvider.getSigner(chain), + ); + } else { + this.logger.debug( + `A ProxyAdmin config has not been supplied for chain ${chain}, deploying a new contract`, + ); + + proxyAdmin = await this.deployContractFromFactory( + chain, + this.factories.proxyAdmin, + 'proxyAdmin', + [], + ); + } let timelockController: TimelockController; let adminOwner: string; diff --git a/typescript/sdk/src/router/schemas.ts b/typescript/sdk/src/router/schemas.ts index d49a72dbd2..571cb0d1f5 100644 --- a/typescript/sdk/src/router/schemas.ts +++ b/typescript/sdk/src/router/schemas.ts @@ -4,7 +4,7 @@ import { ProxyFactoryFactoriesSchema } from '../deploy/schemas.js'; import { HookConfigSchema } from '../hook/schemas.js'; import { IsmConfigSchema } from '../ism/schemas.js'; import { ZHash } from '../metadata/customZodTypes.js'; -import { OwnableSchema } from '../schemas.js'; +import { DeployedOwnableSchema, OwnableSchema } from '../schemas.js'; export const MailboxClientConfigSchema = OwnableSchema.extend({ mailbox: ZHash, @@ -29,6 +29,7 @@ export const RouterConfigSchema = MailboxClientConfigSchema.merge( ).merge( z.object({ remoteRouters: RemoteRoutersSchema.optional(), + proxyAdmin: DeployedOwnableSchema.optional(), }), ); diff --git a/typescript/sdk/src/schemas.ts b/typescript/sdk/src/schemas.ts index 9e9b0ee1ca..ef37e92fb9 100644 --- a/typescript/sdk/src/schemas.ts +++ b/typescript/sdk/src/schemas.ts @@ -7,6 +7,10 @@ export const OwnableSchema = z.object({ ownerOverrides: z.record(ZHash).optional(), }); +export const DeployedOwnableSchema = OwnableSchema.extend({ + address: ZHash.optional(), +}); + export const PausableSchema = OwnableSchema.extend({ paused: z.boolean(), }); diff --git a/typescript/sdk/src/token/EvmERC20WarpModule.hardhat-test.ts b/typescript/sdk/src/token/EvmERC20WarpModule.hardhat-test.ts index 33b9bdaa47..5895d783fc 100644 --- a/typescript/sdk/src/token/EvmERC20WarpModule.hardhat-test.ts +++ b/typescript/sdk/src/token/EvmERC20WarpModule.hardhat-test.ts @@ -271,6 +271,7 @@ describe('EvmERC20WarpHyperlaneModule', async () => { paused: false, }, ]; + it('should deploy and set a new Ism', async () => { const config = { ...baseConfig, @@ -489,7 +490,7 @@ describe('EvmERC20WarpHyperlaneModule', async () => { ismFactoryAddresses, } as TokenRouterConfig; - const owner = randomAddress(); + const owner = signer.address.toLowerCase(); const evmERC20WarpModule = await EvmERC20WarpModule.create({ chain, config: { @@ -498,7 +499,9 @@ describe('EvmERC20WarpHyperlaneModule', async () => { }, multiProvider, }); - expect(owner).to.equal(owner); + + const currentConfig = await evmERC20WarpModule.read(); + expect(currentConfig.owner.toLowerCase()).to.equal(owner); const newOwner = randomAddress(); await sendTxs( @@ -519,6 +522,49 @@ describe('EvmERC20WarpHyperlaneModule', async () => { expect(txs.length).to.equal(0); }); + it('should update the ProxyAdmin owner only if they are different', async () => { + const config: TokenRouterConfig = { + ...baseConfig, + type: TokenType.native, + hook: hookAddress, + ismFactoryAddresses, + }; + + const owner = signer.address.toLowerCase(); + const evmERC20WarpModule = await EvmERC20WarpModule.create({ + chain, + config: { + ...config, + interchainSecurityModule: ismAddress, + }, + multiProvider, + }); + + const currentConfig = await evmERC20WarpModule.read(); + expect(currentConfig.proxyAdmin?.owner.toLowerCase()).to.equal(owner); + + const newOwner = randomAddress(); + const updatedWarpCoreConfig: TokenRouterConfig = { + ...config, + proxyAdmin: { + address: currentConfig.proxyAdmin!.address, + owner: newOwner, + }, + }; + await sendTxs(await evmERC20WarpModule.update(updatedWarpCoreConfig)); + + const latestConfig: TokenRouterConfig = normalizeConfig( + await evmERC20WarpModule.read(), + ); + expect(latestConfig.proxyAdmin?.owner).to.equal(newOwner); + // Sanity check to be sure that the owner of the warp route token has not been updated if not changed + expect(latestConfig.owner).to.equal(owner); + + // No op if the same owner + const txs = await evmERC20WarpModule.update(updatedWarpCoreConfig); + expect(txs.length).to.equal(0); + }); + it('should update the destination gas', async () => { const domain = 3; const config: TokenRouterConfig = { diff --git a/typescript/sdk/src/token/EvmERC20WarpModule.ts b/typescript/sdk/src/token/EvmERC20WarpModule.ts index 462a5b6469..a62e7a05bd 100644 --- a/typescript/sdk/src/token/EvmERC20WarpModule.ts +++ b/typescript/sdk/src/token/EvmERC20WarpModule.ts @@ -14,6 +14,7 @@ import { addressToBytes32, assert, deepEquals, + eqAddress, isObjEmpty, objMap, rootLogger, @@ -108,6 +109,7 @@ export class EvmERC20WarpModule extends HyperlaneModule< ...this.createRemoteRoutersUpdateTxs(actualConfig, expectedConfig), ...this.createSetDestinationGasUpdateTxs(actualConfig, expectedConfig), ...this.createOwnershipUpdateTxs(actualConfig, expectedConfig), + ...this.updateProxyAdminOwnershipTxs(actualConfig, expectedConfig), ); return transactions; @@ -286,6 +288,38 @@ export class EvmERC20WarpModule extends HyperlaneModule< ); } + updateProxyAdminOwnershipTxs( + actualConfig: Readonly, + expectedConfig: Readonly, + ): AnnotatedEV5Transaction[] { + const transactions: AnnotatedEV5Transaction[] = []; + + // Return early because old warp config files did not have the + // proxyAdmin property + if (!expectedConfig.proxyAdmin) { + return transactions; + } + + const actualProxyAdmin = actualConfig.proxyAdmin!; + assert( + eqAddress(actualProxyAdmin.address!, expectedConfig.proxyAdmin.address!), + `ProxyAdmin contract addresses do not match. Expected ${expectedConfig.proxyAdmin.address}, got ${actualProxyAdmin.address}`, + ); + + transactions.push( + // Internally the createTransferOwnershipTx method already checks if the + // two owner values are the same and produces an empty tx batch if they are + ...transferOwnershipTransactions( + this.domainId, + actualProxyAdmin.address!, + actualProxyAdmin, + expectedConfig.proxyAdmin, + ), + ); + + return transactions; + } + /** * Updates or deploys the ISM using the provided configuration. * diff --git a/typescript/sdk/src/token/EvmERC20WarpRouteReader.ts b/typescript/sdk/src/token/EvmERC20WarpRouteReader.ts index 0be06f7ca5..1ec2e99809 100644 --- a/typescript/sdk/src/token/EvmERC20WarpRouteReader.ts +++ b/typescript/sdk/src/token/EvmERC20WarpRouteReader.ts @@ -6,6 +6,7 @@ import { HypERC4626Collateral__factory, HypERC4626OwnerCollateral__factory, HypERC4626__factory, + ProxyAdmin__factory, TokenRouter__factory, } from '@hyperlane-xyz/core'; import { @@ -22,6 +23,7 @@ import { } from '@hyperlane-xyz/utils'; import { DEFAULT_CONTRACT_READ_CONCURRENCY } from '../consts/concurrency.js'; +import { DeployedOwnableConfig } from '../deploy/types.js'; import { EvmHookReader } from '../hook/EvmHookReader.js'; import { EvmIsmReader } from '../ism/EvmIsmReader.js'; import { MultiProvider } from '../providers/MultiProvider.js'; @@ -29,6 +31,7 @@ import { DestinationGas, RemoteRouters } from '../router/types.js'; import { ChainNameOrId } from '../types.js'; import { HyperlaneReader } from '../utils/HyperlaneReader.js'; +import { proxyAdmin } from './../deploy/proxy.js'; import { CollateralExtensions } from './config.js'; import { TokenMetadata } from './types.js'; @@ -64,12 +67,14 @@ export class EvmERC20WarpRouteReader extends HyperlaneReader { const baseMetadata = await this.fetchMailboxClientConfig(warpRouteAddress); const tokenMetadata = await this.fetchTokenMetadata(type, warpRouteAddress); const remoteRouters = await this.fetchRemoteRouters(warpRouteAddress); + const proxyAdmin = await this.fetchProxyAdminConfig(warpRouteAddress); const destinationGas = await this.fetchDestinationGas(warpRouteAddress); return { ...baseMetadata, ...tokenMetadata, remoteRouters, + proxyAdmin, destinationGas, type, } as TokenRouterConfig; @@ -248,6 +253,21 @@ export class EvmERC20WarpRouteReader extends HyperlaneReader { ); } + async fetchProxyAdminConfig( + tokenAddress: Address, + ): Promise { + const proxyAdminAddress = await proxyAdmin(this.provider, tokenAddress); + const proxyAdminInstance = ProxyAdmin__factory.connect( + proxyAdminAddress, + this.provider, + ); + + return { + address: proxyAdminAddress, + owner: await proxyAdminInstance.owner(), + }; + } + async fetchDestinationGas( warpRouteAddress: Address, ): Promise { diff --git a/typescript/sdk/src/token/deploy.ts b/typescript/sdk/src/token/deploy.ts index 1c55191268..aa7376c883 100644 --- a/typescript/sdk/src/token/deploy.ts +++ b/typescript/sdk/src/token/deploy.ts @@ -29,6 +29,7 @@ import { hypERC721factories, } from './contracts.js'; import { + TokenMetadataSchema, TokenRouterConfig, isCollateralConfig, isNativeConfig, @@ -106,13 +107,16 @@ abstract class TokenDeployer< for (const [chain, config] of Object.entries(configMap)) { if (isTokenMetadata(config)) { - return config; + return TokenMetadataSchema.parse(config); } if (isNativeConfig(config)) { const nativeToken = multiProvider.getChainMetadata(chain).nativeToken; if (nativeToken) { - return { totalSupply: DERIVED_TOKEN_SUPPLY, ...nativeToken }; + return TokenMetadataSchema.parse({ + totalSupply: DERIVED_TOKEN_SUPPLY, + ...nativeToken, + }); } } @@ -128,11 +132,11 @@ abstract class TokenDeployer< erc721.name(), erc721.symbol(), ]); - return { + return TokenMetadataSchema.parse({ name, symbol, totalSupply: DERIVED_TOKEN_SUPPLY, - }; + }); } let token: string; @@ -161,7 +165,12 @@ abstract class TokenDeployer< erc20.decimals(), ]); - return { name, symbol, decimals, totalSupply: DERIVED_TOKEN_SUPPLY }; + return TokenMetadataSchema.parse({ + name, + symbol, + decimals, + totalSupply: DERIVED_TOKEN_SUPPLY, + }); } }