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 admin.getInstance #274

Merged
merged 5 commits into from
Jan 15, 2021
Merged

Conversation

pinglamb
Copy link
Contributor

@pinglamb pinglamb commented Jan 7, 2021

This PR is for kicking off #173.

An deployedProxyAdmin method is added to admin module to get ProxyAdmin instance (feel free to suggest names). My use case is simply to get the address programmatically without fs.readFileSync the manifest.

@pinglamb
Copy link
Contributor Author

pinglamb commented Jan 7, 2021

@frangio Please feel free to suggest the proper naming and code structure. Thanks!

@frangio
Copy link
Contributor

frangio commented Jan 7, 2021

Thanks @pinglamb! What is the reason for exposing it as a function (i.e. upgrades.admin.deployedProxyAdmin()) rather than just a getter (upgrades.admin.instance)?

@pinglamb
Copy link
Contributor Author

pinglamb commented Jan 7, 2021

It seems that if it is an instance, once hre.upgrades is called it will try to load the ProxyAdmin object. For example, if you are using it for the first time, it will throw the No ProxyAdmin ... error. Also I am not sure if there will be other issues if we are caching the contract object with hre.upgrades. I guess the safest is loading it on demand? What do you think?

@pinglamb pinglamb changed the title Added admin.instance Added admin.deployedProxyAdmin Jan 7, 2021
@frangio
Copy link
Contributor

frangio commented Jan 14, 2021

Yes that makes sense. I think the function name getInstance sounds better to me. It's used as upgrades.admin.getInstance().

We have to add the same function in the Truffle plugin. The code is very similar. Can you do these changes?

@pinglamb pinglamb marked this pull request as ready for review January 15, 2021 08:37
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.

Thank you @pinglamb! I've changed the tests a little to give us more confidence that the return value is correct.

@frangio frangio changed the title Added admin.deployedProxyAdmin Add admin.getInstance Jan 15, 2021
@frangio frangio merged commit 0d62287 into OpenZeppelin:master Jan 15, 2021
@pinglamb pinglamb deleted the proxy-admin-instance branch January 15, 2021 22:45
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.

2 participants