Skip to content

Commit

Permalink
Auction4Reputation small optimization (#540)
Browse files Browse the repository at this point in the history 
* Auction4Reputation small optimization

* Fix small bug and readablility

Added spaces for readablility
Added missing require to disallow re-initialization.
Changed use of storage var to memory var in an init function.
  • Loading branch information
@ben-kaufman @orenyodfat
ben-kaufman authored and orenyodfat committed Sep 27, 2018
1 parent ce5fcc1 commit bba1a43
Show file tree
Hide file tree
Showing 7 changed files with 85 additions and 89 deletions.
36 changes: 17 additions & 19 deletions contracts/schemes/Auction4Reputation.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ contract Auction4Reputation is Ownable {
mapping(uint=>Auction) public auctions;

Avatar public avatar;
uint public reputationReward;
uint public reputationRewardLeft;
uint public auctionsEndTime;
uint public auctionsStartTime;
Expand Down Expand Up @@ -61,21 +60,20 @@ contract Auction4Reputation is Ownable {
external
onlyOwner
{
require(avatar == Avatar(0),"can be called only one time");
require(_avatar != Avatar(0),"avatar cannot be zero");
//number of auctions cannot be zero
//auctionsEndTime should be greater than auctionsStartTime
require(avatar == Avatar(0), "can be called only one time");
require(_avatar != Avatar(0), "avatar cannot be zero");
// number of auctions cannot be zero
// auctionsEndTime should be greater than auctionsStartTime
auctionPeriod = (_auctionsEndTime.sub(_auctionsStartTime)).div(_numberOfAuctions);
require(auctionPeriod > 0,"auctionPeriod should be > 0");
require(auctionPeriod > 0, "auctionPeriod should be > 0");
token = _token;
avatar = _avatar;
reputationReward = _reputationReward;
auctionsStartTime = _auctionsStartTime;
auctionsEndTime = _auctionsEndTime;
numberOfAuctions = _numberOfAuctions;
wallet = _wallet;
auctionReputationReward = reputationReward/numberOfAuctions;
reputationRewardLeft = reputationReward;
auctionReputationReward = _reputationReward / _numberOfAuctions;
reputationRewardLeft = _reputationReward;
}

/**
Expand All @@ -84,18 +82,18 @@ contract Auction4Reputation is Ownable {
* @param _auctionId the auction id to redeem from.
* @return bool
*/
function redeem(address _beneficiary,uint _auctionId) public returns(bool) {
function redeem(address _beneficiary, uint _auctionId) public returns(bool) {
// solium-disable-next-line security/no-block-members
require(now >= auctionsEndTime,"check the auctions period pass");
require(now >= auctionsEndTime, "check the auctions period pass");
Auction storage auction = auctions[_auctionId];
uint bid = auction.bids[_beneficiary];
require(bid > 0,"bidding amount should be > 0");
require(bid > 0, "bidding amount should be > 0");
auction.bids[_beneficiary] = 0;
int256 repRelation = int216(bid).toReal().mul(int216(auctionReputationReward).toReal());
uint reputation = uint256(repRelation.div(int216(auction.totalBid).toReal()).fromReal());
//check that the reputation is sum zero
// check that the reputation is sum zero
reputationRewardLeft = reputationRewardLeft.sub(reputation);
require(ControllerInterface(avatar.owner()).mintReputation(reputation, _beneficiary, avatar),"mint reputation should success");
require(ControllerInterface(avatar.owner()).mintReputation(reputation, _beneficiary, avatar), "mint reputation should success");
emit Redeem(_auctionId, _beneficiary, reputation);
return true;
}
Expand All @@ -106,14 +104,14 @@ contract Auction4Reputation is Ownable {
* @return auctionId
*/
function bid(uint _amount) public returns(uint auctionId) {
require(_amount > 0,"bidding amount should be > 0");
require(_amount > 0, "bidding amount should be > 0");
// solium-disable-next-line security/no-block-members
require(now <= auctionsEndTime,"bidding should be within the allowed bidding period");
require(now <= auctionsEndTime, "bidding should be within the allowed bidding period");
// solium-disable-next-line security/no-block-members
require(now >= auctionsStartTime,"bidding is enable only after bidding auctionsStartTime");
require(token.transferFrom(msg.sender, wallet, _amount),"transferFrom should success");
require(now >= auctionsStartTime, "bidding is enable only after bidding auctionsStartTime");
require(token.transferFrom(msg.sender, wallet, _amount), "transferFrom should success");
// solium-disable-next-line security/no-block-members
auctionId = (now - auctionsStartTime)/auctionPeriod;
auctionId = (now - auctionsStartTime) / auctionPeriod;
Auction storage auction = auctions[auctionId];
auction.totalBid = auction.totalBid.add(_amount);
auction.bids[msg.sender] = auction.bids[msg.sender].add(_amount);
Expand Down
17 changes: 9 additions & 8 deletions contracts/schemes/ExternalLocking4Reputation.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@ import "openzeppelin-solidity/contracts/ownership/Ownable.sol";
* @title A scheme for external locking Tokens for reputation
*/

contract ExternalLocking4Reputation is Locking4Reputation,Ownable {
contract ExternalLocking4Reputation is Locking4Reputation, Ownable {

address public externalLockingContract;
string public getBalanceFuncSignature;

// locker->bool
mapping(address=>bool) public externalLockers;
// locker -> bool
mapping(address => bool) public externalLockers;

/**
* @dev initialize
Expand All @@ -39,7 +39,7 @@ contract ExternalLocking4Reputation is Locking4Reputation,Ownable {
external
onlyOwner
{
require(_lockingEndTime > _lockingStartTime,"_lockingEndTime should be greater than _lockingStartTime");
require(_lockingEndTime > _lockingStartTime, "_lockingEndTime should be greater than _lockingStartTime");
externalLockingContract = _externalLockingContract;
getBalanceFuncSignature = _getBalanceFuncSignature;
super._initialize(
Expand All @@ -55,11 +55,11 @@ contract ExternalLocking4Reputation is Locking4Reputation,Ownable {
* @return lockingId
*/
function lock() public returns(bytes32) {
require(avatar != Avatar(0),"should initialize first");
require(externalLockers[msg.sender] == false,"locking twice is not allowed");
require(avatar != Avatar(0), "should initialize first");
require(externalLockers[msg.sender] == false, "locking twice is not allowed");
externalLockers[msg.sender] = true;
// solium-disable-next-line security/no-low-level-calls
bool result = externalLockingContract.call(abi.encodeWithSignature(getBalanceFuncSignature,msg.sender));
bool result = externalLockingContract.call(abi.encodeWithSignature(getBalanceFuncSignature, msg.sender));
uint lockedAmount;
// solium-disable-next-line security/no-inline-assembly
assembly {
Expand All @@ -69,6 +69,7 @@ contract ExternalLocking4Reputation is Locking4Reputation,Ownable {
case 0 { revert(0, returndatasize) }
default { lockedAmount := mload(0) }
}
return super._lock(lockedAmount,1,msg.sender);

return super._lock(lockedAmount, 1, msg.sender);
}
}
46 changes: 18 additions & 28 deletions contracts/schemes/FixReputationAllocation.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,16 @@ import "openzeppelin-solidity/contracts/ownership/Ownable.sol";
* This scheme can be used to allocate a pre define amount of reputation to whitelisted
* beneficiaries.
*/

contract FixedReputationAllocation is Ownable {
using SafeMath for uint;
using RealMath for int216;
using RealMath for int256;

event Redeem(address indexed _beneficiary,uint _amount);
event Redeem(address indexed _beneficiary, uint _amount);
event BeneficiaryAddressAdded(address _beneficiary);

// beneficiary -> exist
mapping(address=>bool) public beneficiaries;
mapping(address => bool) public beneficiaries;

Avatar public avatar;
uint public reputationReward;
Expand All @@ -33,13 +32,9 @@ contract FixedReputationAllocation is Ownable {
* @param _avatar the avatar to mint reputation from
* @param _reputationReward the total reputation this contract will reward
*/
function initialize(
Avatar _avatar,
uint _reputationReward)
external
onlyOwner
{
require(avatar == Avatar(0),"can be called only one time");
function initialize(Avatar _avatar, uint _reputationReward) external onlyOwner {
require(avatar == Avatar(0), "can be called only one time");
require(_avatar != Avatar(0), "avatar cannot be zero");
reputationReward = _reputationReward;
avatar = _avatar;
}
Expand All @@ -50,25 +45,26 @@ contract FixedReputationAllocation is Ownable {
* @return bool
*/
function redeem(address _beneficiary) public returns(bool) {
require(isEnable,"require to be enable");
require(beneficiaries[_beneficiary],"require _beneficiary to exist in the beneficiaries map");
require(ControllerInterface(avatar.owner()).mintReputation(beneficiaryReward,_beneficiary,avatar),"mint reputation should success");
emit Redeem(_beneficiary,beneficiaryReward);
require(isEnable, "require to be enable");
require(beneficiaries[_beneficiary], "require _beneficiary to exist in the beneficiaries map");
require(ControllerInterface(avatar.owner()).mintReputation(beneficiaryReward, _beneficiary, avatar), "mint reputation should success");

emit Redeem(_beneficiary, beneficiaryReward);

return true;
}

/**
* @dev addBeneficiary function
* @param _beneficiary to be whitelisted
*/
function addBeneficiary(address _beneficiary)
public
onlyOwner
{
require(!isEnable,"can add beneficiary only if not already enable");
function addBeneficiary(address _beneficiary) public onlyOwner {
require(!isEnable, "can add beneficiary only if not already enable");

if (!beneficiaries[_beneficiary]) {
beneficiaries[_beneficiary] = true;
numberOfBeneficiaries++;

emit BeneficiaryAddressAdded(_beneficiary);
}
}
Expand All @@ -77,10 +73,7 @@ contract FixedReputationAllocation is Ownable {
* @dev add addBeneficiaries function
* @param _beneficiaries addresses
*/
function addBeneficiaries(address[] _beneficiaries)
public
onlyOwner
{
function addBeneficiaries(address[] _beneficiaries) public onlyOwner {
for (uint256 i = 0; i < _beneficiaries.length; i++) {
addBeneficiary(_beneficiaries[i]);
}
Expand All @@ -89,12 +82,9 @@ contract FixedReputationAllocation is Ownable {
/**
* @dev enable function
*/
function enable()
public
onlyOwner
{
function enable() public onlyOwner {
isEnable = true;
//Calculate beneficiary reward
// Calculate beneficiary reward
beneficiaryReward = uint256(int216(reputationReward).div(int256(numberOfBeneficiaries)).fromReal());
}
}
55 changes: 30 additions & 25 deletions contracts/schemes/Locking4Reputation.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@ contract Locking4Reputation {
using RealMath for int216;
using RealMath for int256;

event Redeem(bytes32 indexed _lockingId, address indexed _beneficiary,uint _amount);
event Release(bytes32 indexed _lockingId, address indexed _beneficiary,uint _amount);
event Lock(bytes32 indexed _lockingId,uint _amount,uint _period,address _locker);
event Redeem(bytes32 indexed _lockingId, address indexed _beneficiary, uint _amount);
event Release(bytes32 indexed _lockingId, address indexed _beneficiary, uint _amount);
event Lock(bytes32 indexed _lockingId, uint _amount, uint _period, address _locker);

struct Locker {
uint amount;
Expand All @@ -25,9 +25,9 @@ contract Locking4Reputation {
Avatar public avatar;

// A mapping from lockers addresses their lock balances.
mapping(address=>mapping(bytes32=>Locker)) public lockers;
mapping(address => mapping(bytes32=>Locker)) public lockers;
// A mapping from lockers addresses to their scores.
mapping(address=>uint) public scores;
mapping(address => uint) public scores;

uint public totalLocked;
uint public totalLockedLeft;
Expand All @@ -45,18 +45,21 @@ contract Locking4Reputation {
* @param _lockingId the locking id to release
* @return bool
*/
function redeem(address _beneficiary,bytes32 _lockingId) public returns(bool) {
function redeem(address _beneficiary, bytes32 _lockingId) public returns(bool) {
// solium-disable-next-line security/no-block-members
require(block.timestamp >= lockingEndTime,"check the lock period pass");
require(scores[_beneficiary] > 0,"score should be > 0");
require(block.timestamp >= lockingEndTime, "check the lock period pass");
require(scores[_beneficiary] > 0, "score should be > 0");
uint score = scores[_beneficiary];
scores[_beneficiary] = 0;
int256 repRelation = int216(score).toReal().mul(int216(reputationReward).toReal());
uint reputation = uint256(repRelation.div(int216(totalScore).toReal()).fromReal());

//check that the reputation is sum zero
reputationRewardLeft = reputationRewardLeft.sub(reputation);
require(ControllerInterface(avatar.owner()).mintReputation(reputation,_beneficiary,avatar),"mint reputation should success");
emit Redeem(_lockingId,_beneficiary,reputation);
require(ControllerInterface(avatar.owner()).mintReputation(reputation, _beneficiary, avatar), "mint reputation should success");

emit Redeem(_lockingId, _beneficiary, reputation);

return true;
}

Expand All @@ -66,15 +69,16 @@ contract Locking4Reputation {
* @param _lockingId the locking id to release
* @return bool
*/
function _release(address _beneficiary,bytes32 _lockingId) internal returns(uint amount) {
function _release(address _beneficiary, bytes32 _lockingId) internal returns(uint amount) {
Locker storage locker = lockers[_beneficiary][_lockingId];
require(locker.amount > 0,"amount should be > 0");
require(locker.amount > 0, "amount should be > 0");
amount = locker.amount;
locker.amount = 0;
// solium-disable-next-line security/no-block-members
require(block.timestamp >= locker.releaseTime,"check the lock period pass");
require(block.timestamp >= locker.releaseTime, "check the lock period pass");
totalLockedLeft = totalLockedLeft.sub(amount);
emit Release(_lockingId,_beneficiary,amount);

emit Release(_lockingId, _beneficiary, amount);
}

/**
Expand All @@ -85,14 +89,13 @@ contract Locking4Reputation {
* @return lockingId
*/
function _lock(uint _amount, uint _period, address _locker) internal returns(bytes32 lockingId) {

require(_amount > 0,"locking amount should be > 0");
require(_period <= maxLockingPeriod,"locking period should be <= maxLockingPeriod");
require(_period > 0,"locking period should be > 0");
require(_amount > 0, "locking amount should be > 0");
require(_period <= maxLockingPeriod, "locking period should be <= maxLockingPeriod");
require(_period > 0, "locking period should be > 0");
// solium-disable-next-line security/no-block-members
require(now <= lockingEndTime,"lock should be within the allowed locking period");
require(now <= lockingEndTime, "lock should be within the allowed locking period");
// solium-disable-next-line security/no-block-members
require(now >= lockingStartTime,"lock should start after lockingStartTime");
require(now >= lockingStartTime, "lock should start after lockingStartTime");

lockingId = keccak256(abi.encodePacked(this, lockingsCounter));
lockingsCounter++;
Expand All @@ -105,7 +108,8 @@ contract Locking4Reputation {
totalLockedLeft = totalLocked;
scores[_locker] = scores[_locker].add(_period.mul(_amount));
totalScore = totalScore.add(scores[_locker]);
emit Lock(lockingId,_amount,_period,_locker);

emit Lock(lockingId, _amount, _period, _locker);
}

/**
Expand All @@ -127,11 +131,12 @@ contract Locking4Reputation {
uint _maxLockingPeriod)
internal
{
require(avatar == Avatar(0),"can be called only one time");
require(_avatar != Avatar(0),"avatar cannot be zero");
require(_lockingEndTime > _lockingStartTime,"locking end time should be greater than locking start time");
require(avatar == Avatar(0), "can be called only one time");
require(_avatar != Avatar(0), "avatar cannot be zero");
require(_lockingEndTime > _lockingStartTime, "locking end time should be greater than locking start time");

reputationReward = _reputationReward;
reputationRewardLeft = reputationReward;
reputationRewardLeft = _reputationReward;
lockingEndTime = _lockingEndTime;
maxLockingPeriod = _maxLockingPeriod;
avatar = _avatar;
Expand Down
5 changes: 3 additions & 2 deletions contracts/schemes/LockingEth4Reputation.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import "openzeppelin-solidity/contracts/ownership/Ownable.sol";
* @title A scheme for locking ETH for reputation
*/

contract LockingEth4Reputation is Locking4Reputation,Ownable {
contract LockingEth4Reputation is Locking4Reputation, Ownable {

/**
* @dev initialize
Expand Down Expand Up @@ -45,8 +45,9 @@ contract LockingEth4Reputation is Locking4Reputation,Ownable {
* @return bool
*/
function release(address _beneficiary, bytes32 _lockingId) public returns(bool) {
uint amount = super._release(_beneficiary,_lockingId);
uint amount = super._release(_beneficiary, _lockingId);
_beneficiary.transfer(amount);

return true;
}

Expand Down
Loading

0 comments on commit bba1a43

Please sign in to comment.