Vesting Escrow Wallet #316
Conversation
…m in the contract
…ury to configure function
…okens/numClaimedVestedTokens/numUnclaimedVestedTokens
| * @notice Type of the Module factory | ||
| */ | ||
| function getType() public view returns(uint8) { | ||
| return 5; |
There was a problem hiding this comment.
Please take the latest code pull from the development-1.5.0 branch. this function is now changed to getTypes()
There was a problem hiding this comment.
Resolved with this commit.
There was a problem hiding this comment.
It still not compatible with the latest commit it should look as -
function getTypes() public view returns(uint8[]) {
uint8[] memory res = new uint8[](1);
res[0] = 6;
return res;
}
I am afraid you need to change the type make it 6 as we are using 5 for the Burn type module
…-core into development-1.5.0
There was a problem hiding this comment.
@shanefontaine I think below requirement is missing
As an issuer I want to be able to view all my vesting schedules and it's progressing so I can have a quick overview of how much tokens each of my employees/affiliates have
|
|
||
| vestingTemplates[templateCount] = VestingTemplate(_totalAllocation, _vestingDuration, _vestingFrequency); | ||
|
|
||
| templateCount = templateCount == 0 ? 0 : templateCount +1; |
There was a problem hiding this comment.
This line should be written after the emission of event otherwise you will emit the wrong templateCount. And please change the name of the variable make it tempalteIndex.
| uint256[] _vestingFrequency | ||
| ) | ||
| public | ||
| onlyOwner |
There was a problem hiding this comment.
No need of onlyOwner modifier if we are using the withPerm() modifier.
| contract VestingEscrowWallet is IWallet { | ||
| using SafeMath for uint; | ||
|
|
||
| bytes32 public constant ISSUER = "ISSUER"; |
There was a problem hiding this comment.
Why we need to permission type to be a ISSUER if the only Issuer is going to call those functions then we can use onlyOwner modifier everywhere. I suggest change the Permission type and give it a different name as WALLET_ADMIN or VESTING_ADMIN or ADMIN etc.
| uint256 _startDate | ||
| ) | ||
| public | ||
| onlyOwner |
There was a problem hiding this comment.
same remove the onlyOwner
| * @param _vestingScheduleIndex Index of the vesting schedule for the target | ||
| * @param _revokeVestingSchedule True if the issuer is reclaiming the tokens out of the contract | ||
| */ | ||
| function cancelVestingSchedule(address _target, uint256 _vestingScheduleIndex, bool _revokeVestingSchedule) |
There was a problem hiding this comment.
rename the _target variable to _vestee. It will be more clear. Please do it in the whole contract
|
|
||
| require(_vestingSchedule.vestingId != 0, "Schedule not initialized"); | ||
|
|
||
| uint256 _currentTranche = _calculateCurrentTranche(_vestingSchedule.startDate, _vestingSchedule.vestingDuration); |
There was a problem hiding this comment.
if _currentTrnache value is 0 then the function should revert no need to process further.
| uint256 _vestingDuration | ||
| ) | ||
| internal | ||
| view |
| * @param _vestingScheduleIndex Index of the vesting schedule for the target | ||
| * @param _revokeVestingSchedule True if the issuer is reclaiming the tokens out of the contract | ||
| */ | ||
| function cancelVestingSchedule(address _target, uint256 _vestingScheduleIndex, bool _revokeVestingSchedule) |
There was a problem hiding this comment.
could you please change the name of the function instead of word cancel could you use some meaning full word that signifies the working of function.
ex- drop, remove etc
| if (setupCost > 0) | ||
| require(polyToken.transferFrom(msg.sender, owner, setupCost), "Failed transferFrom because of sufficent Allowance is not provided"); | ||
| address vestingEscrwoWallet = new VestingEscrowWallet(msg.sender, address(polyToken)); | ||
| return vestingEscrwoWallet; |
There was a problem hiding this comment.
you are not emitting the event GenerateModuleFromFactory
| * @notice Type of the Module factory | ||
| */ | ||
| function getType() public view returns(uint8) { | ||
| return 5; |
There was a problem hiding this comment.
It still not compatible with the latest commit it should look as -
function getTypes() public view returns(uint8[]) {
uint8[] memory res = new uint8[](1);
res[0] = 6;
return res;
}
I am afraid you need to change the type make it 6 as we are using 5 for the Burn type module
|
Closing due to inactivity. |
Please check if the PR fulfills these requirements
What kind of change does this PR introduce?
Add VestingEscrowWallet.sol for Gitcoin Bounty #269.
What is the current behavior?
Everything functions as described in #269.
What is the new behavior?
Everything functions as described in #269.
Does this PR introduce a breaking change?
No
Any Other information: