We should set up another job in Travis to run the tests on a development node in Geth, and possibly Parity.

address developer is already present in kernel's Release.

Should we include this state variable in all ImplementationDirectorys?

Function upgradeProxy from AppManagerWrapper should not send a transaction if the implementation address of the proxy matches the desired one.

Create basic tests for the example's DonationsV1.sol and DonationsV2.sol, agnostic of the zos app manager functionality.

Nice to have within Package

function versionsList() public returns (string[] versions)

Suggested by @martriay

From #12:

Add a RegistryAwareProxyFactory (name TBD), that inherits from UpgradeabilityProxyFactory, that does have a reference to the registry, and provides the convenience method createProxy(string version) that was lost with this update.

The Proxy contract calls delegatecall to some other implementation which may run out of gas. In that case, the proxy will revert. It will probably still have gas left because the delegatecall (like normal call) only forwards a fraction of the gas left to the callee (see EIP-150). Therefore, there is some information lost for the caller of the proxy: they cannot see that the call ran out of gas.

I think this is part of a bigger problem of the platform which is that there is no way of knowing whether a transaction failed because of running out of gas or some other reason.

We want to consider making the proxy "run out of gas" if the delegatecall failed and it consumed all available gas, so as to propagate this information upwards the call chain. It's not clear if or how this could be done.

Currently when we use a PackagedAppManager we have to register a new version in the Package and then ask the manager to upgrade each proxy. Discuss whether this functionality could be carried out by the manager.

Additionally consider renaming upgradeTo to upgrade

Atm, package.json shows that the dependency used is a relative path to a file:
"openzeppelin-zos": "file:../../../../zeppelin-solidity/openzeppelin-zos-1.9.0-beta.tgz"

I would like for us to review if and how the ProjectController should publicly expose the create and createAndCall functions.

Pros:

• Anyone can ask for a proxy that uses an implementation registered by the owner

Cons:

• The owner is responsible for tracking, upgrading and paying for the upgrade of every proxy that was created through her ProjectController since the user can't
• The owner can upgrade a user-created proxy without the user noticing it. The user should to be fully aware of this.

• Consider renaming upgradeTo to upgrade in contracts
• Review all other names in zos-lib
• Review all other names in zos-cli
• Replace all occurrences of zeppelin_os with ZeppelinOS

In the REAMDE, it says:

"Note that when we update our contract's code, we can't change its pre-existing storage structure. This means we can't remove any previously existing contract variable. We can, however, remove functions we don't want to use anymore [...]"

This should be expanded. First, we need to explain why we need to preserve the storage structure: if we remove variables, then the remaining ones will use the wrong address in memory. We should explain that the only thing that we must not do is to remove variables, but we can add new ones. Also, that we can add or remove functions, this is only for variables.

About functions, @frangio mentioned that we might get in trouble if we are unlucky and get a collision on the initial bits of the hashes. We should mention this, and make recommendations to avoid it.

From #12:

Add an UpgradeManager contract, to be used as UpgradeabilityOwner for the proxy, which holds the current version and the reference to the registry, and only exposes methods for upgrading to a version registered in the registry. This would be the glue between proxy and registry, and could be especialised into different strategies in the future.

This simple UpgradeManager (again, name TBD) contract should be managed by a single owner, who should be able to upgrade to a version defined on the registry (instead to an arbitrary address).

As mentioned in #105 (comment):

I think this is a bit messy. That there is a script which is running npm install, because it has dependencies that are not covered by zos-lib's dependencies. This probably messes up with Travis' caching of dependencies and slows our build.

Wait until solc 0.4.22 is released which includes ethereum/solidity#3308

There might be inconsistent that ContractDirectory returns 0x0 when the contract is not defined, but Package reverts if the version isn't (@martriay)

The registry keeps track of the implementations for a given application. As such, only its owner should be able to register new contracts in it.

AppManager, Distribution, ContractsProvider, etc, were all moved from CLI to this repo. In CLI, they were being tested indirectly via the commands. However, they do not have any tests in this repo that can catch any errors.

Add unit tests for all JS classes here in lib.

The idea is to model migrations as contracts instead of a single method as we have right now. In this case, a developer who provides a new version of a contract should publish all the possible migrations coming from different versions to the new one. Then the proxy will be responsible for checking whether a migration is necessary or not in case of an upgrade, and validate if the migration process finished successfully or not.

We're using it for ES6 modules and object spread syntax, and IMO it is not worth the additional complexity, indirection, and potential source of error.

We currently have two options for upgrading a proxied contract:

• upgradeTo, which changes the implementation and does nothing else.
• upgradeToAndCall, which changes the implementation and allows the execution of arbitrary code in the resulting contract.

The former is easy to use but of limited use, and the latter is very powerful but hard to use.
I propose creating an intermediate version, upgradeToAndMigrate, that changes the implementation and then calls a migrate() function with no parameters. This would allow an easy way of implementing simple migrations (where no parameters are needed) without having to craft the call data (which is too complex to some developers)

In the first case, for example, indexing implementation would enable to efficiently query when a Proxy began using a given implementation.

Explore the idea of handling multiple fallback providers within a project controller. The idea would be to let the project owner decide whether to use zOS or another providers regulated by other tokens for example.

There are many alternatives we can explore to implement this:

• We can implement a way to decide the fallback provider based on the given distribution and version
• We could also define a way to set priorities between different providers
• We may need another parameter besides distribution and version to tell the provider of the implementation

Currently the hashes we use for proxy slots are recalculated in every transaction.

We should write the precalculated values, and in the constructor check them against a hash obtained on chain.

We've already written some tests to ensure the proxy uses the storage slots we are expecting in the labs repo, we should probably cherry-pick them and/or add some more if necessary. See https://github.com/zeppelinos/labs/blob/master/upgradeability_using_fixed_storage/test/Storage.js

Some issues here:

• the free memory pointer is never updated
• we are using ptr again here without re-reading it.

🚧 This proposal is a work in progress.

@spalladino and I recently discussed a new approach to getting constructor code to run on a newly created proxy.

Currently, with the initializers implemented in the Migratable contract, the initializer code is stored like any other external function inside the contract bytecode. This initializer function has to be marked with the isInitializer modifier which will ensure it is only run once. Then it is called externally in the newly created proxy.

The new approach is inspired by how contract creation normally works in the EVM. Normally, the constructor code is not part of the bytecode that will be deployed. It is run at contract creation and then discarded.

We can somewhat emulate this by adding an additional parameter to the UpgradeabilityProxy constructor: an address that will be delegatecalled during construction, which would only serve to initialize the necessary storage. This same argument can be added to the _upgradeTo function to implement migrations/generalized constructors. When not needed, the argument can be set to zero.

Thus, the run-only-once property of the initializer is enforced at the level of the proxy.

The address of initializer and migration contracts can be stored in the ContractDirectory for each of the provided contracts. In the case of migrations it may be necessary to store more than one migration contract, to be chosen depending on the previous implementation that is being switched out.

When registering an implementation, it is stored in Registry.sol to be used by a Proxy. If someone interacts with the implementation directly, we don't really have a problem because they are dealing with a separate storage from that of the proxy.

However, a user may accidentally send ether or tokens to the implementation, and even though this doesn't represent a problem for the "proxied" version, it may represent a problem to the user. It could also be confusing for developers adopting zos.

We could consider implementing a solution that "locks up" implementations that are not wrapped by a proxy. I.e. some way to make the registered implementations unresponsive, non payable, etc, if they are not being called via a proxy.

Currently the project controller works with a fallback implementation provider given by the owner which is intended to be zeppelin_os. The idea here was to allow projects to work with multiple zOS kernel distributions.
Given there will probably be one kernel distribution (OZ) initially, there is no need for such overhead at this time.

The current upgradeability model only prevents you from registering or upgrading to a zero address. Apart from that, it does not check if you upgrade a contract to a version registered for another contract, or if the contract that you are upgrading follows whatever was required by your previous version (e.g. storage or function signatures), etc.

We should explore how we can improve current model to handle most of these scenarios preventing projects from registering and/or upgrading to undesired addresses.

It appears that decodeLogs.js is not properly dealing with the situation exposed below.
It attempts to read an object that does not exist.

CLI command executed:

[AppManager] 
Setting implementation of Basil in contract directory...
[AppManager]  Implementation set: 0x77092acc78ed6cc2439fdc6bdf07d51271d76028
[AppManager] 
Setting stdlib [object Object]...
[PackageFilesInterface] Successfully written package.zos.development.json
zos create-proxy Basil --init --params 0x65a59bb097403551a3bb6cb2bd987389f2872d40 --network development
Using network 'development'.

[AppManager] 
Creating Basil proxy: initialize, 0x65a59bb097403551a3bb6cb2bd987389f2872d40...
[AppManager]  TX receipt received: 0xb6049b7a15aad01486d59d5b53a7376dd80c62e810b5aeff547f80ef74678d64

Error thrown:

TypeError: Cannot read property 'inputs' of undefined
    at new SolidityEvent (/home/ajs/zos/lib/node_modules/web3/lib/web3/event.js:35:25)
    at logs.map.log (/home/ajs/zos/lib/lib/decodeLogs.js:7:19)
    at Array.map (<anonymous>)
    at decodeLogs (/home/ajs/zos/lib/lib/decodeLogs.js:4:15)
    at AppManagerWrapper.createProxy (/home/ajs/zos/cli/src/models/AppManager.js:98:18)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:118:7)

console.log(JSON.stringify(log, null, 2)) in decodeLogs.js, after line 4:

{
  "logIndex": 1,
  "transactionIndex": 0,
  "transactionHash": "0xb6049b7a15aad01486d59d5b53a7376dd80c62e810b5aeff547f80ef74678d64",
  "blockHash": "0x053e80721d45f5eb17e6b2382518e91ed7c0e13cf574652831e440775599d233",
  "blockNumber": 94,
  "address": "0x9c45ea32ca605d59bc82a966c73be2b29647fb53",
  "data": "0x0",
  "topics": [
    "0xbc7cd75a20ee27fd9adebab32041f755214dbc6bffa90cc0225b39da2e5c2d3b",
    "0x00000000000000000000000077092acc78ed6cc2439fdc6bdf07d51271d76028"
  ],
  "type": "mined"
}

console.log(JSON.stringify(contract.events, null, 2))

{
  "0x00fffc2da0b561cae30d9826d37709e9421c4725faebc226cbbb7ef5fc5e7349": {
    "anonymous": false,
    "inputs": [
      {
        "indexed": false,
        "name": "proxy",
        "type": "address"
      }
    ],
    "name": "ProxyCreated",
    "type": "event"
  }
}

We need to provide a thorough explanation of why fixed storage is a secure and therefore viable storage upgradeability solution.

The post about proxy patterns briefly mentions that constructors can't be used because the proxy will not know about the variable values that are set by the constructors. This should be expanded, because it leaves more questions than answers.

We should mention things like when the Contract.new() is called, the constructor is executed but the code for the constructor is not stored on the blockchain, so we can't call the constructor later as we can call the initialize function. When Contract.at(proxy.address) is called, this has a new storage space so the variables already initialized are not part of the proxied contract. Also, that it's not possible to copy the storage space of the contract to the proxy because it would be too expensive.

Atm, _fallbackProvider can only be set in the constructor. If a user deployed a project controller with no fallback provider, and then decided to add one in order to access zos implementations, she would have to throw the ProjectController away and set up a completely new one.

If changing a provider once set is a controversial operation, we could consider allowing to set the provider only if the current one is not 0x0.

I'm not sure if it's flaky tests or that it consistently runs out of gas.
https://travis-ci.org/zeppelinos/zos-lib/jobs/377481302#L3518

It may be that we're hitting the gas limit after the instrumentation for coverage.

ProjectController.sol is starting to become a central contract that allows people to hook up their contracts to zos. People can set a controller up, register implementations for their contracts, get proxies back, upgrade them, etc.

A user may want to hook up to zos, but not necessarily for upgradeability. Do we want to consider this?

ATM they may do so by simply not adhering their non-upgradeable contracts to the controller's registry, and simply interacting with zos from within their contracts, but this implies managing their project's contracts (deployment, etc) in 2 distinct ways. We may want to simplify their workflow by allowing the controller to manage all the contracts in their project.

When reviewing complex example with @ajsantander, we noted that Contracts fails as is, since it cannot find truffle artifacts. This is fixed easily by adding a global.artifacts = artifacts; line at the very beginning of the file, before importing zos-lib, but it may be confusing and is quite error prone.

Let's think of a better way of injecting artifacts into Contracts. Maybe making Contracts a singleton, and requiring a call to an "init" where it receives the truffle artifacts as a parameter? This would also allow to use Contracts as a regular import, rather than having to inject it as a global object, which is not the best design pattern.

cc @facuspagnuolo

Consider possibility of correcting mistakes.

At the moment, BaseAppManager has the functions getImplementation and getImplementationOrRevert. The latter may seem redundant but exists to allow getImplementation to respond without throwing an error or reverting.

If this is considered redundant and is removed, there should be a way to make a query that doesnt throw an error such as hasImplementation in the ContractProvider interface.

PackagedAppManager: branch coverage of the require statement on line 27.
UpgradeabilityProxyFactory: branch coverage of the require statement on line 38.

I think migration ids should be integers, because it communicates better their sequential nature.

Experiment upgradeability_using_fixed_storage in labs explored the possibility of getting rid of inheritance in the user contracts altogether in the upgradeability mechanism, by saving the required upgradeability variables in a "remote" location in storage.

This mechanism is preferable to the one currently implemented in core, and should already be mature enough for integration.

One of the prerequisites for an upgrade to be correct is that the underlying Solidity implementations have the same storage layout. This means that the contracts have the same state variables in the same places, and if there are new state variables in the upgrade they shouldn't conflict with the storage of the state variables in the previous implementation.

We should provide a script to check this. I've begun working on it but I'm blocked by some things about the inheritance graph linearization that I'm waiting to discuss with a few people.

See: #36 (comment)

This will include using a mechanism to deploy zos in a local network.