feat: add StakeVault and implements StakeManager interface into RewardsStreamerMP #39

3esmit · 2024-10-08T18:38:59Z

Description

This commit adds the following changes:

Add the contract StakeVault so funds can be stored safely by the user. Move StakeVault into staking-reward-streamer repository #14
Added the interface ITrustedCodehashAccess and contract TrustedCodehashAccess in the src/access directory, which implements the ITrustedCodehashAccess interface and provides functionality to set or update the trust status for a contract's codehash and implemented it on RewardStreamerMP. Add stake vault whitelist capabilities #15
added the interface IStakeManager and implemented it on RewardStreamerMP Merging existing solutions #13 These changes are necessary to enforce security measures and restrict access based on the codehash of the caller, and allow for better reuse of code between StakeManager and RewardStreamerMP.

Checklist

Ensure you completed all of the steps below before submitting your pull request:

Added natspec comments?
Ran pnpm adorno?
Ran pnpm verify?

…dsStreamerMP This commit adds the following changes: - Add the contract `StakeVault` so funds can be stored safely by the user. #14 - Added the interface `ITrustedCodehashAccess` and contract `TrustedCodehashAccess` in the `src/access` directory, which implements the `ITrustedCodehashAccess` interface and provides functionality to set or update the trust status for a contract's codehash and implemented it on `RewardStreamerMP`. #15 - added the interface `IStakeManager` and implemented it on `RewardStreamerMP` #13 These changes are necessary to enforce security measures and restrict access based on the codehash of the caller, and allow for better reuse of code between StakeManager and RewardStreamerMP.

0x-r4bbit · 2024-10-09T05:59:10Z

src/IStakeManager.sol

+    function unstake(uint256 _amount) external;
+    function lock(uint256 _secondsIncrease) external;
+    function leave() external returns (bool _leaveAccepted);
+    function acceptUpdate() external returns (address _migrated);


Let's leave upgradeability out and tackle it once we have #13 done.
We'll do #20 after that.

I would like to leave it here in PR, we don't have to implement it yet, but just define the interface.

It has a return (address _migrated) which in case of upgradabilty through proxy, it would return the address of itself. In case the upgradabiltiy is to a new address, it would return the new address - so this manage the upgradability for migration style and proxy style.

0x-r4bbit · 2024-10-09T06:06:12Z

src/IStakeManager.sol

+    function totalSupply() external view returns (uint256 _totalSupply);
+    function totalSupplyMinted() external view returns (uint256 _totalSupply);
+    function pendingReward() external view returns (uint256);
+    function getStakedBalance(address _vault) external view returns (uint256 _balance);


I think here we can drop some as well.

pendingReward() is not necessary as the won't be any rewards.
totalSupplyMinted() currently says totalStaked + totalMP + potentialMP. I don't think we want that. potentialMP is the maximum MP that can exist in the system based on the stake amount (it's basicallly maxMP from the other system).

So we only need:

potentialMP (and userPotentialMP I guess)

totalMP (all minted MPs)

totalStaked (all staked balance)

totalSupply (totalStaked + totalMP)

pendingReward() is not necessary as the won't be any rewards.

I would leave it here, because the contract being updated STILL have rewarding. Once the rewarding is fully removed, we can remove it there afterwards.

potentialMP (and userPotentialMP I guess)

This shouldn't be part of the interface because is not common to StakeManager.sol

totalSupply (totalStaked + totalMP)

I'll update the logic to remove potentialMPs from here. And seems like totalSupplyMinted() should be dropped from the interface because its something related just to StakeManager? Or maybe we can change it so:

totalSupply() => totalStaked + totalMP + calcMP(totalStaked, block.timestamp - lastMPUpdatedTime)

At the end it depends on what we want totalSupply() to represent.
Is it the actual supply that is in existence (totalMP + totalStaked) or is it whatever is in existence + whatever there is left to mint as per potentialMP

@gravityblast what are your thougths on this

totalSupply() => totalStaked + totalMP + calcMP(totalStaked, block.timestamp - lastMPUpdatedTime) is actually wrong, as its not considering potentialMP, but it should be something like that.

The scenario is:

For an external system, which wants to read the "current balance" of a user, and compare for how much of "total supply" exists, it would want that both values, for user balance, and for total supply, to represent how much it would have if the contract is updated to last mint.

If we dont have this, than, an external UI would not be able to read the values as if the contact would be updated. It would always have the values from last mint.

At the end it depends on what we want totalSupply() to represent.

@0x-r4bbit given that we are not sure, and it's not a token, I think we don't need a method called totalSupply

Yea very good point. The totalSupply in original StakeManager I think is a relict from the past where we weren't yet sure if the token will be represented by the contract as well.

Now that we're working on xp-token contract, we'll have a dedicated totalSupply there.

0x-r4bbit · 2024-10-09T06:07:04Z

src/IStakeManager.sol

+    function MIN_LOCKUP_PERIOD() external view returns (uint256);
+    function MAX_LOCKUP_PERIOD() external view returns (uint256);
+    function MP_APY() external view returns (uint256);
+    function MAX_BOOST() external view returns (uint256);


Currently MAX_BOOST is known as MAX_MULTIPLIER in this code (which I think we can also keep as it is a better fitting name)

Ok, I'll use it as MAX_MULTIPLIER

0x-r4bbit · 2024-10-09T06:08:10Z

src/RewardsStreamerMP.sol

+    function acceptUpdate() external onlyTrustedCodehash returns (address _migrated) {
+        //TODO: handle update/migration
+        revert("Not implemented");
+    }


As mentioned above, let's tackle that in #20

Ok, we can leave it as Not Implemented here for now and later we implement the logic #20

Sounds good to me, but are we sure we'll end up with an acceptUpdate function?
If yes, then it's fine

If we want to keep compatibility for StakeVault with both systems, yes, but for this contact, acceptUpdate might be just a return address(this), if users don't need to accept updates. But that discussion is far more complex, as a Proxy Update means a breach of contract, and morally it should need to accept the change - however I don't see other developers caring about the morals of contract breaching.

If we want to keep compatibility for StakeVault with both systems, yes,

This is not a priority atm.

gravityblast · 2024-10-09T09:04:11Z

src/RewardsStreamerMP.sol

-            revert StakingManager__AmountCannotBeZero();
+    function stake(uint256 _amount, uint256 _seconds) external onlyTrustedCodehash nonReentrant {
+        if (_amount == 0) {
+            revert StakeManager__StakeIsTooLow();


what do you think about the previous __AmountCannotBeZero? I happy with both in the end, but more too low sounds like there's a minimum while cannot be zero feels more like any positive amount

The reason I am chaning to StakeIsTooLow because it would return the idea that cannot be zero and I want to keep the same error messages as possible from StakeManager.sol so we can reuse tests in both systems.

Actually, we need to verify your minimum is actually 1 token.

1 wei or 1e18?
if it's 1e18, then it makes sense to use too low, but if the minimum is 1 it means it cannot be zero, and I would avoid not rename it just not to rename tests

gravityblast · 2024-10-09T09:10:42Z

src/RewardsStreamerMP.sol

        }
    }

-    function getStakedBalance(address userAddress) external view returns (uint256) {
-        return users[userAddress].stakedBalance;
+    function calculateMP(uint256 _balance, uint256 _deltaTime) public pure returns (uint256) {


given that it's used only in one point, doesn't it makes sense to have it internal or hardcoded like before?
I don't see a reason to have a function like that called externally, unless you call it for a specific user

It's important for UI and tests. we can have an internal that is called by this public function (and make it external) for gas sake.

gravityblast · 2024-10-09T09:16:10Z

src/RewardsStreamerMP.sol

-    function getUserInfo(address userAddress) external view returns (UserInfo memory) {
-        return users[userAddress];
+    function pendingReward() external view returns (uint256 _pendingReward) {
+        return STAKE_TOKEN().balanceOf(address(this)) - accountedRewards;


I think the pending rewards are actually potentialMP. Otherwise if we only use balanceOf we lose the initial MPs

I'll revist that.

0x-r4bbit · 2024-10-11T07:56:07Z

@3esmit how do you feel about breaking this down a bit.
It'd be cool if we had a commit that introduces the StakeVault, at least in so far that it talks to the RewardStreamer.
The StakeVault only needs to provide the functionality listed in #14

Then another PR could introduce the whitelisting (or at least a separate commit.

I feel like the StakeVault could've already been in a mergable state by now.

What do you think?

3esmit · 2024-10-12T22:48:30Z

StakeVault and TrustedCodehashAccess are part of the same functionality. Without the trusted code hash access the stake vault would not work properly.

0x-r4bbit · 2024-10-14T08:14:38Z

StakeVault and TrustedCodehashAccess are part of the same functionality. Without the trusted code hash access the stake vault would not work properly.

As I understand, the TrustedCodehashAccess just enables the StakeManager to control who can call certain methods.

If, as a first step, we're simply adding StakeVault and leave TrustedCodeHashAccess out of the equation, it can just talk to the RewardStreamer contract like any other account.
But okay, I see that this might be unnecessary.

Then at least make it so that this PR:

Adds StakeVault (again only the functionality that overlaps with RewardStreamer)
Adds TrustedCodeHashAccess
Make sure RewardStreamer make use of it
Adjust tests, as now only StakeVault can talk to it

Add sub issues in #13 for things left to port over.
All refactor related things we put in a separate PR.

This introduces a first version of `IStakeManager`, highly inspired by the changes done in #39. However, in this commit, it only adds the methods that are currently supported by both, `StakeManager` and `RewardStreamerMP`. Future commits will add APIs for locking and leaving.

This enables whitelisting in the staking manager contract using the `TrustedCodehashAccess` interface. These changes were largely taken from #39 with few changes: - Due to different OZ versions, `Ownable()` constructor needs to be called with owner Closes #15

This introduces a first version of `IStakeManager`, highly inspired by the changes done in #39. However, in this commit, it only adds the methods that are currently supported by both, `StakeManager` and `RewardStreamerMP`. Future commits will add APIs for locking and leaving.

This enables whitelisting in the staking manager contract using the `TrustedCodehashAccess` interface. These changes were largely taken from #39 with few changes: - Due to different OZ versions, `Ownable()` constructor needs to be called with owner Closes #15

0x-r4bbit · 2024-10-17T19:10:28Z

Hey @3esmit

I since I'd like to get your changes in, I started splitting up the changes into dedicated PRs that address the issues.
You can check them here:

Prior to these two, I've also done #62

All PRs should be passing CI.

I left out the functionality that's not yet working in reward streamer (upgradeability, exiting and locking after staking).
We can address those next week then.

This introduces a first version of `IStakeManager`, highly inspired by the changes done in #39. However, in this commit, it only adds the methods that are currently supported by both, `StakeManager` and `RewardStreamerMP`. Future commits will add APIs for locking and leaving.

This enables whitelisting in the staking manager contract using the `TrustedCodehashAccess` interface. These changes were largely taken from #39 with few changes: - Due to different OZ versions, `Ownable()` constructor needs to be called with owner Closes #15

This introduces a first version of `IStakeManager`, highly inspired by the changes done in #39. However, in this commit, it only adds the methods that are currently supported by both, `StakeManager` and `RewardStreamerMP`. Future commits will add APIs for locking and leaving.

This enables whitelisting in the staking manager contract using the `TrustedCodehashAccess` interface. These changes were largely taken from #39 with few changes: - Due to different OZ versions, `Ownable()` constructor needs to be called with owner Closes #15

0x-r4bbit · 2024-10-22T10:03:32Z

Closing this in favor of #61 and #63

0x-r4bbit reviewed Oct 9, 2024

View reviewed changes

gravityblast reviewed Oct 9, 2024

View reviewed changes

0x-r4bbit mentioned this pull request Oct 17, 2024

feat: introduce StakeVault and IStakeManager #61

Merged

3 tasks

0x-r4bbit mentioned this pull request Oct 17, 2024

feat: add TrustedCodehashAccess contract and interface #63

Merged

3 tasks

0x-r4bbit changed the base branch from main to refactor/user-account October 17, 2024 19:10

0x-r4bbit changed the base branch from refactor/user-account to main October 17, 2024 19:10

0x-r4bbit closed this Oct 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add StakeVault and implements StakeManager interface into RewardsStreamerMP #39

feat: add StakeVault and implements StakeManager interface into RewardsStreamerMP #39

3esmit commented Oct 8, 2024

0x-r4bbit Oct 9, 2024

3esmit Oct 9, 2024

0x-r4bbit Oct 9, 2024

3esmit Oct 9, 2024

0x-r4bbit Oct 9, 2024

3esmit Oct 9, 2024 •

edited

Loading

gravityblast Oct 9, 2024

0x-r4bbit Oct 11, 2024

0x-r4bbit Oct 9, 2024

3esmit Oct 9, 2024

0x-r4bbit Oct 9, 2024

3esmit Oct 9, 2024

0x-r4bbit Oct 9, 2024

3esmit Oct 9, 2024

0x-r4bbit Oct 9, 2024

gravityblast Oct 9, 2024

3esmit Oct 9, 2024

gravityblast Oct 9, 2024

gravityblast Oct 9, 2024

3esmit Oct 9, 2024

gravityblast Oct 9, 2024

3esmit Oct 9, 2024

0x-r4bbit commented Oct 11, 2024

3esmit commented Oct 12, 2024

0x-r4bbit commented Oct 14, 2024

0x-r4bbit commented Oct 17, 2024

0x-r4bbit commented Oct 22, 2024

feat: add StakeVault and implements StakeManager interface into RewardsStreamerMP #39

feat: add StakeVault and implements StakeManager interface into RewardsStreamerMP #39

Conversation

3esmit commented Oct 8, 2024

Description

Checklist

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

3esmit Oct 9, 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

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

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

0x-r4bbit commented Oct 11, 2024

3esmit commented Oct 12, 2024

0x-r4bbit commented Oct 14, 2024

0x-r4bbit commented Oct 17, 2024

0x-r4bbit commented Oct 22, 2024

3esmit Oct 9, 2024 •

edited

Loading