feat: Batch NXM withdrawals

roxdanila commented 2 months ago

Context

Closes https://github.com/NexusMutual/smart-contracts/issues/1173

Add TokenController.withdrawNXM to withdraw all NXM from the platform. The caller must do the checks if the NXM is withdrawable and modify params accordingly to skip / include specific NXM withdrawals.

Changes proposed in this pull request

Assessment
- add unstakeForAll
- convert requires to custom error
- remove redundant isMember check in withdrawRewardsTo
TokenController
- add withdrawNXM
- add _withdrawClaimAssessmentTokensForUser internal function
- remove withdrawPendingRewards in favour of withdrawNXM

Test plan

integration tests
fork tests

Checklist

[x] Rebased the base branch
[x] Attached corresponding Github issue
[x] Prefixed the name with the type of change (i.e. feat, chore, test)
[x] Performed a self-review of my own code
[x] Followed the style guidelines of this project
[x] Made corresponding changes to the documentation
[x] Didn't generate new warnings
[x] Didn't generate failures on existing tests
[x] Added tests that prove my fix is effective or that my feature works

Review

When reviewing a PR, please indicate intention in comments using the following emojis:

:cake: = Nice to have but not essential.
:bulb: = Suggestion or a comment based on personal opinion
:hammer: = I believe this should be changed.
:thinking: = I don’t understand something, do you mind giving me more context?
:rocket: = Feedback

rackstar commented 1 month ago

There is an edge case where the user might be prevented from withdrawing available rewards. If user has expired stake / claimable rewards for a tranche but has locked NXM for governance.

withdrawNXM is hardcoded to always withdraw both stake and rewards,
withdraw stake will fail since user has locked NXM for governance, this will effectively block the user from withdrawing available rewards as it will revert.
there is no option for them to skip stake and only claim rewards in this scenario
user will have to wait for gov lock to expire before they can withdraw rewards

https://github.com/NexusMutual/smart-contracts/blob/release-candidate/contracts/modules/staking/StakingPool.sol#L554

I guess, waiting for gov lock to expire is not a big deal. but wanted to flag this scenario in any case

@shark0der @roxdanila @MilGard91

rackstar commented 1 month ago

marking as draft so it won't be accidentally merged while on audit

NexusMutual / smart-contracts