feat: Batch NXM withdrawals #1201

roxdanila · 2024-08-01T10:45:50Z

Context

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

Rebased the base branch
Attached corresponding Github issue
Prefixed the name with the type of change (i.e. feat, chore, test)
Performed a self-review of my own code
Followed the style guidelines of this project
Made corresponding changes to the documentation
Didn't generate new warnings
Didn't generate failures on existing tests
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:

🍰 = Nice to have but not essential.
💡 = Suggestion or a comment based on personal opinion
🔨 = I believe this should be changed.
🤔 = I don’t understand something, do you mind giving me more context?
🚀 = Feedback

contracts/modules/assessment/Assessment.sol

contracts/modules/token/TokenController.sol

shark0der · 2024-08-01T12:43:21Z

contracts/modules/token/TokenController.sol

+    if (withdrawOptions.assessmentStake) {
+      IAssessment _assessment = assessment();
+      (uint96 amount, , ) = _assessment.stakeOf(msg.sender);
+      _assessment.unstakeFor(msg.sender, amount, msg.sender);


I'd create one more function that doesn't need the amount as an input and withdraws all to avoid extra calls and extraneous validations.

new assessment.unstakeAllFor function:

drop all checks except for assessment vote locked check

is it worth adding validation that only TokenController can call assessment.unstakeAllFor?

Could it be considered a griefing attack if anyone can unstake the staker? (staker receives the stake so funds are safe)

contracts/modules/token/TokenController.sol

contracts/modules/assessment/Assessment.sol

roxdanila · 2024-08-07T13:40:20Z

contracts/modules/assessment/Assessment.sol

@@ -189,20 +213,14 @@ contract Assessment is IAssessment, MasterAwareV2 {
    address destination,
    uint104 batchSize
  ) internal returns (uint withdrawn, uint withdrawnUntilIndex) {
-    require(


why was this removed?

its a redundant check as it duplicates the check that TokenController.mint does.

relevant discussion: #1201 (comment)

rackstar · 2024-08-09T07:54:17Z

contracts/modules/token/TokenController.sol

+    if (withdrawAssessment.rewards) {
+      // pass in 0 batchSize to withdraw ALL Assessment rewards
+      assessment().withdrawRewards(msg.sender, 0);
+    }
+
+    // governance rewards
+    uint governanceRewards = governance().claimReward(msg.sender, govRewardsBatchSize);


We separated the batchSize that is passed to the assessment and governance rewards to fix the batchSize issue.

Hardcoded 0 batchSize for assessment.withdrawRewards, which effectively withdraws ALL assessment rewards.

And the batchSize from the parameters (govRewardsBatchSize ) will now only be used in governance rewards.

is it possible that all assessment rewards withdrawal won't fit in a block? I guess that's why the batchSize argument was implemented in the first place

There has been only 21 assessment so far so it seemed relatively safe to hardcode 0. but now in hindsight, this is a bad design.

it better to expose the assessment batchSize and let the caller (FE) have the option to withdraw all or in batches.

i will add a new assessmentRewardsBatchSize param. the withdrawNxm will now look like:

function withdrawNXM( WithdrawAssessment calldata withdrawAssessment, StakingPoolDeposit[] calldata stakingPoolDeposits, StakingPoolManagerReward[] calldata stakingPoolManagerRewards, uint assessmentRewardsBatchSize, uint govRewardsBatchSize ) external {

roxdanila requested a review from shark0der August 1, 2024 10:45

roxdanila linked an issue Aug 1, 2024 that may be closed by this pull request

[RELEASE] NXM batch withdrawals #1184

Closed