Modernize codebase #296

petejkim · 2020-05-12T20:31:30Z

Auto-format code using Prettier
Update Truffle and Ganache to latest
Fix tests that broke due to Truffle Upgrade
Fix coverage tool
Use ESLint for static code analysis
Use latest ECMAScript syntax to clean up codebase
Fix lint errors, and fix tests that were really broken but were only working accidentally (e.g. due to accidental misuse of global variable assignments and test inter-dependencies)
Set up TypeScript (existing JS files aren't converted)
Set up TypeScript and Solidity linters
Add tests to check that storage slot positions stay the same between FiatTokenV1 and V2. This test also ensures Solidity upgrade does not affect storage slots going forward.
Update smart contract from Solidity v0.4 to Solidity v0.6, making syntactical and best practices modifications where necessary
Add a pre-commit hook to ensure checks are run

Note: Requires Node v12 (LTS)

Many tests were broken and were only accidentally passing because of rampant misuse of global assignments and test inter-dependencies

eztierney · 2020-05-14T13:29:56Z

.travis.yml

  - yarn install --frozen-lockfile

 script:
-  - yarn check --integrity && truffle compile && npm test


Did you mean to remove check --integrity? Is it no longer needed?

Yes, it's deprecated and removed from Yarn 2.x for being unreliable. It also fails when there are cross-platform differences. Since we are not bundling node_modules in this repo, it shouldn't be necessary.

ok, that makes sense. We were trying to stop openzeppelin-solidity and zos-lib from being modified, but it looks like the lock file is enough to handle this. I guess we never needed check-integritr.y

petejkim · 2020-05-14T17:33:57Z

test/v1/ProxyNegativeTests.js

@@ -96,13 +96,6 @@ async function run_tests(newToken, _accounts) {
    await checkVariables([token], [customVars]);
  });

-  it("nut007 should fail to call proxy function with non-adminAccount", async () => {


This test is removed because the ifAdmin modifier for admin() and implementation() are removed due to compilation errors. Those are marked as view functions, but if a non-admin invokes those functions, the modifier could cause a delegate call to the functions with the same name in the implementation contract that may not be read-only (i.e. not view). New versions of Solidity compiler correctly catches this and errors out. Besides there wasn't really a point since those addresses were visible regardless, and the implementation contract should never contain meaningful admin() and implementation() overrides that relies on the modifier to be callable anyway.

This test ensures slot positions are retained post-upgrade by manually checking the use of the slots at expected positions.

petejkim · 2020-05-15T06:00:56Z

Apologies for all the force-pushes, I was amending commits as I clean up more things.

eztierney · 2020-05-18T20:27:02Z

contracts/FiatTokenV1.sol

+pragma solidity 0.6.8;
+
+import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+import { SafeMath } from "@openzeppelin/contracts/math/SafeMath.sol";


Did you have to switch openzeppelin libraries due to compiler errors? Were there any significant differences for the contracts we use from openzeppelin?

This was to upgrade to Solidity 0.6. The previous version of OZ was using 0.4. We only use IERC20 which is just an interface and SafeMath for safe uint arithmetic, so no difference other than the language upgrade.

eztierney · 2020-05-18T20:37:10Z

contracts/upgradeability/AdminUpgradeabilityProxy.sol

+
+pragma solidity 0.6.8;
+
+import { UpgradeabilityProxy } from "./UpgradeabilityProxy.sol";


Why did you copy these 3 proxy related contracts into the source tree instead of using the ones in zos-lib? At a quick glance they look very similar - were there changes you had to make?

I vendored those files so that I could update the syntax to Solidity 0.6.

Could you add a github link to the exact version this is copied from? (Also use the commit hash instead of master in the URL)

https://github.com/zeppelinos/zos-lib/blob/v1.3.0/contracts/upgradeability/AdminUpgradeabilityProxy.sol

Addressed in 283eca9

dmdque · 2020-05-19T01:13:45Z

contracts/Ownable.sol


 /**
 * @title Ownable
- * @dev The Ownable contract from https://github.com/zeppelinos/labs/blob/master/upgradeability_ownership/contracts/ownership/Ownable.sol 
+ * @dev The Ownable contract from https://github.com/zeppelinos/labs/blob/master/upgradeability_ownership/contracts/ownership/Ownable.sol


With the change on 72, this isn't a direct copy anymore. Should update this comment to reflect that. (Also the exact commit hash should be used instead of master for future proofing.)

I just left the URL untouched. I can have it updated

Addressed in 283eca9

dmdque · 2020-05-19T01:22:03Z

contracts/Pausable.sol

+     * @dev throws if called by any account other than the pauser
+     */
+    modifier onlyPauser() {
+        require(msg.sender == pauser, "Pausable: caller is not the pauser");


I think this convention of prefixing the error message e.g. Pausable: is redundant, especially as tooling improves. This convention could also be tough to stay consistent with as the number of contracts grows.

Just following the convention in OpenZeppelin repo for now. Do you suggest we remove the prefix altogether? Many other contracts, such as Uniswap V2 also follow the same convention.

I think its fine to keep the prefix and match the existing conventions.

dmdque · 2020-05-19T01:43:47Z

contracts/test/UpgradedFiatTokenNewFieldsTest.sol

+        bool _newBool,
+        address _newAddress,
+        uint256 _newUint
+    ) public {


Since access permissions aren't locked down, we must ensure upgrades are only invoked through upgradeToAndCall (and not upgradeTo), to prevent an attacker from calling initV2. It may be worth locking this down with an only modifier to be safe.

This was an existing test, didn't touch it, but I agree.

I thought about removing removeTo altogether, but turns out that would break a ton of existing tests, and we may want removeTo for upgrades that don't require any initialization. I think the upgrade instructions should be documented very well and be very explicit.

dmdque · 2020-05-19T01:46:47Z

contracts/upgradeability/AdminUpgradeabilityProxy.sol

+
+pragma solidity 0.6.8;
+
+import { UpgradeabilityProxy } from "./UpgradeabilityProxy.sol";


Could you add a github link to the exact version this is copied from? (Also use the commit hash instead of master in the URL)

dmdque · 2020-05-19T02:03:32Z

package.json

  },
  "repository": {
    "type": "git",
    "url": "git+https://github.com/centrehq/centre-tokens.git"
  },
  "keywords": [],
  "author": "",
-  "license": "ISC",
+  "license": "MIT",


Why this? They're equivalent, except that ISC removes a couple of words.

Because the license text in the source files and the LICENSE file is MIT not ISC

dmdque · 2020-05-19T02:06:45Z

test/v1/TokenTestUtils.js

+const util = require("util");
+const abi = require("ethereumjs-abi");
+const _ = require("lodash");
+const BN = require("bn.js");


I see a bunch of tests were moved. Is BigNumber -> BN the only difference?

Diff can be seen here: 7172076#diff-dab0d1598c750386404320ffdffcb4c5

petejkim force-pushed the modernize-codebase branch 4 times, most recently from f4e5d21 to b046934 Compare May 12, 2020 21:54

petejkim added 11 commits May 12, 2020 16:05

Autoformat code using Prettier

2e804ea

Update npm packages

9544b92

Update Dockerfile

20c4053

Update Travis config

61c3997

Fix solidty-coverage

ac48f00

Autoformat Markdown files

9343b15

Fix tests that broke due to Truffle upgrade

7172076

Remove unused dependencies

332ab67

Setup ESLint

e7b44a2

Fix lint errors and tests

9771630

Many tests were broken and were only accidentally passing because of rampant misuse of global assignments and test inter-dependencies

Cleanup migrations

626553f

petejkim force-pushed the modernize-codebase branch from b046934 to 626553f Compare May 12, 2020 23:06

petejkim added 3 commits May 13, 2020 23:37

Set up TypeScript

f0480e8

Move V1 tests to test/v1 and fix lint errors

3bb1363

Rename mock upgraded contract

8592ac2

petejkim mentioned this pull request May 14, 2020

Modernize codebase Part 2 #297

Closed

eztierney reviewed May 14, 2020

View reviewed changes

petejkim marked this pull request as draft May 14, 2020 17:21

petejkim marked this pull request as ready for review May 14, 2020 17:29

petejkim requested a review from eztierney May 14, 2020 17:30

petejkim commented May 14, 2020

View reviewed changes

petejkim added 2 commits May 14, 2020 10:42

Add storage slot test to ensure slots are retained

58f79be

This test ensures slot positions are retained post-upgrade by manually checking the use of the slots at expected positions.

Set up solhint

3f1c4cf

petejkim force-pushed the modernize-codebase branch from 0c172d2 to 330d9a1 Compare May 14, 2020 17:43

Upgrade contracts to solidity v0.6.8

abfbe60

petejkim force-pushed the modernize-codebase branch from 330d9a1 to abfbe60 Compare May 14, 2020 17:51

petejkim force-pushed the modernize-codebase branch from 8a3beee to 5a3e3b3 Compare May 14, 2020 20:55

Add pre-commit hook

6b53b46

petejkim force-pushed the modernize-codebase branch from 5a3e3b3 to 9989702 Compare May 15, 2020 04:22

Reorganize test files

4c42934

petejkim force-pushed the modernize-codebase branch from 9989702 to 4c42934 Compare May 15, 2020 05:59

petejkim added 2 commits May 15, 2020 18:07

Add SPDX license identifier

23a0a2d

Ensure locally installed prettier is used to format Solidity code

fdafe06

petejkim mentioned this pull request May 16, 2020

FiatTokenV1.1: Rescuable #299

Merged

eztierney reviewed May 18, 2020

View reviewed changes

dmdque reviewed May 19, 2020

View reviewed changes

Update notes in forked contracts

283eca9

petejkim force-pushed the modernize-codebase branch from bab078a to 283eca9 Compare May 19, 2020 18:12

eztierney approved these changes May 22, 2020

View reviewed changes

eztierney merged commit 8aa0ddf into circlefin:master May 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modernize codebase #296

Modernize codebase #296

petejkim commented May 12, 2020 •

edited

eztierney May 14, 2020

petejkim May 14, 2020 •

edited

eztierney May 15, 2020

petejkim May 14, 2020 •

edited

petejkim commented May 15, 2020 •

edited

eztierney May 18, 2020

petejkim May 18, 2020 •

edited

eztierney May 18, 2020

petejkim May 18, 2020 •

edited

dmdque May 19, 2020

petejkim May 19, 2020

petejkim May 19, 2020 •

edited

dmdque May 19, 2020

petejkim May 19, 2020

petejkim May 19, 2020 •

edited

dmdque May 19, 2020

petejkim May 19, 2020 •

edited

eztierney May 22, 2020

dmdque May 19, 2020

petejkim May 19, 2020 •

edited

petejkim May 19, 2020

dmdque May 19, 2020

dmdque May 19, 2020

petejkim May 19, 2020 •

edited

dmdque May 19, 2020

petejkim May 19, 2020


		pragma solidity 0.6.8;

		import { UpgradeabilityProxy } from "./UpgradeabilityProxy.sol";

Modernize codebase #296

Modernize codebase #296

Conversation

petejkim commented May 12, 2020 • edited

Choose a reason for hiding this comment

petejkim May 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petejkim May 14, 2020 • edited

Choose a reason for hiding this comment

petejkim commented May 15, 2020 • edited

Choose a reason for hiding this comment

petejkim May 18, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petejkim May 18, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petejkim May 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petejkim May 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petejkim May 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petejkim May 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petejkim May 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petejkim commented May 12, 2020 •

edited

petejkim May 14, 2020 •

edited

petejkim May 14, 2020 •

edited

petejkim commented May 15, 2020 •

edited

petejkim May 18, 2020 •

edited

petejkim May 18, 2020 •

edited

petejkim May 19, 2020 •

edited

petejkim May 19, 2020 •

edited

petejkim May 19, 2020 •

edited

petejkim May 19, 2020 •

edited

petejkim May 19, 2020 •

edited