Closed yondonfu closed 5 years ago
coveralls
commented
5 years ago | Totals | |
|---|---|
| Change from base Build 1056: | 0.0% |
| Covered Lines: | 681 |
| Relevant Lines: | 681 |
yondonfu
commented
5 years ago I spent some time investigating supporting multiple compiler versions in a Truffle project. We can support multiple compiler versions using a combination of custom scripts and Truffle's external compiler feature, but I don't think it is worth the effort required to restructure the repo.
The current deployment plan is:
TicketBroker which is a completely new contractBondingManagerRoundsManagerNote: The Minter contains some changes right now that can be reverted so we don't need to upgrade it - will create a GH issue for that.
The BondingManager and RoundsManager already interact with other contracts in the system via abstract contracts (while these are technically different from interfaces they accomplish the same task of hiding implementation details from the contract that imports them). But, currently, contracts that will not be upgraded also inherit from these abstract contracts. For example, the BondingManager interacts with the IMinter abstract contract. But, the actual Minter contract also inherits from IMinter. So, if we want to keep Minter at a solc version < 0.5.0, IMinter would also need to be at a solc version < 0.5.0. As a result, if we want to keep the BondingManager at solc version 0.5.x, we would need to create a new abstract contract/interface that is a duplicate of IMinter with a different name i.e. IMinterV2 that can be used by the BondingManager. Thus, if we wanted to avoid changing any of the contracts that will not be upgraded we will need to duplicate abstract contracts inherited from these contracts that are used by contracts that will be upgraded.
All of the contacts to be deployed (both new ones and ones to be upgraded) inherit from ManagerProxyTarget which inherits from Manager. But, so do the contracts that will not be upgraded. So, if want to use solc version 0.5.x for the contracts to be deployed and a solc version < 0.5.0 for the contracts that will not be upgraded we will have to duplicate ManagerProxyTarget and Manager - one set would be compatible with solc 0.5.x and one set would be compatible with solc < 0.5.0.
These aforementioned requirements can be met, but it is worth considering what we are trying to accomplish. The utility of keeping the implementation of contracts that will not be upgraded frozen at the compiler version that is used by the currently deployed contracts on mainnet is so that in tests we can check that new/upgraded contracts using solc version 0.5.x are interacting with contracts using solc version < 0.5.0 which would reflect what will happen on mainnet after the upgrades occur. However, we can also accomplish this without actually compiling older contracts. We have access to the ABI and bytecode of currently deployed contracts. So, we could always use the ABI and bytecode to deploy versions of those contracts on any network that we want and this would not require dealing with solc compiler versioning. For that reason I think it is ok not to support multiple compiler versions in the repo right now and instead we can make sure to test interactions between the new contracts and the old contracts (deployed using the bytecode on mainnet).
yondonfu
commented
5 years ago I pushed a few updates:
a3b063e: Adds the openzeppelin-solidity dependencies back. I realized that since the latest version of the contracts in openzeppelin-solidity have updated pragma directives and are compatible with solc 0.5.x that we can depend on those instead of checking them into this repo. The remaining contracts in contracts/zeppelin are still around because our contracts depended on those APIs and names, but openzeppelin-solidity has since tweaked the APIs and names. So, our contracts can just depend on the contracts in contracts/zeppelin without changing any imports
84a16e8: Fix a small linting error with the above commit
2fa0163: Use a signMsg helper in our tests that make sure that signatures are generated with v-value = 27/28 which is a requirement in the latest ECDSA contract from openzeppelin-solidity
kyriediculous
commented
5 years ago The changes look good to me.
The changes in the non-upgradeable contracts such as the minter need to be reverted and we should address interoperability through adding those additional interfaces indeed.
Is this something we plan to address in this PR?
yondonfu
commented
5 years ago The changes in the non-upgradeable contracts such as the minter need to be reverted and we should address interoperability through adding those additional interfaces indeed. Is this something we plan to address in this PR?
Re: reverting Minter changes - this is being tracked in #347 and its not that related to the changes in this PR so we can address it in a separate PR.
Re: interoperability through interfaces - what do you have in mind here? The contracts already interact with each other via abstract contracts which should effectively have the same behavior as using interfaces i.e. none of the implementation code gets compiled since it is separated from the abstract contract/interface
yondonfu
commented
5 years ago Rebased!
This PR updates the contracts to compile with solc 0.5.11.
Since this PR is pretty big because there are small changes across many files, here is a commit breakdown:
8857299: Update truffle to 0.5.37. This required setting the
solc.parserfield intruffle.jstosolcjsin order to avoid super slow contract compilation using Docker solc. Apparently, Truffle uses a parser to quickly resolve imports before continuing with contract compilation, but usingsolcjsfor the parser is significantly faster than using Docker or nativesolc75f6d2f: Update ganache-cli to 6.7.0. This required updating the block based tests in
test/RoundsManager.js. In older versions of ganache-cli,eth_blockNumberwould return the number of the block to be mined. In newer versions of ganache-cli after 6.5.1,eth_blockNumberwould return the number of the last mined block4697f1a: Updates solium to 1.2.5 and also turned off the
no-empty-blockrule since we have empty constructors for our upgradeable contracts325fe88: This commit checks in all the contract dependencies we have from
openzeppelin-solidity. Checking in these dependencies makes it easier to upgrade the solc version used (especially if it is a breaking version i.e. 0.5.x) without waiting for theopenzeppelin-soliditycontracts to be updated to use a newer solc version. Also removed theopenzepplin-soliditydependency since all the contracts dependencies are now checked in.badc050: Update all contracts to compile with solc 0.5.11. There are a lot of changes in this commit, but they are mainly pragma updates and syntactical changes across many files. The one big addition is that I deleted
contracts/test/helpers/Assert.soland replaced it withcontracts/test/helpers/trufflewhich contains a bunch of library files. In more recent versions of Truffle,Assert.solwas updated to be compatible with solc 0.5.11 and in the process it was split into many smaller library files. As a reminder: the reason we check-in these library files despite Truffle already including them is because we actually run the Solidity unit tests via a JS test using thetest/unit/helpers/runSolidityTest.jshelper. We do this because solidity-coverage cannot handle Solidity unit tests right out of the box. But, by running the Solidity unit tests as JS tests, Truffle does not automatically link the Assert libraries properly. So, we have to manually link the libraries ourselves inrunSolidityTest.js- in order to access the artifacts for those libraries we have to check them in 😢0ee5510: Update solidity-coverage to 0.6.7. This update fixes coverage testing!
5a2f7dc: Update CircleCI to download Docker solc 0.5.11. This fixes CI!
I recommend reviewing commit by commit.
Fixes #243 Fixes #312