Added error msgs to require statements in minting/* contracts #242

mirathewhite · 2019-01-02T16:25:59Z

Added error messages to all require statements in the new minting/* contracts. Left original contracts in USDC unchanged.

…ible

mirathewhite · 2019-01-02T18:23:29Z

Created expectError(promise, errorMsg) that checks that a transaction returns an error with a specific message.

Test MINT contracts using the expectError function when appropriate
Upgrade to truffle 5, but still use compiler 4.24.0 for backwards compatibility

eztierney

The build failed - I think you may need to update .travis.yml?

eztierney · 2019-01-03T16:38:38Z

yarn.lock

@@ -6802,6 +6817,18 @@ sol-explore@^1.6.2:
  version "1.6.2"
  resolved "https://registry.yarnpkg.com/sol-explore/-/sol-explore-1.6.2.tgz#43ae8c419fd3ac056a05f8a9d1fb1022cd41ecc2"

+solc@0.5.0:


I assume we need this because truffle 5.0 depends on it? Even though we aren't using it?

Migrating requires adding bn.js for big numbers. I'm updating the yarn.lock file.

…tation of BN for comparison, convert all addresses to uppercase prior to comparison, change some function calls to comply with web3js 1.0.0 api.

o-a-hudson

Wondering if for the for the inline tests in test functions that use upperCaseAddress https://github.com/centrehq/centre-tokens/pull/242/files#diff-e70111a4751894a1ea34a2b82306a2a1R69 in a assert.equals statement, it might be cleaner to do something like: assert.isTrue(addressEquals(address1, address2)) where addressEquals could be a function that compared both addresses after applying the upperCaseAddress function you wrote? Just because it is starting to get hard to remember what has to be uppercased and a function would hide that detail from the user. What do you think?

o-a-hudson · 2019-01-04T16:36:10Z

contracts/minting/Controller.sol

@@ -40,7 +40,7 @@ contract Controller is Ownable {
     * @dev ensure that the caller is the controller of a non-zero worker address
     */
    modifier onlyController() {
-        require(controllers[msg.sender] != address(0));
+        require(controllers[msg.sender] != address(0), "Sender must be a controller.");


Maybe this error should be "Sender address cannot be the null" since it check checking that it is not address(0)? Thinking it should be consistent with the same condition here: https://github.com/centrehq/centre-tokens/pull/242/files#diff-668c7639279550e7f3cd2913080190f6R57

Modified to "The value of controller[msg.sender] must be non-zero."

I think we should never convert to all upper-case. All addresses coming back from Web3 will be checksummed (mixed-case), so we need to make sure the addresses we have hardcoded are also checksummed, and then if we ever get an address from a source other than Web3, use web3.utils.toChecksum (https://web3js.readthedocs.io/en/1.0/web3-utils.html#tochecksumaddress) on it.

I think we should never convert to all upper-case. All addresses coming back from Web3 will be checksummed (mixed-case), so we need to make sure the addresses we have hardcoded are also checksummed, and then if we ever get an address from a source other than Web3, use web3.utils.toChecksum (https://web3js.readthedocs.io/en/1.0/web3-utils.html#tochecksumaddress) on it.

If things are coming back as checksummed and not uppercase, I agree this would be a great approach and we can disregard the suggestions around an address comparison function.

o-a-hudson · 2019-01-04T16:38:20Z

contracts/minting/MintController.sol

@@ -91,7 +91,7 @@ contract MintController is Controller {
     */
     function incrementMinterAllowance(uint256 allowanceIncrement) onlyController public returns (bool) {
        address minter = controllers[msg.sender];
-        require(minterManager.isMinter(minter));
+        require(minterManager.isMinter(minter), "Can only increment allowance for enabled minter.");


Very minor: should we drop the word "enabled" and just say for minter as I don't think we have a notion of an "enabled" minter?

Changed to "Can only increment allowance for minters in minterManager."

o-a-hudson · 2019-01-04T18:16:51Z

test/TokenTestUtils.js

 function calculateFeeAmount(amount) {
    return Math.floor((fee / feeBase) * amount);
 }

 function checkMinterConfiguredEvent(configureMinterEvent, minter, minterAllowedAmount) {
    assert.equal(configureMinterEvent.logs[0].event, 'MinterConfigured')
-    assert.equal(configureMinterEvent.logs[0].args.minter, minter)
+    assert.equal(upperCaseAddress(configureMinterEvent.logs[0].args.minter), upperCaseAddress(minter));


Are we able to uppercase only the first argument to assert.equal in these cases since the addresses have been changed to uppercase?

I think the solution you recommended to write addressEquals(address1, address2) is cleaner. I don't want to keep track of which argument is already uppercased.

o-a-hudson · 2019-01-04T18:26:12Z

test/TokenTestUtils.js

-            'tokenOwner': tokenOwner,
-            'proxiedTokenAddress': proxiedTokenAddress,
-            'proxyOwner': proxyOwner,
+            'masterMinter': upperCaseAddress(masterMinter),


Aren't these already uppercased when you changed the hardcoded address to be in uppercase? (Also, from what I can tell it looks like the BN conversion function will make them uppercase anyway) Do you think we need these upperCaseAddress calls here?

addressEquals() will make this unnecessary.

o-a-hudson · 2019-01-04T18:29:22Z

test/TokenTestUtils.js

-        var totalSupply = new BigNumber(await token.totalSupply())
-        assert(balanceSum.isEqualTo(totalSupply), "sum of balances is not equal to totalSupply");
+        var totalSupply = newBigNumber(await token.totalSupply())
+        assert(balanceSum.cmp(totalSupply)==0, "sum of balances is not equal to totalSupply");


Minor, but maybe we want to check for equals here instead of comparison, so we don't have to check == 0 ?

The BN library does not work with assert.equals - it returns false even if the numbers are equal because the internal representation might be different. This is one of the reasons I had to to use the hex representation for the deepEquals function.

o-a-hudson · 2019-01-04T18:30:56Z

test/TokenTestUtils.js

@@ -404,17 +429,17 @@ async function setLongDecimalFeesTransferWithFees(token, ownerAccount, arbitrary
    let balance3 = await token.balanceOf(arbitraryAccount);
    assert.equal(balance3, 1000);
    let balanceFeeAccount = await token.balanceOf(feeAccount);
-    assert.isTrue(new BigNumber(balanceFeeAccount).minus(new BigNumber(initialBalanceFeeAccount)).isEqualTo(new BigNumber(feeAmount)));
+    assert.isTrue(balanceFeeAccount.sub(initialBalanceFeeAccount).cmp(newBigNumber(feeAmount))==0);


I would suggest using equals here instead of cmp as well just to avoid having to write ==0 in each test that uses it.

see note about regarding BN library and assert.equals.

o-a-hudson · 2019-01-04T18:32:09Z

test/TokenTestUtils.js

@@ -483,8 +508,8 @@ async function redeem(token, account, amount) {
 function validateTransferEvent(transferEvent, from, to, value) {
    let eventResult = transferEvent.logs[0];
    assert.equal(eventResult.event, 'Transfer');
-    assert.equal(eventResult.args.from, from);
-    assert.equal(eventResult.args.to, to);
+    assert.equal(upperCaseAddress(eventResult.args.from), upperCaseAddress(from));


Wondering in this case as well if we need to call upperCaseAddress on from and to arguments?

Will use addressEquals()

o-a-hudson · 2019-01-04T18:48:13Z

test/TokenTestUtils.js

@@ -216,6 +222,23 @@ function buildExpectedState(token, customVars) {
    return expectedState;
 }

+// Replaces all BN objects with 32 character hex strings
+function mapBNToHex(state) {


Perhaps here it might be easier to use the web3 tohex function that can take BN to hex (https://web3js.readthedocs.io/en/1.0/web3-utils.html#tohex)?

Will do this for all BN to hex conversions.

o-a-hudson · 2019-01-10T15:34:36Z

test/minting2/MintControllerTests.js

@@ -136,7 +137,7 @@ async function run_tests(newToken, accounts) {
        await checkMINTp0([token, mintController], [expectedTokenState, expectedMintControllerState]);

        // increment minter allowance
-        await expectRevert(mintController.incrementMinterAllowance(amount, {from: Accounts.controller1Account}));
+        await expectError(mintController.incrementMinterAllowance(amount, {from: Accounts.controller1Account}), "Can only increment allowance for enabled minter");


I think there might be a bug with our expectError method because in this line we are expecting to find a revert string "Can only increment allowance for enabled minter" but that is no longer in the contract. Yet the tests are still passing, so this looks like the expectError function is not checking the expected revert string properly.

o-a-hudson · 2019-01-10T19:40:31Z

Tests seem to be failing after expectRevert update: https://travis-ci.com/centrehq/centre-tokens/jobs/169567434

o-a-hudson · 2019-01-10T22:12:18Z

Tests seem to be failing after expectRevert update: https://travis-ci.com/centrehq/centre-tokens/jobs/169567434

We'll fix this in a separate PR.

mirathewhite added 4 commits January 2, 2019 11:24

Added error msgs to require statements in minting/* contracts

091ca83

migrating to truffle 5, MINT unit tests use expectError whenever poss…

75f4602

…ible

removing comments

dfdde1c

updating yarn.lock with truffle 5 dependency

6f2ee9c

eztierney reviewed Jan 3, 2019

View reviewed changes

mirathewhite added 6 commits January 3, 2019 18:26

migrated test/assert/* to truffle 5, checkvariables uses hex represen…

eca2ef4

…tation of BN for comparison, convert all addresses to uppercase prior to comparison, change some function calls to comply with web3js 1.0.0 api.

migrated remaining unit tests to truffle 5

5bab945

updated yarn.lock

e3d2c9b

fixed package name from BN.js to bn.js

f1605c0

updating to truffle 5.0.0 in .travis.yml

bfb3473

updating yarn.lock

4f98d94

o-a-hudson reviewed Jan 4, 2019

View reviewed changes

mirathewhite added 6 commits January 9, 2019 10:04

changed contract error messages and address comparison in tests

68b32ff

fixed compile errors

4b0c4f1

replaced assert.true with assert.isTrue

7a3c042

turned addressEquals into an assert and added error message

39f7426

removed assert.true and added addressNotEquals

59d4e93

fixed missing import of addressEquals and bad zeroAddress

8461b41

o-a-hudson reviewed Jan 10, 2019

View reviewed changes

mirathewhite added 2 commits January 10, 2019 11:31

fixed bug in expectError

d374759

fixed bug in expectRevert

9da5cf2

removed error message test because results inconsistent

b2a944e

o-a-hudson approved these changes Jan 10, 2019

View reviewed changes

o-a-hudson merged commit f7f8124 into circlefin:multi-issuer Jan 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added error msgs to require statements in minting/* contracts #242

Added error msgs to require statements in minting/* contracts #242

mirathewhite commented Jan 2, 2019

mirathewhite commented Jan 2, 2019

eztierney left a comment

eztierney Jan 3, 2019

mirathewhite Jan 4, 2019

o-a-hudson left a comment

o-a-hudson Jan 4, 2019

mirathewhite Jan 4, 2019

walkerq Jan 4, 2019

o-a-hudson Jan 4, 2019 •

edited

Loading

o-a-hudson Jan 4, 2019

mirathewhite Jan 4, 2019

o-a-hudson Jan 4, 2019

mirathewhite Jan 4, 2019

o-a-hudson Jan 4, 2019

mirathewhite Jan 4, 2019

o-a-hudson Jan 4, 2019

mirathewhite Jan 4, 2019

o-a-hudson Jan 4, 2019

mirathewhite Jan 4, 2019

o-a-hudson Jan 4, 2019

mirathewhite Jan 4, 2019

o-a-hudson Jan 4, 2019

mirathewhite Jan 4, 2019

o-a-hudson Jan 10, 2019

o-a-hudson commented Jan 10, 2019

o-a-hudson commented Jan 10, 2019

Added error msgs to require statements in minting/* contracts #242

Added error msgs to require statements in minting/* contracts #242

Conversation

mirathewhite commented Jan 2, 2019

mirathewhite commented Jan 2, 2019

eztierney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

o-a-hudson left a comment

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

o-a-hudson Jan 4, 2019 • edited Loading

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

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

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

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

o-a-hudson commented Jan 10, 2019

o-a-hudson commented Jan 10, 2019

o-a-hudson Jan 4, 2019 •

edited

Loading