Skip to content

ExternalLocking4Rep : add registrar #562

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.

Sign up for GitHub

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

Merged
orenyodfat merged 1 commit into from
Nov 20, 2018
Merged

ExternalLocking4Rep : add registrar #562

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
52 changes: 37 additions & 15 deletions contracts/schemes/ExternalLocking4Reputation.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,24 @@ import "openzeppelin-solidity/contracts/ownership/Ownable.sol";

contract ExternalLocking4Reputation is Locking4Reputation, Ownable {

event Register(address indexed _beneficiary);
Copy link
Contributor

@dkent600 dkent600 Nov 18, 2018

Choose a reason for hiding this comment

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

I think I would refer to this everywhere as "Authorizing" rather than "Registering"


address public externalLockingContract;
string public getBalanceFuncSignature;

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

@dkent600 dkent600 Nov 18, 2018

Choose a reason for hiding this comment

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

I think the code would be more clear if this were called "claimers".

Copy link
Contributor

@dkent600 dkent600 Nov 18, 2018

Choose a reason for hiding this comment

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

Even better: "claimants".

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

/**
* @dev initialize
* @param _avatar the avatar to mint reputation from
* @param _reputationReward the total reputation this contract will reward
* for the token locking
* @param _lockingStartTime locking starting period time.
* @param _lockingEndTime the locking end time.
* locking is disable after this time.
* @param _claimingStartTime claiming starting period time.
* @param _claimingEndTime the claiming end time.
* claiming is disable after this time.
* @param _redeemEnableTime redeem enable time .
* redeem reputation can be done after this time.
* @param _externalLockingContract the contract which lock the token.
Expand All @@ -33,36 +37,45 @@ contract ExternalLocking4Reputation is Locking4Reputation, Ownable {
function initialize(
Avatar _avatar,
uint _reputationReward,
uint _lockingStartTime,
uint _lockingEndTime,
uint _claimingStartTime,
uint _claimingEndTime,
uint _redeemEnableTime,
address _externalLockingContract,
string _getBalanceFuncSignature)
external
onlyOwner
{
require(_lockingEndTime > _lockingStartTime, "_lockingEndTime should be greater than _lockingStartTime");
require(_claimingEndTime > _claimingStartTime, "_claimingEndTime should be greater than _claimingStartTime");
externalLockingContract = _externalLockingContract;
getBalanceFuncSignature = _getBalanceFuncSignature;
super._initialize(
_avatar,
_reputationReward,
_lockingStartTime,
_lockingEndTime,
_claimingStartTime,
_claimingEndTime,
_redeemEnableTime,
1);
}

/**
* @dev lock function
* @return lockingId
* @dev claim function
* @param _beneficiary the beneficiary address to claim for
* if _beneficiary == 0 the claim will be for the msg.sender.
* @return claimId
*/
function lock() public returns(bytes32) {
function claim(address _beneficiary) public returns(bytes32) {
Copy link
Contributor

@dkent600 dkent600 Nov 18, 2018

Choose a reason for hiding this comment

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

Can we call "_beneficiary" "_claimer"?

Copy link
Contributor

@leviadam leviadam Nov 18, 2018

Choose a reason for hiding this comment

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

But it is not the claimer, as one can claim for someone else, who was registered.

Copy link
Contributor

@dkent600 dkent600 Nov 18, 2018

Choose a reason for hiding this comment

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

@leviadam I view this as a person having "authorized" anyone else to claim the tokens for them. The word "claimant" would be better here than "claimer".

So I might rename the function "Register" to something else. A legal term is "Release" (to release the exclusive right to do something). Maybe "Authorize", though one is not authorizing any particular account to do the claim.

Mainly I think we're being inconsistent inside the contract with the sense of "locking" and "claiming", and that is the primary source of confusion.

Further, frankly I'm not wild about "claim" either. My understanding of the Magnolia contract is that what this "claim" method is doing is simply placing the claimant's tokens into a mapping from which the tokens can eventually be "unlocked" (placed in another mapping), then withdrawn (placed in another mapping) that finally allows the owner of the tokens to have control over the tokens.

So what "claim" is doing is not in fact claiming anything, as the Magnolia contract currently stands (in Kovan, at 0x4eDc383aDEa781762b74E7082C03F423523e61Bb).

Unless the contract is changing, "Lock" seems appropriate here.

Copy link
Contributor

@dkent600 dkent600 Nov 18, 2018

Choose a reason for hiding this comment

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

@leviadam

msg.sender in this case is claiming on behalf of the claimant (here _beneficiary), who "registered" (poor word IMHO) to allow someone else (here msg.sender) to "claim" on their behalf.

msg.sender is not here claiming anything, rather is executing a claim an behalf of someone else.

require(avatar != Avatar(0), "should initialize first");
require(externalLockers[msg.sender] == false, "locking twice is not allowed");
externalLockers[msg.sender] = true;
address beneficiary;
if (_beneficiary == address(0)) {
beneficiary = msg.sender;
} else {
require(registrar[_beneficiary],"beneficiary should be register");
beneficiary = _beneficiary;
}
require(externalLockers[beneficiary] == false, "claiming twice is not allowed");
externalLockers[beneficiary] = 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, beneficiary));
uint lockedAmount;
// solium-disable-next-line security/no-inline-assembly
assembly {
Expand All @@ -73,6 +86,15 @@ contract ExternalLocking4Reputation is Locking4Reputation, Ownable {
default { lockedAmount := mload(0) }
}

return super._lock(lockedAmount, 1, msg.sender);
return super._lock(lockedAmount, 1, beneficiary);
}

/**
* @dev register function
* register for external locking claim
*/
function register() public {
registrar[msg.sender] = true;
emit Register(msg.sender);
}
}
73 changes: 50 additions & 23 deletions test/externallocking4reputation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@ const constants = require('./constants');
var ExternalLocking4Reputation = artifacts.require("./ExternalLocking4Reputation.sol");
var ExternalTokenLockerMock = artifacts.require("./ExternalTokenLockerMock.sol");

const setup = async function (accounts,_repAllocation = 100,_lockingStartTime = 0,_lockingEndTime = 3000,_redeemEnableTime = 3000,_initialize = true) {
const setup = async function (accounts,_repAllocation = 100,_claimingStartTime = 0,_claimingEndTime = 3000,_redeemEnableTime = 3000,_initialize = true) {
var testSetup = new helpers.TestSetup();
var controllerCreator = await ControllerCreator.new({gas: constants.ARC_GAS_LIMIT});
testSetup.daoCreator = await DaoCreator.new(controllerCreator.address,{gas:constants.ARC_GAS_LIMIT});
testSetup.org = await helpers.setupOrganization(testSetup.daoCreator,accounts[0],1000,1000);
var block = await web3.eth.getBlock("latest");
testSetup.lockingEndTime = block.timestamp + _lockingEndTime;
testSetup.lockingStartTime = block.timestamp + _lockingStartTime;
testSetup.lockingEndTime = block.timestamp + _claimingEndTime;
testSetup.lockingStartTime = block.timestamp + _claimingStartTime;
testSetup.redeemEnableTime = block.timestamp + _redeemEnableTime;
testSetup.extetnalTokenLockerMock = await ExternalTokenLockerMock.new();
await testSetup.extetnalTokenLockerMock.lock(100,{from:accounts[0]});
Expand Down Expand Up @@ -46,9 +46,9 @@ contract('ExternalLocking4Reputation', accounts => {
assert.equal(await testSetup.externalLocking4Reputation.getBalanceFuncSignature(),"lockedTokenBalances(address)");
});

it("lock", async () => {
it("claim", async () => {
let testSetup = await setup(accounts);
var tx = await testSetup.externalLocking4Reputation.lock();
var tx = await testSetup.externalLocking4Reputation.claim(helpers.NULL_ADDRESS);
var lockingId = await helpers.getValueFromLogs(tx, '_lockingId',1);
assert.equal(tx.logs.length,1);
assert.equal(tx.logs[0].event,"Lock");
Expand All @@ -58,52 +58,79 @@ contract('ExternalLocking4Reputation', accounts => {
assert.equal(tx.logs[0].args._locker,accounts[0]);
});

it("cannot lock before set parameters", async () => {
it("claim on behalf of a beneficiary", async () => {
let testSetup = await setup(accounts);
var tx = await testSetup.externalLocking4Reputation.register({from:accounts[1]});
assert.equal(tx.logs.length,1);
assert.equal(tx.logs[0].event,"Register");
assert.equal(tx.logs[0].args._beneficiary,accounts[1]);
tx = await testSetup.externalLocking4Reputation.claim(accounts[1]);
var lockingId = await helpers.getValueFromLogs(tx, '_lockingId',1);
assert.equal(tx.logs.length,1);
assert.equal(tx.logs[0].event,"Lock");
assert.equal(tx.logs[0].args._lockingId,lockingId);
assert.equal(tx.logs[0].args._amount,200);
assert.equal(tx.logs[0].args._period,1);
assert.equal(tx.logs[0].args._locker,accounts[1]);
});

it("cannot claim on behalf of a beneficiary if not register", async () => {
let testSetup = await setup(accounts);
try {
await testSetup.externalLocking4Reputation.claim(accounts[1]);
assert(false, "cannot claim on behalf of a beneficiary if not register");
} catch(error) {
helpers.assertVMException(error);
}
});


it("cannot claim before set parameters", async () => {
let testSetup = await setup(accounts,100,0,3000,3000,false);
try {
await testSetup.externalLocking4Reputation.lock();
await testSetup.externalLocking4Reputation.claim(helpers.NULL_ADDRESS);
assert(false, "cannot lock before set parameters");
} catch(error) {
helpers.assertVMException(error);
}
});

it("lock with value == 0 should revert", async () => {
it("claim with value == 0 should revert", async () => {
let testSetup = await setup(accounts);
try {
await testSetup.externalLocking4Reputation.lock({from:accounts[4]});
await testSetup.externalLocking4Reputation.claim(helpers.NULL_ADDRESS,{from:accounts[4]});
assert(false, "lock with value == 0 should revert");
} catch(error) {
helpers.assertVMException(error);
}
});

it("lock after _lockingEndTime should revert", async () => {
it("claim after _claimingEndTime should revert", async () => {
let testSetup = await setup(accounts);
await helpers.increaseTime(3001);
try {
await testSetup.externalLocking4Reputation.lock();
assert(false, "lock after _lockingEndTime should revert");
await testSetup.externalLocking4Reputation.claim(helpers.NULL_ADDRESS);
assert(false, "lock after _claimingEndTime should revert");
} catch(error) {
helpers.assertVMException(error);
}
});

it("lock before start should revert", async () => {
it("claim before start should revert", async () => {
let testSetup = await setup(accounts,100,100);
try {
await testSetup.externalLocking4Reputation.lock();
await testSetup.externalLocking4Reputation.claim(helpers.NULL_ADDRESS);
assert(false, "lock before start should revert");
} catch(error) {
helpers.assertVMException(error);
}
});

it("cannot lock twice for the same user", async () => {
it("cannot claim twice for the same user", async () => {
let testSetup = await setup(accounts);
await testSetup.externalLocking4Reputation.lock();
await testSetup.externalLocking4Reputation.claim(helpers.NULL_ADDRESS);
try {
await testSetup.externalLocking4Reputation.lock();
await testSetup.externalLocking4Reputation.claim(helpers.NULL_ADDRESS);
assert(false, "cannot lock twice for the same user");
} catch(error) {
helpers.assertVMException(error);
Expand All @@ -112,7 +139,7 @@ contract('ExternalLocking4Reputation', accounts => {

it("redeem", async () => {
let testSetup = await setup(accounts);
var tx = await testSetup.externalLocking4Reputation.lock();
var tx = await testSetup.externalLocking4Reputation.claim(helpers.NULL_ADDRESS);
await helpers.increaseTime(3001);
tx = await testSetup.externalLocking4Reputation.redeem(accounts[0]);
assert.equal(tx.logs.length,1);
Expand All @@ -124,8 +151,8 @@ contract('ExternalLocking4Reputation', accounts => {

it("redeem score ", async () => {
let testSetup = await setup(accounts);
await testSetup.externalLocking4Reputation.lock({from:accounts[0]});
await testSetup.externalLocking4Reputation.lock({from:accounts[2]});
await testSetup.externalLocking4Reputation.claim(helpers.NULL_ADDRESS,{from:accounts[0]});
await testSetup.externalLocking4Reputation.claim(helpers.NULL_ADDRESS,{from:accounts[2]});
await helpers.increaseTime(3001);
await testSetup.externalLocking4Reputation.redeem(accounts[0]);
await testSetup.externalLocking4Reputation.redeem(accounts[2]);
Expand All @@ -135,7 +162,7 @@ contract('ExternalLocking4Reputation', accounts => {

it("redeem cannot redeem twice", async () => {
let testSetup = await setup(accounts);
await testSetup.externalLocking4Reputation.lock();
await testSetup.externalLocking4Reputation.claim(helpers.NULL_ADDRESS);
await helpers.increaseTime(3001);
await testSetup.externalLocking4Reputation.redeem(accounts[0]);
try {
Expand All @@ -148,7 +175,7 @@ contract('ExternalLocking4Reputation', accounts => {

it("redeem before lockingEndTime should revert", async () => {
let testSetup = await setup(accounts);
await testSetup.externalLocking4Reputation.lock();
await testSetup.externalLocking4Reputation.claim(helpers.NULL_ADDRESS);
await helpers.increaseTime(50);
try {
await testSetup.externalLocking4Reputation.redeem(accounts[0]);
Expand All @@ -160,7 +187,7 @@ contract('ExternalLocking4Reputation', accounts => {

it("redeem before redeemEnableTime should revert", async () => {
let testSetup = await setup(accounts,100,0,3000,4000,true);
await testSetup.externalLocking4Reputation.lock();
await testSetup.externalLocking4Reputation.claim(helpers.NULL_ADDRESS);
await helpers.increaseTime(3500);
try {
await testSetup.externalLocking4Reputation.redeem(accounts[0]);
Expand Down