Skip to content
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.

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

Custom Deployments #273

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

Conversation

bh2smith
Copy link

@bh2smith bh2smith commented Jan 6, 2021

Addresses Issue #272 by allowing projects depending on this plugin to specify their own deploy function. If a deployment executor is not specified, then the initial (default) implementation is used as a fallback.

Currently this remains untested apart from the fact that all existing tests still pass. Open to suggestions for testing!

Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

Hey @bh2smith, thank you so much for this PR. Sorry I took a while to get to this, I was focused on getting #261 done.

I think this is a really good idea and something I've also been thinking about. I've written some comments to explore the idea a little bit more before committing to it.

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

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.

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!

@bh2smith
Copy link
Author

Hey @frangio and thanks for your response. I will attempt to address your questions/comment, thinking that only 1 of the two will be possible. I am, however, running into some awkward inconveniences with the return value of ContractTransaction from our end and will submit a draft PR pointing out this issue ASAP. At least then we will have a use case in mind and maybe get some impression of how to properly test this.

@frangio
Copy link
Contributor

frangio commented Jan 18, 2021

@bh2smith I took a quick look at your PR. Having to manually fill in the values for a ContractTransaction is definitely bad. It makes sense for the default deployer which simply does factor.deploy(...args) but for other kinds of deployments it forces you to fill in those values, returning only the transaction hash would be a lot easier.

Regarding your ignoring the factory argument, I understood that you will change that instead of hardcoding your contract inside the deployer, right? I didn't understand if that was motivated by a problem in the plugin.

I imagine you weren't able to use your branch because this is a monorepo. Unfortunately I don't know if there is a way to overcome that.

@nfurfaro
Copy link

nfurfaro commented Jan 18, 2021

Thanks for the work on this front. Really looking forward to being able to pass in a custom deploy function. Ideally, I would be able to use the deploy function (from the Hardhat-Deploy plugin) as my deploy function, as this makes so many things simpler(especially if you're already using the hardhat-deploy plugin in parallel with hardhat-upgrades).

https://hardhat.org/plugins/hardhat-deploy.html

@bh2smith
Copy link
Author

bh2smith commented Jan 19, 2021

returning only the transaction hash would be a lot easier.

Yes, I would really hope we can make a move towards this ASAP!

Regarding your ignoring the factory argument, I understood that you will change that instead of hardcoding your contract inside the deployer, right? I didn't understand if that was motivated by a problem in the plugin.

Yes, this is only temporary so I can see that the authenticator is actually upgradable

@bh2smith
Copy link
Author

@nfurfaro

especially if you're already using the hardhat-deploy

Yes, we are indeed!

@bh2smith
Copy link
Author

I have reverted the introduction of the temporary HardhatDeployment and everything seems fine from this side. I will attempt to make out working example align with this and see how it looks.

mergify bot pushed a commit to gnosis/gp-v2-contracts that referenced this pull request Feb 3, 2021
Closes #167 

This PR introduces minor changes to the `AllowListAuthenticator` making it upgradable and includes tests verifying upgradability. In particular:

- removing `customInitiallyOwnable` as it is no longer used: Not yet sure how this will affect the `deployments` directory which still includes it.
- Updating authenticator deployment script to deploy as proxy.
- rename authenticator `owner` to `manager` because of name collision with proxy owner
- introduce proxy.ts with helper methods to fetch implementation and owner address.
- Include two unit tests: one verifying that upgrade is possible and the other to ensure preservation of storage.

Note that the unit tests involving Authenticator's non-upgrade related functionality do not use the proxy deployment as specified in the migration scripts, so that manager must be set immediately after deployment.

We had originally planned/hoped to use the `hardhat-upgrades` plugin and opened the following two PRs to `openzeppelin-upgrades`, but this plugin turned out to be unnecessary.

- Custom Deployments: OpenZeppelin/openzeppelin-upgrades#273
- Manual Safety Override: OpenZeppelin/openzeppelin-upgrades#280

### Test Plan

Old + new unit and e2e tests.
@frangio
Copy link
Contributor

frangio commented Feb 9, 2021

This is something we're interested in but are not prioritizing at the moment. If anyone is interested in this feature for their setup let us know!

@Ratimon
Copy link

Ratimon commented Jun 14, 2021

This is something we're interested in but are not prioritizing at the moment. If anyone is interested in this feature for their setup let us know!

Yes, I am looking forward to using this feature

@Amjad-W
Copy link

Amjad-W commented Apr 11, 2022

I love this feature, will this merge pass through?

@bh2smith
Copy link
Author

I love this feature, will this merge pass through?

This branch is now 10 months old. I think it should be considered stale. But you are welcome to brush the dust off and take over.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants