From 9d32202d29dc9c023084916d9b3d1b10e87ba37e Mon Sep 17 00:00:00 2001 From: Ezerous Date: Sun, 6 Dec 2020 18:06:57 +0200 Subject: [PATCH] Refactoring, solhint --- packages/concordia-contracts/.solhint.json | 8 ++- .../concordia-contracts/contracts/Forum.sol | 67 +++++++++---------- .../contracts/Migrations.sol | 10 +-- .../contracts/PostVoting.sol | 36 +++++----- .../concordia-contracts/contracts/Voting.sol | 59 ++++++++-------- .../migrations/2_deploy_contracts.js | 9 +-- .../concordia-contracts/test/TestVoting.sol | 10 +-- 7 files changed, 99 insertions(+), 100 deletions(-) diff --git a/packages/concordia-contracts/.solhint.json b/packages/concordia-contracts/.solhint.json index d7c3de9..4f56664 100644 --- a/packages/concordia-contracts/.solhint.json +++ b/packages/concordia-contracts/.solhint.json @@ -1,3 +1,9 @@ { - "extends": "solhint:default" + "extends": "solhint:recommended", + "rules": { + "compiler-version": ["error","^0.7.5"], + "func-visibility": ["warn",{"ignoreConstructors" : true}], + "not-rely-on-time": "off", + "state-visibility": "off" + } } diff --git a/packages/concordia-contracts/contracts/Forum.sol b/packages/concordia-contracts/contracts/Forum.sol index bd94ce7..d1a0b5a 100644 --- a/packages/concordia-contracts/contracts/Forum.sol +++ b/packages/concordia-contracts/contracts/Forum.sol @@ -2,6 +2,11 @@ pragma solidity 0.7.5; contract Forum { + // Error messages for require() + string public constant USER_HAS_NOT_SIGNED_UP = "User hasn't signed up yet."; + string public constant USERNAME_TAKEN = "Username is already taken."; + string public constant TOPIC_DOES_NOT_EXIST = "Topic doesn't exist."; + string public constant POST_DOES_NOT_EXIST = "Post doesn't exist."; //----------------------------------------USER---------------------------------------- struct User { @@ -12,25 +17,24 @@ contract Forum { bool signedUp; // Helper variable for hasUserSignedUp() } - mapping (address => User) users; - mapping (string => address) userAddresses; + mapping(address => User) users; + mapping(string => address) userAddresses; event UserSignedUp(string username, address userAddress); event UsernameUpdated(string newName, string oldName,address userAddress); function signUp(string memory username) public returns (bool) { - require (!hasUserSignedUp(msg.sender), "User has already signed up."); - require(!isUserNameTaken(username), "Username is already taken."); - users[msg.sender] = User(username, - new uint[](0), new uint[](0), block.timestamp, true); + require (!hasUserSignedUp(msg.sender), USER_HAS_NOT_SIGNED_UP); + require(!isUserNameTaken(username), USERNAME_TAKEN); + users[msg.sender] = User(username, new uint[](0), new uint[](0), block.timestamp, true); userAddresses[username] = msg.sender; emit UserSignedUp(username, msg.sender); return true; } function updateUsername(string memory newUsername) public returns (bool) { - require (hasUserSignedUp(msg.sender), "User hasn't signed up yet."); - require(!isUserNameTaken(newUsername), "Username is already taken."); + require (hasUserSignedUp(msg.sender), USER_HAS_NOT_SIGNED_UP); + require(!isUserNameTaken(newUsername), USERNAME_TAKEN); string memory oldUsername = getUsername(msg.sender); delete userAddresses[users[msg.sender].username]; users[msg.sender].username = newUsername; @@ -40,7 +44,7 @@ contract Forum { } function getUsername(address userAddress) public view returns (string memory) { - require (hasUserSignedUp(userAddress), "User hasn't signed up yet."); + require (hasUserSignedUp(userAddress), USER_HAS_NOT_SIGNED_UP); return users[userAddress].username; } @@ -53,27 +57,26 @@ contract Forum { } function isUserNameTaken(string memory username) public view returns (bool) { - if (getUserAddress(username)!=address(0)) + if (getUserAddress(username) != address(0)) return true; return false; } function getUserTopics(address userAddress) public view returns (uint[] memory) { - require (hasUserSignedUp(userAddress), "User hasn't signed up yet."); + require (hasUserSignedUp(userAddress), USER_HAS_NOT_SIGNED_UP); return users[userAddress].topicIDs; } function getUserPosts(address userAddress) public view returns (uint[] memory) { - require (hasUserSignedUp(userAddress), "User hasn't signed up yet."); + require (hasUserSignedUp(userAddress), USER_HAS_NOT_SIGNED_UP); return users[userAddress].postIDs; } function getUserDateOfRegister(address userAddress) public view returns (uint) { - require (hasUserSignedUp(userAddress), "User hasn't signed up yet."); + require (hasUserSignedUp(userAddress), USER_HAS_NOT_SIGNED_UP); return users[userAddress].timestamp; } - //----------------------------------------POSTING---------------------------------------- struct Topic { uint topicID; @@ -89,17 +92,17 @@ contract Forum { uint topicID; } - uint numTopics; // Total number of topics - uint numPosts; // Total number of posts + uint public numTopics; // Total number of topics + uint public numPosts; // Total number of posts - mapping (uint => Topic) topics; - mapping (uint => Post) posts; + mapping(uint => Topic) topics; + mapping(uint => Post) posts; event TopicCreated(uint topicID, uint postID); event PostCreated(uint postID, uint topicID); function createTopic() public returns (uint, uint) { - require(hasUserSignedUp(msg.sender)); // Only registered users can create topics + require(hasUserSignedUp(msg.sender), USER_HAS_NOT_SIGNED_UP); //Creates topic uint topicID = numTopics++; topics[topicID] = Topic(topicID, msg.sender, block.timestamp, new uint[](0)); @@ -116,8 +119,8 @@ contract Forum { } function createPost(uint topicID) public returns (uint) { - require(hasUserSignedUp(msg.sender)); // Only registered users can create posts - require(topicExists(topicID)); // Only allow posting to a topic that exists + require(hasUserSignedUp(msg.sender), USER_HAS_NOT_SIGNED_UP); + require(topicExists(topicID), TOPIC_DOES_NOT_EXIST); uint postID = numPosts++; posts[postID] = Post(postID, msg.sender, block.timestamp, topicID); topics[topicID].postIDs.push(postID); @@ -126,26 +129,16 @@ contract Forum { return postID; } - // Verify that topic exists function topicExists(uint topicID) public view returns (bool) { - return topicID 0, UP -> 1, DOWN -> 2 - - Option constant defaultOption = Option.NONE; - - function getDefaultChoice() public pure returns (uint) { - return uint(defaultOption); - } + enum Option { DEFAULT, UP, DOWN } // DEFAULT -> 0, UP -> 1, DOWN -> 2 struct PostBallot { - mapping (address => Option) votes; - mapping (Option => address[]) voters; + mapping(address => Option) votes; + mapping(Option => address[]) voters; } - mapping (uint => PostBallot) postBallots; + mapping(uint => PostBallot) postBallots; event UserVotedPost(address userAddress, uint postID, Option option); function getVote(uint postID, address voter) public view returns (Option) { - require(forum.postExists(postID)); + require(forum.postExists(postID), forum.POST_DOES_NOT_EXIST()); return postBallots[postID].votes[voter]; } - // Gets vote count for a specific option (Option.UP/ Option.DOWN) + // Gets vote count for a specific option (Option.UP/ Option.DOWN only!) function getVoteCount(uint postID, Option option) private view returns (uint) { - require(forum.postExists(postID)); + require(forum.postExists(postID), forum.POST_DOES_NOT_EXIST()); return (postBallots[postID].voters[option].length); } @@ -48,7 +42,7 @@ contract PostVoting { // Gets voters for a specific option (Option.UP/ Option.DOWN) function getVoters(uint postID, Option option) private view returns (address[] memory) { - require(forum.postExists(postID)); + require(forum.postExists(postID), forum.POST_DOES_NOT_EXIST()); return (postBallots[postID].voters[option]); } @@ -61,8 +55,8 @@ contract PostVoting { } function getVoterIndex(uint postID, address voter) private view returns (uint) { - require(forum.hasUserSignedUp(voter)); - require(forum.postExists(postID)); + require(forum.hasUserSignedUp(voter), forum.USER_HAS_NOT_SIGNED_UP()); + require(forum.postExists(postID), forum.POST_DOES_NOT_EXIST()); PostBallot storage postBallot = postBallots[postID]; Option votedOption = getVote(postID, voter); @@ -76,8 +70,8 @@ contract PostVoting { } function vote(uint postID, Option option) private { - require(forum.hasUserSignedUp(msg.sender)); - require(forum.postExists(postID)); // Only allow voting if post exists + require(forum.hasUserSignedUp(msg.sender), forum.USER_HAS_NOT_SIGNED_UP()); + require(forum.postExists(postID), forum.POST_DOES_NOT_EXIST()); PostBallot storage postBallot = postBallots[postID]; address voter = msg.sender; @@ -87,7 +81,7 @@ contract PostVoting { return; // Remove previous vote if exists - if(prevOption != Option.NONE){ + if(prevOption != Option.DEFAULT){ uint voterIndex = getVoterIndex(postID, voter); // Swap with last voter address and delete vote postBallot.voters[prevOption][voterIndex] = postBallot.voters[prevOption][postBallot.voters[prevOption].length - 1]; @@ -95,7 +89,7 @@ contract PostVoting { } // Add new vote - if(option != Option.NONE) + if(option != Option.DEFAULT) postBallot.voters[option].push(voter); postBallot.votes[voter] = option; emit UserVotedPost(voter, postID, option); @@ -110,6 +104,6 @@ contract PostVoting { } function unvote(uint postID) public{ - vote(postID, Option.NONE); + vote(postID, Option.DEFAULT); } } diff --git a/packages/concordia-contracts/contracts/Voting.sol b/packages/concordia-contracts/contracts/Voting.sol index cc5c655..c264ffa 100644 --- a/packages/concordia-contracts/contracts/Voting.sol +++ b/packages/concordia-contracts/contracts/Voting.sol @@ -4,6 +4,13 @@ pragma solidity 0.7.5; import "./Forum.sol"; contract Voting { + // Error messages for require() + string constant TOPIC_POLL_DIFFERENT_CREATOR = "Only topic's author can create a poll."; + string constant POLL_EXISTS = "Poll already exists."; + string constant POLL_DOES_NOT_EXIST = "Poll does not exist."; + string constant INVALID_OPTION = "Invalid option."; + string constant USER_HAS_NOT_VOTED = "User hasn't voted."; + Forum public forum; constructor(Forum addr) { @@ -14,18 +21,17 @@ contract Voting { uint topicID; uint numOptions; string dataHash; - mapping (address => uint) votes; - mapping (uint => address[]) voters; + mapping(address => uint) votes; + mapping(uint => address[]) voters; bool enableVoteChanges; uint timestamp; } - mapping (uint => Poll) polls; + mapping(uint => Poll) polls; event PollCreated(uint topicID); event UserVotedPoll(address userAddress, uint topicID, uint vote); - // Verifies that a poll exists function pollExists(uint topicID) public view returns (bool) { if (polls[topicID].timestamp != 0) return true; @@ -33,10 +39,10 @@ contract Voting { } function createPoll(uint topicID, uint numOptions, string memory dataHash, bool enableVoteChanges) public returns (uint) { - require(forum.hasUserSignedUp(msg.sender)); // Only registered users can create polls - require(forum.topicExists(topicID)); // Only allow poll creation if topic exists - require (forum.getTopicAuthor(topicID) == msg.sender); // Only allow poll creation from the author of the topic - require(!pollExists(topicID)); // Only allow poll creation if it doesn't already exist + require(forum.hasUserSignedUp(msg.sender), forum.USER_HAS_NOT_SIGNED_UP()); + require(forum.topicExists(topicID), forum.TOPIC_DOES_NOT_EXIST()); + require (forum.getTopicAuthor(topicID) == msg.sender, TOPIC_POLL_DIFFERENT_CREATOR); + require(!pollExists(topicID), POLL_EXISTS); Poll storage poll = polls[topicID]; poll.topicID = topicID; @@ -50,46 +56,45 @@ contract Voting { } function getPollInfo(uint topicID) public view returns (uint, string memory, uint, uint) { - require(pollExists(topicID)); + require(pollExists(topicID), POLL_DOES_NOT_EXIST); uint totalVotes = getTotalVotes(topicID); return ( - polls[topicID].numOptions, - polls[topicID].dataHash, - polls[topicID].timestamp, - totalVotes + polls[topicID].numOptions, + polls[topicID].dataHash, + polls[topicID].timestamp, + totalVotes ); } function isOptionValid(uint topicID, uint option) public view returns (bool) { - require(pollExists(topicID)); + require(pollExists(topicID), POLL_DOES_NOT_EXIST); if (option <= polls[topicID].numOptions) // Option 0 is valid as well (no option chosen) return true; return false; } function hasVoted(uint topicID, address voter) public view returns (bool) { - require(pollExists(topicID)); + require(pollExists(topicID), POLL_DOES_NOT_EXIST); if (polls[topicID].votes[voter] != 0) return true; return false; } function getVote(uint topicID, address voter) public view returns (uint) { - require(hasVoted(topicID, voter)); + require(hasVoted(topicID, voter), USER_HAS_NOT_VOTED); return polls[topicID].votes[voter]; } - // Gets vote count for a specific option function getVoteCount(uint topicID, uint option) public view returns (uint) { - require(pollExists(topicID)); - require(isOptionValid(topicID, option)); + require(pollExists(topicID), POLL_DOES_NOT_EXIST); + require(isOptionValid(topicID, option), INVALID_OPTION); return (polls[topicID].voters[option].length); } function getTotalVotes(uint topicID) public view returns (uint) { - require(pollExists(topicID)); + require(pollExists(topicID), POLL_DOES_NOT_EXIST); Poll storage poll = polls[topicID]; uint totalVotes = 0; @@ -102,13 +107,13 @@ contract Voting { // Gets voters for a specific option function getVoters(uint topicID, uint option) public view returns (address[] memory) { - require(pollExists(topicID)); + require(pollExists(topicID), POLL_DOES_NOT_EXIST); return (polls[topicID].voters[option]); } function getVoterIndex(uint topicID, address voter) public view returns (uint) { - require(pollExists(topicID)); - require(hasVoted(topicID, voter)); + require(pollExists(topicID), POLL_DOES_NOT_EXIST); + require(hasVoted(topicID, voter), USER_HAS_NOT_VOTED); Poll storage poll = polls[topicID]; uint votedOption = getVote(topicID, voter); address[] storage optionVoters = poll.voters[votedOption]; @@ -121,16 +126,16 @@ contract Voting { } function vote(uint topicID, uint option) public { - require(forum.hasUserSignedUp(msg.sender)); - require(pollExists(topicID)); - require(isOptionValid(topicID, option)); + require(forum.hasUserSignedUp(msg.sender), forum.USER_HAS_NOT_SIGNED_UP()); + require(pollExists(topicID), POLL_DOES_NOT_EXIST); + require(isOptionValid(topicID, option), INVALID_OPTION); Poll storage poll = polls[topicID]; address voter = msg.sender; uint prevOption = poll.votes[voter]; if(prevOption == option) return; - // Voter hadn't voted before + // Voter hasn't voted before if(prevOption == 0){ poll.voters[option].push(voter); poll.votes[voter] = option; diff --git a/packages/concordia-contracts/migrations/2_deploy_contracts.js b/packages/concordia-contracts/migrations/2_deploy_contracts.js index 7d1f449..15daf19 100644 --- a/packages/concordia-contracts/migrations/2_deploy_contracts.js +++ b/packages/concordia-contracts/migrations/2_deploy_contracts.js @@ -4,8 +4,9 @@ const PostVoting = artifacts.require('PostVoting'); // eslint-disable-next-line func-names module.exports = function (deployer) { - deployer.deploy(Forum).then(async (forum) => { - await deployer.deploy(Voting, forum.address); - await deployer.deploy(PostVoting, forum.address); - }); + return deployer.deploy(Forum) + .then(async (forum) => Promise.all([ + deployer.deploy(Voting, forum.address), + deployer.deploy(PostVoting, forum.address), + ])); }; diff --git a/packages/concordia-contracts/test/TestVoting.sol b/packages/concordia-contracts/test/TestVoting.sol index 6925740..3c801a0 100644 --- a/packages/concordia-contracts/test/TestVoting.sol +++ b/packages/concordia-contracts/test/TestVoting.sol @@ -13,7 +13,7 @@ contract TestVoting { function beforeAll() public { forum = Forum(DeployedAddresses.Forum()); - forum.signUp('testAccount'); + forum.signUp("testAccount"); (firstTopicId,) = forum.createTopic(); } @@ -28,7 +28,7 @@ contract TestVoting { function testCreatePoll() public { Voting voting = Voting(DeployedAddresses.Voting()); - uint actual = voting.createPoll(firstTopicId, 3, 'asdf', false); + uint actual = voting.createPoll(firstTopicId, 3, "asdf", false); Assert.equal(actual, firstTopicId, "Topic Id should be 1"); } @@ -47,7 +47,7 @@ contract TestVoting { (uint actualNumberOfOptions, string memory actualDataHash, , uint actualNumberOfVotes) = voting.getPollInfo(firstTopicId); Assert.equal(actualNumberOfOptions, 3, "Number of votes should be 0"); - Assert.equal(actualDataHash, 'asdf', "Number of votes should be 0"); + Assert.equal(actualDataHash, "asdf", "Number of votes should be 0"); Assert.equal(actualNumberOfVotes, 0, "Number of votes should be 0"); } @@ -76,7 +76,7 @@ contract TestVoting { Voting voting = Voting(DeployedAddresses.Voting()); (uint topicId,) = forum.createTopic(); - voting.createPoll(topicId, 3, 'asdf', false); + voting.createPoll(topicId, 3, "asdf", false); voting.vote(topicId, 1); uint actualVotesOption0 = voting.getVoteCount(topicId, 1); @@ -95,7 +95,7 @@ contract TestVoting { Voting voting = Voting(DeployedAddresses.Voting()); (uint topicId,) = forum.createTopic(); - voting.createPoll(topicId, 3, 'asdf', true); + voting.createPoll(topicId, 3, "asdf", true); voting.vote(topicId, 1); uint actualVotesOption0 = voting.getVoteCount(topicId, 1);