Skip to content

Conversation

orenyodfat
Copy link
Contributor

@orenyodfat orenyodfat commented Sep 3, 2019

bootstrap scheme for nectar dao.

@orenyodfat orenyodfat requested a review from leviadam as a code owner September 3, 2019 14:12
Copy link
Contributor

@leviadam leviadam left a comment

Choose a reason for hiding this comment

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

Logic lgtm.
I think the names are making it very hard to understand the code.

event Release(bytes32 indexed _lockingId, address indexed _beneficiary, uint256 _amount);
event LockToken(address indexed _locker, bytes32 indexed _lockingId, uint256 _amount, uint256 _period);

struct Locking {
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 Batch is a better wording here.

uint256 public startTime;
uint256 public numberOfLockingPeriods;
uint256 public redeemEnableTime;
uint256 public maxLockingPeriods;
Copy link
Contributor

Choose a reason for hiding this comment

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

Having two variables with different meaning with such close names maxLockingPeriods and MAX_LOCKING_PERIODS is very confusing.
I propose, maxLockingBatches and MAX_LOCKING_BATCHES_HARDCAP.
Another way is to make MAX_LOCKING_BATCHES_HARDCAP private, as it is not very interesting to the user.

*/
uint256 constant private REAL_ONE = uint256(1) << REAL_FBITS;

uint256 constant public MAX_PERIODS = 100;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would make this a hardcap, and allow the user to initiate the actual value, as it is close to max locking.
Would also make this one private.

uint256 _periodsUnit,
uint256 _redeemEnableTime,
uint256 _maxLockingPeriods,
uint256 _repRewardConstA,
Copy link
Contributor

Choose a reason for hiding this comment

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

No @param comment.

mapping(address=>uint256) scores;
}

struct Locker {
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 not representing a locker but rather a specific locking.

uint256 j = _period;
//fill in the next lockings scores.
//todo : check limitation of _period and require that on the init function.
for (int256 i = int256(lockingPeriodToLockIn + _period - 1); i >= int256(lockingPeriodToLockIn); i--) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it simpler to write the loop forward?

Oren Sokolowsky added 2 commits September 8, 2019 17:16
@orenyodfat
Copy link
Contributor Author

thanks @leviadam .
see 00e2858 and b22f5c3

Copy link
Contributor

@leviadam leviadam left a comment

Choose a reason for hiding this comment

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

Hi @orenyodfat,
I think it is still not good enough. The names are not really coherent.
We can also ask someone like @jellegerbrandy, who is good naming to take a look.

// A mapping from lockers addresses their lock balances.
mapping(address => mapping(bytes32=>Locker)) public lockers;
mapping(address => mapping(bytes32=>Lock)) public lockers;
// A mapping from locking index to locking.
Copy link
Contributor

Choose a reason for hiding this comment

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

A mapping from batch index to batch.

uint256 public redeemEnableTime;
uint256 public maxLockingPeriods;
uint256 public maxLockingBatches;
uint256 public periodsUnit;
Copy link
Contributor

Choose a reason for hiding this comment

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

batchTime?

Lock storage locker = lockers[_beneficiary][_lockingId];
uint256 periodToRedeemFrom = (locker.lockingTime - startTime) / periodsUnit;
// solhint-disable-next-line not-rely-on-time
uint256 currentLockingPeriod = (now - startTime) / periodsUnit;
Copy link
Contributor

Choose a reason for hiding this comment

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

currentBatch?

@orenyodfat orenyodfat closed this Sep 8, 2019
@orenyodfat orenyodfat reopened this Sep 8, 2019
@orenyodfat
Copy link
Contributor Author

thanks @leviadam

@orenyodfat orenyodfat merged commit ae65363 into master Sep 9, 2019
@orenyodfat orenyodfat deleted the infinitrepscheme branch September 9, 2019 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants