feat: Staking facet implementation #254
Conversation
👷 Deploy request for compose-diamonds pending review.Visit the deploys page to approve it
|
|
Hi @maxnorm , kindly review this PR and let met know if there's any structure or style that needs to be amend. Thanks! |
Coverage Report
Last updated: Tue, 23 Dec 2025 21:30:40 GMT for commit |
Gas ReportNo gas usage changes detected between All functions maintain the same gas costs. ✅ Last updated: Tue, 23 Dec 2025 00:05:34 GMT for commit |
mudgen
left a comment
mudgen
left a comment
There was a problem hiding this comment.
Hi @0xhaz,
I appreciate your work on this. I looked at some of this implementation and left some comments. This staking functionality will/would require a lot of additional research and work.
I think we should probably put a pause on this functionality because I realize now that we may will not need it because some developers are currently working on an implementation of ERC4626 for Compose that might take care of most staking needs.
| uint256 stakedAt; | ||
| uint256 lastClaimedAt; |
There was a problem hiding this comment.
Use uint64 for timestamps that are stored in contract storage. That will use less gas for writing and reading when they are packed together.
| } | ||
| stakeERC20(_tokenAddress, _amount); | ||
| } else if (s.supportedTokens[_tokenAddress].isERC721) { | ||
| if (IERC721(_tokenAddress).ownerOf(_tokenId) != msg.sender) { |
There was a problem hiding this comment.
I don't see a need here to check the owner of a token here because that will be done in by the transferFrom function.
| if (IERC721(_tokenAddress).ownerOf(_tokenId) != msg.sender) { | ||
| revert StakingNotTokenOwner(msg.sender, _tokenAddress, _tokenId); | ||
| } | ||
| IERC721(_tokenAddress).safeTransferFrom(msg.sender, address(this), _tokenId); |
There was a problem hiding this comment.
Why use safeTransferFrom here instead of transferFrom? transferFrom would be better in this case because it would avoid an unnecessary call that safeTransferFrom makes.
| IERC721(_tokenAddress).safeTransferFrom(msg.sender, address(this), _tokenId); | ||
| stakeERC721(_tokenAddress, _tokenId); | ||
| } else if (s.supportedTokens[_tokenAddress].isERC1155) { | ||
| if (IERC1155(_tokenAddress).balanceOf(msg.sender, _tokenId) < _amount) { |
There was a problem hiding this comment.
I don't see why we need to check the balance here because this would be done by the safeTransferFrom function.
| revert StakingUnsupportedToken(_tokenAddress); | ||
| } | ||
|
|
||
| if (s.minStakeAmount > 0) { |
There was a problem hiding this comment.
I don't see why we are checking s.minStakeAmount here when setStakingParameters reverts if the value is 0.
| } | ||
|
|
||
| if (s.supportedTokens[_tokenAddress].isERC20) { | ||
| bool success = IERC20(_tokenAddress).transferFrom(msg.sender, address(this), _amount); |
There was a problem hiding this comment.
ERC20 transfer edge cases are not mentioned or handled here.
| uint256 maxStakeAmount; | ||
| uint256 minStakeAmount; |
There was a problem hiding this comment.
Why are max and min stake amount requirements needed?
2 participants
Summary
Implement StakingFacet and StakingMod
Checklist
Before submitting this PR, please ensure:
Code follows the Solidity feature ban - No inheritance, constructors, modifiers, public/private variables, external library functions,
using fordirectives, orselfdestructCode follows Design Principles - Readable, uses diamond storage, favors composition over inheritance
Code matches the codebase style - Consistent formatting, documentation, and patterns (e.g. ERC20Facet.sol)
Code is formatted with
forge fmtExisting tests pass - Run tests to be sure existing tests pass.
New tests are optional - If you don't provide tests for new functionality or changes then please create a new issue so this can be assigned to someone.
All tests pass - Run
forge testand ensure everything worksDocumentation updated - If applicable, update relevant documentation
Make sure to follow the contributing guidelines.