Skip to content

etherGC.sol +reputationGC.sol #756

New issue

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

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

Already on GitHub? Sign in to your account

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
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
84 changes: 84 additions & 0 deletions contracts/globalConstraints/EtherGC.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
pragma solidity 0.5.17;

import "openzeppelin-solidity/contracts/token/ERC20/IERC20.sol";
import "./GlobalConstraintInterface.sol";
import "openzeppelin-solidity/contracts/math/SafeMath.sol";


/**
* @title EtherGC ether constraint per period
*/
contract EtherGC {
Copy link
Contributor

Choose a reason for hiding this comment

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

you have a GlobalConstraint interface to inherit from?

using SafeMath for uint256;

uint256 public constraintPeriodWindow; //a static window
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe call it "periodLength"? and add in the comment that the unit is blocks

uint256 public amountAllowedPerPeriod;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this should be percentage-wise, or absolute values. Probably better to keep it absolute.

address public avatar;
uint256 public latestEthBalance;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what this is fore, this is just set in the initliaze call... if you need it, give it a more meaningful name perhaps?

mapping(uint256=>uint256) public totalAmountSentForPeriods;
Copy link
Contributor

Choose a reason for hiding this comment

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

totalAmountSentPerPeriod, a mapping from period indexes to amounts

uint256 public startBlock;
address public controller;
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs some explanation, I think. So the constraint holds for what? it is a limit on how much the funds from the avatar the controller is allowed to spend per period.

uint256 public avatarBalanceBefore;

/**
* @dev initialize
* @param _avatar the avatar to mint reputation from
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a bit a confusing comment

* @param _constraintPeriodWindow the constraintPeriodWindow in blocks units
* @param _amountAllowedPerPeriod the amount of eth to constraint for each period
*/
function initialize(
address _avatar,
uint256 _constraintPeriodWindow,
uint256 _amountAllowedPerPeriod,
address _controller
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need the controller as a separate argument, can't you get that from the avatar_?

)
external
{
require(avatar == address(0), "can be called only one time");
require(_avatar != address(0), "avatar cannot be zero");
avatar = _avatar;
constraintPeriodWindow = _constraintPeriodWindow;
amountAllowedPerPeriod = _amountAllowedPerPeriod;
latestEthBalance = avatar.balance;
startBlock = block.number;
Copy link
Contributor

Choose a reason for hiding this comment

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

Start time/block is actually not really required, as you can pick any point in time to count from. But it makes the mapping more readable (start from index 0 instead of a random shift). Guessing it is worth the one-time 20k gas.

controller = _controller;
}

/**
* @dev check the constraint before the action.
* @return true
*/
function pre(address, bytes32, bytes32) public returns(bool) {
require(msg.sender == controller, "only controller is authorize to call");
avatarBalanceBefore = avatar.balance;
return true;
}

/**
* @dev check the allowance of ether sent per period.
Copy link
Contributor

Choose a reason for hiding this comment

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

"and throws an error if the constraint is violated"

* @return bool which represents a success
*/
function post(address, bytes32, bytes32) public returns(bool) {
require(msg.sender == controller, "only controller is authorize to call");
uint256 currentPeriodIndex = (block.number - startBlock)/constraintPeriodWindow;
if ((block.number - startBlock) % constraintPeriodWindow > 0) {
currentPeriodIndex = currentPeriodIndex.add(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need this? Was line 53 not enough?

}
if (avatarBalanceBefore >= avatar.balance) {
Copy link
Contributor

Choose a reason for hiding this comment

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

so you are checking the netto balance, not how much ether the avatar actually did send (i.e. if the constraint is 1 ETH/day, then it would be ok if the avatar receives 100.000 and sends 99.999)

uint256 ethSentAmount = avatarBalanceBefore.sub(avatar.balance);
totalAmountSentForPeriods[currentPeriodIndex] =
totalAmountSentForPeriods[currentPeriodIndex].add(ethSentAmount);
require(totalAmountSentForPeriods[currentPeriodIndex] <= amountAllowedPerPeriod,
"exceed the amount allowed");
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe at a reference to where this error message comes from, something like `Violation of Global constraint EtherGC: amount sent exceed in current period°

}
return true;
}

/**
* @dev when return if this globalConstraints is pre, post or both.
* @return CallPhase enum indication Pre, Post or PreAndPost.
*/
function when() public pure returns(GlobalConstraintInterface.CallPhase) {
return GlobalConstraintInterface.CallPhase.PreAndPost;
}
}
75 changes: 75 additions & 0 deletions test/etherGC.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
import * as helpers from './helpers';
const DAOToken = artifacts.require("./DAOToken.sol");
const EtherGC = artifacts.require('./globalConstraints/EtherGC.sol');
const Controller = artifacts.require("./Controller.sol");
const Reputation = artifacts.require("./Reputation.sol");
const Avatar = artifacts.require("./Avatar.sol");
var constants = require('../test/constants');


let reputation, avatar,token,controller,etherGC;
const setup = async function (permission='0') {
var _controller;
token = await DAOToken.new("TEST","TST",0);
// set up a reputation system
reputation = await Reputation.new();
avatar = await Avatar.new('name', token.address, reputation.address);
if (permission !== '0'){
_controller = await Controller.new(avatar.address,{from:accounts[1],gas: constants.ARC_GAS_LIMIT});
await _controller.registerScheme(accounts[0],0,permission,0,{from:accounts[1]});
await _controller.unregisterSelf(0,{from:accounts[1]});
}
else {
_controller = await Controller.new(avatar.address,{gas: constants.ARC_GAS_LIMIT});
}
controller = _controller;
await avatar.transferOwnership(controller.address);
etherGC = await EtherGC.new();
await etherGC.initialize(avatar.address,10,web3.utils.toWei('5', "ether"),controller.address); //10 blocks ,5 eth
return _controller;
};

contract('EtherGC', accounts => {
it("initialize", async () => {
await setup();
assert.equal(await etherGC.avatar(),avatar.address);
assert.equal(await etherGC.amountAllowedPerPeriod(),web3.utils.toWei('5', "ether"));
assert.equal(await etherGC.constraintPeriodWindow(),10);
});

it("send ether check", async () => {

await setup();
try {
await etherGC.initialize(avatar.address,20,web3.utils.toWei('5', "ether"),controller.address); //10 blocks ,5 eth
Copy link
Contributor

Choose a reason for hiding this comment

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

comment says 10 blocks, code says 20

assert(false,"cannpt init twice ");
} catch(ex){
helpers.assertVMException(ex);
}
await controller.addGlobalConstraint(etherGC.address,helpers.NULL_HASH,avatar.address);
//move 10 ether to avatar
await web3.eth.sendTransaction({from:accounts[0],to:avatar.address, value: web3.utils.toWei('10', "ether")});
await controller.sendEther(web3.utils.toWei('1', "ether"), accounts[2],avatar.address);
await controller.sendEther(web3.utils.toWei('4', "ether"), accounts[2],avatar.address);

try {
await controller.sendEther(web3.utils.toWei('1', "ether"), accounts[2],avatar.address);
assert(false,"sendEther should fail due to the etherGC global constraint ");
}
catch(ex){
helpers.assertVMException(ex);
}
for (var i = 0 ;i< 21;i++) {
//increment 21 blocks in ganache
await Reputation.new();

}

await controller.sendEther(web3.utils.toWei('1', "ether"), accounts[2],avatar.address);
await controller.sendEther(web3.utils.toWei('4', "ether"), accounts[2],avatar.address);
//send more than 10 eth
await web3.eth.sendTransaction({from:accounts[0],to:avatar.address, value: web3.utils.toWei('10', "ether")});
await controller.sendEther(web3.utils.toWei('4', "ether"), accounts[2],avatar.address);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to explain in a comment why this statement is not failing. Afaics, if this and the previous lines fall within the same period, you have sent 9 ether and violated the constraint. So by some coincidence, this last transfer happens in the next period?


});
});