Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

convenience contract test cases and revisions #287

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions packages/convenience/contracts/Convenience.sol
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ contract Convenience is Ownable {
else
{
delegateState[i] = api3Voting.getVoterState(voteIds[i], delegateAt[i]);
voterState[i] = delegateState[i];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the voterState match the delegateState or am I mistaken?

Copy link
Member

@bbenligiray bbenligiray Jun 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
}
Expand Down Expand Up @@ -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--)
Copy link
Contributor Author

@Ashar2shahid Ashar2shahid Jun 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for (uint256 i = votesLength - 1; i >= 0; i--)

This was leading to underflow

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It did with the previous version but not this one because if votesLength is 0 it returns early

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right 👍

{
(
bool open,
Expand All @@ -272,7 +273,7 @@ contract Convenience is Ownable {
, // nay
, // votingPower
// script
) = api3Voting.getVote(i);
) = api3Voting.getVote(i-1);
if (open)
{
countOpenVote++;
Expand All @@ -288,7 +289,7 @@ contract Convenience is Ownable {
}
voteIds = new uint256[](countOpenVote);
uint256 countAddedVote = 0;
for (uint256 i = votesLength - 1; i >= 0; i--)
for (uint256 i = votesLength ; i > 0; i--)
{
if (countOpenVote == countAddedVote)
{
Expand All @@ -305,10 +306,10 @@ contract Convenience is Ownable {
, // nay
, // votingPower
// script
) = api3Voting.getVote(i);
) = api3Voting.getVote(i-1);
if (open)
{
voteIds[countAddedVote] = i;
voteIds[countAddedVote] = i-1;
countAddedVote++;
}
}
Expand Down
133 changes: 133 additions & 0 deletions packages/convenience/contracts/mock/Api3TokenMock.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
//SPDX-License-Identifier: MIT
Copy link
Member

@bbenligiray bbenligiray Jun 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

pragma solidity 0.8.4;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/access/Ownable.sol";
import "../../interfaces/v0.8.4/IApi3Token.sol";


/// @title API3 token contract
/// @notice The API3 token contract is owned by the API3 DAO, which can grant
/// minting privileges to addresses. Any account is allowed to burn their
/// tokens, but this functionality is put behind a barrier that requires the
/// account to make a call to remove.
contract MockApi3Token is ERC20, Ownable, IApi3Token {
/// @dev If an address is authorized to mint tokens.
/// Token minting authorization is granted by the token contract owner
/// (i.e., the API3 DAO).
mapping(address => bool) private isMinter;
/// @dev If an address is authorized to burn tokens.
/// Token burning authorization is granted by the address itself, i.e.,
/// anyone can declare themselves a token burner.
mapping(address => bool) private isBurner;

/// @param contractOwner Address that will receive the ownership of the
/// token contract
/// @param mintingDestination Address that the tokens will be minted to
constructor(
address contractOwner,
address mintingDestination
)
public
ERC20("API3", "API3")
{
transferOwnership(contractOwner);
// Initial supply is 100 million (100e6)
// We are using ether because the token has 18 decimals like ETH
_mint(mintingDestination, 100e6 ether);
}

/// @notice The OpenZeppelin renounceOwnership() implementation is
/// overriden to prevent ownership from being renounced accidentally.
function renounceOwnership()
public
override
onlyOwner
{
revert("Ownership cannot be renounced");
}

/// @notice Updates if an address is authorized to mint tokens
/// @param minterAddress Address whose minter authorization status will be
/// updated
/// @param minterStatus Updated minter authorization status
function updateMinterStatus(
address minterAddress,
bool minterStatus
)
external
override
onlyOwner
{
require(
isMinter[minterAddress] != minterStatus,
"Input will not update state"
);
isMinter[minterAddress] = minterStatus;
emit MinterStatusUpdated(minterAddress, minterStatus);
}

/// @notice Updates if the caller is authorized to burn tokens
/// @param burnerStatus Updated minter authorization status
function updateBurnerStatus(bool burnerStatus)
external
override
{
require(
isBurner[msg.sender] != burnerStatus,
"Input will not update state"
);
isBurner[msg.sender] = burnerStatus;
emit BurnerStatusUpdated(msg.sender, burnerStatus);
}

/// @notice Mints tokens
/// @param account Address that will receive the minted tokens
/// @param amount Amount that will be minted
function mint(
address account,
uint256 amount
)
external
override
{
require(isMinter[msg.sender], "Only minters are allowed to mint");
_mint(account, amount);
}

/// @notice Burns caller's tokens
/// @param amount Amount that will be burned
function burn(uint256 amount)
external
override
{
require(isBurner[msg.sender], "Only burners are allowed to burn");
_burn(msg.sender, amount);
}

/// @notice Returns if an address is authorized to mint tokens
/// @param minterAddress Address whose minter authorization status will be
/// returned
/// @return minterStatus Minter authorization status
function getMinterStatus(address minterAddress)
external
view
override
returns(bool minterStatus)
{
minterStatus = isMinter[minterAddress];
}

/// @notice Returns if an address is authorized to burn tokens
/// @param burnerAddress Address whose burner authorization status will be
/// returned
/// @return burnerStatus Burner authorization status
function getBurnerStatus(address burnerAddress)
external
view
override
returns(bool burnerStatus)
{
burnerStatus = isBurner[burnerAddress];
}
}
113 changes: 113 additions & 0 deletions packages/convenience/contracts/mock/Api3VotingMock.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
pragma solidity 0.8.4;

contract MockApi3Voting {

enum VoterState { Absent, Yea, Nay }

struct Vote {
bool open;
bool executed;
uint64 startDate;
uint64 snapshotBlock;
uint64 supportRequired;
uint64 minAcceptQuorum;
uint256 yea;
uint256 nay;
uint256 votingPower;
bytes script;
}

Vote[] private votes;

function addVote(
bool open,
bool executed,
uint64 startDate,
uint64 snapshotBlock,
uint64 supportRequired,
uint64 minAcceptQuorum,
uint256 yea,
uint256 nay,
uint256 votingPower,
bytes memory script
)
external
{
votes.push(Vote({
open: open,
executed: executed,
startDate: startDate,
snapshotBlock: snapshotBlock,
supportRequired: supportRequired,
minAcceptQuorum: minAcceptQuorum,
yea: yea,
nay: nay,
votingPower: votingPower,
script: script
}));
}

function getVote(uint256 _voteId)
external
view
returns (
bool open,
bool executed,
uint64 startDate,
uint64 snapshotBlock,
uint64 supportRequired,
uint64 minAcceptQuorum,
uint256 yea,
uint256 nay,
uint256 votingPower,
bytes memory script
)
{
require(_voteId < votes.length, "No such vote");
open = votes[_voteId].open;
executed = votes[_voteId].executed;
startDate = votes[_voteId].startDate;
snapshotBlock = votes[_voteId].snapshotBlock;
supportRequired = votes[_voteId].supportRequired;
minAcceptQuorum = votes[_voteId].minAcceptQuorum;
yea = votes[_voteId].yea;
nay = votes[_voteId].nay;
votingPower = votes[_voteId].votingPower;
script = votes[_voteId].script;
}


function votesLength()
external
view
returns (uint256)
{
return votes.length;
}

function getVoterState(uint256 _voteId, address _voter)
external
view
returns (VoterState state)
{
if( _voteId == 1) {
state = VoterState.Yea ;
}
else if(_voteId == 2) {
state = VoterState.Absent ;
}
else {
state = VoterState.Nay ;
}
}


function voteTime()
external
view
returns (uint256)
{
return block.timestamp - 30 days;
}

}
17 changes: 0 additions & 17 deletions packages/convenience/contracts/mock/MockApi3Pool.sol

This file was deleted.

Loading