feat: create a claimAndUnstake function #67
Conversation
WalkthroughThis update to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Hyperstaker
User->>Hyperstaker: claimAndUnstake(hypercertId)
loop For each round except last
Hyperstaker->>Hyperstaker: calculateReward(hypercertId, roundId)
alt reward > 0 and not claimed
Hyperstaker->>Hyperstaker: _claimReward(hypercertId, roundId, reward)
end
end
Hyperstaker->>Hyperstaker: _unstake(hypercertId)
Hyperstaker-->>User: Hypercert and rewards transferred
Possibly related issues
Possibly related PRs
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/Hyperstaker.sol (1)
174-181: Gas & redundancy concerns inclaimAndUnstakeloop
Using
rounds.length - 1recalculates the bound on every iteration.
Cache it once to save gas.
unstake(_hypercertId)triggers theonlyStakermodifier a second time, even
though the caller was already validated. Calling the internal_unstake
directly avoids an extra SLOAD/gas and keeps the error surface smaller.function claimAndUnstake(uint256 _hypercertId) external whenNotPaused onlyStaker(_hypercertId) { - for (uint256 i = 0; i < rounds.length - 1; i++) { + uint256 lastCompletedRound = rounds.length - 1; + for (uint256 i = 0; i < lastCompletedRound; i++) { uint256 reward = calculateReward(_hypercertId, i); if (reward != 0) { _claimReward(_hypercertId, i, reward); } } - unstake(_hypercertId); + _unstake(_hypercertId); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Hyperstaker.sol(6 hunks)
🔇 Additional comments (1)
src/Hyperstaker.sol (1)
239-242: State-before-interaction looks goodStake data is deleted and the
Unstakedevent emitted before the external
safeTransferFromcall, reducing re-entrancy risk. ✔️
| require(stakes[_hypercertId].stakingStartTime != 0, NotStaked()); | ||
| if (isRoundClaimed(_hypercertId, _roundId)) { |
There was a problem hiding this comment.
Same compilation issue in calculateReward
The new check mirrors the pattern above and needs the same fix.
- require(stakes[_hypercertId].stakingStartTime != 0, NotStaked());
+ if (stakes[_hypercertId].stakingStartTime == 0) revert NotStaked();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| require(stakes[_hypercertId].stakingStartTime != 0, NotStaked()); | |
| if (isRoundClaimed(_hypercertId, _roundId)) { | |
| if (stakes[_hypercertId].stakingStartTime == 0) revert NotStaked(); | |
| if (isRoundClaimed(_hypercertId, _roundId)) { |
| modifier onlyStaker(uint256 _hypercertId) { | ||
| require(stakes[_hypercertId].stakingStartTime != 0, NotStaked()); | ||
| address staker = stakes[_hypercertId].staker; | ||
| require(staker == msg.sender, NotStakerOfHypercert(staker)); | ||
| _; | ||
| } |
There was a problem hiding this comment.
❌ Custom-error syntax breaks compilation
require only accepts a boolean condition and an optional string message.
Passing a custom error identifier as the second argument (require(condition, NotStaked());) is invalid Solidity syntax and will not compile.
modifier onlyStaker(uint256 _hypercertId) {
- require(stakes[_hypercertId].stakingStartTime != 0, NotStaked());
- address staker = stakes[_hypercertId].staker;
- require(staker == msg.sender, NotStakerOfHypercert(staker));
+ if (stakes[_hypercertId].stakingStartTime == 0) revert NotStaked();
+ address staker = stakes[_hypercertId].staker;
+ if (staker != msg.sender) revert NotStakerOfHypercert(staker);
_;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| modifier onlyStaker(uint256 _hypercertId) { | |
| require(stakes[_hypercertId].stakingStartTime != 0, NotStaked()); | |
| address staker = stakes[_hypercertId].staker; | |
| require(staker == msg.sender, NotStakerOfHypercert(staker)); | |
| _; | |
| } | |
| modifier onlyStaker(uint256 _hypercertId) { | |
| if (stakes[_hypercertId].stakingStartTime == 0) revert NotStaked(); | |
| address staker = stakes[_hypercertId].staker; | |
| if (staker != msg.sender) revert NotStakerOfHypercert(staker); | |
| _; | |
| } |
| if (rewardToken != address(0)) { | ||
| require(IERC20(rewardToken).transfer(msg.sender, _reward), RewardTransferFailed()); | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer SafeERC20 for wide-token compatibility
Some ERC-20 tokens (e.g. USDT, certain roll-ups) don’t return a boolean or
otherwise deviate from the ERC-20 spec, causing IERC20.transfer(...) to revert
or return misleading values.
Using OpenZeppelin’s SafeERC20 handles these edge-cases and reduces the chance
of silent transfer failures.
-import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
…
- require(IERC20(rewardToken).transfer(msg.sender, _reward), RewardTransferFailed());
+ SafeERC20.safeTransfer(IERC20(rewardToken), msg.sender, _reward);(Remember to add using SafeERC20 for IERC20;.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (rewardToken != address(0)) { | |
| require(IERC20(rewardToken).transfer(msg.sender, _reward), RewardTransferFailed()); | |
| } else { | |
| // At the top of src/Hyperstaker.sol, update your imports and add the `using` directive: | |
| import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; | |
| import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; | |
| using SafeERC20 for IERC20; | |
| // … later in your function … | |
| if (rewardToken != address(0)) { | |
| // SafeERC20 will handle non-standard ERC-20s and revert on failure | |
| SafeERC20.safeTransfer(IERC20(rewardToken), msg.sender, _reward); | |
| } else { | |
| // … |
Summary by CodeRabbit
New Features
Improvements
Bug Fixes