Lockup Volume Restrictions #290
Conversation
Development 1.5.0
|
Heads up - this isn't ready yet. I'm adding some parameter checking. |
| // lets the owner / admin create a volume restriction lockup. | ||
| // takes a userAddress and lock up params, and creates a lockup for the user. See the LockUp struct def for info on what each parameter actually is | ||
| function addLockUp(address userAddress, uint lockUpPeriodSeconds, uint releaseFrequencySeconds, uint startTime, uint totalAmount) public withPerm(ADMIN) { | ||
|
|
There was a problem hiding this comment.
I prefer to add some sanity checks for the input parameters
| newLockUp.releaseFrequencySeconds = releaseFrequencySeconds; | ||
| newLockUp.startTime = startTime; | ||
| newLockUp.totalAmount = totalAmount; | ||
| userLockUps.push(newLockUp); |
There was a problem hiding this comment.
Instead of the current approach, it can be re-write in a single line as
lockUps[userAddress].push(LockUP(lockUpPeriodSeconds, releaseFrequencySeconds, startTime , totalAmount))
|
|
||
| enum LockUpOperationType { Add, Remove, Edit } | ||
|
|
||
| event LogModifyLockUp( |
There was a problem hiding this comment.
As we change our event naming convention, So please remove the Log prefix from the name.
and I prefer the new event for operation type so it is easily understandable with the name.
ex-
event addNewLockUp(...)
event removeLockUP(...)
event modifyLockUp(...)
| userLockUps.length--; | ||
| } | ||
|
|
||
| function editLockUp(address userAddress, uint lockUpIndex, uint lockUpPeriodSeconds, uint releaseFrequencySeconds, uint startTime, uint totalAmount) public withPerm(ADMIN) { |
There was a problem hiding this comment.
rename the function name to modifyLockUp()
| if (startTime == 0) { | ||
| startTime = now; | ||
| } | ||
|
|
There was a problem hiding this comment.
sanity checks are missing
There was a problem hiding this comment.
Hey @satyamakgec - could you clarify this one? What kind of sanity check were you thinking?
It was my thinking that a startTime before "now" may be desired if you want to backdate someone's vesting, which I think is a common thing. And obviously a startTime after "now" could be desired if you're awarding tokens to an employee who, for example, starts working in 2 weeks or something.
|
@glitch003 I miss reading the |
…ll work long term so moving it into this branch
…ction-transfer-manager-with-internal-withdrawn-balances
…r-with-internal-withdrawn-balances Volume restriction transfer manager with internal withdrawn balances
Development 1.5.0
Merge in latest dev-1.5.0 from polymath
… are happier on travis
|
@satyamakgec ok, this should be good to go. I made the requested changes, and now the withdrawn amounts are tracked inside the transfer module. Let me know if there's anything else that needs to be changed |
|
Hey @adamdossa thanks for those fixes to update my branch with dev-1.5.0. Much appreciated! I put in one more fix, and now the tests pass. Let me know if there are any more changes needed to get this merged. Thanks! |
Please check if the PR fulfills these requirements
What kind of change does this PR introduce?
#287
Does this PR introduce a breaking change?
No
Any Other information:
Some notes:
Thanks!