New staking v3 dev #429
Draft
New staking v3 dev #429
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Tests/staking v3
tests: Added decreaseWalletShares test cases
Test/add claim tests
zaryab2000
reviewed
Oct 29, 2024
There was a problem hiding this comment.
If Staking Moves out of Push Core V3, then a lot of staking-specific state variables in v3 will be UNUSED
The current CoreStorage file doesn't mark it accordingly. should be done for doc purposes. @Zartaj0
zaryab2000
reviewed
Oct 29, 2024
There was a problem hiding this comment.
Suggestion: Arrange state variables based on their category.
For instance
- All token holder specific staking variable should be grouped together
- Wallet Holder Staking variables should be grouped together.
makes it readable.
zaryab2000
reviewed
Oct 30, 2024
| event Staked(address indexed user, uint256 indexed amountStaked); | ||
| event Unstaked(address indexed user, uint256 indexed amountUnstaked); | ||
| event RewardsHarvested(address indexed user, uint256 indexed rewardAmount, uint256 fromEpoch, uint256 tillEpoch); | ||
| event NewSharesIssued(address indexed Wallet, uint256 indexed Shares); |
There was a problem hiding this comment.
remove upper-case in event argument to make it follow the standard coding-style overall. @Zartaj0
| * 1. removes the shares from a wallet completely | ||
| * 2. Can be called by governance. | ||
| * 3. Updates WALLET_TOTAL_SHARES | ||
| * 4. Emits out an event. |
There was a problem hiding this comment.
Natspec Suggestions:
- Remove serial number before every comment unless indicating options/types
- Replace all CamelCase with low camel case - TotalShare becomes totalShare ( following coding style of the project)
- Replace "Emits out an event" with Emits EVENT_NAME.
| * 1. Internal helper function | ||
| * 2. helps in getting the shares to be assigned for a wallet based on params passed in this function | ||
| */ | ||
| function getSharesAmount( |
| emit SharesRemoved(_walletAddress, sharesToBeRemoved); | ||
| } | ||
|
|
||
| function decreaseWalletShare( |
There was a problem hiding this comment.
Important:
- Decrement of Wallet Share should follow the procedure mentioned in this doc - https://www.notion.so/pushprotocol/On-Removal-Decrement-of-Wallet-Shares-12f188aea7f480b0b505f8c6d506b347
- Ideally all relinquished shares should just go to foundation and no change in share percentage should occur in existing wallets. @0xNilesh @Zartaj0
| rewards = rewards + claimableReward; | ||
| } | ||
|
|
||
| usersRewardsClaimed[msg.sender] = usersRewardsClaimed[msg.sender] + rewards; |
There was a problem hiding this comment.
Medium Issue:
- The contract uses the same mapping ( usersRewardsClaimed ) for updating the claimed rewards for both Token Holder and Wallet Holder.
- It is possible that 0xABC is a Wallet Address assigned by the governance to addShares() but then 0xABC also becomes a staker as it holds the token.
- In such a case, the usersRewardsClaimed mapping holds rewards from two different incentive-sources and different reward criteras, which isn't ideal
- Note: The only reason this is medium or low for now is coz the usersRewardsClaimed is not used for any other logic other than just read-only purpose. If in future upgrades, this map plays a role in some logic or validation, it becomes critical.
| * 4. Emits out an event. | ||
| */ | ||
|
|
||
| function removeWalletShare(address _walletAddress) public onlyGovernance { |
There was a problem hiding this comment.
ToDo:
- Add a check that governance cannot remove a wallet from the system before at least 1 epoch ends.
- Although this is rare scenario, an added layer ensures this is always followed. similar to token holder staking.
| / epochToTotalShares[_epochId]; | ||
| } | ||
|
|
||
| function claimShareRewards() external returns (uint256 rewards) { |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.