👷 KSX - Remove escrow on inflationary rewards and remove staking cooldown #252

Flocqst · 2024-07-05T15:00:43Z

Remove escrow on inflationary rewards + Remove staking cooldown

contracts/StakingRewardsV2.sol

JaredBorders · 2024-07-18T03:31:28Z

contracts/StakingRewardsV2.sol

+        }
+    }
+
+    function _getRewardCompounding(address _account)


needs natspec. i am confused what this is supposed to do

This is called when compounding, basically it is doing the same as _getReward but without transfering because the rewards are staked right after this (can be staked immediatly after calling this (see _compound()).

Added natspec to specify this.

Hmm, is there a difference to this vs. the code in _getReward other than transferring? I see duplicate code. I was thinking that this reward calculating logic would be shared between the two and getReward only has the distinction of transferring to the user.

ie.

getReward() { (kwenta, usdc) = _calculateReward() transfer kwenta transfer USDC } compound() { (kwenta, usdc) = _calculateReward() stake kwenta transfer USDC } _calculateReward() { ... }

TLDR; we should be practicing DRY here to minimize room for error.

Refactored with a _processReward so that there is no duplicate logic

Flocqst · 2024-08-27T14:53:08Z

I've merged the 'Staking Cooldown Removal' feature into this branch and also pulled the USDC rewards from main branch. This PR should be the one that goes live with the KSX deployment.

JaredBorders

Everything LGTM! Nice work

JaredBorders · 2024-08-28T15:42:08Z

contracts/RewardEscrowV2.sol

@@ -19,7 +19,7 @@ import {IEscrowMigrator} from "./interfaces/IEscrowMigrator.sol";
 /// @title KWENTA Reward Escrow V2
 /// @author Originally inspired by SYNTHETIX RewardEscrow
 /// @author Kwenta's RewardEscrow V1 by JaredBorders ([email protected]), JChiaramonte7 ([email protected])
-/// @author RewardEscrowV2 by tommyrharper ([email protected])
+/// @author RewardEscrowV2 by tommyrharper ([email protected]), Flocqst ([email protected])


nit: I would break each author up on separate lines

notion-workspace · 2024-09-03T15:41:15Z

Modify stakingV2 to remove escrow for future rewards

notion-workspace · 2024-09-03T15:41:35Z

Modify stakingV2 to remove cooldown

contracts/RewardEscrowV2.sol

contracts/StakingRewardsV2.sol

jcmonte · 2024-09-03T20:03:41Z

contracts/StakingRewardsV2.sol

+        }
+    }
+
+    function _getRewardCompounding(address _account)


Hmm, is there a difference to this vs. the code in _getReward other than transferring? I see duplicate code. I was thinking that this reward calculating logic would be shared between the two and getReward only has the distinction of transferring to the user.

ie.

getReward() { (kwenta, usdc) = _calculateReward() transfer kwenta transfer USDC } compound() { (kwenta, usdc) = _calculateReward() stake kwenta transfer USDC } _calculateReward() { ... }

TLDR; we should be practicing DRY here to minimize room for error.

jcmonte · 2024-09-11T21:29:39Z

contracts/StakingRewardsV2.sol

-        whenNotPaused
-        updateReward(_account)
-        returns (uint256 reward)
+        returns (uint256 kwentaReward)


Doesn't seem you're using kwentaReward outside of _processReward, but for consistency sake you should either return both kwenta and USDC reward as a tuple or return neither.

cmontecoding

initial review of main contracts
looks pretty straight forward so far

cmontecoding · 2024-10-24T14:48:24Z

contracts/StakingRewardsV2.sol

@@ -81,9 +75,6 @@ contract StakingRewardsV2 is
    /// @notice summation of rewardRate divided by total staked tokens
    uint256 public rewardPerTokenStored;

-    /// @inheritdoc IStakingRewardsV2
-    uint256 public cooldownPeriod;


are we redeploying staking v2 or upgrading? because im pretty sure if we upgrade this removal will brick everything

cmontecoding · 2024-10-24T15:01:06Z

contracts/StakingRewardsV2.sol

@@ -81,9 +75,6 @@ contract StakingRewardsV2 is
    /// @notice summation of rewardRate divided by total staked tokens
    uint256 public rewardPerTokenStored;

-    /// @inheritdoc IStakingRewardsV2
-    uint256 public cooldownPeriod;
-
    /// @notice represents the rewardPerToken
    /// value the last time the staker calculated earned() rewards
    mapping(address => uint256) public userRewardPerTokenPaid;


it wont let me comment a couple lines below for some reason but mapping(address => uint256) public userLastStakeTime; should be removed because we remove the use of it everywhere in the code

cmontecoding · 2024-10-24T15:09:03Z

contracts/StakingRewardsV2.sol

            // update state (first)
            rewards[_account] = 0;

            // emit reward claimed event and index account
-            emit RewardPaid(_account, reward);
+            emit RewardPaid(_account, kwentaReward);


the other event is called RewardPaidUSDC

this should be called RewardPaidKwenta to match

cmontecoding · 2024-10-24T15:14:01Z

contracts/StakingRewardsV2.sol

+    {
+        // Process Kwenta reward
+        kwentaReward = rewards[_account];
+        if (kwentaReward > 0) {
            // update state (first)
            rewards[_account] = 0;


if transferKwenta is set to false meaning they want to compound their rewards then doesnt this line ruin that?

their rewards are set to 0 but they dont get transferred their kwenta, so next time this is called again (im assuming after they waited for it to compound in this contract), then their rewards will be 0 or close to 0 and therefore they lost their kwenta entirely?

if im understanding this correctly then this rewards[_account] = 0; line shouldnt apply if transferKwenta == false

oh upon further look, kwentaReward is extrapolating this value? and it hopefully gets relogged when calling _stake in _compound?

cmontecoding · 2024-10-24T15:20:52Z

contracts/StakingRewardsV2.sol

@@ -397,8 +386,8 @@ contract StakingRewardsV2 is
    /// @dev internal helper to compound for a given account
    /// @param _account the account to compound for
    function _compound(address _account) internal {
-        _getReward(_account);
-        _stakeEscrow(_account, unstakedEscrowedBalanceOf(_account));
+        uint256 reward = _processReward(_account, _account, false);


will reward be 0 here?

cmontecoding

just reviewed all the tests. aside: i cant find where we explicitly test these new _processReward changes.

important: we need to test the upgrade from a current fork --> new deployment (much like how cc-vesting does it). this should be done before we upgrade to ensure everything will be good, especially storage slots.
this may be doable in stakingV2.upgrade.t.sol. if not then we might have to add a new file. this may be difficult because i think everthing (testing and upgrading) is baked together and deeply webbed

cmontecoding · 2024-10-28T21:06:41Z

test/foundry/unit/StakingRewardsV2/StakingRewardsV2OnBehalfActionsTests.t.sol

-                        Get Reward On Behalf
-    //////////////////////////////////////////////////////////////*/
-
-    function test_getRewardOnBehalf() public {


why do we remove this test and the accompanying fuzz test?

cmontecoding · 2024-10-28T21:10:31Z

test/foundry/unit/StakingRewardsV2/StakingRewardsV2OnBehalfActionsTests.t.sol

    //////////////////////////////////////////////////////////////*/

-    function test_Get_Reward_And_Stake_On_Behalf() public {
+    function test_Get_Reward_On_Behalf() public {


its good that we are testing getReward() by itself now (im assuming we didnt before) but why are we just replacing the "on behalf" test? we no longer test getting the reward AND staking anymore

we should have both this test & the test_Get_Reward_And_Stake. not either or

this reminds me: where do we test getReward (at least the new changes)? cant find it

cmontecoding · 2024-10-28T21:26:37Z

test/foundry/unit/StakingRewardsV2/StakingV2Checkpointing.t.sol

@@ -354,7 +336,7 @@ contract StakingV2CheckpointingTests is DefaultStakingV2Setup {
            assertEq(value, previousTotal + amountToStake);

            // move beyond cold period
-            vm.warp(block.timestamp + stakingRewardsV2.cooldownPeriod());
+            vm.warp(block.timestamp + 2 weeks);


how come we mostly remove the cooldown period in this test file but then sometimes we add back this warp forward 2 weeks?

cmontecoding · 2024-10-28T21:42:26Z

test/foundry/unit/StakingRewardsV2/StakingV2RewardCalculations.t.sol

            rewardsUsdc = usdc.balanceOf(user1);
-            assertEq(rewards, expectedRewards);
-            assertApproxEqAbs(rewardsUsdc, expectedUsdcRewards, 10);
+            assertApproxEqAbs(rewardsUsdc, expectedUsdcRewards, 50);


why do we change this from 10 to 50? does that make the assert less accurate?

i just tested this locally with 10 and it still passes

cmontecoding · 2024-10-28T21:47:27Z

test/foundry/integration/stakingV2.upgrade.t.sol

        assertEq(2 ether + 1 weeks, stakingRewardsV2.balanceOf(user1));
-        assertEq(1 ether + 1 weeks, stakingRewardsV2.escrowedBalanceOf(user1));
+        assertEq(1 ether, stakingRewardsV2.escrowedBalanceOf(user1));
        assertEq(0, rewardEscrowV2.unstakedEscrowedBalanceOf(user1));


this test suite needs to also test that all the accessible storage slots are not bricked

Flocqst added 10 commits July 2, 2024 16:43

👷 remove staking cooldown

62462f1

👷 update interface

6006911

✅ Adapt test suite to staking cooldown removal

1052a6d

📚 update doc

5142e90

👷 Remove escrow on inflationary rewards

1735867

📚 @Custom:todo adjust tests to non escrowed rewards

43300af

👷 Adjust compound rewards to newly non escrowed rewards

371f847

👷 adjust for compounding on behalf

b4b69a7

✅ Adjust tests to non escrowed rewards

d33ee53

✅ fix fork test

9dee0eb

Flocqst marked this pull request as ready for review July 16, 2024 16:53

Flocqst requested review from JaredBorders and jcmonte July 16, 2024 16:53

Flocqst added 3 commits July 17, 2024 15:44

✅ Add unstakeEscrowAdmin tests

fb03e81

✅ rename function helper

6336698

📚 Add author handle

7ff1a0a

Flocqst mentioned this pull request Jul 17, 2024

Staking cooldown removal #251

Closed

JaredBorders suggested changes Jul 18, 2024

View reviewed changes

Flocqst added 6 commits July 22, 2024 16:13

👷 fix updateReward modifier for compoundOnBehalf

15eea33

🔀 Resolved conflicts after merging main into staking-cooldown

9cb9509

🔀 Resolved conflicts after merging main into remove-inflation-escrow

427134b

✅ Adjust stake Escrow on behalf tests

827e85c

Merge branch 'staking-cooldown' into remove-inflation-escrow

e4d6166

✅ Adjust precision delta usdc reward calculation

48a2b16

JaredBorders approved these changes Aug 28, 2024

View reviewed changes

Flocqst changed the title ~~👷 Remove escrow on inflationary rewards~~ 👷 KSX - Remove escrow on inflationary rewards and remove staking cooldown Sep 3, 2024

jcmonte requested changes Sep 3, 2024

View reviewed changes

Flocqst added 2 commits September 11, 2024 14:55

👷 Remove uses of userLastStakeTime

0d173e7

👷 Refactor processReward

0f1db85

Flocqst requested a review from jcmonte September 11, 2024 13:07

jcmonte reviewed Sep 11, 2024

View reviewed changes

cmontecoding reviewed Oct 24, 2024

View reviewed changes

cmontecoding added 3 commits October 28, 2024 15:41

👷 add updateReward(_account) modifier to _processReward()

c9e48f6

✅ test_compound add assert

bf210e9

✅ add asserts to test_compoundOnBehalf

c6d26b6

cmontecoding requested changes Oct 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

👷 KSX - Remove escrow on inflationary rewards and remove staking cooldown #252

👷 KSX - Remove escrow on inflationary rewards and remove staking cooldown #252

Flocqst commented Jul 5, 2024 •

edited

Loading

JaredBorders Jul 18, 2024

Flocqst Jul 22, 2024

jcmonte Sep 3, 2024

Flocqst Sep 11, 2024

Flocqst commented Aug 27, 2024

JaredBorders left a comment

JaredBorders Aug 28, 2024

notion-workspace bot commented Sep 3, 2024

notion-workspace bot commented Sep 3, 2024

jcmonte Sep 3, 2024

jcmonte Sep 11, 2024

cmontecoding left a comment

cmontecoding Oct 24, 2024

cmontecoding Oct 24, 2024

cmontecoding Oct 24, 2024

cmontecoding Oct 24, 2024

cmontecoding Oct 24, 2024

cmontecoding Oct 24, 2024

cmontecoding left a comment

cmontecoding Oct 28, 2024

cmontecoding Oct 28, 2024

cmontecoding Oct 28, 2024

cmontecoding Oct 28, 2024

cmontecoding Oct 28, 2024

cmontecoding Oct 28, 2024 •

edited

Loading

👷 KSX - Remove escrow on inflationary rewards and remove staking cooldown #252

Are you sure you want to change the base?

👷 KSX - Remove escrow on inflationary rewards and remove staking cooldown #252

Conversation

Flocqst commented Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Flocqst commented Aug 27, 2024

JaredBorders left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notion-workspace bot commented Sep 3, 2024

notion-workspace bot commented Sep 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmontecoding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmontecoding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmontecoding Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Flocqst commented Jul 5, 2024 •

edited

Loading

cmontecoding Oct 28, 2024 •

edited

Loading