Skip to content

updateable contracts part 1 #552

New issue

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

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

Already on ? Sign in to your account

Merged
merged 12 commits into from
Mar 4, 2022
Merged

Conversation

Westlad
Copy link
Contributor

@Westlad Westlad commented Mar 1, 2022

DO NOT RELY ON contract-update.md for reviewing this PR. The pattern used here is quite different. Instead, read the Openzeppelin Upgades link below. We'll update the documentation when these changes are finalised.

This PR deploys proxied contracts that can be upgraded. To keep PRs small, this is the first of (probably) two PRs. This PR simply adds upgradable contracts, it does not provide any means to carry out an upgrade; that will follow in a second PR.

The PR leans heavily on Openzepplin's Upgrades library. Nevertheless, some changes are required to make contracts upgradeable. This is mainly:

  • proxies are oblivious to constructors and thus constructors are replaced by initialize() functions (note the US spelling or expect hard to find bugs). The initializer modifier ensures they can only be called once.
  • .transfer no longer works because it has heavily restricted gas to prevent reentrancy. We replace it with .call. This increases the risk of reentrancy attacks and so functions use a checks-effects-interactions pattern normally, but all are additionally protected by the Openzepplin RentrancyGuard modifier which prevents a modified function being called reenetrantly in any case.

Testing is as normal but you will need to run ./setup-nightfall to incorporate changes to the packages used. Note that the contracts are actually being called transparently through their proxies in the tests.

Copy link
Contributor

@signorecello signorecello left a comment

Choose a reason for hiding this comment

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

Great stuff! I would just remove the console.logs used for debugging, though

@WestladWestlad added the DNMDo not mergelabel Mar 1, 2022
@WestladWestlad force-pushed the westlad/update-contracts branch from dea3f73 to a2fe752 Compare March 1, 2022 16:46
@WestladWestlad removed the DNMDo not mergelabel Mar 1, 2022
@ChaitanyaKonda
Copy link
Contributor

This PR looks good.

To understand what I should review, I relied on the plugin documentation link you provided.
I presume the following changes as suggested in upgrade plugin will come in the next PR as suggested in your description.

The PR leans heavily on Openzepplin's Upgrades library. Nevertheless, some changes are required to make contracts upgradeable.

  • Config.sol defining initial values for field declarations
  • Stateful.sol to initialise state.sol inside initialize()
  • Adding Upgradeable suffix to contract names
  • Utils.sol defining initial values for field declarations

@ChaitanyaKondaChaitanyaKonda self-requested a review March 4, 2022 08:32
@ChaitanyaKondaChaitanyaKonda merged commit 2efb78d into master Mar 4, 2022
@ChaitanyaKondaChaitanyaKonda deleted the westlad/update-contracts branch March 4, 2022 08:33
@WestladWestlad mentioned this pull request Mar 9, 2022
Sign up for free to join this conversation on . 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