Depending on whether deployment promises resolve once the transaction was broadcast or mined, it may happen that two consecutive proxy deploys fail at the second one:

const proxy1 = await deployProxy(Impl, []);
const proxy2 = await deployProxy(Impl, []);

The first one will deploy Impl and put it in the impl store, the second one will fetch its address and verify that it has code using eth_getCode, but if it hasn't been mined yet it will fail (unless there is a way to call eth_getCode on the mempool?).

Note that this will probably not be detected by testing on the Buidler EVM (#21).

We currently serialize the deployment transactions for an implementation and a ProxyAdmin contract. For a faster experience we may want to send them to the blockchain at the same time. On the other hand, deploying a ProxyAdmin is only meant to happen once, and adding concurrency may increase the chance of odd issues happening, so we can decide to keep them serialized.

See 0004212 for an implementation of this idea for Buidler.

There was a global config object introduced in Truffle 5.1.35. It includes the contracts_build_directory, which is sometimes an automatic temporary directory, and is where we should be reading artifacts from instead of the currently hardcoded build/contracts directory.

We are showing users links along with validation errors that don't exist yet.

We have to create them. Ideally they would point to a new page that explains each error in detail. But as a quicker intermediate option we can redirect them all to https://docs.openzeppelin.com/upgrades/2.8/writing-upgradeable#potentially-unsafe-operations.

Include this project in docs.openzeppelin.com.

At the moment of deployProxy, we have to validate that the linked libraries are using code that has been validated to be upgrade safe. In the solc output for a contract we find the locations in the bytecode where the addresses of linked libraries will be found. During deployProxy we have to extract those addresses, fetch the bytecode at that address, and look it up in the validations object.

We may have issues with fetching the bytecode if the library deployment hasn't been mined yet. I think this is a real concern that we should think about a little bit before proceeding. This is what I meant in the first paragraph. It's different from #22 because we deploy those impls ourselves, but in my proposed workflow for libraries, it's the user who deploys those libraries so we don't have the information about the library deployment (e.g. "version", txid, etc.) unless we can somehow retrieve the bytecode from the address.

I don't think structs are enums are supported in storage layout checking. We don't have any specific code to handle that, and I think we may have to specifically check that there are no structs and enums and let the user know that we can't automatically verify their safety. We may also want to add a way to force an upgrade even if this can't be checked.

This will be until #3 is implemented.

The extracted storage layout contains type identifiers that contain AST node ids, such as t_struct(MyComplexStruct)1029_storage where 1029 is a node id.

I wrote a function stabilizeTypeIdentifier that removes node ids, but it would be an error to use it as is.

The problem is this transformation on type identifiers can produce clashes if there is a type (e.g. a struct) with the same name defined in different places.

struct S {
    uint u;
}

contract CParent {
    S s1;
}

contract CChild is CParent {
    struct S {
        uint u1;
        uint u2;
    }
    S s2;
}

The storage layout we get for contract CChild, once we include also the inherited state variables, will contain instances of both the top level S struct and the CChild.S struct, which have different layouts. Simply removing node ids from type identifiers will result in two equal type identifiers for different types.

We will run into this same issue once we use the storage layout returned by solc itself (#3).

I haven't thought about potential solutions yet. Maybe we can use the fully qualified name of the struct (e.g. CChild.S) in the type identifier.

Swapping the order in which the base contracts are declared doesn't result in storage layout error

Example below MyContract is A, B whilst MyContractV2 is B, A

//SPDX-License-Identifier: MIT
pragma solidity ^0.6.0;

contract A {
    uint256 private a;
}

//SPDX-License-Identifier: MIT
pragma solidity ^0.6.0;

contract B {
    string private b;
}

//SPDX-License-Identifier: MIT
pragma solidity ^0.6.0;

import "./A.sol";
import "./B.sol";

contract MyContract is A, B {
    function initialize() public {}
}

//SPDX-License-Identifier: MIT
pragma solidity ^0.6.0;

import "./A.sol";
import "./B.sol";

contract MyContractV2 is B, A {
}

Should use the debug package.

Reporting of failed validations and storage incompatibilities should be well polished.

Ideally we would print the line number where the error is presented, but initially we can print the name of the variable that introduced it.

Need a mechanism to be able to initialize state when upgrading so that it is part of the upgrade transaction.
Otherwise users would need to protect with some form of access control so they could do this in a separate transaction.

For example, adding a new state variable otherValue in version 2 and needing a safe way to initialize it on upgrade.

    function upgradeV2(uint256 initialValue) public {
        require (!_upgradeV2, "Box: already upgraded to V2");
        _upgradeV2 = true;
        
        otherValue = initialValue;
    }

We currently automatically call the function with name initialize. We may want to allow the user to choose some other function.

We currently don't check that a contract's linked libraries are also upgrade safe. The check for delegatecall leaves out libraries by looking for baredelegatecall types.

The difficulty with libraries is that it's not known at compile time what implementation is going to be linked. One option we could follow is to store the output from the compiler that specifies the bytecode locations where library addresses are inserted, then extract those addresses from a linked contract, obtain their version id (by fetching the bytecode and hashing it), and then look for the corresponding information in the manifest or in the validations cache (in the latter case then storing it in the manifest). We can assume for now that a contract will always be linked to a library whose source code will be present in the current project.

Consider prefixing the validations file with an upgrades-specific name like ${config.paths.cache}/upgrades.validations.json so it doesn't conflict with external cache files that might exist.

I initiall used sha256 because it's available without dependencies, but this will make it impossible to migrate OpenZeppelin CLI manifests as in #15. It's also more common in Ethereum to use keccak256 even for hashes that will only live off chain.

See src-decoder.ts.

Currently there doesn't appear to be a mechanism to get the proxy address other than copy the address from the output of deploying AdminUpgradeabilityProxy and hardcode it into migration scripts.

3_upgrade.js

const GreeterV2 = artifacts.require('GreeterV2');

const { upgradeProxy } = require('@openzeppelin/upgrades-truffle');

module.exports = async function (deployer) {
  const proxyAddress = "0x72440269630E393d38975Db7fA7Cb4D14e7eC061";
  await upgradeProxy(proxyAddress, GreeterV2, { deployer });
};

$ npx truffle console --network development
truffle(development)> const proxyAddress = "0x72440269630E393d38975Db7fA7Cb4D14e7eC061";
undefined
truffle(development)> greeter = await Greeter.at(proxyAddress)
undefined

Today we need to move to each package and run tests individually

Using the plugins to deploy proxies creates manifest files under the .openzeppelin directory. They are currently named 0x1234.json but in #19 they will be renamed to friendlier names like mainnet.json.

We need to have a clear recommendation on whether to put these files under version control.

The recommendation that I think makes sense generally is that all manifests except those for local development networks (Ganache, Buidler EVM) should be put under version control. I think this is also the current recommendation put forward for OpenZeppelin CLI users.

The version control recommendation is relevant to the naming decision in #19. It would be easier if we could propose a .gitignore rule like .openzeppelin/dev-*. If we decide to go with names like ganache.json and buidlerevm.json, each of those would need to have their own entry in .gitignore.

What is the best place to tell users to add the relevant .gitignore rules? I think it would make sense to make it a part of the installation instructions.

In #47 I introduced waitAndValidateDeployment to wait until a tx is mined.

There's at least two parameters here that te user might want to configure: the polling interval and the timeout. Libraries like Web3, Truffle, and Ethers already allow the user to configure these parameters and we should try to reuse them. We can also evaluate reusing their implementations for waiting a tx instead of rolling our own.

Related to #15. But we just have to finalize the format and define a manifestVersion field.

To avoid race conditions and corrupting the manifest file if it's ever used by two processes concurrently.

The OpenZeppelin CLI does a very coarse locking which only allows running a single instance at the same time. We should aim for finer locking, which we should be able to do given the way Manifest was designed.

1. Contracts with no initialize function.
2. Contracts with two initialize functions with different signatures. What should happen in this case?

Additionally, we might want to make the array argument optional for initializers with no arguments.

We currently retrieve the implementation using a function call:

We should read directly from storage using getStorageAt.

I would introduce a new module to contain this new getImplementation function. There should also be a getAdmin function even though it won't be used until #2 is implemented.

Can we improve upgrade error messages to not include the stack trace?

Error: New storage layout is incompatible
    at Object.assertStorageUpgradeSafe (/home/abcoathup/projects/uptruf/node_modules/@openzeppelin/upgrades-truffle/node_modules/@openzeppelin/upgrades-core/src/storage.ts:56:11)
    at upgradeProxy (/home/abcoathup/projects/uptruf/node_modules/@openzeppelin/upgrades-truffle/src/index.ts:94:3)
Truffle v5.1.36 (core: 5.1.36)
Node v10.21.0

State variable renaming is not allowed for upgrades, because it could have actually been an error by the user which implies a storage layout change.

We should offer a way to rename anyway. My preference would be a way for developers to explicitly mark a state variable as being a rename of the previously existing variable in the same storage location. Something like...

string private admin; // formerly: owner

The Buidler plugin will currently deploy both impl and proxy using the default signer. We should allow users to specify a different signer.

Instead of receing a contractName: string as argument to deployProxy and upgradeProxy, we could receive a ContractFactory instance which the user can create with any signer they want. Since the only element identifying the contract in the ContractFactory is the creation bytecode, this may imply changes in the format we identify contracts, since the version id is currently the hash of the deployed bytecode.

Solidity 0.5.13 introduced the storageLayout compiler output, which gives accurate information about the size and alignment of items in a storage layout. We should use this information instead of the AST. Given this accurate information, the use of the Levenshtein algorithm may have to change quite a bit.

Currently the tests for Truffle and buidler have deployProxy and upgradeProxy in the same script. In production they will be used in separate scripts as an upgrade will happen sometime after a deploy.

The existing upgrades library shows a single script doing deploy and upgrade and this has caused confusion as there aren't examples showing these distinct steps.

The tests act as examples (and will likely be used as the basis for documentation examples), so we should show how they intend to be used.

For example in Truffle we could use something like the following:

2_deploy.js

const Greeter = artifacts.require('Greeter');
const GreeterV2 = artifacts.require('GreeterV2');

const { deployProxy } = require('@openzeppelin/upgrades-truffle');

module.exports = async function (deployer) {
  await deployProxy(Greeter, ['Hello Truffle'], { deployer });
};

3_upgrade.js

const Greeter = artifacts.require('Greeter');
const GreeterV2 = artifacts.require('GreeterV2');

const { upgradeProxy } = require('@openzeppelin/upgrades-truffle');

module.exports = async function (deployer) {
  const greeter = await Greeter.deployed();
  await upgradeProxy(greeter.address, GreeterV2, { deployer });
};

I started building the manifest format from scratch because it was easier for me, but we have to decide whether we'll reuse the format that exists in OpenZeppelin CLI. It turns out that the existing CLI manifest contains a lot of information that is irrelevant for the plugins so we will define a new one. Thus we have to add the manifestVersion field (3.0?) and implementing automatic migration from older to newer manifest version for users who decide to migrate to the plugins.

• @openzeppelin/upgrades-buidler → @openzeppelin/buidler-upgrades
• @openzeppelin/upgrades-truffle → @openzeppelin/truffle-upgrades

Currently deployProxy and upgradeProxy are simply exported from a file. Buidler plugins are expected to extend the BRE using extendEnvironment.

Renaming state variable results in error New storage layout is incompatible

Renaming doesn't change storage layout.

//SPDX-License-Identifier: MIT
pragma solidity ^0.6.0;

contract MyContract {
    uint256 private x;
    string private y;

    function initialize() public {}
}

//SPDX-License-Identifier: MIT
pragma solidity ^0.6.0;

contract MyContractV2 {
    uint256 private x;
    string private renamey;
}

When using upgradeProxy no notification is shown that the upgrade has occurred.

All we see is that a new logic contract has been deployed:

$ npx truffle migrate --network development
 ...

   Deploying 'GreeterV2'
   ---------------------
   > transaction hash:    0x5336034f54058e62cd24f3e2ef1d408203a4312e8255e2a4e9806292f37367db
   > Blocks: 0            Seconds: 0
   > contract address:    0x55Db01E043a48bc0d11037c51E3911A9Eb8dbe24
   > block number:        212
   > block timestamp:     1596005809
   > account:             0x90F8bf6A479f320ead074411a4B0e7944Ea8c9C1
   > balance:             99.06622414
   > gas used:            290127 (0x46d4f)
   > gas price:           20 gwei
   > value sent:          0 ETH
   > total cost:          0.00580254 ETH


   > Saving migration to chain.
   > Saving artifacts
   -------------------------------------
   > Total cost:          0.04147856 ETH

There is currently only a simple test for the happy path.

A new module admin containing two new funcitons to handle admin and ownership.

• admin.transferOwnership
• admin.changeProxyAdmin

If the current sender does not own the ProxyAdmin, they get a revert with an Ownable error:

Error: Returned error: VM Exception while processing transaction: revert Ownable: caller is not the owner -- Reason given: Ownable: caller is not the owner.

As a nice to have, we could prevent this by checking if the sender is the owner of the proxyadmin before sending the tx. And since we're at it, we can also check that the proxy to upgrade is indeed owned by the proxyadmin.

We're currently using ${chainId}.json, which results in files named like 0x7a69.json. OpenZeppelin CLI uses a user friendly network name like mainnet.json, and anything that doesn't have a known name gets called dev-${networkId}.json. I think user friendly names make sense so that people know what the file is, but I don't like the dev- naming because some unknown ids will not be development, so maybe it should be id-.

The function getVersionId takes a hash of the deployedBytecode. This bytecode contains a hash of the metadata of the source files, which can change due to simple formatting changes in source files. We want to remove this metadata before taking the hash, so that changing formatting or comments doesn't result in a new "version" of the bytecode.

OpenZeppelin CLI should already have code that does this.

See 8be3822. Had to disable concurrency in Buidler tests that were using different instances of the Buidler EVM at the same time. This may be related to the fact that Buidler runs are deterministic, but it's weird that even tx hashes would be the same. We need to understand exactly what was going on there.

This is particularly important for the timeout in waitAndValidateDeployment because users may want to handle it in their code to retry.

Depends on trufflesuite/truffle#3221.

Truffle is ignoring our util.inspect.custom error display logic and as a result the user doesn't see validation errors. It seems it only picks up the message property in the error object.

We have to change our UpgradesError class so that it works in that context. The solution may be to add a message getter to our UpgradesError class that returns the full message + details.

We should still keep [util.inspect.custom] to get color in the terminal under Buidler.

If the plugins are interrupted while waiting for a transaction to be mined, we should make it possible to re-run it and pick up where it was interrupted. It should not be necessary to re-send the transaction (paying the cost again).

The way that we implement this is by storing on disk the transaction id as soon as it's sent to the network. The way I see this designed for deployments is that wherever we can store an object { address: string } (for example for storing the address of the proxy admin) we can also store an object { tx: string } that indicates a transaction is in progress and should be awaited instead of deploying. We have to consider the possibility that that tx never gets mined and how to detect that. For transactions which are not deployments, such as upgradeTo, I'm not sure where we should store the tx id.

This can be developed in two stages:

1. Deploy instances with a ProxyAdmin
2. Reuse ProxyAdmin

Now that we have both Truffle and Buidler plugins, there is a lot of duplication between their respective deployProxy and upgradeProxy functions, there is even duplication between them within the same plugin, and they are also incidentally very long functions.

It seems to me that we should be able to write abstract deployProxy and upgradeProxy functions that could be instantiated with the Truffle- or Buidler-specific parts.

I think what has to be astracted is:

• provider
• where validations come from
• how the impl is depoyed, given a contract factory
• how the proxy is deployed
• how the initialize data is encoded
• how to obtain the instance that will be returned

We may want to briefly experiment with using TypeDoc to generate input for the docs site. But if it's too complicated we should just write it fully by hand.