upkeep

[aalavandhan]

• Bumped up solidity version on all contracts
• Using latest version of oz dependencies for all contracts (except UFragments and UFragmentsPolicy which are upgradable and live on mainnet, so we keep the old dependencies to preserve their storage layout)
• Added a deployment script to replicate mainnet setup
• Testnet deployment on goerli

[brandoniles]

I noticed you kept Safemath for uFragments, UFragmentsPolicy, and WAMPL (maybe others). Thinking to update those to normal arithmetic operators eventually?

[aalavandhan]

I noticed you kept Safemath for uFragments, UFragmentsPolicy, and WAMPL (maybe others). Thinking to update those to normal arithmetic operators eventually?

Only uFragments and UFragmentsPolicy use them (keeping them for preserving the storage state as mentioned above). Dont think it's used anywhere else, but if not we can safely move to normal operators.

[moodmosaic]

@aalavandhan, what was the motivation to make this (and others, e.g. MedianOracle) upgradeable?

[aalavandhan]

I didn't want to use both the upgradable and non-upgradable versions of the openzeppelin contracts as dependencies, as effectively they are the same except how the storage layout is organized.

Ultimately weather or not its upgradable (ie sits behind a proxy) depends on how its deployed.

AMPL and the policy we use deployProxy https://github.com/ampleforth/ampleforth-contracts/blob/master/scripts/deploy.ts#L52

For wampl, median oracle and others we use deployContract https://github.com/ampleforth/ampleforth-contracts/blob/master/scripts/deploy.ts#L201

[moodmosaic]

With the proxy-based upgradeability system, no constructors can be used, though. So you end up with this:

constructor(address ampl) {
    _ampl = ampl;
 }

function init(string memory name_, string memory symbol_) public initializer {
    __ERC20_init(name_, symbol_);
    __ERC20Permit_init(name_);

but what you really need is this (since it's not really upgradable) which is both idiomatic and beautiful

constructor(
    address ampl,
    string memory name_,
    string memory symbol_
) ERC20(name_, symbol_) ERC20Permit(name_) {
    _ampl = ampl;

Some of the bugs I've seen, originate from the fact that we (sometimes myself included) tend to program in a language vs into a language.

It's just that using openzeppelin/contracts-upgradeable for non-upgradable contracts makes things more implicit, but we've seen in the past that usually explicit is better than implicit (and simple is better than complex).

So why not just use both, at the end:

   "dependencies": {
+    "@openzeppelin/contracts": "^4.7.3",
     "@openzeppelin/contracts-upgradeable": "^4.7.3"
   },

And if there are strong reasons for keeping things as-is, perhaps it'd be nice to see some slither checks.

---

Absolutely love the project and your work, hope my comment didn't scare you! ❤️ 🍻

[moodmosaic]

So why not just use both, at the end:

   "dependencies": {
+    "@openzeppelin/contracts": "^4.7.3",
     "@openzeppelin/contracts-upgradeable": "^4.7.3"
   },

Or, perhaps, since a potential V2 version of the contracts might be non-upgradable, why not just remove contracts-upgradeable entirely on master. I gave this a shot and it looks pretty clean, and all tests are passing:

diff --git a/contracts/MedianOracle.sol b/contracts/MedianOracle.sol
index c201c8a..ce62860 100644
--- a/contracts/MedianOracle.sol
+++ b/contracts/MedianOracle.sol
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-3.0-or-later
 pragma solidity 0.8.4;
 
-import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
+import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
 import "./lib/Select.sol";
 
 interface IOracle {
@@ -14,7 +14,7 @@ interface IOracle {
  * @notice Provides a value onchain that's aggregated from a whitelisted set of
  *         providers.
  */
-contract MedianOracle is OwnableUpgradeable, IOracle {
+contract MedianOracle is Ownable, IOracle {
     struct Report {
         uint256 timestamp;
         uint256 payload;
@@ -57,17 +57,16 @@ contract MedianOracle is OwnableUpgradeable, IOracle {
      * @param minimumProviders_ The minimum number of providers with valid
      *                          reports to consider the aggregate report valid.
      */
-    function init(
+    constructor(
         uint256 reportExpirationTimeSec_,
         uint256 reportDelaySec_,
         uint256 minimumProviders_
-    ) public initializer {
+    ) public {
         require(reportExpirationTimeSec_ <= MAX_REPORT_EXPIRATION_TIME);
         require(minimumProviders_ > 0);
         reportExpirationTimeSec = reportExpirationTimeSec_;
         reportDelaySec = reportDelaySec_;
         minimumProviders = minimumProviders_;
-        __Ownable_init();
     }
 
     /**
diff --git a/contracts/WAMPL.sol b/contracts/WAMPL.sol
index dc412f1..2023856 100644
--- a/contracts/WAMPL.sol
+++ b/contracts/WAMPL.sol
@@ -1,13 +1,12 @@
 // SPDX-License-Identifier: GPL-3.0-or-later
+
 pragma solidity 0.8.4;
 
-import {IERC20Upgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol";
+import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
-import {SafeERC20Upgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol";
+import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
-import {ERC20Upgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol";
+import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
-import {ERC20PermitUpgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/draft-ERC20PermitUpgradeable.sol";
+import {ERC20Permit} from "@openzeppelin/contracts/token/ERC20/extensions/draft-ERC20Permit.sol";
 
 /**
  * @title WAMPL (Wrapped AMPL).
@@ -26,8 +25,8 @@ import {ERC20PermitUpgradeable} from "@openzeppelin/contracts-upgradeable/token/
  *
  *      We call wAMPL the "wrapper" token and AMPL the "underlying" or "wrapped" token.
  */
-contract WAMPL is ERC20Upgradeable, ERC20PermitUpgradeable {
-    using SafeERC20Upgradeable for IERC20Upgradeable;
+contract WAMPL is ERC20, ERC20Permit {
+    using SafeERC20 for IERC20;
 
     //--------------------------------------------------------------------------
     // Constants
@@ -43,18 +42,15 @@ contract WAMPL is ERC20Upgradeable, ERC20PermitUpgradeable {
 
     //--------------------------------------------------------------------------
 
     /// @notice Contract constructor.
     /// @param ampl The AMPL ERC20 token address.
-    constructor(address ampl) {
-        _ampl = ampl;
-    }
-
-    /// @notice Contract state initialization.
     /// @param name_ The wAMPL ERC20 name.
     /// @param symbol_ The wAMPL ERC20 symbol.
-    function init(string memory name_, string memory symbol_) public initializer {
-        __ERC20_init(name_, symbol_);
-        __ERC20Permit_init(name_);
+    constructor(
+        address ampl,
+        string memory name_,
+        string memory symbol_
+    ) ERC20(name_, symbol_) ERC20Permit(name_) {
+        _ampl = ampl;
     }
 
     //--------------------------------------------------------------------------
@@ -237,7 +233,7 @@ contract WAMPL is ERC20Upgradeable, ERC20PermitUpgradeable {
         uint256 amples,
         uint256 wamples
     ) private {
-        IERC20Upgradeable(_ampl).safeTransferFrom(from, address(this), amples);
+        IERC20(_ampl).safeTransferFrom(from, address(this), amples);
 
         _mint(to, wamples);
     }
@@ -255,13 +251,13 @@ contract WAMPL is ERC20Upgradeable, ERC20PermitUpgradeable {
     ) private {
         _burn(from, wamples);
 
-        IERC20Upgradeable(_ampl).safeTransfer(to, amples);
+        IERC20(_ampl).safeTransfer(to, amples);
     }
 
     /// @dev Queries the current total supply of AMPL.
     /// @return The current AMPL supply.
     function _queryAMPLSupply() private view returns (uint256) {
-        return IERC20Upgradeable(_ampl).totalSupply();
+        return IERC20(_ampl).totalSupply();
     }
 
     //--------------------------------------------------------------------------
diff --git a/package.json b/package.json
index 32d611c..865af40 100644
--- a/package.json
+++ b/package.json
@@ -30,7 +30,7 @@
     "lint"
   ],
   "dependencies": {
-    "@openzeppelin/contracts-upgradeable": "^4.7.3"
+    "@openzeppelin/contracts": "^4.7.3"
   },
   "devDependencies": {
     "@ethersproject/abi": "^5.6.4",
diff --git a/test/unit/MedianOracle.ts b/test/unit/MedianOracle.ts
index ee2eb02..19636f6 100644
--- a/test/unit/MedianOracle.ts
+++ b/test/unit/MedianOracle.ts
@@ -22,8 +22,7 @@ async function setupContractsAndAccounts() {
   C = accounts[3]
   D = accounts[4]
   factory = await ethers.getContractFactory('MedianOracle')
-  oracle = await factory.deploy()
-  await oracle.init(60, 10, 1)
+  oracle = await factory.deploy(60, 10, 1)
   await oracle.deployed()
 }
 
diff --git a/test/unit/WAMPL.ts b/test/unit/WAMPL.ts
index b78bf97..0d7cf8d 100644
--- a/test/unit/WAMPL.ts
+++ b/test/unit/WAMPL.ts
@@ -46,8 +46,8 @@ async function setupContracts() {
   await ampl.setMonetaryPolicy(deployerAddress)
 
   const wAMPLFactory = await ethers.getContractFactory('WAMPL')
-  wAMPL = await wAMPLFactory.connect(deployer).deploy(ampl.address)
-  await wAMPL.init(NAME, SYMBOL)
+  wAMPL = await wAMPLFactory.connect(deployer).deploy(ampl.address, NAME, SYMBOL)
 }
 
 describe('WAMPL', () => {
diff --git a/yarn.lock b/yarn.lock
index 2016367..0bdcdaa 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -798,10 +798,10 @@
     "@types/sinon-chai" "^3.2.3"
     "@types/web3" "1.0.19"
 
-"@openzeppelin/contracts-upgradeable@^4.7.3":
-  version "4.7.3"
-  resolved "https://registry.yarnpkg.com/@openzeppelin/contracts-upgradeable/-/contracts-upgradeable-4.7.3.tgz#f1d606e2827d409053f3e908ba4eb8adb1dd6995"
-  integrity sha512-+wuegAMaLcZnLCJIvrVUDzA9z/Wp93f0Dla/4jJvIhijRrPabjQbZe6fWiECLaJyfn5ci9fqf9vTw3xpQOad2A==
+"@openzeppelin/contracts@^4.7.3":
+  version "4.8.3"
+  resolved "https://registry.yarnpkg.com/@openzeppelin/contracts/-/contracts-4.8.3.tgz#cbef3146bfc570849405f59cba18235da95a252a"
+  integrity sha512-bQHV8R9Me8IaJoJ2vPG4rXcL7seB7YVuskr4f+f5RyOStSZetwzkWtoqDMl5erkBJy0lDRUnIR2WIkPiC0GJlg==
 
 "@openzeppelin/hardhat-upgrades@^1.19.0":
   version "1.21.0"