Conversation
|
Can you spell out "GC" in the contract name? No idea what it means... |
There was a problem hiding this comment.
this is a bit a confusing comment
There was a problem hiding this comment.
maybe call it "periodLength"? and add in the comment that the unit is blocks
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
totalAmountSentPerPeriod, a mapping from period indexes to amounts
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
"and throws an error if the constraint is violated"
test/etherGC.js
Outdated
There was a problem hiding this comment.
comment says 10 blocks, code says 20
There was a problem hiding this comment.
you have a GlobalConstraint interface to inherit from?
There was a problem hiding this comment.
why do you need the controller as a separate argument, can't you get that from the avatar_?
test/etherGC.js
Outdated
There was a problem hiding this comment.
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?
leviadam
left a comment
leviadam
left a comment
There was a problem hiding this comment.
I added some comments. Mainly about time/blocks.
Mostly, looks ok.
There was a problem hiding this comment.
I think period should not be in blocks, as block time is changing.
The timestamp cannot be trusted up to seconds, but it can be for large periods, as in this case.
There was a problem hiding this comment.
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.
| using SafeMath for uint256; | ||
|
|
||
| uint256 public periodLength; //the period length in blocks units | ||
| uint256 public amountAllowedPerPeriod; |
There was a problem hiding this comment.
Not sure if this should be percentage-wise, or absolute values. Probably better to keep it absolute.
| totalAmountSentPerPeriod[currentPeriodIndex] = | ||
| totalAmountSentPerPeriod[currentPeriodIndex].add(avatarBalanceBefore.sub(address(avatar).balance)); | ||
| require(totalAmountSentPerPeriod[currentPeriodIndex] <= amountAllowedPerPeriod, | ||
| "Violation of Global constraint EtherGC:amount sent exceed in current period"); |
There was a problem hiding this comment.
Should set back avatarBalanceBefore to zero, as it will save gas.
5 participants
No description provided.