VAL-114 First cut of adding an upgradeable contract #127
Conversation
| /** | ||
| * @dev Whether the protocol is paused. | ||
| */ | ||
| bool public paused = false; | ||
| bool public paused; |
There was a problem hiding this comment.
All non-constant values should be set in the initializer, ideally, otherwise they risk getting reset on an upgrade unexpectedly.
| /** | ||
| * @inheritdoc UUPSUpgradeable | ||
| */ | ||
| function _authorizeUpgrade(address) internal override onlyDeployer {} |
There was a problem hiding this comment.
This is where I understand the access control is enforced. I wrote tests verifying this, but it'd be great to get a 2nd set of eyes that there's not another entrypoint that I missed.
| * upgrades. | ||
| */ | ||
| abstract contract DeployerUUPSUpgradeable is | ||
| IServiceConfigurable, |
There was a problem hiding this comment.
Not 100% on this pattern, but the idea was:
- Have a parent type that would force derived types to have a reference to the serviceConfig (done through IServiceConfigurable).
- Have an abstract or concrete type that implements the critical-path, access control for upgrades (DeployerUUPSUpgradeable).
Types needing upgradeability inherit from this type, add an initialize(), ensure they have a reference to the ServiceConfig, and should be good to go.
| "PerimeterPoolToken", | ||
| "PPT" | ||
| // Create beacon proxy | ||
| BeaconProxy proxy = new BeaconProxy( |
| import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; | ||
| import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; | ||
| import {SafeERC20Upgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol"; |
There was a problem hiding this comment.
Annoyingly, everything needs to be the *Upgradeable OpenZep library
| @@ -1421,4 +1429,25 @@ describe("Pool", () => { | |||
| ); | |||
| }); | |||
| }); | |||
|
|
|||
| describe("Updates", () => { | |||
There was a problem hiding this comment.
Updates for the Pool ala beacon proxies
| @@ -189,4 +205,37 @@ describe("ServiceConfiguration", () => { | |||
| ); | |||
| }); | |||
| }); | |||
|
|
|||
| describe("Upgrades", () => { | |||
There was a problem hiding this comment.
Upgrades for ServiceConfig using UUPS
| /** | ||
| * Get commonly-used signers. | ||
| */ | ||
| export async function getCommonSigners() { |
There was a problem hiding this comment.
I think this makes testing a lot easier, but lmk if you see any downsides...
There was a problem hiding this comment.
maybe bob and alice can just be bob and alice, but I think this is nice to get named accounts. Only downside is it still requires proper setup to make their semantic names accurate.
| @@ -60,10 +62,7 @@ export async function deployPool({ | |||
| ); | |||
|
|
|||
| const PoolFactory = await ethers.getContractFactory("PoolFactory", { | |||
| signer: poolAdmin, | |||
There was a problem hiding this comment.
PoolFactory now longer needs it since it just emits proxies (basically blank contracts)
| */ | ||
| modifier onlyDeployer() { | ||
| require( | ||
| msg.sender != address(0) && |
There was a problem hiding this comment.
is msg.sender ever the null address?
There was a problem hiding this comment.
I don't think so, but wasn't sure if there was some odd lifecycle consideration. Will remove if you think it's safe to do so!
Introduces 2 patterns for upgrades:
DeployerUUPSUpgradeablethat can be derived from to simplify usage. Basically just needs the service config to be injected into the contract, and the constructor to be implemented as an initialize method.For 1), I updated
ServiceConfigurationto use this. For 2), I updated the PoolFactory / Pool.