convenience contract test cases and revisions #287
Conversation
| @@ -227,6 +227,7 @@ contract Convenience is Ownable { | |||
| else | |||
| { | |||
| delegateState[i] = api3Voting.getVoterState(voteIds[i], delegateAt[i]); | |||
| voterState[i] = delegateState[i]; | |||
There was a problem hiding this comment.
Shouldn't the voterState match the delegateState or am I mistaken?
There was a problem hiding this comment.
So one way would have been to keep a single voterState and keep the actual voter state if the user didn't have a delegate and keep the delegate state if the user had a delegate (this would reduce the number of variables actually, so may want to do this if we want to return more data and are hitting stack limits). I went with the explicit route instead, keep seperate voter and delegate states and the user of this function will need to decide on which to read based on if the user had a delegate. So according to that convention, the added line is redundant.
There was a problem hiding this comment.
makes sense, I was receiving an array of 0s from the voterState when a delegate was present but according to the convention above, when a delegate is present we will be ignoring this array. Kind of mixed on returning an array of 0s as well because the 0s signify a state as well.
| @@ -259,7 +260,7 @@ contract Convenience is Ownable { | |||
| return new uint256[](0); | |||
| } | |||
| uint256 countOpenVote = 0; | |||
| for (uint256 i = votesLength - 1; i >= 0; i--) | |||
| for (uint256 i = votesLength ; i > 0; i--) | |||
There was a problem hiding this comment.
for (uint256 i = votesLength - 1; i >= 0; i--)
This was leading to underflow
There was a problem hiding this comment.
It did with the previous version but not this one because if votesLength is 0 it returns early
There was a problem hiding this comment.
Even in the previous version, at the last iteration of the loop i.e i=0 it will call i-- and cause the i value to underflow for the next comparison of the loop.
|
Also I don't think this line of code is needed because enum implicitly rejects anything outside its range |
| @@ -0,0 +1,133 @@ | |||
| //SPDX-License-Identifier: MIT | |||
There was a problem hiding this comment.
A mock contract should only include what is needed. I would replace this with
pragma solidity 0.8.4;
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
contract MockApi3Token is ERC20 {
function getMinterStatus(address minterAddress)
external
view
returns (bool)
{
return false;
}
}This allows you to skip mintReward() calls without crashing.
| @@ -0,0 +1,45 @@ | |||
| //SPDX-License-Identifier: MIT | |||
There was a problem hiding this comment.
Mock contracts don't need an interface, you can remove this
Can you remove it and try calling it with with 5 for example? |
| const VOTER_STATE = ["ABSENT", "YEA", "NAY"].reduce((state, key, index) => { | ||
| state[key] = index; | ||
| return state; | ||
| }, {}); | ||
|
|
||
| const VOTING_TYPE = ["Primary", "Secondary"].reduce((state, key, index) => { | ||
| state[key] = index; | ||
| return state; | ||
| }, {}); |
There was a problem hiding this comment.
The enum declaration above looks much nicer, I don't know why you went this. Also, even though these are defined as constants, semantically they are enums and enum names tend to be CamelCase.
There was a problem hiding this comment.
The older test cases used this convention. So I kept the same convention. I'll revise it in the update
| @@ -1,46 +1,259 @@ | |||
| const { expect } = require("chai"); | |||
There was a problem hiding this comment.
The file is not prettified & linted. Husky is supposed to do this automatically but it's broken at the moment, will have to do that manually at the project root.
|
Yup running it with anything other than 0 and 1 causes |
The underflow error is a problem so I will be leaving that unless there is some other solution.
Alright
My bad, |
Reverting explicitly with a string is still nice though, but okay |
I've created most of the test cases that have 90% test coverage let me know if I've missed anything