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

Add arg configuration to proxyAdmin and allow name to act as artifact #142

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

Conversation

outdoteth
Copy link

@outdoteth outdoteth commented Jun 25, 2021

At the moment [owner] is passed as the constructor argument to every custom AdminContract in the proxy option. This is not desireable since a lot of ProxyAdmin contracts will take different constructor arguments. Specifically, for my use case the OpenZeppelin ProxyAdmin has no constructor at all.

This change allows you to specify what arguments get passed to the AdminContract.

This change also adds a fall back to the artifact being the name if no artifactName is specified. So here, because no artifactName is specified, both the name and artifact is "ProxyAdmin".

For example:

    await deploy('Foo', {
        from: deployer,
        proxy: {
            proxyContract: "TransparentUpgradeableProxy",
            viaAdminContract: { 
                name: "ProxyAdmin", // Use "ProxyAdmin" as the artifact as well as the name
                args: [], // Allows you to pass in custom arguments for ProxyAdmin constructor here
            },
        },
    });

@outdoteth
Copy link
Author

Just a side note, I think the naming could be improved too. viaAdminContract is a little verbose. Just adminContract would be more consistent imo.

src/helpers.ts Outdated
try {
proxyAdminDeployed = await partialExtension.get(proxyAdminName);
} catch (e) {
console.log(
Copy link
Owner

Choose a reason for hiding this comment

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

it should not be a console.log as this would show up everywhere including test output

Copy link
Author

Choose a reason for hiding this comment

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

Ok I removed the log completely as I see you have done in here:

Screenshot 2021-07-12 at 13 51 51

Copy link
Author

Choose a reason for hiding this comment

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

@wighawag just pinging.

Copy link
Owner

Choose a reason for hiding this comment

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

Hi Dylan, sorry did not find the time yet to fully review it, but in principle it looks good. Hopefully I ll find some time before I am back from my travel.

@marcelomorgado
Copy link

Hi guys, any update here? Would be useful having args for custom proxy admin contracts.

@marcelomorgado
Copy link

Hi guys, any update here? Would be useful having args for custom proxy admin contracts.

Sorry @Dylan-Kerler @wighawag but I'm really needing this 🤣

@outdoteth
Copy link
Author

Hey @marcelomorgado no update from me. AFAIK this is good to merge, it’s just waiting on @wighawag to review it.

@wighawag
Copy link
Owner

I ll try to review soon.

By the way regarding openzeppelin proxy admin and the impossibility for it to be provided an specific owner at construction time, the hardhat-deploy proxy admin is a simple modification to it so you do not waste gas by setting the owner twice, see discussion here : OpenZeppelin/openzeppelin-contracts#2639 (comment)

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.

3 participants