Refactoring, solhint

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"
+  }
 }

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<numTopics;
+        return topicID < numTopics;
     }
 
-    // Verify that post exists
     function postExists(uint postID) public view returns (bool) {
-        return postID<numPosts;
-    }
-
-    function getNumberOfTopics() public view returns (uint) {
-        return numTopics;
-    }
-
-    function getNumberOfPosts() public view returns (uint) {
-        return numPosts;
+        return postID < numPosts;
     }
 
     function getTopic(uint topicID) public view returns (address, string memory, uint, uint[] memory) {
-        require(topicExists(topicID));
+        require(topicExists(topicID), TOPIC_DOES_NOT_EXIST);
         return (
             topics[topicID].author,
             users[topics[topicID].author].username,
@@ -155,17 +148,17 @@ contract Forum {
     }
 
     function getTopicPosts(uint topicID) public view returns (uint[] memory) {
-        require(topicExists(topicID)); // Topic should exist
+        require(topicExists(topicID), TOPIC_DOES_NOT_EXIST);
         return topics[topicID].postIDs;
     }
 
     function getTopicAuthor(uint topicID) public view returns (address) {
-        require(topicExists(topicID)); // Topic should exist
+        require(topicExists(topicID), TOPIC_DOES_NOT_EXIST);
         return topics[topicID].author;
     }
 
     function getPost(uint postID) public view returns (address, string memory, uint, uint) {
-        require(postExists(postID));
+        require(postExists(postID), POST_DOES_NOT_EXIST);
         return (
             posts[postID].author,
             users[posts[postID].author].username,

packages/concordia-contracts/contracts/Migrations.sol

@@ -3,7 +3,7 @@ pragma solidity 0.7.5;
 
 contract Migrations {
     address public owner;
-    uint public last_completed_migration;
+    uint public lastCompletedMigration;
 
     constructor() {
         owner = msg.sender;
@@ -14,11 +14,11 @@ contract Migrations {
     }
 
     function setCompleted(uint completed) public restricted {
-        last_completed_migration = completed;
+        lastCompletedMigration = completed;
     }
 
-    function upgrade(address new_address) public restricted {
-        Migrations upgraded = Migrations(new_address);
-        upgraded.setCompleted(last_completed_migration);
+    function upgrade(address newAddress) public restricted {
+        Migrations upgraded = Migrations(newAddress);
+        upgraded.setCompleted(lastCompletedMigration);
     }
 }

packages/concordia-contracts/contracts/PostVoting.sol

@@ -10,31 +10,25 @@ contract PostVoting {
         forum = Forum(addr);
     }
 
-    enum Option { NONE, UP, DOWN }  // NONE -> 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);
     }
 }

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;

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),
+    ]));
 };

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);