Skip to content

Commit

Permalink
fix: warp deploy apply ownership fixes (#4726)
Browse files Browse the repository at this point in the history
### 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 #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>
  • Loading branch information
xeno097 and paulbalaji authored Nov 1, 2024
1 parent fa06690 commit 4c0605d
Show file tree
Hide file tree
Showing 12 changed files with 232 additions and 18 deletions.
6 changes: 6 additions & 0 deletions .changeset/pink-bats-mix.md
Original file line number Diff line number Diff line change
@@ -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
21 changes: 19 additions & 2 deletions typescript/cli/src/config/warp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { stringify as yamlStringify } from 'yaml';

import {
ChainMap,
DeployedOwnableConfig,
IsmConfig,
IsmType,
MailboxClientConfig,
Expand All @@ -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';

Expand Down Expand Up @@ -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 = {};
Expand All @@ -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
Expand Down Expand Up @@ -190,6 +202,7 @@ export async function createWarpRouteDeployConfig({
mailbox,
type,
owner,
proxyAdmin,
isNft,
interchainSecurityModule,
token: await input({
Expand All @@ -203,6 +216,7 @@ export async function createWarpRouteDeployConfig({
type,
owner,
isNft,
proxyAdmin,
collateralChainName: '', // This will be derived correctly by zod.parse() below
interchainSecurityModule,
};
Expand All @@ -216,6 +230,7 @@ export async function createWarpRouteDeployConfig({
mailbox,
type,
owner,
proxyAdmin,
isNft,
interchainSecurityModule,
token: await input({
Expand All @@ -230,6 +245,7 @@ export async function createWarpRouteDeployConfig({
mailbox,
type,
owner,
proxyAdmin,
isNft,
interchainSecurityModule,
token: await input({
Expand All @@ -242,6 +258,7 @@ export async function createWarpRouteDeployConfig({
mailbox,
type,
owner,
proxyAdmin,
isNft,
interchainSecurityModule,
};
Expand Down
59 changes: 58 additions & 1 deletion typescript/cli/src/utils/input.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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<DeployedOwnableConfig> {
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.
Expand Down
4 changes: 3 additions & 1 deletion typescript/sdk/src/deploy/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<typeof OwnableSchema>;

export type DeployedOwnableConfig = z.infer<typeof DeployedOwnableSchema>;

export interface CheckerViolation {
chain: ChainName;
type: string;
Expand Down
1 change: 1 addition & 0 deletions typescript/sdk/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ export { HyperlaneProxyFactoryDeployer } from './deploy/HyperlaneProxyFactoryDep
export {
CheckerViolation,
OwnableConfig,
DeployedOwnableConfig,
OwnerViolation,
ProxyAdminViolation,
ViolationType,
Expand Down
29 changes: 23 additions & 6 deletions typescript/sdk/src/router/ProxiedRouterDeployer.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { constants } from 'ethers';

import {
ProxyAdmin,
ProxyAdmin__factory,
Router,
TimelockController,
TimelockController__factory,
Expand Down Expand Up @@ -82,12 +84,27 @@ export abstract class ProxiedRouterDeployer<
chain: ChainName,
config: Config,
): Promise<HyperlaneContracts<Factories & ProxiedFactories>> {
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;
Expand Down
3 changes: 2 additions & 1 deletion typescript/sdk/src/router/schemas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -29,6 +29,7 @@ export const RouterConfigSchema = MailboxClientConfigSchema.merge(
).merge(
z.object({
remoteRouters: RemoteRoutersSchema.optional(),
proxyAdmin: DeployedOwnableSchema.optional(),
}),
);

Expand Down
4 changes: 4 additions & 0 deletions typescript/sdk/src/schemas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
});
50 changes: 48 additions & 2 deletions typescript/sdk/src/token/EvmERC20WarpModule.hardhat-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,7 @@ describe('EvmERC20WarpHyperlaneModule', async () => {
paused: false,
},
];

it('should deploy and set a new Ism', async () => {
const config = {
...baseConfig,
Expand Down Expand Up @@ -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: {
Expand All @@ -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(
Expand All @@ -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 = {
Expand Down
34 changes: 34 additions & 0 deletions typescript/sdk/src/token/EvmERC20WarpModule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
addressToBytes32,
assert,
deepEquals,
eqAddress,
isObjEmpty,
objMap,
rootLogger,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -286,6 +288,38 @@ export class EvmERC20WarpModule extends HyperlaneModule<
);
}

updateProxyAdminOwnershipTxs(
actualConfig: Readonly<TokenRouterConfig>,
expectedConfig: Readonly<TokenRouterConfig>,
): 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.
*
Expand Down
Loading

0 comments on commit 4c0605d

Please sign in to comment.