zeppelinos / zos-lib Goto Github PK
View Code? Open in Web Editor NEW:warning: Deprecated repo in favour of https://github.com/zeppelinos/zos
Home Page: https://zeppelinos.org/
:warning: Deprecated repo in favour of https://github.com/zeppelinos/zos
Home Page: https://zeppelinos.org/
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.
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
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.
Wait until solc 0.4.22 is released which includes ethereum/solidity#3308
zos-lib
zos-cli
zeppelin_os
with ZeppelinOS
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 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.
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.
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:
distribution
and version
distribution
and version
to tell the provider of the implementationIn the first case, for example, indexing implementation
would enable to efficiently query when a Proxy began using a given implementation.
We should set up another job in Travis to run the tests on a development node in Geth, and possibly Parity.
We need to provide a thorough explanation of why fixed storage is a secure and therefore viable storage upgradeability solution.
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.
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.
This will include using a mechanism to deploy zos in a local network.
PackagedAppManager: branch coverage of the require statement on line 27.
UpgradeabilityProxyFactory: branch coverage of the require statement on line 38.
Line 7 in b8f1764
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 byzos-lib
's dependencies. This probably messes up with Travis' caching of dependencies and slows our build.
๐ง 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 delegatecall
ed 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 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.
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.
Consider possibility of correcting mistakes.
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.
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.
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.
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.
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"
Create basic tests for the example's DonationsV1.sol and DonationsV2.sol, agnostic of the zos app manager functionality.
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.
I would like for us to review if and how the ProjectController
should publicly expose the create
and createAndCall
functions.
Pros:
Cons:
ProjectController
since the user can'tFrom #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).
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"
}
}
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.
See: #36 (comment)
There might be inconsistent that ContractDirectory
returns 0x0
when the contract is not defined, but Package
reverts if the version isn't (@martriay)
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
address developer
is already present in kernel's Release
.
Should we include this state variable in all ImplementationDirectory
s?
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.
I think migration ids should be integers, because it communicates better their sequential nature.
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
.
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.
Nice to have within Package
function versionsList() public returns (string[] versions)
Suggested by @martriay
Some issues here:
ptr
again here without re-reading it.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.
Function upgradeProxy from AppManagerWrapper should not send a transaction if the implementation address of the proxy matches the desired one.
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)
A declarative, efficient, and flexible JavaScript library for building user interfaces.
๐ Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. ๐๐๐
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google โค๏ธ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.