Skip to content

Custom Deployments #273

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Custom Deployments #273

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
cf991ce
allow users to provide their own deployment implementation
bh2smith Jan 6, 2021
3901829
Merge branch 'master' into custom-deployment
bh2smith Jan 19, 2021
2eed04f
reverting introduction of contract deployment
bh2smith Jan 19, 2021
da68e95
Merge branch 'custom-deployment' of github.com:bh2smith/openzeppelin-…
bh2smith Jan 19, 2021
8b724d9
rename DeploymentExecutor to TxExecutor
bh2smith Jan 19, 2021
8d7fba2
allow users to provide their own deployment implementation
bh2smith Jan 6, 2021
67b95de
reverting introduction of contract deployment
bh2smith Jan 19, 2021
fb3acd6
rename DeploymentExecutor to TxExecutor
bh2smith Jan 19, 2021
ca259db
Merge branch 'custom-deployment' of github.com:bh2smith/openzeppelin-…
bh2smith Jan 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions packages/plugin-hardhat/src/deploy-proxy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {

import { getProxyFactory, getProxyAdminFactory } from './proxy-factory';
import { readValidations } from './validations';
import { deploy } from './utils/deploy';
import { defaultDeploy, DeploymentExecutor, intoCoreDeployment } from './utils/deploy';

export interface DeployFunction {
(ImplFactory: ContractFactory, args?: unknown[], opts?: DeployOptions): Promise<Contract>;
Expand All @@ -22,6 +22,7 @@ export interface DeployFunction {

export interface DeployOptions extends ValidationOptions {
initializer?: string | false;
executor?: DeploymentExecutor;
}

export function makeDeployProxy(hre: HardhatRuntimeEnvironment): DeployFunction {
Expand All @@ -35,6 +36,7 @@ export function makeDeployProxy(hre: HardhatRuntimeEnvironment): DeployFunction
args = [];
}

const deploy = opts.executor || defaultDeploy;
const { provider } = hre.network;
const validations = await readValidations(hre);

Expand All @@ -43,17 +45,19 @@ export function makeDeployProxy(hre: HardhatRuntimeEnvironment): DeployFunction
assertUpgradeSafe(validations, version, opts);

const impl = await fetchOrDeploy(version, provider, async () => {
const deployment = await deploy(ImplFactory);
const deployment = intoCoreDeployment(await deploy(ImplFactory, []));
const layout = getStorageLayout(validations, version);
return { ...deployment, layout };
});

const AdminFactory = await getProxyAdminFactory(hre, ImplFactory.signer);
const adminAddress = await fetchOrDeployAdmin(provider, () => deploy(AdminFactory));
const adminAddress = await fetchOrDeployAdmin(provider, async () =>
intoCoreDeployment(await deploy(AdminFactory, [])),
);

const data = getInitializerData(ImplFactory, args, opts.initializer);
const ProxyFactory = await getProxyFactory(hre, ImplFactory.signer);
const proxy = await ProxyFactory.deploy(impl, adminAddress, data);
const proxy = await deploy(ProxyFactory, [impl, adminAddress, data]);

const inst = ImplFactory.attach(proxy.address);
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
Expand Down
15 changes: 10 additions & 5 deletions packages/plugin-hardhat/src/upgrade-proxy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,27 +16,32 @@ import {

import { getProxyAdminFactory } from './proxy-factory';
import { readValidations } from './validations';
import { deploy } from './utils/deploy';
import { defaultDeploy, DeploymentExecutor, intoCoreDeployment } from './utils/deploy';

export interface UpgradeOptions extends ValidationOptions {
executor?: DeploymentExecutor;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be a "tx executor" and not only a deployment executor.

I'm thinking that for upgradeProxy it should also take care of sending the upgrade function call. What do you think?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems legit, I will look into this!

}

export type PrepareUpgradeFunction = (
proxyAddress: string,
ImplFactory: ContractFactory,
opts?: ValidationOptions,
opts?: UpgradeOptions,
) => Promise<string>;

export type UpgradeFunction = (
proxyAddress: string,
ImplFactory: ContractFactory,
opts?: ValidationOptions,
opts?: UpgradeOptions,
) => Promise<Contract>;

async function prepareUpgradeImpl(
hre: HardhatRuntimeEnvironment,
manifest: Manifest,
proxyAddress: string,
ImplFactory: ContractFactory,
opts: ValidationOptions,
opts: UpgradeOptions,
): Promise<string> {
const deploy = opts.executor || defaultDeploy;
const { provider } = hre.network;
const validations = await readValidations(hre);

Expand All @@ -51,7 +56,7 @@ async function prepareUpgradeImpl(
assertStorageUpgradeSafe(deployment.layout, layout, opts.unsafeAllowCustomTypes);

return await fetchOrDeploy(version, provider, async () => {
const deployment = await deploy(ImplFactory);
const deployment = intoCoreDeployment(await deploy(ImplFactory, []));
return { ...deployment, layout };
});
}
Expand Down
20 changes: 15 additions & 5 deletions packages/plugin-hardhat/src/utils/deploy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,18 @@
import type { ContractFactory, ContractTransaction } from 'ethers';
import type { Deployment } from '@openzeppelin/upgrades-core';
import type { ContractFactory } from 'ethers';
export interface DeploymentExecutor {
(factory: ContractFactory): Promise<HardhatDeployment>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason this does not already return a "CoreDeployment"? In other words why do we need a HardhatDeployment with a ContractTransaction instead of the transaction hash.

I think the existence of intoCoreDeployment should be unnecessary.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I can't exactly recall why this was necessary, but I believe it had something to do with the transformation of the proxy deployment from

const proxy = await ProxyFactory.deploy(impl, adminAddress, data);

into

const proxy = await deploy(ProxyFactory, [impl, adminAddress, data]);

and its return values not matching up. Notice how we don't use intoCoreDeployment on this particular line.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, yes, I believe I recall why we needed HardhatDeployment here is because at the time we pass the function onto the deployer, we do not actually have the transaction hash and the functionality of your code base requires a provider in order to fetch transaction by hash... Sorry if this isn't entirely clear just yet, but I will try again and see if I can remind myself exactly why this workaround was introduced.

Copy link
Contributor

@frangio frangio Jan 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd imagine the deploy function that was originally in this file would have been a perfectly good default DeploymentExecutor, returning the hash instead of the full ContractTransaction.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I insist on this is that we currently have duplication of this layer across the Hardhat and Truffle plugins but we will want to merge those soon, and using the transaction hash is a more agnostic way to represent a deployment, instead of the Ethers-specific ContractTransaction.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally fine with me. I prefer to return only the hash any way. Have made the necessary adjustments and it also made the code on our end much more attractive and easy to write.

}

export interface HardhatDeployment {
address: string;
deployTransaction: ContractTransaction;
}

export function defaultDeploy(factory: ContractFactory, args: unknown[]): Promise<HardhatDeployment> {
return factory.deploy(...args);
}

export async function deploy(factory: ContractFactory): Promise<Deployment> {
const { address, deployTransaction } = await factory.deploy();
const txHash = deployTransaction.hash;
return { address, txHash };
export function intoCoreDeployment({ address, deployTransaction }: HardhatDeployment): Deployment {
return { address, txHash: deployTransaction.hash };
}