Escrow Contract #200
Escrow Contract #200
Conversation
ghost
added
the
ghost
requested review from
contracts/escrow/escrow.sol
Outdated
| * @dev Can only be called after the token state is changed to Holding. | ||
| */ | ||
| function withdrawTokensByMultisig(address _receiverAddress) public onlyMultisig checkStatus(Status.Holding) { | ||
| address receiverAddress = msg.sender; |
There was a problem hiding this comment.
address receiverAddress = _receiverAddress != address(0) ? _receiverAddress : msg.sender; might be more readable (it's subjective)
contracts/escrow/escrow.sol
Outdated
| * Once the token deposit is higher than the total deposits done, the contract state is changed to Withdraw. | ||
| */ | ||
| function depositTokensByMultisig(uint256 _amount) public onlyMultisig checkStatus(Status.Holding) { | ||
| require(_amount > 0, "Amount needs to be bigger than zero."); |
There was a problem hiding this comment.
this check is probably redundant, as the function is onlyMultisig and there is no harm in depositing 0 tokens
| function withdrawTokensAndReward() public checkRelease checkStatus(Status.Withdraw) { | ||
| // Reward calculation have to be done initially as the User Balance is zeroed out . | ||
| uint256 reward = userBalances[msg.sender].mul(totalRewardDeposit).div(totalDeposit); | ||
| withdrawTokens(); |
There was a problem hiding this comment.
super.withdrawTokens() is slightly better, as it is more explicit
There was a problem hiding this comment.
Initially, didn't add it because EscrowReward doesn't have a withdrawTokens() and thus should not be a doubt on which function call to make.
| @@ -163,14 +191,20 @@ contract Escrow { | |||
| */ | |||
| function depositTokens(uint256 _amount) public checkStatus(Status.Deposit) { | |||
| require(_amount > 0, "Amount needs to be bigger than zero."); | |||
| uint256 amount = _amount; | |||
There was a problem hiding this comment.
either of amount and _amount is enough, you don't need both
There was a problem hiding this comment.
I believe both are required because we need to find the right value.
_amount will always contain the total value to be deposited.
Whereas, amount will contain either the total value user tries to deposit, or the total value the contract allows (depositLimit).
There was a problem hiding this comment.
really? i don't see why both would be needed here
| @@ -94,8 +99,7 @@ contract EscrowReward is Escrow { | |||
| uint256 reward = userBalances[msg.sender].mul(totalRewardDeposit).div(totalDeposit); | |||
| withdrawTokens(); | |||
|
|
|||
| bool txStatus = rewardToken.transfer(msg.sender, reward); | |||
| require(txStatus, "Token transfer was not successful. Check receiver address."); | |||
| lockedSOV.depositSOV(msg.sender, reward); | |||
There was a problem hiding this comment.
function name and natspec don't fit the function's logic
There was a problem hiding this comment.
This is the NatSpec:
/**
* @notice Withdraws token from the contract by User.
* @dev Only works after the contract state is in Withdraw.
*/
Maybe I can try to explain a bit more regarding the LockedSOV part, is that what you meant?
There was a problem hiding this comment.
That's definitely subjective. When I see withdraw I expect that caller receives assets. The code implements a more complex scheme
A contract that would be used as an escrow for SOV deposits for the Liquidity Pools.