From 67c8d76273fc3be7920a01707de8bf9aea727b62 Mon Sep 17 00:00:00 2001 From: Dheeraj Manjunath Date: Thu, 16 Jul 2020 09:49:26 -0700 Subject: [PATCH 1/4] [N01] Inconsistent style for error messages --- eth-contracts/contracts/Governance.sol | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/eth-contracts/contracts/Governance.sol b/eth-contracts/contracts/Governance.sol index f9d92193192..caa50d77c58 100644 --- a/eth-contracts/contracts/Governance.sol +++ b/eth-contracts/contracts/Governance.sol @@ -202,7 +202,7 @@ contract Governance is InitializableV2 { ); maxInProgressProposals = _maxInProgressProposals; - require(_maxDescriptionLength > 0, "Requires non-zero _maxDescriptionLength"); + require(_maxDescriptionLength > 0, "Governance: Requires non-zero _maxDescriptionLength"); maxDescriptionLength = _maxDescriptionLength; require( @@ -379,7 +379,7 @@ contract Governance is InitializableV2 { // Ensure previous vote is not None require( previousVote != Vote.None, - "Governance::updateVote: To submit new vote, call submitVote()" + "Governance: To submit new vote, call submitVote()" ); // Override previous vote @@ -547,7 +547,7 @@ contract Governance is InitializableV2 { _requireIsInitialized(); require(msg.sender == address(this), ERROR_ONLY_GOVERNANCE); - require(_stakingAddress != address(0x00), "Requires non-zero _stakingAddress"); + require(_stakingAddress != address(0x00), "Governance: Requires non-zero _stakingAddress"); stakingAddress = _stakingAddress; } @@ -605,7 +605,7 @@ contract Governance is InitializableV2 { _requireIsInitialized(); require(msg.sender == address(this), ERROR_ONLY_GOVERNANCE); - require(_newMaxInProgressProposals > 0, "Requires non-zero _newMaxInProgressProposals"); + require(_newMaxInProgressProposals > 0, "Governance: Requires non-zero _newMaxInProgressProposals"); maxInProgressProposals = _newMaxInProgressProposals; } @@ -618,7 +618,7 @@ contract Governance is InitializableV2 { _requireIsInitialized(); require(msg.sender == address(this), "Only callable by self"); - require(_newMaxDescriptionLength > 0, "Requires non-zero _newMaxDescriptionLength"); + require(_newMaxDescriptionLength > 0, "Governance: Requires non-zero _newMaxDescriptionLength"); maxDescriptionLength = _newMaxDescriptionLength; } From eb0811f68e212769f490a648444b85918d50ba49 Mon Sep 17 00:00:00 2001 From: Dheeraj Manjunath Date: Thu, 16 Jul 2020 09:55:03 -0700 Subject: [PATCH 2/4] lint --- eth-contracts/contracts/Governance.sol | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/eth-contracts/contracts/Governance.sol b/eth-contracts/contracts/Governance.sol index caa50d77c58..01ecdd31e64 100644 --- a/eth-contracts/contracts/Governance.sol +++ b/eth-contracts/contracts/Governance.sol @@ -605,7 +605,10 @@ contract Governance is InitializableV2 { _requireIsInitialized(); require(msg.sender == address(this), ERROR_ONLY_GOVERNANCE); - require(_newMaxInProgressProposals > 0, "Governance: Requires non-zero _newMaxInProgressProposals"); + require( + _newMaxInProgressProposals > 0, + "Governance: Requires non-zero _newMaxInProgressProposals" + ); maxInProgressProposals = _newMaxInProgressProposals; } @@ -618,7 +621,10 @@ contract Governance is InitializableV2 { _requireIsInitialized(); require(msg.sender == address(this), "Only callable by self"); - require(_newMaxDescriptionLength > 0, "Governance: Requires non-zero _newMaxDescriptionLength"); + require( + _newMaxDescriptionLength > 0, + "Governance: Requires non-zero _newMaxDescriptionLength" + ); maxDescriptionLength = _newMaxDescriptionLength; } From 800a047cfab7513f6bb4355b8bb564353f0adedb Mon Sep 17 00:00:00 2001 From: Dheeraj Manjunath Date: Thu, 16 Jul 2020 12:02:55 -0700 Subject: [PATCH 3/4] Check all require statements --- eth-contracts/contracts/Governance.sol | 14 +++++++------- eth-contracts/contracts/ServiceProviderFactory.sol | 5 ++++- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/eth-contracts/contracts/Governance.sol b/eth-contracts/contracts/Governance.sol index 01ecdd31e64..0827e3dfae3 100644 --- a/eth-contracts/contracts/Governance.sol +++ b/eth-contracts/contracts/Governance.sol @@ -775,7 +775,7 @@ contract Governance is InitializableV2 { require( _proposalId <= lastProposalId && _proposalId > 0, - "Must provide valid non-zero _proposalId" + "Governance: Must provide valid non-zero _proposalId" ); return (proposals[_proposalId].contractHash); @@ -795,7 +795,7 @@ contract Governance is InitializableV2 { require( _proposalId <= lastProposalId && _proposalId > 0, - "Must provide valid non-zero _proposalId" + "Governance: Must provide valid non-zero _proposalId" ); return (proposals[_proposalId].description); @@ -1023,7 +1023,7 @@ contract Governance is InitializableV2 { private view returns (uint256) { require( _proposalId <= lastProposalId && _proposalId > 0, - "Governance::_validateVoteAndGetVoterStake: Must provide valid non-zero _proposalId" + "Governance: Must provide valid non-zero _proposalId" ); // Require voter was active Staker at proposal submission time @@ -1033,13 +1033,13 @@ contract Governance is InitializableV2 { ); require( voterStake > 0, - "Governance::_validateVoteAndGetVoterStake: Voter must be active staker with non-zero stake." + "Governance: Voter must be active staker with non-zero stake." ); // Require proposal is still active require( proposals[_proposalId].outcome == Outcome.InProgress, - "Governance::_validateVoteAndGetVoterStake: Cannot vote on inactive proposal." + "Governance: Cannot vote on inactive proposal." ); // Require proposal votingPeriod is still active. @@ -1047,13 +1047,13 @@ contract Governance is InitializableV2 { uint256 endBlockNumber = submissionBlockNumber.add(votingPeriod); require( block.number > submissionBlockNumber && block.number <= endBlockNumber, - "Governance::_validateVoteAndGetVoterStake: Proposal votingPeriod has ended" + "Governance: Proposal votingPeriod has ended" ); // Require vote is either Yes or No require( _vote == Vote.Yes || _vote == Vote.No, - "Governance::_validateVoteAndGetVoterStake: Can only submit a Yes or No vote" + "Governance: Can only submit a Yes or No vote" ); return voterStake; diff --git a/eth-contracts/contracts/ServiceProviderFactory.sol b/eth-contracts/contracts/ServiceProviderFactory.sol index 48a2aab032a..8f240a907bc 100644 --- a/eth-contracts/contracts/ServiceProviderFactory.sol +++ b/eth-contracts/contracts/ServiceProviderFactory.sol @@ -938,7 +938,10 @@ contract ServiceProviderFactory is InitializableV2 { // ========================================= Private Functions ========================================= function _requireStakingAddressIsSet() private view { - require(stakingAddress != address(0x00), "stakingAddress is not set"); + require( + stakingAddress != address(0x00), + "ServiceProviderFactory: stakingAddress is not set" + ); } function _requireDelegateManagerAddressIsSet() private view { From 6be76e7a6be38d28c4944c5ac6a7eca65eb410fe Mon Sep 17 00:00:00 2001 From: Dheeraj Manjunath Date: Thu, 16 Jul 2020 12:06:58 -0700 Subject: [PATCH 4/4] missed one --- eth-contracts/contracts/ServiceTypeManager.sol | 2 +- eth-contracts/test/serviceProvider.test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/eth-contracts/contracts/ServiceTypeManager.sol b/eth-contracts/contracts/ServiceTypeManager.sol index 94729775b3e..c9c2f989bb8 100644 --- a/eth-contracts/contracts/ServiceTypeManager.sol +++ b/eth-contracts/contracts/ServiceTypeManager.sol @@ -65,7 +65,7 @@ contract ServiceTypeManager is InitializableV2 { function setGovernanceAddress(address _governanceAddress) external { _requireIsInitialized(); - require(msg.sender == governanceAddress, "Only governance"); + require(msg.sender == governanceAddress, ERROR_ONLY_GOVERNANCE); _updateGovernanceAddress(_governanceAddress); } diff --git a/eth-contracts/test/serviceProvider.test.js b/eth-contracts/test/serviceProvider.test.js index 212e3d0c1e1..3566f120840 100644 --- a/eth-contracts/test/serviceProvider.test.js +++ b/eth-contracts/test/serviceProvider.test.js @@ -1149,7 +1149,7 @@ contract('ServiceProvider test', async (accounts) => { // Confirm only governance can call set functions await _lib.assertRevert( serviceTypeManager.setGovernanceAddress(_lib.addressZero), - 'Only governance' + 'Only callable by Governance contract' ) await _lib.assertRevert( serviceTypeManager.setServiceVersion(web3.utils.utf8ToHex('fake-type'), web3.utils.utf8ToHex('0.0')),