Vesting creator automation #264
Conversation
ghost
assigned 
smbsp
force-pushed
the
vesting-creator-automation
branch
from
eaaaeee
to
5bb334c
Compare
smbsp
force-pushed
the
vesting-creator-automation
branch
from
5bb334c
to
0fedb12
Compare
|
Changes are done. Awaiting audit results. Will resume working on this when the front end development starts on June 28. |
|
Tested Vesting Migrations successfully. Kept the temporary files here: https://drive.google.com/drive/folders/1NgrTxyqZQ43hcanssGAGvXmqvIUNEPJp?usp=sharing |
…e/Sovryn-smart-contracts into vesting-creator-automation
| function setVestingFactory(address _vestingFactory) public onlyOwner { | ||
| _setVestingFactory(_vestingFactory); | ||
| } | ||
|
|
||
| /** | ||
| * @notice Internal function that sets vesting factory address | ||
| * @param _vestingFactory the address of vesting factory contract | ||
| */ | ||
| function _setVestingFactory(address _vestingFactory) internal { | ||
| require(_vestingFactory != address(0), "vestingFactory address invalid"); | ||
| vestingFactory = IVestingFactory(_vestingFactory); | ||
| } |
There was a problem hiding this comment.
It would be better to declare visibilty of setVestingFactory as external
| require(_receiver != address(0), "receiver address invalid"); | ||
| require(_amount != 0, "amount invalid"); | ||
|
|
||
| IERC20(SOV).transfer(_receiver, _amount); |
There was a problem hiding this comment.
doesn't check returned value of ERC20 token
There was a problem hiding this comment.
Added a check:
require(IERC20(SOV).transfer(_receiver, _amount), "transfer failed");
| uint256[] storage vestingIds = vestingsOf[_tokenOwner]; | ||
| Vesting[] memory _vestings = new Vesting[](vestingIds.length); | ||
| for (uint256 i = 0; i < vestingIds.length; i++) { | ||
| Vesting storage _vesting = vestings[vestingIds[i]]; |
There was a problem hiding this comment.
It seems like storage is not necessary
| Vesting[] memory _vestings = new Vesting[](vestingIds.length); | ||
| for (uint256 i = 0; i < vestingIds.length; i++) { | ||
| Vesting storage _vesting = vestings[vestingIds[i]]; | ||
| _vestings[i] = Vesting(_vesting.vestingType, _vesting.vestingCreationType, _vesting.vestingAddress); |
There was a problem hiding this comment.
It was possible not to declare new Vesting but just to copy (_vestings[i] = vestings[vestingIds[i]])
| SOV = IERC20(_SOV); | ||
| vestingRegistryLogic = VestingRegistryLogic(_vestingRegistryLogic); |
There was a problem hiding this comment.
Consider to declare as immutable to save gas consumption
There was a problem hiding this comment.
I think Immutable keyword is not available in Solidity 0.5
| require(_receiver != address(0), "receiver address invalid"); | ||
| require(_amount != 0, "amount invalid"); |
There was a problem hiding this comment.
SOV is your ERC20 token the transfer of which checks receiver != address(0). It means that you can remove your require to reduce gas consumption. And also you can remove _amount != 0 since transfer won't revert in case of 0
There was a problem hiding this comment.
I think we cannot remove the check receiver != address(0) as it will fail with 'function call to a non-contract' while passing zero. I think it is good to have both the checks.
| bool vestingCreated; | ||
|
|
||
| /// @notice 2 weeks in seconds. | ||
| uint256 constant TWO_WEEKS = 1209600; |
There was a problem hiding this comment.
Visibility should be declared explicitly
| function getVestingsOf(address _tokenOwner) external view returns (Vesting[] memory) { | ||
| uint256[] storage vestingIds = vestingsOf[_tokenOwner]; | ||
| Vesting[] memory _vestings = new Vesting[](vestingIds.length); | ||
| for (uint256 i = 0; i < vestingIds.length; i++) { |
There was a problem hiding this comment.
It would be better to declare new local variable with vestingIds.length value to reduce gas consumption
| * @notice adds vestings to be processed to the list | ||
| * @dev if account doesn't have another vesting of the same type vesting data will be added to vestingDataList | ||
| */ | ||
| function addVestings( |
There was a problem hiding this comment.
External instead of public. Same for other functions
| * by using this contract instead, that inherits from UpgradableProxy | ||
| * the possibility of being enhanced and re-deployed. | ||
| * */ | ||
| contract VestingRegistryProxy is VestingRegistryStorage, UpgradableProxy { |
There was a problem hiding this comment.
UpgradableProxy contract is similar to EIP-1967 which means that it would be better if the proxy does not know about the logic of VestingRegistryLogic and its storage
There was a problem hiding this comment.
We have used the same approach in Staking and it works well. I am curious to understand your suggestion here.
…e/Sovryn-smart-contracts into vesting-creator-automation
…e/Sovryn-smart-contracts into vesting-creator-automation
…e/Sovryn-smart-contracts into vesting-creator-automation
…e/Sovryn-smart-contracts into vesting-creator-automation
The change is essentially divided into three parts:
a) Vesting Registry - a new upgradable Vesting Registry contract is created that will replace VestingRegistry, VestingRegistry2 and VestingRegistry3 contracts
b) Vesting Migrations - the data from previous registries will be migrated to the new registry
c) Vesting Creator - Used for vesting creation and token staking for team compensation, bug bounty etc.
Technical Spec: https://docs.google.com/document/d/1MI0XzymjsHzI8jIz7F1pw_DSTFrv-f2veX_AM0n1c_w/edit#heading=h.vu27ydtqj65m
Related PR: #195
Note: The deployment script is not complete yet