Conversation
cygnusv
left a comment
cygnusv
left a comment
There was a problem hiding this comment.
Looks good in general! just some comments
| ApplicationInfo storage applicationStruct = applicationInfo[msg.sender]; | ||
| require( | ||
| applicationStruct.status == ApplicationStatus.APPROVED, | ||
| "Application is not approved" | ||
| ); |
There was a problem hiding this comment.
Can we remove this check? Given that TACoApp is trusted in this context, it seems a bit redundant.
There was a problem hiding this comment.
I was trying to get same logic as in another methods, and that part is already tested
There was a problem hiding this comment.
I see. My concern was cost for a migration of dozens of stakers, but apparently since we do this in a batch TX, only the first access to applicationStruct.status would get the full SLOAD cost, and the rest will only be 100 gas.
| // Unstake | ||
| stakingProviderStruct.tStake -= authorization.deauthorizing; | ||
| decreaseStakeCheckpoint(stakingProvider, authorization.deauthorizing); | ||
| emit Unstaked(stakingProvider, authorization.deauthorizing); | ||
| token.safeTransfer(stakingProviderStruct.owner, authorization.deauthorizing); | ||
|
|
||
| authorization.deauthorizing = 0; |
There was a problem hiding this comment.
What's the goal of these lines?
There was a problem hiding this comment.
unstake the rest, everything over minimum
contracts/staking/TokenStaking.sol
Outdated
|
|
||
| require(authorization.authorized >= amount, "Not enough authorization"); | ||
|
|
||
| decreaseStakeCheckpoint(stakingProvider, stakingProviderStruct.tStake); |
There was a problem hiding this comment.
Why was this line removed? In all cases, tStake tokens are leaving the contract, so this line should stay.
There was a problem hiding this comment.
my bad, missed it through rearranging
contracts/staking/TokenStaking.sol
Outdated
| ); | ||
| uint96 toUnstake = stakingProviderStruct.tStake; | ||
| stakingProviderStruct.tStake = 0; | ||
| decreaseStakeCheckpoint(stakingProvider, 0); |
There was a problem hiding this comment.
| decreaseStakeCheckpoint(stakingProvider, 0); | |
| decreaseStakeCheckpoint(stakingProvider, toUnstake); |
The parameter here is the amount to decrease, so we want here to decrease all.
| ); | ||
| toUnstake -= amount; | ||
| authorization.authorized = 0; | ||
| if (amount > 0) { |
There was a problem hiding this comment.
If we make amount fixed, then this condition will always be true.
| toUnstake -= amount; | ||
| authorization.authorized = 0; | ||
| if (amount > 0) { | ||
| token.safeTransfer(msg.sender, amount); |
There was a problem hiding this comment.
I think that, technically, this amount is also Unstaked, at least from the Threshold Network perspective, so there should be an Unstaked event.
| await expect(tx) | ||
| .to.emit(tokenStaking, "Unstaked") | ||
| .withArgs(stakingProvider.address, amount.sub(amountToTransfer)) | ||
| }) |
There was a problem hiding this comment.
We need to add voting weight checks here as well. It should go to 0.
| } | ||
|
|
||
| /// Migration | ||
| function migrateAndRelease(address stakingProvider, uint96 amount) |
There was a problem hiding this comment.
I'm starting to think we should fix the amount to migrate as constant/immutable, because that reduces trust in the app implementation and makes the review process . Also it's a tiny gas reduction.
Co-authored-by: David Núñez <david@nucypher.com>
2 participants
No description provided.