feat: redesign and implement the reward vault #102
Conversation
WalkthroughThe pull request introduces modifications primarily focused on correcting typographical errors and enhancing the functionality of smart contracts related to reward management. Key changes include the renaming of the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
… claimPrincipalFromExocore
adu-web3
requested review from
MaxMustermann2,
magj2006,
bwhour,
cloud8little and
mikebraver
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 45
🧹 Outside diff range and nitpick comments (50)
src/storage/RewardVaultStorage.sol (4)
6-16: Consider adding a setter or constructor for thegatewayvariable.The use of a
_gaparray for future storage variables is a good practice for upgradeable contracts. However, thegatewayaddress lacks initialization. Consider adding a setter function or initializing it in the constructor to ensure it's properly set.constructor(address _gateway) { gateway = _gateway; }or
function setGateway(address _gateway) external onlyOwner { gateway = _gateway; }Make sure to implement appropriate access control for the setter function if you choose that option.
9-13: LGTM: Mappings are well-structured. Consider adding NatSpec comments.The mappings
withdrawableBalancesandtotalDepositedRewardsare appropriately designed for tracking balances and rewards. To improve clarity and maintainability, consider adding NatSpec comments to explain their purpose and structure. For example:/// @notice Mapping of token address to staker address to withdrawable balance /// @dev token address => staker address => balance mapping(address => mapping(address => uint256)) public withdrawableBalances; /// @notice Mapping of token address to AVS address to total deposited rewards /// @dev token address => AVS address => total rewards mapping(address => mapping(address => uint256)) public totalDepositedRewards;This documentation will help other developers understand the purpose and structure of these mappings more easily.
18-41: LGTM: Events are well-defined and documented. Minor suggestion for consistency.The events
RewardDeposited,RewardUnlocked, andRewardWithdrawnare appropriately defined with indexed parameters and clear NatSpec comments. They cover the main actions related to rewards, which will be useful for off-chain tracking and analysis.For consistency, consider using the same parameter name for the AVS address in the
RewardDepositedevent as used in thetotalDepositedRewardsmapping. ChangeavstoavsAddressin the event definition:event RewardDeposited(address indexed token, address indexed avsAddress, uint256 amount);This minor change will align the event parameter naming with the mapping, improving overall code consistency.
1-43: Consider clarifying the contract's role and implementing access control.The
RewardVaultStoragecontract appears to be designed as a base contract for inheritance, focusing on storage and events related to reward management. To improve its design and functionality, consider the following suggestions:
If this is intended as a base contract, rename it to
AbstractRewardVaultStorageto follow common naming conventions for abstract contracts.Implement basic access control, such as using OpenZeppelin's
Ownableor a customonlyGatewaymodifier, to protect sensitive operations in derived contracts.Add an
abstractkeyword to the contract declaration to make its purpose clear:abstract contract AbstractRewardVaultStorage is Ownable { // ... existing code ... modifier onlyGateway() { require(msg.sender == gateway, "Caller is not the gateway"); _; } // ... rest of the contract ... }
- Consider adding interface methods (without implementation) for the main operations that will be needed in the derived contracts, such as:
function depositReward(address token, address avsAddress, uint256 amount) external virtual; function unlockReward(address token, address staker, uint256 amount) external virtual; function withdrawReward(address token, address staker, address recipient, uint256 amount) external virtual;These changes will provide a clearer structure for the contract's intended use and improve its integration with the rest of the system.
src/interfaces/IRewardVault.sol (3)
6-10: Consider revising the parameter naming convention.The
initializefunction looks good overall. However, the parametergateway_uses an underscore suffix, which is not a common convention in Solidity. Consider removing the underscore for consistency with typical Solidity naming conventions.Suggested change:
- function initialize(address gateway_) external; + function initialize(address gateway) external;
12-18: Improve NatSpec comment for thedepositfunction.The
depositfunction's NatSpec comment needs some improvements:
- Add a description for the
depositorparameter.- Clarify the description of the
avsparameter, as it's an address type, not an ID.Suggested changes:
/** * @notice Deposits a token into the reward vault. * @param token The address of the token to be deposited. + * @param depositor The address of the account depositing the tokens. - * @param avs The avs ID to which the token is deposited. + * @param avs The address of the AVS to which the token is deposited. * @param amount The amount of the token to be deposited. */
1-53: Overall, theIRewardVaultinterface is well-designed but could benefit from some enhancements.The interface provides a solid foundation for managing a separate reward vault, which aligns well with the PR objectives. It covers essential functionalities such as depositing, withdrawing, unlocking rewards, and querying balances.
Consider the following suggestions for improvement:
- Add events for important state changes (e.g., deposits, withdrawals, reward unlocks).
- Include a function to query the total rewards (both locked and unlocked) for a user.
- Consider adding a function to check if a user has any locked rewards.
These additions would enhance the interface's functionality and improve its integration with other parts of the system.
src/interfaces/IVault.sol (2)
21-24: Approve addition ofunlockPrincipalfunction with minor documentation suggestion.The addition of the
unlockPrincipalfunction aligns well with the PR objective of separating rewards from principal. It provides a clear mechanism for managing principal withdrawals independently.Consider enhancing the function documentation by adding a brief explanation of how this function relates to the overall withdrawal process. For example:
/// @notice Unlock and increase the withdrawable balance of a user for later withdrawal. /// @param staker The address of the staker whose principal balance is being unlocked. /// @param amount The amount of principal to be unlocked. /// @dev This function only unlocks the principal. The actual withdrawal should be performed using the `withdraw` function. function unlockPrincipal(address staker, uint256 amount) external;This additional context will help developers understand the two-step process of unlocking and then withdrawing.
Line range hint
1-46: Overall, the changes to IVault.sol align well with PR objectives.The modifications to this interface, including the updated
depositfunction, removal ofupdateWithdrawableBalance, and addition ofunlockPrincipal, collectively support the creation of a separate reward vault and improve the management of principal and rewards. These changes are consistent with the PR's goal of redesigning and implementing the reward vault.To ensure a smooth integration:
- Verify that all calls to
deposithave been updated to not send Ether.- Confirm that any previous usage of
updateWithdrawableBalancehas been properly refactored.- Update relevant documentation to reflect these architectural changes.
Consider adding a brief comment at the top of the interface explaining the separation of principal and reward management, as this is a significant architectural change that future developers should be aware of.
src/core/LSTRestakingController.sol (2)
Line range hint
1-91: Summary of changes and implicationsThe changes in this file represent a significant shift in how the Exocore protocol handles principal and reward management:
- The renaming of
withdrawPrincipalFromExocoretoclaimPrincipalFromExocoreimproves clarity and aligns with the concept of users claiming their principal.- The removal of
withdrawRewardFromExocoresuggests a major change in reward handling, likely related to the new reward vault mentioned in the PR objectives.These changes align well with the PR's goal of redesigning and implementing a separate reward vault. However, it's crucial to ensure that:
- The new reward withdrawal process is clearly documented and communicated to users.
- All related parts of the system are updated to reflect these changes.
- The integration between this contract and the new reward vault is properly implemented and tested.
Consider adding comments or documentation within the contract to explain the new reward handling process or to direct users/developers to the appropriate method for reward withdrawal. This will help maintain clarity in the contract's functionality and assist in future development and maintenance.
Incomplete Renaming of
REQUEST_WITHDRAW_LSTThe
Action.REQUEST_WITHDRAW_LSTis still referenced in multiple files across the codebase, indicating that the renaming fromREQUEST_WITHDRAW_LSTtoREQUEST_CLAIM_LSTis incomplete.
- src/storage/GatewayStorage.sol
- src/libraries/ActionAttributes.sol
- test/mocks/ExocoreGatewayMock.sol
- src/core/LSTRestakingController.sol
- src/core/ExocoreGateway.sol
- script/TestPrecompileErrorFixed.s.sol
- script/5_Withdraw.s.sol
- test/foundry/DepositWithdrawPrinciple.t.sol
- test/foundry/unit/ExocoreGateway.t.sol
Please update all occurrences to maintain consistency and prevent potential bugs.
🔗 Analysis chain
Line range hint
45-62: Approve renaming with a minor suggestionThe renaming of
withdrawPrincipalFromExocoretoclaimPrincipalFromExocoreis a good change that better reflects the nature of the operation. It aligns well with the PR objectives of redesigning the reward vault and clarifies the distinction between principal and rewards.Consider updating the
Action.REQUEST_WITHDRAW_LSTenum toAction.REQUEST_CLAIM_LSTfor consistency with the new naming convention. This would require changes in other parts of the codebase, so please verify the impact before implementing.To ensure consistency across the codebase, let's check for other occurrences of
WITHDRAW_LST:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for WITHDRAW_LST in the codebase rg --type solidity "WITHDRAW_LST"Length of output: 79
Script:
#!/bin/bash # Search for WITHDRAW_LST in Solidity (.sol) files rg "WITHDRAW_LST" --glob "*.sol"Length of output: 2110
script/1_Prerequisities.s.sol (1)
10-10: LGTM! Consider removing the blank line.The change from
ClaimRewardMocktoRewardMockaligns with the PR objectives of redesigning the reward system. This modification reflects the shift towards a more structured approach to managing rewards.Consider removing the blank line at line 10 to maintain consistent spacing between import statements.
Also applies to: 13-13
src/interfaces/IExoCapsule.sol (1)
59-61: LGTM! Consider enhancing the documentation.The new
unlockETHPrincipalfunction aligns well with the PR objectives of creating a separate mechanism for handling rewards and principal. It effectively replaces the removedupdateWithdrawableBalancefunction with a more specific purpose.Consider adding more context to the function documentation:
/// @notice Unlock and increase the withdrawable balance of the ExoCapsule for later withdrawal. /// @param amount The amount of the ETH principal to be unlocked. /// @dev This function is used to manage the principal amount separately from rewards, /// allowing for more granular control over withdrawals. function unlockETHPrincipal(uint256 amount) external;script/TestPrecompileErrorFixed.s.sol (1)
71-72: LGTM: Setup changes align with reward system redesign.The changes to use
RewardMockinstead ofClaimRewardMockand the updated precompile address are consistent with the PR objectives. The use ofgetDeployedCodeis also an improvement.Consider adding a comment explaining the purpose of the
REWARD_PRECOMPILE_ADDRESSfor better code documentation:+ // Bind the RewardMock contract to the reward precompile address for local testing bytes memory WithdrawRewardMockCode = vm.getDeployedCode("RewardMock.sol"); vm.etch(REWARD_PRECOMPILE_ADDRESS, WithdrawRewardMockCode);src/core/ClientGatewayLzReceiver.sol (1)
147-158: Approved: Function redesign aligns with PR objectivesThe renaming of
_updatePrincipalWithdrawableBalanceto_unlockPrincipaland the corresponding changes in the internal function calls (unlockETHPrincipalandunlockPrincipal) are consistent with the PR objectives. This redesign clearly separates the handling of principal amounts from rewards.However, to further improve clarity:
Consider updating the function documentation to explicitly mention that this function interacts with either the
IExoCapsuleorIVaultinterface, depending on the token type. This would provide more context about the function's behavior.docs/reward-vault.md (7)
9-23: Design principles are well-defined and comprehensive.The design principles effectively capture the key aspects of the Reward Vault system, aligning well with the PR objectives and the linked issue #78. The emphasis on permissionless rewards and separation of concerns directly addresses the need for a distinct reward management system.
Consider adding a brief explanation of how these principles contribute to solving the problem outlined in issue #78, specifically regarding the limitation of the current deposit function.
25-113: Architecture section is detailed and well-structured.The architecture section provides a comprehensive overview of the smart contracts, key functions, data structures, and the beacon proxy pattern implementation. The new functions added to the ClientChainGateway contract directly address the needs outlined in issue #78, particularly the ability to deposit rewards.
Consider adding a brief explanation or diagram showing the interaction flow between the RewardVault, ClientChainGateway, and the Exocore chain. This visual representation could enhance understanding of the overall system architecture.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~31-~31: Loose punctuation mark.
Context: ... token, address staker, uint256 amount)`: Allows the Gateway to unlock rewards fo...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~32-~32: Loose punctuation mark.
Context: ...wer, address recipient, uint256 amount)`: Allows the Gateway to withdraw claimed ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~33-~33: Loose punctuation mark.
Context: ...eBalance(address token, address staker)`: Returns the withdrawable balance of a s...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~34-~34: Loose punctuation mark.
Context: ...itedRewards(address token, address avs)`: Returns the total deposited rewards of ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~44-~44: Loose punctuation mark.
Context: ...mExocore(address token, uint256 amount)`: Initiates a claim request to the Exocor...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~45-~45: Loose punctuation mark.
Context: ...ken, address recipient, uint256 amount): Calls RewardVault'swithdraw` to trans...(UNLIKELY_OPENING_PUNCTUATION)
115-144: Key processes are well-defined and comprehensive.The section effectively outlines the three main processes: Reward Submission, Reward Distribution and Accounting, and Reward Claiming and Withdrawal. The step-by-step descriptions provide a clear understanding of how rewards are handled throughout their lifecycle.
Consider adding a brief note on error handling or potential edge cases for each process. For example, what happens if a reward submission fails, or if there's a discrepancy between the Exocore chain's records and the RewardVault's records?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~119-~119: A determiner appears to be missing. Consider inserting it.
Context: ...ocesses ### 4.1. Reward Submission 1. Depositor callssubmitRewardon the Gateway, sp...(AI_EN_LECTOR_MISSING_DETERMINER)
[uncategorized] ~121-~121: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...`, which: a. Transfers the specified amount of tokens from the depositor to itself....(AMOUNTOF_TO_NUMBEROF)
[uncategorized] ~125-~125: You might be missing the article “the” here.
Context: ...o account for the deposited rewards. 4. Exocore chain processes the request and emits a...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~129-~129: You might be missing the article “the” here.
Context: ... Reward Distribution and Accounting 1. Exocore chain handles the distribution and acco...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~130-~130: You might be missing the article “the” here.
Context: ...ctivities and the rewards submitted. 2. Exocore chain maintains the record of each stak...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~136-~136: You might be missing the article “the” here.
Context: ... claim request to the Exocore chain. 3. Exocore chain verifies the request and sends a ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
146-157: Security considerations are relevant and well-considered.The section effectively addresses key security aspects including access control, token compatibility, and upgradeability. The emphasis on careful management of upgrades is crucial for maintaining system integrity.
Consider adding a subsection on potential attack vectors and mitigation strategies. For example, discuss how the system prevents front-running attacks during reward claims or how it handles potential issues with malicious ERC20 tokens.
159-165: Gas optimization and upgradability sections provide valuable considerations.The suggestion for batch operations in the Gas Optimization section is a good consideration for improving efficiency. The Upgradability section outlines a solid approach using the OpenZeppelin Upgrades plugin and a multisig wallet for governance, which enhances security and flexibility.
For the Gas Optimization section, consider adding more specific suggestions or examples of how batch operations could be implemented. For the Upgradability section, it might be helpful to briefly explain the process for proposing and approving upgrades through the multisig wallet.
167-183: Events and Future Considerations sections demonstrate thoroughness and forward-thinking.The Events section comprehensively covers all significant actions in the system, which is excellent for transparency and off-chain tracking. The Future Considerations section shows forethought in addressing potential future needs such as emergency withdrawals and scalability.
For the Events section, consider adding a brief explanation of how these events can be used by external systems or for analytics purposes. In the Future Considerations section, it might be beneficial to prioritize these considerations or provide a rough timeline for when they might be addressed.
1-183: Excellent and comprehensive design document for the Reward Vault system.This design document provides a thorough and well-structured blueprint for the Reward Vault system. It effectively addresses the objectives outlined in the PR and linked issue #78, particularly the need for a separate and dedicated reward vault. The document covers all major aspects of the system, including architecture, key processes, security considerations, and future scalability.
Key strengths:
- Clear alignment with PR objectives and issue feat: miss function for AVS to deposit reward into vault #78 requirements.
- Well-defined architecture and data structures.
- Comprehensive coverage of key processes in the reward lifecycle.
- Strong focus on security and upgradeability.
- Consideration of gas optimization and future scalability.
To further enhance this excellent document, consider the following suggestions:
- Add a high-level system diagram showing the interaction between RewardVault, ClientChainGateway, and the Exocore chain.
- Include a section on testing strategy, outlining key test scenarios and how they relate to the design principles and security considerations.
- Provide more details on the governance process for upgrades, including the decision-making process and potential timeframes.
- Add a glossary of terms to ensure clear understanding of domain-specific terminology used throughout the document.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~31-~31: Loose punctuation mark.
Context: ... token, address staker, uint256 amount)`: Allows the Gateway to unlock rewards fo...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~32-~32: Loose punctuation mark.
Context: ...wer, address recipient, uint256 amount)`: Allows the Gateway to withdraw claimed ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~33-~33: Loose punctuation mark.
Context: ...eBalance(address token, address staker)`: Returns the withdrawable balance of a s...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~34-~34: Loose punctuation mark.
Context: ...itedRewards(address token, address avs)`: Returns the total deposited rewards of ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~44-~44: Loose punctuation mark.
Context: ...mExocore(address token, uint256 amount)`: Initiates a claim request to the Exocor...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~45-~45: Loose punctuation mark.
Context: ...ken, address recipient, uint256 amount): Calls RewardVault'swithdraw` to trans...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~119-~119: A determiner appears to be missing. Consider inserting it.
Context: ...ocesses ### 4.1. Reward Submission 1. Depositor callssubmitRewardon the Gateway, sp...(AI_EN_LECTOR_MISSING_DETERMINER)
[uncategorized] ~121-~121: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...`, which: a. Transfers the specified amount of tokens from the depositor to itself....(AMOUNTOF_TO_NUMBEROF)
[uncategorized] ~125-~125: You might be missing the article “the” here.
Context: ...o account for the deposited rewards. 4. Exocore chain processes the request and emits a...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~129-~129: You might be missing the article “the” here.
Context: ... Reward Distribution and Accounting 1. Exocore chain handles the distribution and acco...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~130-~130: You might be missing the article “the” here.
Context: ...ctivities and the rewards submitted. 2. Exocore chain maintains the record of each stak...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~136-~136: You might be missing the article “the” here.
Context: ... claim request to the Exocore chain. 3. Exocore chain verifies the request and sends a ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
src/libraries/Errors.sol (1)
307-313: LGTM! Consider adding context to the InsufficientBalance error.The addition of the RewardVault error section and the
InsufficientBalance()error aligns well with the PR objectives of implementing a separate reward vault. The error is clear and follows the naming convention used throughout the file.To provide more context and aid in debugging, consider expanding the error to include relevant information:
/// @dev RewardVault: insufficient balance for the operation /// @param available The available balance /// @param required The required balance for the operation error InsufficientBalance(uint256 available, uint256 required);This would allow developers to quickly identify the exact shortfall when this error occurs.
src/core/ExoCapsule.sol (2)
286-289: Approve function renaming and suggest documentation updateThe renaming of the function from
updateWithdrawableBalancetounlockETHPrincipalis a positive change. It accurately reflects the function's purpose and maintains consistency with the event naming.Consider updating the function's documentation to reflect its new name and purpose. For example:
/// @notice Unlocks a specified amount of ETH principal, making it available for withdrawal. /// @param unlockPrincipalAmount The amount of ETH principal to unlock. function unlockETHPrincipal(uint256 unlockPrincipalAmount) external onlyGateway { // ... existing implementation ... }
Line range hint
26-289: Summary of changes in ExoCapsule contractThe modifications in this contract effectively support the PR objective of redesigning the reward vault:
- The event renaming from
WithdrawableBalanceUpdatedtoETHPrincipalUnlockedimproves clarity.- The event parameter update from
additionalAmounttounlockedAmountmaintains consistency.- The function renaming from
updateWithdrawableBalancetounlockETHPrincipalaccurately reflects its purpose.These changes collectively enhance the contract's readability and align with the goal of separating principal and rewards in the Exocore protocol.
As you continue to implement the reward vault redesign, ensure that all related contracts and interfaces are updated consistently. This may include updating any external contracts that interact with
ExoCapsule, as well as adjusting relevant tests to reflect these naming changes.docs/client-chain-contracts-design.md (6)
151-177: Significant changes to theIVaultinterface require careful consideration.The
IVaultinterface has undergone substantial modifications:
- The
depositfunction is no longerpayable, which may impact how deposits are processed.- Functions for updating user balances (
updatePrincipalBalance,updateRewardBalance,updateWithdrawableBalance) have been removed, suggesting a shift in balance management strategy.- A new
unlockPrincipalfunction has been added, which seems to replace the previous balance update functions.- New TVL management functions (
setTvlLimit,getTvlLimit,getConsumedTvl) have been introduced, indicating improved control over the vault's capacity.These changes appear to streamline the vault's functionality and improve its management capabilities. However, ensure that all dependent systems and documentation are updated to reflect these interface changes.
Consider documenting the rationale behind these changes, especially the shift in balance management strategy, to provide context for future developers.
217-239: Enhanced reward management inIBaseRestakingControllerThe
IBaseRestakingControllerinterface has been updated with significant changes:
- The
claimfunction has been renamed towithdrawPrincipal, which more accurately describes its purpose.- New reward management functions have been added:
submitReward: Allows AVS to submit rewards.claimRewardFromExocore: Enables claiming rewards from Exocore.withdrawReward: Facilitates withdrawing rewards from the vault.These changes indicate a more comprehensive approach to reward management within the system. The separation of principal and reward operations provides better clarity and control over different types of assets.
Ensure that the documentation for these new functions is comprehensive, explaining the flow of rewards through the system and any potential interactions between principal and reward operations.
262-271: Improved clarity and security in principal withdrawal processThe
claimPrincipalFromExocorefunction (previouslywithdrawPrincipalFromExocore) now describes a two-transaction process for principal withdrawal:
- User initiates the withdrawal request.
- Exocore responds, updating the user's
principalBalanceand claimable balance.This change improves the system's security and consistency by ensuring that the Exocore chain validates the withdrawal before updating balances on the client chain.
Consider adding a flowchart or sequence diagram to the documentation to visually represent this two-step process, making it easier for developers to understand the flow of operations.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~264-~264: You might be missing the article “the” here.
Context: ...This involves the correct accounting on Exocore chain as well as the correct update of ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~264-~264: You might be missing the article “the” here.
Context: ...balance. If this process is successful, user should be able to withdraw the correspo...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~264-~264: You might be missing the article “the” here.
Context: ...to withdraw the corresponding assets on client chain to destination address. The prin...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~264-~264: You might be missing the article “the” here.
Context: ...corresponding assets on client chain to destination address. The principal withdrawal work...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~268-~268: You might be missing the article “the” here.
Context: ...to send principal withdrawal request to Exocore chain. 2. Response from Exocore: call `...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~269-~269: You might be missing the article “the” here.
Context: ...rchainMsgto receive the response from Exocore chain, and callunlockPrincipal` to up...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~269-~269: Possible missing article found.
Context: ...cipalBalance` and claimable balance. If response indicates failure, no user balance shou...(AI_HYDRA_LEO_MISSING_THE)
277-280: Clarified preconditions forwithdrawPrincipalThe description of the
withdrawPrincipalfunction has been updated to emphasize important preconditions:
- Users must ensure sufficient principal is unlocked by calling
claimPrincipalFromExocorebefore withdrawal.- The implementation should check against the user's claimable balance before transferring tokens.
These clarifications align well with the new two-step withdrawal process and help prevent potential issues with premature withdrawals.
Consider adding error handling guidelines to the documentation, specifying how the function should behave if these preconditions are not met (e.g., revert with specific error messages).
Line range hint
283-284: New convenience functiondepositThenDelegateToThe addition of the
depositThenDelegateTofunction provides a user-friendly way to deposit and delegate in a single transaction. This ease-of-use feature can improve the user experience by reducing the number of interactions required.While this function offers convenience, ensure that it maintains the same level of security and consistency as performing these operations separately. Consider documenting any potential edge cases or limitations of using this combined function compared to separate deposit and delegation operations.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~264-~264: You might be missing the article “the” here.
Context: ...This involves the correct accounting on Exocore chain as well as the correct update of ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~264-~264: You might be missing the article “the” here.
Context: ...balance. If this process is successful, user should be able to withdraw the correspo...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~264-~264: You might be missing the article “the” here.
Context: ...to withdraw the corresponding assets on client chain to destination address. The prin...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~264-~264: You might be missing the article “the” here.
Context: ...corresponding assets on client chain to destination address. The principal withdrawal work...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~268-~268: You might be missing the article “the” here.
Context: ...to send principal withdrawal request to Exocore chain. 2. Response from Exocore: call `...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~269-~269: You might be missing the article “the” here.
Context: ...rchainMsgto receive the response from Exocore chain, and callunlockPrincipal` to up...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~269-~269: Possible missing article found.
Context: ...cipalBalance` and claimable balance. If response indicates failure, no user balance shou...(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~275-~275: You might be missing the article “an” here.
Context: ... frozen/slashed. 3. The asset is not in unbonding state. ###withdrawPrincipalThis f...(AI_EN_LECTOR_MISSING_DETERMINER_AN)
Line range hint
264-284: Minor grammatical improvementsThere are several opportunities to improve the grammar in this section of the document by adding articles ("the", "an") in appropriate places. While these are minor changes, they would enhance the overall readability of the documentation.
Consider reviewing the following sentences for potential article additions:
- "This involves the correct accounting on the Exocore chain..."
- "If this process is successful, the user should be able to withdraw..."
- "...to withdraw the corresponding assets on the client chain to the destination address."
- "...to send principal withdrawal request to the Exocore chain."
- "If the response indicates failure, no user balance should be modified."
- "The asset is not in an unbonding state."
🧰 Tools
🪛 LanguageTool
[uncategorized] ~264-~264: You might be missing the article “the” here.
Context: ...This involves the correct accounting on Exocore chain as well as the correct update of ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~264-~264: You might be missing the article “the” here.
Context: ...balance. If this process is successful, user should be able to withdraw the correspo...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~264-~264: You might be missing the article “the” here.
Context: ...to withdraw the corresponding assets on client chain to destination address. The prin...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~264-~264: You might be missing the article “the” here.
Context: ...corresponding assets on client chain to destination address. The principal withdrawal work...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~268-~268: You might be missing the article “the” here.
Context: ...to send principal withdrawal request to Exocore chain. 2. Response from Exocore: call `...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~269-~269: You might be missing the article “the” here.
Context: ...rchainMsgto receive the response from Exocore chain, and callunlockPrincipal` to up...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~269-~269: Possible missing article found.
Context: ...cipalBalance` and claimable balance. If response indicates failure, no user balance shou...(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~275-~275: You might be missing the article “an” here.
Context: ... frozen/slashed. 3. The asset is not in unbonding state. ###withdrawPrincipalThis f...(AI_EN_LECTOR_MISSING_DETERMINER_AN)
test/foundry/unit/ExocoreGateway.t.sol (1)
75-76: LGTM: SetUp contract updated to use RewardMock.The change from
ClaimRewardMocktoRewardMockis consistent with the import modifications and reflects the updated reward system implementation.Consider renaming the variable
WithdrawRewardMockCodetoRewardMockCodefor better consistency with the new mock contract name.src/interfaces/precompiles/IReward.sol (2)
7-7: Capitalize 'Reward' in documentation for consistencyFor consistency with naming conventions, consider capitalizing 'Reward' in the documentation comment.
Apply this diff to correct the capitalization:
-/// @dev The reward contract's address. +/// @dev The Reward contract's address.
14-14: Capitalize 'Reward' in the title for consistencyTo maintain consistency, the title should have 'Reward' capitalized.
Apply this diff to update the title:
-/// @title reward Precompile Contract +/// @title Reward Precompile Contractsrc/interfaces/ILSTRestakingController.sol (2)
21-23: Ensure consistency in terminology for asset retrievalThe documentation for
claimPrincipalFromExocorementions that assets can be "withdrawn" by the user upon approval. Since the method is namedclaimPrincipalFromExocore, consider using consistent terminology like "claim" instead of "withdraw" to avoid confusion.
21-27: Align parameter documentation formattingThe
@paramdescriptions forprincipalAmountspan multiple lines and are misaligned. Consolidate the description into a single line or adjust the indentation for better readability.Apply this diff to fix the formatting:
/// @param token The address of the specific token that the user wants to claim from Exocore. -/// @param principalAmount The principal amount of assets the user deposited into Exocore for delegation and -/// staking. +/// @param principalAmount The principal amount of assets the user deposited into Exocore for delegation and staking.src/interfaces/IBaseRestakingController.sol (2)
21-27: Ensure consistency in parameter descriptions.The
@paramdescriptions fortoken,amount, andrecipientshould be consistent in style and wording for clarity and professionalism.Consider revising the parameter descriptions:
-/// @param token The address of specific token that the user wants to claim from the vault. +/// @param token The address of the specific token that the user wants to withdraw from the vault. -/// @param amount The amount of the token that the user wants to claim from the vault. +/// @param amount The amount of the token that the user wants to withdraw from the vault. -/// @param recipient The destination address that the assets would be transferred to. +/// @param recipient The address where the assets will be transferred.
39-43: Ensure consistency in parameter ordering.In the
withdrawRewardfunction, the parameters are ordered astoken,recipient,rewardAmount. In other functions likewithdrawPrincipal, the amount comes before the recipient. For consistency, consider maintaining the same parameter order across similar functions.Consider rearranging the parameters:
-function withdrawReward(address token, address recipient, uint256 rewardAmount) external; +function withdrawReward(address token, uint256 rewardAmount, address recipient) external;And update the documentation accordingly:
/// @param token The address of the specific token that the user wants to withdraw as a reward. -/// @param recipient The address of the recipient of the reward tokens. /// @param rewardAmount The amount of reward tokens that the user wants to withdraw. +/// @param recipient The address where the reward tokens will be transferred.script/12_RedeployClientChainGateway.s.sol (1)
15-15: Ensure the Necessity ofProxyAdminImportYou have imported
ProxyAdminfrom OpenZeppelin. Please verify whether this import is required. IfProxyAdminis not used in the script, consider removing it to keep the code clean.test/foundry/unit/RewardVault.t.sol (1)
33-54: Simplify thesetUpFunction by Using Default AddressesIn the
setUpfunction, hardcoded addresses likeaddress(0x1),address(0x2), andaddress(0x3)are used fordepositor,withdrawer, andavs. To improve readability, consider assigning these addresses to labeled variables or usingaddress(this)where appropriate.src/storage/ExocoreGatewayStorage.sol (1)
51-52: Clarify parameter descriptions in documentation commentsThe parameter descriptions for
avsOrWithdrawercould be clearer. Instead of combining two roles into one parameter, consider separating them or providing more detailed explanations to avoid confusion.Update the documentation to improve clarity:
- /// @param avsOrWithdrawer The address of the avs or withdrawer, avs for submit reward, withdrawer for claim reward. + /// @param avsOrWithdrawer For submit reward operations, the address of the AVS; for claim reward operations, the address of the withdrawer.src/core/BaseRestakingController.sol (1)
Line range hint
36-50: Consider emitting an event after successful principal withdrawalThe
withdrawPrincipalfunction does not emit any event upon a successful withdrawal. Emitting events for significant actions like fund withdrawals enhances transparency and facilitates off-chain tracking and auditing.Consider adding an event emission, for example:
event PrincipalWithdrawn(address indexed token, address indexed sender, address indexed recipient, uint256 amount); function withdrawPrincipal(address token, uint256 amount, address recipient) external // existing modifiers { // existing logic emit PrincipalWithdrawn(token, msg.sender, recipient, amount); }test/foundry/unit/Vault.t.sol (1)
91-91: Correct the misleading comment about exceeding TVLOn line 91, the comment states "Give enough tokens to exceed TVL," but the amount transferred (
100 * 10 ** 18) is less than theTVL_LIMITof1_000_000 * 10 ** 18. This might cause confusion during test interpretation.Consider updating the comment to accurately reflect the action:
- token.transfer(withdrawer, amount); // Give enough tokens to exceed TVL + token.transfer(withdrawer, amount); // Transfer tokens to withdrawer for withdrawal testingtest/foundry/WithdrawReward.t.sol (1)
41-41: Verify Token Transfer Success in Test SetupAt line 41, you transfer tokens without checking the success of the operation:
restakeToken.transfer(avsDepositor.addr, 1_000_000);While in tests, failures are usually caught, it's good practice to verify that the token transfer was successful, especially if the
transferfunction can fail or returnfalse. This makes your test more robust and can help identify issues with token contracts that do not revert on failure.Consider updating the code to check the transfer success:
restakeToken.transfer(avsDepositor.addr, 1_000_000); +assertEq(restakeToken.balanceOf(avsDepositor.addr), 1_000_000, "Token transfer failed");test/foundry/Governance.t.sol (1)
139-143: Initialize RewardVault and its beaconThe initialization of
rewardVaultImplementationandrewardVaultBeaconis correctly implemented. To ensure the RewardVault deployment and behavior are as expected, consider adding unit tests covering its functionality.Would you like assistance in creating unit tests for the RewardVault?
test/mocks/ExocoreGatewayMock.sol (1)
501-504: Name the 'lzNonce' parameter for consistencyIn the function
handleOperatorAssociation, theuint64parameter is currently unnamed. For consistency and better readability, consider naming this parameterlzNonce, even if it is not used within the function.Update the function signature as follows:
- function handleOperatorAssociation(uint32 srcChainId, uint64, Action act, bytes calldata payload) + function handleOperatorAssociation(uint32 srcChainId, uint64 lzNonce, Action act, bytes calldata payload)src/core/Bootstrap.sol (3)
Line range hint
393-406: Remove unnecessarypayablemodifier fromclaimPrincipalFromExocoreThe function
claimPrincipalFromExocoreis marked aspayable, but it reverts ifmsg.value > 0. Since it does not accept Ether, thepayablemodifier is unnecessary and can be removed to prevent accidental Ether transfers.Apply this diff to remove the
payablemodifier:-function claimPrincipalFromExocore(address token, uint256 amount) - external - payable - override +function claimPrincipalFromExocore(address token, uint256 amount) + external + override beforeLocked whenNotPaused isTokenWhitelisted(token) isValidAmount(amount) nonReentrant // interacts with Vault { if (msg.value > 0) { revert Errors.NonZeroValue(); } _claim(msg.sender, token, amount); }
Line range hint
393-406: Ensure consistent naming betweenclaimPrincipalFromExocoreandwithdrawPrincipalThe functions
claimPrincipalFromExocore(lines 393-406) andwithdrawPrincipal(lines 455-470) handle principal retrieval but use different terminology (claimvs.withdraw). For clarity and consistency, consider standardizing the function names to use eitherclaimorwithdraw.Also applies to: 455-470
Line range hint
393-406: Add missing NatSpec documentation forclaimPrincipalFromExocoreThe function
claimPrincipalFromExocorelacks NatSpec comments. Adding proper documentation will enhance code readability and maintainability.Here's an example of how you might document the function:
/// @notice Allows a user to claim their principal amount from Exocore. /// @param token The address of the token being claimed. /// @param amount The amount of the token to claim. function claimPrincipalFromExocore(address token, uint256 amount) external override beforeLocked whenNotPaused { ... }src/utils/CustomProxyAdmin.sol (1)
32-32: Clarified parameter description forproxyThe updated parameter description for
proxyimproves documentation clarity by specifying "The proxy contract," aiding in code understanding.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (47)
- docs/client-chain-contracts-design.md (3 hunks)
- docs/reward-vault.md (1 hunks)
- script/12_RedeployClientChainGateway.s.sol (2 hunks)
- script/1_Prerequisities.s.sol (3 hunks)
- script/2_DeployBoth.s.sol (6 hunks)
- script/5_Withdraw.s.sol (1 hunks)
- script/7_DeployBootstrap.s.sol (6 hunks)
- script/BaseScript.sol (4 hunks)
- script/TestPrecompileErrorFixed.s.sol (2 hunks)
- src/core/BaseRestakingController.sol (2 hunks)
- src/core/Bootstrap.sol (3 hunks)
- src/core/ClientChainGateway.sol (6 hunks)
- src/core/ClientGatewayLzReceiver.sol (2 hunks)
- src/core/ExoCapsule.sol (2 hunks)
- src/core/ExocoreGateway.sol (3 hunks)
- src/core/LSTRestakingController.sol (1 hunks)
- src/core/RewardVault.sol (1 hunks)
- src/core/Vault.sol (2 hunks)
- src/interfaces/IBaseRestakingController.sol (1 hunks)
- src/interfaces/ICustomProxyAdmin.sol (1 hunks)
- src/interfaces/IExoCapsule.sol (1 hunks)
- src/interfaces/ILSTRestakingController.sol (1 hunks)
- src/interfaces/IRewardVault.sol (1 hunks)
- src/interfaces/IVault.sol (1 hunks)
- src/interfaces/precompiles/IReward.sol (1 hunks)
- src/libraries/ActionAttributes.sol (1 hunks)
- src/libraries/Errors.sol (1 hunks)
- src/storage/BootstrapStorage.sol (1 hunks)
- src/storage/ClientChainGatewayStorage.sol (5 hunks)
- src/storage/ExocoreGatewayStorage.sol (1 hunks)
- src/storage/GatewayStorage.sol (1 hunks)
- src/storage/RewardVaultStorage.sol (1 hunks)
- src/storage/VaultStorage.sol (1 hunks)
- src/utils/CustomProxyAdmin.sol (3 hunks)
- test/foundry/DepositWithdrawPrinciple.t.sol (8 hunks)
- test/foundry/ExocoreDeployer.t.sol (7 hunks)
- test/foundry/Governance.t.sol (4 hunks)
- test/foundry/WithdrawReward.t.sol (7 hunks)
- test/foundry/unit/Bootstrap.t.sol (12 hunks)
- test/foundry/unit/ClientChainGateway.t.sol (7 hunks)
- test/foundry/unit/CustomProxyAdmin.t.sol (1 hunks)
- test/foundry/unit/ExocoreGateway.t.sol (3 hunks)
- test/foundry/unit/RewardVault.t.sol (1 hunks)
- test/foundry/unit/Vault.t.sol (1 hunks)
- test/mocks/ClaimRewardMock.sol (0 hunks)
- test/mocks/ExocoreGatewayMock.sol (6 hunks)
- test/mocks/RewardMock.sol (1 hunks)
💤 Files with no reviewable changes (1)
- test/mocks/ClaimRewardMock.sol
🧰 Additional context used
📓 Learnings (2)
src/utils/CustomProxyAdmin.sol (2)
Learnt from: MaxMustermann2 PR: ExocoreNetwork/exocore-contracts#16 File: src/core/CustomProxyAdmin.sol:20-22 Timestamp: 2024-10-09T01:26:23.510Z Learning: The `CustomProxyAdmin` contract is designed for a `TransparentUpgradeableProxy` to upgrade itself. The `require(msg.sender == proxy)` condition in the `changeImplementation` function ensures that the proxy is the one initiating the upgrade, preventing unauthorized upgrades. This is a security measure to ensure that only the designated proxy can upgrade itself, and it can only do so once during its lifetime.Learnt from: MaxMustermann2 PR: ExocoreNetwork/exocore-contracts#16 File: src/core/CustomProxyAdmin.sol:20-22 Timestamp: 2024-06-03T11:27:03.714Z Learning: The `CustomProxyAdmin` contract is designed for a `TransparentUpgradeableProxy` to upgrade itself. The `require(msg.sender == proxy)` condition in the `changeImplementation` function ensures that the proxy is the one initiating the upgrade, preventing unauthorized upgrades. This is a security measure to ensure that only the designated proxy can upgrade itself, and it can only do so once during its lifetime.test/foundry/unit/CustomProxyAdmin.t.sol (2)
Learnt from: MaxMustermann2 PR: ExocoreNetwork/exocore-contracts#16 File: src/core/CustomProxyAdmin.sol:20-22 Timestamp: 2024-10-09T01:26:23.510Z Learning: The `CustomProxyAdmin` contract is designed for a `TransparentUpgradeableProxy` to upgrade itself. The `require(msg.sender == proxy)` condition in the `changeImplementation` function ensures that the proxy is the one initiating the upgrade, preventing unauthorized upgrades. This is a security measure to ensure that only the designated proxy can upgrade itself, and it can only do so once during its lifetime.Learnt from: MaxMustermann2 PR: ExocoreNetwork/exocore-contracts#16 File: src/core/CustomProxyAdmin.sol:20-22 Timestamp: 2024-06-03T11:27:03.714Z Learning: The `CustomProxyAdmin` contract is designed for a `TransparentUpgradeableProxy` to upgrade itself. The `require(msg.sender == proxy)` condition in the `changeImplementation` function ensures that the proxy is the one initiating the upgrade, preventing unauthorized upgrades. This is a security measure to ensure that only the designated proxy can upgrade itself, and it can only do so once during its lifetime.
🪛 LanguageTool
docs/client-chain-contracts-design.md
[uncategorized] ~264-~264: You might be missing the article “the” here.
Context: ...This involves the correct accounting on Exocore chain as well as the correct update of ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~264-~264: You might be missing the article “the” here.
Context: ...balance. If this process is successful, user should be able to withdraw the correspo...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~264-~264: You might be missing the article “the” here.
Context: ...to withdraw the corresponding assets on client chain to destination address. The prin...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~264-~264: You might be missing the article “the” here.
Context: ...corresponding assets on client chain to destination address. The principal withdrawal work...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~268-~268: You might be missing the article “the” here.
Context: ...to send principal withdrawal request to Exocore chain. 2. Response from Exocore: call `...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~269-~269: You might be missing the article “the” here.
Context: ...rchainMsgto receive the response from Exocore chain, and callunlockPrincipal` to up...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~269-~269: Possible missing article found.
Context: ...cipalBalance` and claimable balance. If response indicates failure, no user balance shou...(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~275-~275: You might be missing the article “an” here.
Context: ... frozen/slashed. 3. The asset is not in unbonding state. ###withdrawPrincipalThis f...(AI_EN_LECTOR_MISSING_DETERMINER_AN)
docs/reward-vault.md
[uncategorized] ~31-~31: Loose punctuation mark.
Context: ... token, address staker, uint256 amount)`: Allows the Gateway to unlock rewards fo...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~32-~32: Loose punctuation mark.
Context: ...wer, address recipient, uint256 amount)`: Allows the Gateway to withdraw claimed ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~33-~33: Loose punctuation mark.
Context: ...eBalance(address token, address staker)`: Returns the withdrawable balance of a s...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~34-~34: Loose punctuation mark.
Context: ...itedRewards(address token, address avs)`: Returns the total deposited rewards of ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~44-~44: Loose punctuation mark.
Context: ...mExocore(address token, uint256 amount)`: Initiates a claim request to the Exocor...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~45-~45: Loose punctuation mark.
Context: ...ken, address recipient, uint256 amount): Calls RewardVault'swithdraw` to trans...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~119-~119: A determiner appears to be missing. Consider inserting it.
Context: ...ocesses ### 4.1. Reward Submission 1. Depositor callssubmitRewardon the Gateway, sp...(AI_EN_LECTOR_MISSING_DETERMINER)
[uncategorized] ~121-~121: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...`, which: a. Transfers the specified amount of tokens from the depositor to itself....(AMOUNTOF_TO_NUMBEROF)
[uncategorized] ~125-~125: You might be missing the article “the” here.
Context: ...o account for the deposited rewards. 4. Exocore chain processes the request and emits a...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~129-~129: You might be missing the article “the” here.
Context: ... Reward Distribution and Accounting 1. Exocore chain handles the distribution and acco...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~130-~130: You might be missing the article “the” here.
Context: ...ctivities and the rewards submitted. 2. Exocore chain maintains the record of each stak...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~136-~136: You might be missing the article “the” here.
Context: ... claim request to the Exocore chain. 3. Exocore chain verifies the request and sends a ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🔇 Additional comments (122)
src/interfaces/ICustomProxyAdmin.sol (2)
Line range hint
1-20: Alignment with PR objectives: Clarify the relationshipWhile this change to the
ICustomProxyAdmininterface is significant, it's not immediately clear how it relates to the stated PR objectives of redesigning and implementing the reward vault. The removal of thepayablemodifier from thechangeImplementationfunction seems to be a separate concern from the creation of a dedicated reward vault.To ensure this change is intentional and related to the PR objectives, please clarify:
- How does this modification to the proxy admin interface contribute to the redesign of the reward system?
- Is this change part of a larger refactoring effort that indirectly supports the new reward vault implementation?
It would be helpful to update the PR description to explain the connection between this change and the overall objectives, ensuring that reviewers understand the full context and rationale behind all modifications.
18-18: Removal ofpayablemodifier: Verify impact on upgrade processThe
payablemodifier has been removed from thechangeImplementationfunction. This change prevents the function from accepting Ether during execution, which could impact the upgrade process if it previously relied on sending Ether to the new implementation.Consider the following:
- Ensure that removing the ability to send Ether during upgrades aligns with the intended upgrade mechanism for the Exocore protocol.
- Verify that no existing upgrade processes depend on sending Ether to the new implementation.
- Update any documentation or dependent code that might assume this function can receive Ether.
To ensure this change doesn't break existing functionality, please run the following verification:
Consider adding a comment in the interface explaining why this function is not
payable, to prevent future misunderstandings or accidental reintroduction of thepayablemodifier.Example:
/// @dev This function is intentionally not payable to prevent accidental Ether transfers during upgrades. function changeImplementation(ITransparentUpgradeableProxy proxy, address implementation, bytes memory data) external;Would you like me to draft a more detailed explanation of this change for the PR description or generate test cases to ensure the new behavior works as expected?
src/storage/RewardVaultStorage.sol (1)
1-4: LGTM: Contract declaration and pragma are well-defined.The contract uses an appropriate SPDX license identifier, specifies a recent Solidity version, and has a descriptive name that aligns with its purpose.
src/interfaces/IRewardVault.sol (5)
1-4: LGTM: File header and interface declaration are correct.The file header includes the appropriate SPDX license identifier and Solidity version pragma. The interface name follows the convention for Solidity interfaces.
20-27: LGTM: Thewithdrawfunction and its NatSpec comment are well-defined.The function signature and NatSpec comment for the
withdrawfunction are clear, accurate, and provide all necessary information.
29-35: LGTM: TheunlockRewardfunction aligns well with PR objectives.The
unlockRewardfunction and its NatSpec comment are well-defined. This function plays a crucial role in managing rewards separately from principal amounts, which directly addresses the PR's objective of creating a dedicated reward vault.
37-43: LGTM: ThegetWithdrawableBalancefunction is well-defined.The function signature and NatSpec comment for the
getWithdrawableBalancefunction are clear, accurate, and provide all necessary information. The use of a view function for balance queries is appropriate.
45-51: LGTM: ThegetTotalDepositedRewardsfunction supports the PR objectives.The
getTotalDepositedRewardsfunction and its NatSpec comment are well-defined. This function allows querying the total deposited rewards for a specific token and AVS, which aligns with the PR's objective of creating a separate reward vault and managing rewards distinctly from principal amounts.src/interfaces/IVault.sol (2)
Line range hint
1-46: Approve removal ofupdateWithdrawableBalance, but verify impact and update documentation.The removal of the
updateWithdrawableBalancefunction aligns with the PR objective of separating rewards from principal. This change suggests a significant shift in how withdrawable balances are managed.Please run the following script to check for any remaining calls to
updateWithdrawableBalance:#!/bin/bash # Description: Check for calls to updateWithdrawableBalance function rg --type solidity 'updateWithdrawableBalance'If any results are found, they will need to be updated to align with the new architecture.
Consider adding a comment in the interface explaining the rationale behind this removal and how withdrawable balances are now managed. This will help future developers understand the design decision.
19-20: Approve removal ofpayablemodifier, but verify impact.The removal of the
payablemodifier from thedepositfunction aligns with the PR objective of creating a separate reward vault. This change indicates that deposits are now handled differently, possibly using only token transfers instead of Ether.To ensure this change doesn't break existing functionality, please run the following script to check for any remaining calls to
depositthat send Ether:If any results are found, they may need to be updated to align with this interface change.
src/storage/VaultStorage.sol (4)
33-36: LGTM: PrincipalDeposited event addedThe new
PrincipalDepositedevent aligns well with the PR objectives. It provides a clear mechanism to track principal deposits, which is crucial for the new vault structure. The use ofindexedfor the depositor address allows for efficient event filtering.
38-41: Clarify the unlocking process and consider terminologyThe
PrincipalUnlockedevent introduces a new concept of "unlocking" principal. Could you please provide more context on why this intermediate step between deposit and withdrawal is necessary? This would help in understanding the overall flow of funds in the new vault structure.Also, consider changing the parameter name from
stakertowithdrawerfor consistency, as this event seems to be related to the withdrawal process rather than staking.
43-47: LGTM: Improved PrincipalWithdrawn eventThe update from
WithdrawalSuccesstoPrincipalWithdrawnis a good change that aligns with the new focus on principal in the vault structure. The addition of the destination address as an indexed parameter enhances the event's utility for tracking transfers. This change, along with the consistent structure across new events, will improve the contract's observability and make it easier to track fund movements.
33-47: Overall changes align with objectives, but consider implicationsThe changes to the events in this file align well with the PR objectives of creating a separate reward vault. The shift from reward-focused events to principal-focused events reflects the new structure.
However, please consider the following points:
With the removal of
RewardBalanceUpdatedandWithdrawableBalanceUpdatedevents, how will rewards be tracked or emitted in the new structure? It might be beneficial to add a comment explaining this or to include new reward-related events if they're handled elsewhere.These changes may have implications for existing integrations or dependencies that rely on the removed events. Ensure that all affected components are updated accordingly and that these changes are well-documented for other developers and users of the system.
Consider adding a comment at the top of the file explaining the overall change in the vault structure and how it relates to the separation of principal and rewards. This would provide valuable context for future developers working on this code.
Could you provide more information on how rewards will be handled in the new structure? This would help ensure that the removal of reward-related events doesn't create any gaps in functionality or observability.
src/core/LSTRestakingController.sol (1)
Line range hint
1-91: Address the removal ofwithdrawRewardFromExocoreThe removal of the
withdrawRewardFromExocorefunction aligns with the PR objectives of creating a separate reward vault. However, this change has several implications:
- It significantly alters how rewards are handled in the system.
- It may require changes in other parts of the system that previously called this function.
- Users who were using this function to withdraw rewards will need to use a different method.
To ensure that this change is properly handled, please provide information on the following:
- How will users now withdraw their rewards?
- Are there any other parts of the system that need to be updated due to this removal?
To help verify the impact, let's search for any remaining references to
withdrawRewardFromExocore:#!/bin/bash # Search for references to withdrawRewardFromExocore rg --type solidity "withdrawRewardFromExocore"Please review the results of this search and ensure that all necessary updates have been made.
src/libraries/ActionAttributes.sol (1)
43-46: LGTM! Verify enum definition and related code updates.The addition of the
REQUEST_SUBMIT_REWARDaction handling is consistent with the existing pattern and aligns with the PR objectives. It correctly sets theREWARDattribute and uses the appropriate message length.Please ensure the following:
- Verify that
Action.REQUEST_SUBMIT_REWARDis defined in theActionenum.- Check if any other parts of the codebase need to be updated to handle this new action.
Run the following script to verify the
Actionenum definition:✅ Verification successful
Verification Successful!
The
Action.REQUEST_SUBMIT_REWARDis properly defined in theActionenum and is correctly used across the codebase, including inActionAttributes.sol.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the Action enum definition and usage # Test 1: Check if REQUEST_SUBMIT_REWARD is defined in the Action enum echo "Checking Action enum definition:" rg --type solidity -n "enum\s+Action" -A 20 # Test 2: Search for other occurrences of REQUEST_SUBMIT_REWARD echo "\nChecking REQUEST_SUBMIT_REWARD usage:" rg --type solidity -n "REQUEST_SUBMIT_REWARD"Length of output: 321
Script:
#!/bin/bash # Description: Verify the Action enum definition and usage without relying on --type solidity # Test 1: Check if REQUEST_SUBMIT_REWARD is defined in the Action enum echo "Checking Action enum definition:" rg -g "*.sol" -n "enum\s+Action" -A 20 # Test 2: Search for other occurrences of REQUEST_SUBMIT_REWARD echo "\nChecking REQUEST_SUBMIT_REWARD usage:" rg -g "*.sol" -n "REQUEST_SUBMIT_REWARD"Length of output: 2788
script/1_Prerequisities.s.sol (2)
49-49: LGTM! Consistent with import changes.The replacement of
claimRewardMockwithrewardMockis consistent with the import statement changes and aligns with the PR objectives of implementing a new reward handling mechanism.
Line range hint
1-78: Overall assessment: Changes align well with PR objectives.The modifications in this file consistently implement the shift from
ClaimRewardMocktoRewardMock, aligning well with the PR objectives of redesigning the reward system. The changes are coherent and maintain consistency throughout the file. Good job on implementing these changes!src/interfaces/IExoCapsule.sol (1)
Line range hint
1-73: Verify completeness of interface for new reward systemThe addition of
unlockETHPrincipaland removal ofupdateWithdrawableBalancealign well with the PR objectives of redesigning the reward system. However, to ensure full support for the new reward vault system:
- Verify if any additional functions are needed to interact with the new reward vault (e.g., for depositing or withdrawing rewards).
- Consider adding functions to query the current reward balance or total rewards accrued, if applicable.
- Ensure that the existing withdrawal functions (
withdrawandwithdrawNonBeaconChainETHBalance) are sufficient for handling both principal and reward withdrawals, or if they need to be updated.To help verify the completeness of the interface, you can run the following script to search for reward-related functions in other parts of the codebase:
This will help identify if there are any reward-related functions in other contracts that might need to be reflected in the
IExoCapsuleinterface.✅ Verification successful
Verify completeness of interface for new reward system
The
IExoCapsuleinterface includes theunlockETHPrincipalfunction, and no additional reward-related functions were found in the codebase. This confirms that the interface is complete and sufficiently supports the new reward vault system as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for reward-related functions in the codebase # Test: Look for reward-related functions in Solidity files echo "Searching for reward-related functions:" rg --type solidity -i '(reward|vault).*function' -g '!src/interfaces/IExoCapsule.sol'Length of output: 209
Script:
#!/bin/bash # Description: Search for reward-related functions in Solidity (.sol) files echo "Searching for reward-related functions:" rg -i '(reward|vault).*function' -g '*.sol' -g '!src/interfaces/IExoCapsule.sol'Length of output: 171
src/storage/GatewayStorage.sol (3)
Line range hint
1-101: Summary: Changes align with PR objectives, but careful verification is needed.The modifications to the
Actionenum in theGatewayStoragecontract align with the PR objectives of redesigning and implementing the reward vault. The addition ofREQUEST_SUBMIT_REWARDdirectly supports the new reward management functionality.However, the changes to the enum structure, including the addition and reordering of values, require careful verification to ensure they don't introduce unintended consequences in other parts of the system. This includes checking the impact on:
- Any code that relies on specific enum indices or order.
- The
_whiteListFunctionSelectorsmapping.- Event emissions and off-chain applications that filter or listen for these events.
Please ensure that all verification steps mentioned in the previous comments are completed, and any affected code is updated accordingly. This will help maintain the integrity and functionality of the Exocore protocol while implementing the new reward vault feature.
8-21: Verify the impact ofActionenum changes on existing code.The modifications to the
Actionenum, including the addition of new values and reordering of existing ones, align with the PR objectives. However, these changes may have unintended consequences:
- The addition of
RESPONDat the beginning of the enum shifts the indices of all other values.- The reordering and addition of new enum values may break existing code that relies on specific enum indices or assumes a particular order.
To ensure these changes don't introduce bugs, please run the following verification steps:
Review the output to identify any code that may be affected by the enum changes and update accordingly.
✅ Verification successful
The
Actionenum changes do not affect existing code. No issues found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of Action enum values in the codebase # Test 1: Search for direct usage of Action enum values echo "Searching for direct usage of Action enum values:" rg --type solidity -n 'Action\.' -A 3 -B 3 # Test 2: Search for potential array or mapping access using Action as index echo "Searching for potential array or mapping access using Action as index:" rg --type solidity -n '\[Action\.' -A 3 -B 3 # Test 3: Search for switch statements or if-else chains using Action echo "Searching for switch statements or if-else chains using Action:" rg --type solidity -n '(switch|if).*Action\.' -A 10 -B 3Length of output: 646
Script:
#!/bin/bash # Description: Search for usages of Action enum values in Solidity files # Test 1: Search for direct usage of Action enum values echo "Searching for direct usage of Action enum values in .sol files:" rg -g '*.sol' -n 'Action\.' -A 3 -B 3 # Test 2: Search for potential array or mapping access using Action as index echo "Searching for potential array or mapping access using Action as index in .sol files:" rg -g '*.sol' -n '\[Action\.' -A 3 -B 3 # Test 3: Search for switch statements or if-else chains using Action echo "Searching for switch statements or if-else chains using Action in .sol files:" rg -g '*.sol' -n '(switch|if).*Action\.' -A 10 -B 3Length of output: 85299
Line range hint
33-33: Verify the impact ofActionenum changes on mapping and events.The modifications to the
Actionenum may affect other parts of the contract:
- The
_whiteListFunctionSelectorsmapping usesActionas a key. Ensure that any initialization or access to this mapping is updated to reflect the new enum values and order.- The
MessageSentandMessageExecutedevents useActionas an indexed parameter. While this doesn't directly impact the contract's functionality, it may affect event filtering and off-chain applications that rely on these events.To ensure these changes are properly reflected throughout the system, please run the following verification steps:
Review the output to identify any code that may need updating due to the enum changes, particularly focusing on:
- Initialization and access of the
_whiteListFunctionSelectorsmapping.- Event emissions using the new enum values.
- Any off-chain code that filters or listens for these events.
Update the affected code to ensure compatibility with the new enum structure.
Also applies to: 44-44, 51-51
script/5_Withdraw.s.sol (1)
71-71: Verify consistency ofclaimPrincipalFromExocoreacross the codebaseThe change from
withdrawPrincipalFromExocoretoclaimPrincipalFromExocorealigns with the PR objective of redesigning the reward system. This semantic shift from "withdraw" to "claim" might indicate a change in how the action is processed or perceived in the new reward vault structure.Please ensure this change is consistent across the entire codebase. Run the following script to check for any remaining instances of
withdrawPrincipalFromExocoreand to verify the usage ofclaimPrincipalFromExocore:Consider updating any relevant documentation or comments to reflect this change in terminology from "withdraw" to "claim".
This change might affect how users or other parts of the system interact with this functionality. Ensure that all dependent systems and user interfaces are updated to reflect this new terminology and any potential changes in behavior.
✅ Verification successful
Verified:
claimPrincipalFromExocoreSuccessfully ReplaceswithdrawPrincipalFromExocoreThe function
withdrawPrincipalFromExocorehas been consistently renamed toclaimPrincipalFromExocoreacross the entire codebase. No remaining instances of the old function were found, and the new function is appropriately used in all relevant areas, including tests and interface definitions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash echo "Checking for remaining instances of withdrawPrincipalFromExocore:" rg "withdrawPrincipalFromExocore" --type solidity echo "\nVerifying usage of claimPrincipalFromExocore:" rg "claimPrincipalFromExocore" --type solidityLength of output: 408
Script:
#!/bin/bash echo "Checking for remaining instances of withdrawPrincipalFromExocore in .sol files:" rg "withdrawPrincipalFromExocore" --glob "*.sol" echo "\nVerifying usage of claimPrincipalFromExocore in .sol files:" rg "claimPrincipalFromExocore" --glob "*.sol"Length of output: 2858
script/TestPrecompileErrorFixed.s.sol (2)
9-9: LGTM: Import changes align with reward system redesign.The addition of the
IRewardinterface import and the improved readability with the empty line are consistent with the PR objectives of redesigning the reward system.Also applies to: 11-11
Line range hint
75-124: Verify test coverage for the new reward system.While the
runfunction remains unchanged, the modifications in the setup might affect how the tests interact with the new reward system.Please ensure that the existing tests adequately cover the new reward functionality. Consider adding or updating tests if necessary to validate the behavior of the redesigned reward vault.
To help verify the test coverage, you can run the following command:
This will help identify existing reward-related tests and determine if additional tests are needed for the new reward system.
test/foundry/unit/CustomProxyAdmin.t.sol (1)
144-144: Improved type safety inchangeImplementationcallThe change from
address(implementationChanger)toITransparentUpgradeableProxy(address(implementationChanger))enhances type safety and aligns with the expected interface for thechangeImplementationfunction. This modification ensures that the correct contract type is passed to the function, which is crucial for maintaining the integrity of the proxy upgrade mechanism.This change is consistent with the design of the
CustomProxyAdmincontract, where thechangeImplementationfunction expects anITransparentUpgradeableProxyas its first argument. The explicit casting helps prevent potential errors and improves code readability.src/core/ClientGatewayLzReceiver.sol (3)
89-91: Approved: Renaming aligns with PR objectivesThe renaming of
_updatePrincipalWithdrawableBalanceto_unlockPrincipaland_updateRewardWithdrawableBalanceto_unlockRewardaccurately reflects the new functionality and aligns with the PR objective of redesigning the reward vault system. This change enhances code readability and maintains a clear separation between principal and reward handling.
Line range hint
1-236: Summary: Successful implementation of reward vault redesignThe changes in this file effectively implement the redesign of the reward vault system as outlined in the PR objectives. Key improvements include:
- Clear separation of principal and reward handling.
- Simplified reward unlocking process through a dedicated reward vault.
- Consistent renaming of functions to better reflect their purposes.
These changes should significantly enhance the management of rewards within the Exocore protocol. The code is well-structured and aligns with the goals of improving the deposit functionality and creating a distinct mechanism for handling rewards.
To further improve the implementation:
- Consider adding more detailed documentation for the
_unlockPrincipalfunction.- Verify the correct implementation of the
rewardVaultcontract, especially itsunlockRewardfunction.Overall, this is a solid implementation that addresses the issues raised in the linked issue #78.
163-169: Approved: Simplified reward unlocking aligns with PR objectivesThe rewrite of the
_unlockRewardfunction to directly interact with therewardVaultis a significant improvement. It simplifies the reward unlocking process and aligns perfectly with the PR objective of creating a separate and dedicated reward vault.However, to ensure the correctness and security of this change:
Please verify that the
rewardVaultcontract correctly implements theunlockRewardfunction for all supported token types. Run the following script to check therewardVaultimplementation:✅ Verification successful
Verified:
rewardVaultimplementation andunlockRewardfunction are correctly integratedThe changes to the
_unlockRewardfunction effectively delegate the reward unlocking process to therewardVault. TheRewardVaultcontract properly implements theunlockRewardfunction with necessary access restrictions (onlyGateway), ensuring that only authorized gateways can invoke it. Additionally, therewardVaultis correctly imported and initialized within the codebase.
- RewardVault Contracts:
src/core/RewardVault.solsrc/interfaces/IRewardVault.solAll aspects of the
rewardVaultintegration align with the intended simplification and security objectives of the PR.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the rewardVault implementation # Test 1: Check if rewardVault is properly imported and initialized rg --type solidity -A 5 'rewardVault\s*=' src/ # Test 2: Verify the unlockReward function in the RewardVault contract ast-grep --lang solidity --pattern $'contract RewardVault { $$$ function unlockReward(address token, address staker, uint256 amount) { $$$ } $$$ }'Length of output: 359
Script:
#!/bin/bash # Description: Verify the rewardVault implementation by searching Solidity files directly # Test 1: Check if rewardVault is properly imported and initialized rg 'rewardVault\s*=' src/ --glob '*.sol' -A 5 # Test 2: Verify the unlockReward function in the RewardVault contract by searching for the function signature rg 'function unlockReward\(address token, address staker, uint256 amount\)' src/ --glob '*.sol'Length of output: 636
Script:
#!/bin/bash # Description: Comprehensive verification of the RewardVault contract and unlockReward function # Test 1: List all Solidity files containing RewardVault to identify its location fd 'RewardVault' --extension sol src/ # Test 2: Search for any unlockReward function definitions regardless of parameters rg 'function\s+unlockReward\s*\(' src/ --glob '*.sol' # Test 3: Verify the presence of the RewardVault contract and its functions rg 'contract\s+RewardVault' src/ --glob '*.sol' -A 10Length of output: 1794
docs/reward-vault.md (1)
1-7: Overview section is comprehensive and aligns with PR objectives.The overview effectively introduces the Reward Vault, its purpose, and its role within the Exocore ecosystem. The mention of the beacon proxy pattern for upgradeability is a valuable inclusion, demonstrating forethought in the design.
src/core/ExoCapsule.sol (2)
26-29: Approve event renaming for improved clarityThe renaming of the event from
WithdrawableBalanceUpdatedtoETHPrincipalUnlockedis a positive change. It more accurately describes the action being logged and aligns with the PR objective of separating principal and rewards.
28-29: Approve event parameter update for consistencyThe renaming of the event parameter from
additionalAmounttounlockedAmountis appropriate. It maintains consistency with the event name change and provides a clearer description of the emitted value.test/foundry/unit/ExocoreGateway.t.sol (3)
5-7: LGTM: Import changes align with PR objectives.The replacement of
IClaimRewardwithIRewardandClaimRewardMockwithRewardMockreflects the shift towards a more generalized reward system. This change is consistent with the PR's objective of redesigning the reward vault.Also applies to: 11-13
200-201: LGTM: LzReceive contract updated to use RewardMock.The change from
ClaimRewardMocktoRewardMockin the LzReceive contract's setUp function is consistent with the previous modifications and ensures that the updated reward system mock is used in these tests.
Line range hint
1-724: Overall LGTM: Consistent updates to reward system mocks.The changes in this file primarily focus on transitioning from a claim-based reward system to a more general reward system. This is reflected in the updated import statements and the use of
RewardMockinstead ofClaimRewardMock. These modifications are consistent throughout the file and align with the PR objectives.To ensure that these changes haven't affected the test coverage or the behavior being tested, please run the following command:
This will help verify that all tests are still passing and that the coverage hasn't been negatively impacted by the changes.
test/foundry/DepositWithdrawPrinciple.t.sol (6)
32-34: LGTM: New events enhance tracking of principal-related actions.The addition of
PrincipalDeposited,PrincipalUnlocked, andPrincipalWithdrawnevents aligns well with the PR objective of creating a separate reward vault. These events will provide more granular tracking of principal-related actions, improving the overall transparency and auditability of the system.
103-105: LGTM: PrincipalDeposited event emission added to deposit test.The addition of the
PrincipalDepositedevent emission expectation in the_testLSTDepositfunction is consistent with the new event definition. This change ensures that the deposit process correctly emits the event, enhancing the test coverage for the new reward vault functionality.
225-226: LGTM: PrincipalUnlocked event emission added to withdrawal test.The addition of the
PrincipalUnlockedevent emission expectation in the_testLSTWithdrawfunction is consistent with the new event definition. This change ensures that the withdrawal process correctly emits the event, enhancing the test coverage for the new reward vault functionality.
622-622: LGTM: Method name consistently updated toclaimPrincipalFromExocore.The change from
withdrawPrincipalFromExocoretoclaimPrincipalFromExocorein thetest_DepositTvlLimitsfunction is consistent with the earlier modification. This consistency across the test suite reinforces the shift in terminology or functionality related to the new reward vault system.
662-662: Clarify the distinction betweenwithdrawPrincipalandclaimPrincipalFromExocore.The test suite now uses both
withdrawPrincipalandclaimPrincipalFromExocore. Could you please clarify the intended distinction between these two operations? It's important to ensure that this difference is intentional and correctly implemented throughout the codebase.To help understand the usage of these methods, please run the following script:
#!/bin/bash # Search for occurrences of both 'withdrawPrincipal' and 'claimPrincipalFromExocore' echo "Occurrences of 'withdrawPrincipal':" rg --type solidity 'withdrawPrincipal' echo "\nOccurrences of 'claimPrincipalFromExocore':" rg --type solidity 'claimPrincipalFromExocore'Also applies to: 732-732
177-177: LGTM: Method name updated toclaimPrincipalFromExocore.The change from
withdrawPrincipalFromExocoretoclaimPrincipalFromExocorereflects an update in terminology or functionality. This aligns with the PR objectives of redesigning the reward vault system.To ensure consistency, please run the following script to check for any remaining instances of
withdrawPrincipalFromExocore:src/core/RewardVault.sol (1)
11-68: Implementation Looks Correct and SecureThe
RewardVaultcontract is well-implemented with appropriate access controls and secure handling of token transfers. The use of theonlyGatewaymodifier effectively restricts access to critical functions, ensuring that only the designated gateway can invoke them. State changes are performed before external calls, which helps prevent reentrancy attacks. The contract follows Solidity best practices and aligns with the objectives outlined in the PR.src/interfaces/IBaseRestakingController.sol (4)
27-27: Verify access control forwithdrawPrincipalfunction.Ensure that appropriate access controls are implemented for the
withdrawPrincipalfunction in the implementing contract to prevent unauthorized withdrawals.To confirm that only authorized users can call this function, please review the access modifiers and any associated authentication mechanisms in the implementation.
29-33: Consider ifsubmitRewardshould bepayable.The
submitRewardfunction is marked asexternal payable, but it's not clear why it needs to bepayablesince the function deals with token transfers, not Ether.Please verify if the
payablemodifier is necessary. If not, consider removing it to prevent unintended Ether transfers.
34-38: Clarify the purpose ofclaimRewardFromExocore.The documentation for
claimRewardFromExocoredoesn't specify how the function interacts with Exocore or what the expected behavior is.Please ensure that the documentation clearly explains the function's interaction with Exocore and any prerequisites or side effects.
39-43: Verify access control forwithdrawRewardfunction.Similar to
withdrawPrincipal, ensure that proper access controls are in place forwithdrawRewardto prevent unauthorized withdrawals of rewards.Review the implementation to confirm that only authorized entities can call this function.
test/mocks/RewardMock.sol (1)
5-65: Consider adding access control to contract functionsAll functions in the
RewardMockcontract areexternaland can be called by any address. If this is intended for testing purposes, it's acceptable. However, if certain functions should be restricted to specific roles (e.g., only authorized addresses can callsubmitRewardordistributeReward), consider adding access control modifiers to enforce these restrictions.script/12_RedeployClientChainGateway.s.sol (4)
11-11: Addition ofRewardVaultImport – Looks GoodThe import statement for
RewardVaultcorrectly brings in the necessary contract for reward vault integration.
58-67: Inclusion ofrewardVaultBeaconin Constructor – VerifiedAdding
address(rewardVaultBeacon)to theClientChainGatewayconstructor parameters integrates the reward vault functionality as intended.
50-51:⚠️ Potential issueDeclare the
clientChainProxyAdminVariableThe variable
clientChainProxyAdminis used but not declared, leading to a compilation error. Please declare it before using it.Apply this diff to declare the variable:
+ CustomProxyAdmin clientChainProxyAdmin;Likely invalid or redundant comment.
40-42:⚠️ Potential issueDeclare the
rewardVaultBeaconVariableThe variable
rewardVaultBeaconis used but not declared, which will cause a compilation error. Please declare it before using it.Apply this diff to declare the variable:
+ UpgradeableBeacon rewardVaultBeacon;Likely invalid or redundant comment.
test/foundry/unit/RewardVault.t.sol (1)
1-129: Overall Code Quality and Test CoverageThe test suite provides comprehensive coverage of the
RewardVaultfunctionalities, including initialization, deposits, withdrawals, and access control. The tests are well-structured and use best practices for Solidity testing with Forge.src/core/Vault.sol (3)
78-78: Event Emission Update inwithdrawFunctionThe
withdrawfunction now emitsPrincipalWithdrawn, which more accurately reflects the action of principal withdrawal. This enhances clarity in event logs and aligns the event name with its purpose.
Line range hint
85-97: Event Emission Update and Access Control indepositFunctionThe
depositfunction now emitsPrincipalDeposited, providing clear information about the depositor and the amount deposited. Marking the function asexternal onlyGatewayappropriately restricts access to the gateway contract, enhancing security.
101-114: Improved Clarity and Security inunlockPrincipalFunctionThe renaming of
updateWithdrawableBalancetounlockPrincipalclarifies the function's purpose. The logic correctly ensures that a user cannot unlock more principal than they have deposited. The checks againsttotalDepositedPrincipalAmountandtotalUnlockPrincipalAmounteffectively prevent over-unlocking.src/storage/ClientChainGatewayStorage.sol (4)
6-6: Import ofIRewardVaultis appropriate.The import statement correctly adds the
IRewardVaultinterface, ensuring access to the reward vault functions.
100-107: Constructor updates are consistent and well-documented.The addition of the
rewardVaultBeacon_parameter to the constructor is properly implemented and documented. The parameter order in the constructor matches the documentation, maintaining clarity and consistency.
119-122: Parameter validation forrewardVaultBeacon_is correctly implemented.The
requirestatement ensures that therewardVaultBeacon_address is not zero, preventing potential deployment issues due to invalid addresses.
126-126: Assignment ofREWARD_VAULT_BEACONis correct.The immutable variable
REWARD_VAULT_BEACONis properly initialized with the castedrewardVaultBeacon_address, ensuring it references the correct beacon.script/BaseScript.sol (7)
7-7: Import ofIRewardVaultis appropriateThe addition of
import "../src/interfaces/IRewardVault.sol";correctly brings in the interface needed for the reward vault functionality.
27-27: ImportingRewardMock.solfor testing purposesIncluding
import "../test/mocks/RewardMock.sol";ensures that the reward functionalities can be properly mocked and tested.
52-52: Declaration ofIRewardVault rewardVaultAdding the
rewardVaultvariable provides an instance of the reward vault for managing rewards, aligning with the PR objectives to redesign and implement the reward vault.
59-59: Declaration ofIRewardVault rewardVaultImplementationThe
rewardVaultImplementationvariable is added to reference the reward vault implementation contract, which is essential for the new reward vault architecture.
62-62: Declaration ofIBeacon rewardVaultBeaconIntroducing
rewardVaultBeaconallows for an upgradeable proxy pattern using the beacon proxy, which is appropriate for managing the reward vault implementation.
147-147:⚠️ Potential issuePotential misalignment in deploying
RewardMock.solIn the
_bindPrecompileMocks()function, you deployRewardMock.soltoREWARD_PRECOMPILE_ADDRESS, but there is no assignment to therewardMockvariable declared earlier. If you intend to userewardMockto reference this deployed contract, consider assigning the deployed address torewardMock. Otherwise, ifrewardMockis not needed, you may remove the variable declaration.Confirm whether
deployCodeToreturns an address that should be assigned torewardMock. If it does, you can assign it as follows:- deployCodeTo("RewardMock.sol", REWARD_PRECOMPILE_ADDRESS); + rewardMock = deployCodeTo("RewardMock.sol", REWARD_PRECOMPILE_ADDRESS);
69-69:⚠️ Potential issueUnused variable
rewardMockThe variable
rewardMockis declared but not assigned or used within the provided code. To maintain clean code, consider removing it if it's unnecessary, or ensure it is properly initialized and utilized.Run the following script to verify if
rewardMockis used elsewhere:✅ Verification successful
Unused variable
rewardMockconfirmedThe variable
rewardMockis declared inscript/BaseScript.solbut is not used within the file. Please consider removing it if it's unnecessary or utilize it appropriately.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of 'rewardMock' variable outside its declaration. # Expected result: List of all usages of 'rewardMock' besides its declaration. rg --smart-case --type solidity --word-regexp '\brewardMock\b' --ignore-caseLength of output: 111
Script:
#!/bin/bash # Description: Search for usages of 'rewardMock' variable outside its declaration. # Expected result: List of all usages of 'rewardMock' besides its declaration. rg --smart-case --word-regexp '\brewardMock\b' --ignore-case --glob '*.sol' | grep -v "address rewardMock"Length of output: 2057
src/core/ClientChainGateway.sol (4)
6-6: Import ofIRewardVaultis AppropriateThe addition of
IRewardVaultimport is necessary for integrating the Reward Vault functionality and aligns with the changes in the contract.
25-25: Import ofCreate2LibraryImporting
Create2from OpenZeppelin contracts is appropriate for deterministic deployment of contracts using theCreate2opcode.
Line range hint
54-63: Verify Constructor Parameter UpdateThe constructor now includes the
rewardVaultBeacon_parameter, which is correctly passed toClientChainGatewayStorage. Please ensure:
- All instances where the constructor is called are updated with the new parameter.
- The
ClientChainGatewayStorageconstructor properly utilizesrewardVaultBeacon_.
88-89: Deployment of Reward Vault in InitializationCalling
_deployRewardVault()within theinitializefunction ensures the Reward Vault is deployed during contract initialization. This integration appears correct.script/7_DeployBootstrap.s.sol (8)
11-11: ImportingRewardVaultis appropriate.The new import statement correctly includes the
RewardVaultcontract, which is necessary for managing rewards separately from the principal.
73-74: Instantiation ofRewardVaultimplementation and beacon is correct.The creation of
rewardVaultImplementationand its associatedrewardVaultBeaconaligns with the introduction of the new reward vault functionality.
94-94: Usage ofclientChainProxyAdminin proxy deployment is appropriate.Passing
clientChainProxyAdminto theTransparentUpgradeableProxyensures that the correct proxy admin manages the proxy's upgrades.
115-115: Initialization ofclientChainProxyAdminwithbootstrapaddress is correct.This ensures that the
clientChainProxyAdminis properly linked to thebootstrapcontract, allowing it to manage proxy upgrades securely.
123-123: SerializingclientChainProxyAdminaddress under 'proxyAdmin'.Serializing the
clientChainProxyAdminasproxyAdminmaintains consistency with existing keys and simplifies contract management.
132-133: IncludeRewardVaultaddresses in serialized output.Adding
rewardVaultImplementationandrewardVaultBeaconto the serialized contracts ensures they are tracked and accessible for deployment verification and future reference.
80-80: ConfirmClientChainGatewayconstructor acceptsrewardVaultBeacon.Ensure that the
ClientChainGatewaycontract's constructor has been updated to includeaddress(rewardVaultBeacon)as a parameter. This is crucial for integrating the reward vault into the client chain gateway.#!/bin/bash # Description: Verify that 'ClientChainGateway' constructor includes 'rewardVaultBeacon'. # Test: Search for the constructor definition and check for 'rewardVaultBeacon' parameter. rg --type solidity --context 5 'constructor\(' ../src/core/ClientChainGateway.sol
104-104: Verify parameters inBootstrap.initializecall.Confirm that the
Bootstrap.initializefunction expectsclientChainProxyAdmin,clientGatewayLogic, andinitializationas parameters in the specified order to avoid any initialization issues.#!/bin/bash # Description: Verify the 'initialize' function signature in 'Bootstrap' contract. # Test: Search for 'initialize' function definition and check parameter order. rg --type solidity --context 5 'function initialize\(' ../src/core/Bootstrap.solscript/2_DeployBoth.s.sol (7)
66-66: Ensure proper instantiation ofrewardVaultImplementation.The
rewardVaultImplementationis correctly instantiated using theRewardVaultcontract.
71-71: Ensure proper instantiation ofrewardVaultBeacon.The
rewardVaultBeaconis correctly initialized with the address ofrewardVaultImplementation.
160-160: Serialization ofrewardVaultBeaconis appropriate.The
rewardVaultBeaconis serialized correctly, capturing its deployed address.
172-172: Serialization ofrewardPrecompileMockis appropriate.The
rewardPrecompileMockis correctly serialized within theexocoreContractsoutput.
117-117: Ensure constructor parameters forExocoreGatewayMockare correct.Verify that
rewardMockis correctly passed to theExocoreGatewayMockconstructor and that the parameter order aligns with the constructor definition.Run the following script to check the constructor definition:
#!/bin/bash # Description: Check constructor of ExocoreGatewayMock ast-grep --lang solidity --pattern $'constructor($_exocoreLzEndpoint, $_assetsMock, $_rewardMock, $_delegationMock) { $$$ }' test/mocks/ExocoreGatewayMock.sol
85-85: Verify parameter alignment inClientChainGatewayconstructor.Ensure that adding
address(rewardVaultBeacon)matches the updated constructor inClientChainGatewayand that the parameters are in the correct order.Run the following script to verify the constructor parameters:
#!/bin/bash # Description: Check constructor parameters of ClientChainGateway # Search for the constructor with seven parameters ast-grep --lang solidity --pattern $'constructor($_, $_, $_, $_, $_, $_, $_) { $$$ }' src/core/ClientChainGateway.sol
77-77: Verify functionality ofCustomProxyAdmin.Ensure that
CustomProxyAdminprovides all necessary functionalities required for proxy administration and maintains expected security properties.Run the following script to compare
CustomProxyAdminwithProxyAdmin:test/foundry/WithdrawReward.t.sol (1)
269-269: Clarify Recipient Address inwithdrawRewardFunction CallAt line 269, the
withdrawRewardfunction is called with thewithdrawer.addras both the caller and the recipient:clientGateway.withdrawReward(address(restakeToken), withdrawer.addr, amount);If the
withdrawRewardfunction is intended to withdraw rewards to a specified recipient, passing thewithdrawer.addras the recipient may be redundant since the caller is alreadywithdrawer.addr. Ensure that this is the intended behavior, and consider whether the recipient parameter is necessary or if it could introduce security issues.Run the following script to check the usage of
withdrawRewardin the codebase:✅ Verification successful
Retry Shell Script to Locate
withdrawRewardUsageThe previous search failed because the
solidityfile type was not recognized. Please use the following script to search forwithdrawRewardoccurrences in.solfiles:
Recipient Address is Appropriately Specified in
withdrawRewardFunction CallThe
withdrawRewardfunction requires arecipientaddress as defined inIBaseRestakingController.sol. In the testWithdrawReward.t.sol, passingwithdrawer.addras the recipient is intentional to ensure rewards are correctly withdrawn to the intended address.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `withdrawReward` function and confirm the recipient parameter's necessity. # Expected: Find all occurrences of `withdrawReward` and display their contexts. rg --type solidity -A 3 -B 3 'withdrawReward\('Length of output: 82
Script:
#!/bin/bash # Description: Verify the usage of the `withdrawReward` function and confirm the recipient parameter's necessity. # Expected: Find all occurrences of `withdrawReward` and display their contexts. rg -g '*.sol' -A 3 -B 3 'withdrawReward\('Length of output: 2099
test/foundry/Governance.t.sol (3)
28-30: Imports added for RewardVaultThe imports of
RewardVaultandIRewardVaultcorrectly include the necessary contracts for reward management functionality.
67-70: Declaration of RewardVault variablesThe declarations of
rewardVaultImplementationandrewardVaultBeaconproperly set up the reward vault components.
157-157: Integration of rewardVaultBeacon into ClientChainGatewayIncluding
address(rewardVaultBeacon)in theClientChainGatewayconstructor ensures proper integration of the reward vault into the gateway's setup.test/foundry/unit/ClientChainGateway.t.sol (7)
31-34: Imports for RewardVault Added CorrectlyThe
RewardVaultandIRewardVaultimports have been added successfully, integrating the reward vault functionality into the contract.
63-66: Declaration of RewardVault VariablesThe variables
rewardVaultImplementationandrewardVaultBeaconare declared appropriately to support the new reward vault feature.
108-112: Initialization of RewardVault Implementation and BeaconThe
rewardVaultImplementationandrewardVaultBeaconare properly initialized, ensuring that the reward vault can function as intended.
406-408: Test Correctly Expects Vault Does Not Exist ErrorThe test
test_revert_withdrawVirtualStakedETHappropriately expects theVaultDoesNotExisterror when attempting to claim principal withVIRTUAL_STAKED_ETH_ADDRESS.
414-416: Test Validates Non-Whitelisted Token ReversionThe test
test_revert_withdrawNonWhitelistedTokencorrectly checks that the contract reverts with the message "BootstrapStorage: token is not whitelisted" when a non-whitelisted token is used.
420-422: Test Ensures Reversion on Zero Amount WithdrawalThe test
test_revert_withdrawZeroAmountcorrectly expects a revert when attempting to claim a zero amount, ensuring proper validation of input parameters.
Line range hint
127-131: Verify Constructor Parameters Match ClientChainGatewayThe
rewardVaultBeaconhas been added to theClientChainGatewayconstructor parameters. Ensure that the constructor inClientChainGateway.solis updated accordingly to accept this new parameter at the correct position.Run the following script to verify the constructor signature:
src/core/ExocoreGateway.sol (2)
11-11: Importing Reward Contract InterfaceThe
REWARD_CONTRACTis correctly imported fromIReward.sol, allowing interaction with the reward contract.
84-85: Whitelist Updated with New Reward ActionsThe actions
REQUEST_SUBMIT_REWARDandREQUEST_CLAIM_REWARDare appropriately added to_whiteListFunctionSelectors, mapping them tohandleRewardOperation.test/foundry/ExocoreDeployer.t.sol (12)
22-22: ImportingRewardVaultis AppropriateThe addition of
import {RewardVault} from "../../src/core/RewardVault.sol";is necessary for accessing theRewardVaultcontract within this test deployment script. This import is correct and aligns with the integration of the new reward vault functionality.
26-26: IncludingIRewardVaultInterfaceThe import of
IRewardVaultinterface is appropriate for interacting with theRewardVaultcontract via its interface. This ensures that the contract adheres to the defined interface, promoting better abstraction and maintainability.
32-32: ImportingIRewardPrecompile InterfaceThe line
import "../../src/interfaces/precompiles/IReward.sol";correctly adds the interface for the reward precompile. Ensure that the precompile address is accurate and that the interface methods match the expected precompile contract.
60-60: DeclaringrewardVaultVariableThe declaration
IRewardVault rewardVault;introduces therewardVaultvariable to theExocoreDeployercontract. This addition is necessary for managing the reward vault within the deployment script.
69-69: AddingrewardVaultImplementationVariableThe line
IRewardVault rewardVaultImplementation;appropriately declares a variable to hold the reward vault implementation contract instance. This is essential for deploying and upgrading the reward vault logic.
72-72: IntroducingrewardVaultBeaconThe addition of
IBeacon rewardVaultBeacon;declares a beacon for the reward vault implementation. This aligns with the upgradeable pattern used for other contracts in the system, ensuring consistency and modularity.
145-146: InitializingRewardMockContractThe lines:
bytes memory RewardMockCode = vm.getDeployedCode("RewardMock.sol"); vm.etch(REWARD_PRECOMPILE_ADDRESS, RewardMockCode);Correctly deploy the
RewardMockcontract and assign it to theREWARD_PRECOMPILE_ADDRESS. This setup is necessary for mocking the reward precompile in testing scenarios.
336-336: InstantiatingrewardVaultImplementationThe line
rewardVaultImplementation = new RewardVault();properly initializes a new instance of theRewardVaultcontract. This instance will serve as the implementation logic for the reward vault proxy.
341-341: CreatingrewardVaultBeaconThe creation of the reward vault beacon with
rewardVaultBeacon = new UpgradeableBeacon(address(rewardVaultImplementation));is correct. This beacon manages the implementation address for the reward vault proxies, facilitating upgradeability.
376-378: Verifying Reward Vault DeploymentThe code snippet:
rewardVault = clientGateway.rewardVault(); require(address(rewardVault) != address(0), "reward vault should not be empty");Appropriately retrieves the
rewardVaultinstance post-initialization and asserts that it has been deployed correctly. This ensures that the reward vault is available for subsequent operations.
358-358: PassingrewardVaultBeacontoClientChainGatewayIncluding
address(rewardVaultBeacon),in the constructor parameters ofClientChainGatewayensures the reward vault beacon is integrated into the client gateway logic. Verify that theClientChainGatewayconstructor is updated accordingly to accept this new parameter in the correct order.Please confirm that the
ClientChainGatewayconstructor matches the updated parameter list.#!/bin/bash # Description: Verify the constructor parameters of 'ClientChainGateway'. # Expected: Constructor includes 'address rewardVaultBeacon' in the parameter list. rg 'constructor' src/core/ClientChainGateway.sol -A 5
38-38: Updating Import toRewardMockThe import statement
import "../mocks/RewardMock.sol";replaces the previousClaimRewardMockimport, reflecting the renaming in the testing framework. This change is appropriate and maintains consistency in the mock contracts used for testing.Please ensure that all references to the old
ClaimRewardMockare updated toRewardMockthroughout the codebase.Run the following script to check for any outdated references:
✅ Verification successful
Import Update Verified Successfully
All references to
ClaimRewardMockhave been updated toRewardMockthroughout the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any references to 'ClaimRewardMock' in the codebase. # Expected: No occurrences of 'ClaimRewardMock' should be found. rg 'ClaimRewardMock'Length of output: 783
test/foundry/unit/Bootstrap.t.sol (9)
15-16: Addition of Reward Vault ImportsThe imports for
RewardVaultandIRewardVaultare correctly added to support the new reward vault functionality.
68-68: Declaration of Reward Vault VariablesThe variables
rewardVaultImplementationandrewardVaultBeaconare appropriately declared to facilitate the reward vault's implementation.Also applies to: 71-71
95-95: Initialization of Reward Vault InstancesThe
rewardVaultImplementationandrewardVaultBeaconare correctly instantiated, ensuring the reward vault is properly set up.Also applies to: 99-99
406-407: Sequence of Principal WithdrawalThe sequence of calling
claimPrincipalFromExocorefollowed bywithdrawPrincipalis appropriate for withdrawing principal after adjusting the TVL limit.Also applies to: 425-426
Line range hint
941-1000: Addition of Comprehensive Tests forclaimPrincipalFromExocoreThe new test functions effectively cover various scenarios for the
claimPrincipalFromExocoremethod, including token not whitelisted, zero amount, no deposits, and excessive claims. This enhances the robustness of the testing suite.
1428-1428: Test for UnimplementedclaimRewardFromExocoreFunctionThe test correctly verifies that
claimRewardFromExocorereverts withNotYetSupported(), indicating that this functionality is pending implementation.
Line range hint
1431-1459: Addition of Tests forwithdrawPrincipalFunctionThe test functions for
withdrawPrincipalcomprehensively cover successful withdrawals and various edge cases, ensuring that the function behaves correctly under different conditions.
1470-1474: Test for Reversion on Sending Ether withclaimPrincipalFromExocoreThe test correctly ensures that
claimPrincipalFromExocorereverts when called with a non-zero Ether value, enforcing that no Ether should be sent with this function.
121-121: Verify Parameter Order inClientChainGatewayInitializationThe
rewardVaultBeaconis added as a parameter when creating a newClientChainGatewayinstance. Please ensure that the constructor ofClientChainGatewayhas been updated to accept this new parameter and that the parameter order matches.Run the following script to verify the constructor parameters of
ClientChainGateway:✅ Verification successful
Parameter Order in
ClientChainGatewayInitialization VerifiedThe constructor of
ClientChainGatewayhas been updated to includerewardVaultBeaconas the fifth parameter, matching the expected order.
- File:
src/core/ClientChainGateway.sol- Constructor Parameters:
address endpoint_uint32 exocoreChainId_address beaconOracleAddress_address vaultBeacon_address rewardVaultBeacon_🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the constructor of ClientChainGateway accepts rewardVaultBeacon as the fifth parameter. # Test: Search for the constructor definition. Expect: The constructor includes rewardVaultBeacon as the fifth parameter. rg --type solidity -A 5 -e 'constructor\(' -g 'src/core/ClientChainGateway.sol'Length of output: 112
Script:
#!/bin/bash # Description: Re-verify that the constructor of ClientChainGateway accepts rewardVaultBeacon as the fifth parameter by searching within .sol files. # Test: Search for the constructor definition in ClientChainGateway.sol. Expect: The constructor includes rewardVaultBeacon as the fifth parameter. rg 'constructor\(' -A 5 -g '*.sol' src/core/ClientChainGateway.solLength of output: 248
src/utils/CustomProxyAdmin.sol (3)
4-4: ImportingICustomProxyAdmininterfaceThe addition of
ICustomProxyAdminimport aligns with the contract's implementation, ensuring compliance with the specified interface.
15-15: Contract now implementsICustomProxyAdminBy extending
ICustomProxyAdmin, the contract explicitly adheres to the interface's specifications, enhancing clarity and maintainability.
36-39: Enhanced type safety forproxyparameterChanging the
proxyparameter type toITransparentUpgradeableProxyenhances type safety, ensuring that only proxies adhering to this interface can be used.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (12)
script/prerequisiteContracts.json (1)
Line range hint
1-12: Summary of changes and their implicationsThe modifications to
prerequisiteContracts.jsonreflect a significant shift in the project's architecture, particularly in how it handles reward-related functionality:
The removal of mock contracts (
claimRewardPrecompileMock,delegationPrecompileMock,depositPrecompileMock, andwithdrawPrecompileMock) from the "exocore" section aligns with the introduction of the newRewardVaultcontract mentioned in the PR objectives.Retaining the
lzEndpointin both sections suggests that cross-chain functionality remains a key component of the system.These changes support the PR's goal of redesigning and implementing a separate reward vault. However, it's crucial to ensure that:
- All components previously relying on these mock contracts have been updated.
- The new
RewardVaultcontract effectively replaces the functionality of the removed mocks.- Documentation is updated to reflect these architectural changes.
- Any deployment scripts or tests that used this configuration file are adjusted accordingly.
Consider adding a comment in this JSON file or creating a separate README to explain the purpose of each entry and the reasoning behind the recent changes. This will help future developers understand the configuration's structure and its role in the system architecture.
script/10_DeployExocoreGatewayOnly.s.sol (1)
19-20: Approved: Typo correction improves code clarity.The change from
prerequisitiestoprerequisitescorrects a typographical error, enhancing code readability and reducing potential confusion. The functionality remains unchanged, as this is purely a variable name correction.Consider adding a comment explaining the purpose of the
prerequisiteContracts.jsonfile for better documentation:+ // Load prerequisite contract addresses from JSON file string memory prerequisites = vm.readFile("script/prerequisiteContracts.json"); exocoreLzEndpoint = ILayerZeroEndpointV2(stdJson.readAddress(prerequisites, ".exocore.lzEndpoint"));src/core/RewardVault.sol (2)
31-38: LGTM: Well-implemented deposit function with a minor suggestion.The deposit function correctly implements the required logic, uses SafeERC20 for secure token transfers, and emits an appropriate event. However, consider adding a check for a non-zero amount to prevent unnecessary transactions and gas costs.
Consider adding a check for non-zero amount:
function deposit(address token, address depositor, address avs, uint256 amount) external onlyGateway { + require(amount > 0, "Amount must be greater than zero"); IERC20(token).safeTransferFrom(depositor, address(this), amount); totalDepositedRewards[token][avs] += amount; emit RewardDeposited(token, avs, amount); }
40-49: LGTM: Well-implemented withdraw function with a minor suggestion.The withdraw function correctly implements the required logic, including balance checks, secure token transfers, and event emission. However, consider adding a check for a non-zero amount to prevent unnecessary transactions and gas costs.
Consider adding a check for non-zero amount:
function withdraw(address token, address withdrawer, address recipient, uint256 amount) external onlyGateway { + require(amount > 0, "Amount must be greater than zero"); if (withdrawableBalances[token][withdrawer] < amount) { revert Errors.InsufficientBalance(); } withdrawableBalances[token][withdrawer] -= amount; IERC20(token).safeTransfer(recipient, amount); emit RewardWithdrawn(token, withdrawer, recipient, amount); }src/interfaces/IBaseRestakingController.sol (1)
21-27: LGTM! Minor documentation improvement suggested.The
withdrawPrincipalfunction is well-documented and its signature matches the description. The change fromclaimtowithdrawPrincipalprovides a more explicit naming convention, which is a good improvement.However, there's a minor issue in the
@param amountdescription.Consider updating the
@param amountdescription:-/// @param amount The amount of @param token that the user wants to claim from the vault. +/// @param amount The amount of tokens that the user wants to withdraw from the vault.test/mocks/ExocoreGatewayMock.sol (4)
61-74: LGTM: Constructor updated for reward functionality, but consider variable naming.The constructor changes appropriately initialize the new reward-related variables. However, the variable name
CLAIM_REWARD_PRECOMPILE_ADDRESSmentioned in the past review comments hasn't been addressed. Consider renaming it toREWARD_PRECOMPILE_ADDRESSfor consistency.
337-342: LGTM: Enhanced response handling in _lzReceive.The new logic for handling responses from called functions improves the flexibility of inter-chain communication. Consider adding a comment explaining the purpose of this change for better code readability.
348-407: LGTM: Enhanced response handling in transfer functions.The changes in
handleLSTTransferandhandleNSTTransferto return responses are consistent with the new response handling mechanism. This provides more information for withdrawal actions, which is beneficial for the calling chain.Consider adding comments explaining why deposit actions return an empty response while withdrawal actions return the lzNonce and success flag.
410-436: LGTM: New handleRewardOperation function added.The
handleRewardOperationfunction is well-structured and consistent with other handler functions. It properly differentiates between submit and claim actions and emits an appropriate event.Consider adding a check to ensure that the
actparameter is eitherREQUEST_SUBMIT_REWARDorREQUEST_CLAIM_REWARDto prevent unexpected behavior.src/interfaces/precompiles/IReward.sol (2)
4-5: Consider removing TODO comments from the codebase.The TODO comment suggests a potential future change. It's recommended to track such tasks in your issue tracker to keep the codebase clean and maintainable.
Would you like me to open a GitHub issue to track this potential change?
30-31: Improve documentation clarity forclaimReward.
- Line 30: The function description can be clearer. Consider updating it to: "Claim reward on behalf of the staker, which will change the state in the reward module."
- Line 31: Specify which address the note refers to. For example: "Note that the
withdraweraddress cannot be a module account."Apply this diff to update the documentation:
/// @dev Claim reward on behalf of the staker, which will change the state in the reward module -/// Note that this address cannot be a module account. +/// Note that the `withdrawer` address cannot be a module account.test/foundry/unit/RewardVault.t.sol (1)
79-99: Check for event emission intestWithdrawWhile you correctly emit and expect the
RewardWithdrawnevent, consider also verifying that no unintended events are emitted during the withdrawal process. This can help ensure that the contract's event logic remains consistent and only emits events as designed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
- README.md (1 hunks)
- script/10_DeployExocoreGatewayOnly.s.sol (1 hunks)
- script/1_Prerequisites.s.sol (3 hunks)
- script/2_DeployBoth.s.sol (7 hunks)
- script/deployBeaconOracle.s.sol (1 hunks)
- script/prerequisiteContracts.json (1 hunks)
- src/core/RewardVault.sol (1 hunks)
- src/interfaces/IBaseRestakingController.sol (1 hunks)
- src/interfaces/precompiles/IReward.sol (1 hunks)
- test/foundry/unit/RewardVault.t.sol (1 hunks)
- test/foundry/unit/Vault.t.sol (1 hunks)
- test/mocks/ExocoreGatewayMock.sol (6 hunks)
✅ Files skipped from review due to trivial changes (2)
- README.md
- script/deployBeaconOracle.s.sol
🚧 Files skipped from review as they are similar to previous changes (1)
- test/foundry/unit/Vault.t.sol
🧰 Additional context used
📓 Learnings (2)
src/interfaces/IBaseRestakingController.sol (1)
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#102 File: src/interfaces/ILSTRestakingController.sol:27-27 Timestamp: 2024-10-10T07:26:44.743Z Learning: In the `ILSTRestakingController` interface, reward claim functionality is provided through methods inherited from `IBaseRestakingController`.test/foundry/unit/RewardVault.t.sol (2)
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#102 File: test/foundry/unit/RewardVault.t.sol:29-31 Timestamp: 2024-10-10T08:16:23.711Z Learning: In Solidity, events cannot be imported directly from other contracts; they must be re-declared in test contracts when needed.Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#102 File: test/foundry/unit/RewardVault.t.sol:89-98 Timestamp: 2024-10-10T08:57:50.583Z Learning: In the `testUnlockReward` function in `test/foundry/unit/RewardVault.t.sol`, the test focuses solely on `unlockReward`, and depositing tokens before unlocking rewards is unnecessary.
🔇 Additional comments (23)
script/prerequisiteContracts.json (1)
10-11: Verify the impact of removed mock contractsThe removal of several mock contracts from the "exocore" section aligns with the PR objective of redesigning the reward system. However, this change may have broader implications:
- Ensure that all deployment scripts, tests, and other components that previously relied on these mock contracts (
claimRewardPrecompileMock,delegationPrecompileMock,depositPrecompileMock, andwithdrawPrecompileMock) have been updated accordingly.- Confirm that the new
RewardVaultcontract mentioned in the PR objectives properly replaces the functionality of these removed mocks.- Verify that the removal of these mocks doesn't break any existing tests or functionalities that haven't been updated yet.
To help verify the impact, you can run the following script:
This script will help identify any remaining references to the removed mock contracts and locate files that might be using this configuration, allowing you to ensure all necessary updates have been made.
src/core/RewardVault.sol (4)
1-13: LGTM: Appropriate imports and contract declaration.The contract declaration and imports are well-structured. The use of OpenZeppelin's contracts and SafeERC20 for secure token transfers is a good practice.
15-20: LGTM: Well-implemented access control modifier.The
onlyGatewaymodifier effectively restricts access to specific functions. The use of a custom error for reverting is gas-efficient and provides clear error messages.
58-66: LGTM: Well-implemented getter functions.The
getWithdrawableBalanceandgetTotalDepositedRewardsfunctions correctly implement the getter logic, providing a way to query the contract's state externally.
1-68: Overall assessment: Well-implemented RewardVault contract with minor improvement suggestions.The RewardVault contract successfully implements the core functionality of a reward vault, including proper access control, secure token transfers, and event emissions. The contract follows good practices and is generally well-structured.
Key strengths:
- Use of OpenZeppelin's SafeERC20 for secure token transfers.
- Proper access control with the onlyGateway modifier.
- Clear separation of concerns in different functions.
Suggested improvements:
- Change the visibility of the
initializefunction toexternal.- Add checks for non-zero amounts in
depositandwithdrawfunctions.- Implement overflow protection in the
unlockRewardfunction.Implementing these minor improvements will further enhance the contract's security, efficiency, and gas optimization.
src/interfaces/IBaseRestakingController.sol (4)
Line range hint
1-44: Summary: Interface changes align well with PR objectives.The modifications to
IBaseRestakingControllerinterface effectively implement the separate reward vault functionality and enhance reward management. The new functions (submitReward,claimRewardFromExocore, andwithdrawReward) provide clear separation between principal and reward operations, which should improve the overall structure and maintainability of the system.These changes align well with the PR objectives of creating a dedicated reward vault and addressing the limitations in the deposit functionality mentioned in issue #78.
40-44: LGTM! Verify security aspects of the withdrawal process.The
withdrawRewardfunction is well-documented, and its signature matches the description. The function not being payable is appropriate for a withdrawal function.To ensure the correct implementation and security of this new function, please run the following script:
#!/bin/bash # Description: Verify the implementation and security of withdrawReward function # Test: Search for the withdrawReward function implementation rg --type solidity -A 15 'function\s+withdrawReward' # Test: Check for access control mechanisms rg --type solidity 'require\(.*msg\.sender.*\).*withdraw' # Test: Check for events related to reward withdrawal rg --type solidity 'event.*Reward.*Withdraw' # Test: Look for potential reentrancy guards rg --type solidity '(nonReentrant|reentrancyGuard)'
35-38: LGTM! Verify integration with Exocore system.The
claimRewardFromExocorefunction is well-documented, and its signature matches the description. The function being payable is appropriate for claiming rewards.To ensure the correct implementation and integration of this new function with the Exocore system, please run the following script:
#!/bin/bash # Description: Verify the implementation and integration of claimRewardFromExocore function # Test: Search for the claimRewardFromExocore function implementation rg --type solidity -A 10 'function\s+claimRewardFromExocore' # Test: Check for interactions with Exocore system rg --type solidity 'Exocore.*claim' # Test: Check for events related to reward claiming rg --type solidity 'event.*Reward.*Claim'
29-33: LGTM! Verify the implementation ofsubmitReward.The
submitRewardfunction is well-documented, and its signature matches the description. The function being payable is appropriate for submitting rewards.To ensure the correct implementation of this new function, please run the following script:
script/1_Prerequisites.s.sol (4)
10-15: LGTM: Import changes and contract name correction.The changes in this section are appropriate:
- The added blank line (10) improves readability.
- The new import of
RewardMock(13) aligns with the introduction of the new reward vault functionality.- The contract name correction from
PrerequisitiesScripttoPrerequisitesScript(15) fixes a typo.These changes contribute to better code organization and accuracy.
49-49: LGTM: Updated mock contract instantiation.The change from
claimRewardMocktorewardMockis consistent with the newRewardMockcontract import. This update aligns with the broader changes in the reward management system.
67-67: LGTM: Updated serialization for reward mock.The serialization logic has been correctly updated to use
rewardPrecompileMockinstead ofclaimRewardPrecompileMock. This change maintains consistency with the newRewardMockcontract and ensures the correct mock contract address is included in the output JSON.
Line range hint
1-85: Overall assessment: Changes align with PR objectives.The modifications in this file consistently support the introduction of the new reward vault functionality:
- The import statements have been updated to include the new
RewardMock.- The contract name has been corrected from
PrerequisitiesScripttoPrerequisitesScript.- The
runfunction now usesrewardMockinstead ofclaimRewardMock.- The serialization logic has been updated to use
rewardPrecompileMock.These changes align well with the PR objectives of redesigning and implementing the reward vault. The script now correctly sets up the prerequisites for the new reward management system, which is crucial for testing and deployment.
To ensure all references to the old
ClaimRewardMockhave been updated, run the following script:If this script returns any results, those occurrences should be reviewed and updated accordingly.
✅ Verification successful
All references to
ClaimRewardMockhave been successfully removed.No instances of
ClaimRewardMockwere found in the Solidity files, confirming that the replacement was thorough and complete.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all references to ClaimRewardMock have been replaced. # Test: Search for any remaining references to ClaimRewardMock rg --type solidity 'ClaimRewardMock'Length of output: 69
Script:
#!/bin/bash # Description: Verify that all references to ClaimRewardMock have been replaced in Solidity files. # Test: Search for any remaining references to ClaimRewardMock in .sol files rg -g "*.sol" 'ClaimRewardMock'Length of output: 31
test/mocks/ExocoreGatewayMock.sol (4)
7-9: LGTM: Import changes reflect new reward functionality.The removal of an import and addition of
IRewardaligns with the new reward handling capabilities. Ensure that any references to the removed import have been addressed throughout the contract.
43-47: LGTM: New reward-related variables added.The addition of
REWARD_PRECOMPILE_ADDRESSandREWARD_CONTRACTis consistent with the existing pattern and supports the new reward functionality.
102-103: LGTM: New reward operation selectors added to whitelist.The addition of function selectors for
REQUEST_SUBMIT_REWARDandREQUEST_CLAIM_REWARDis consistent with the new reward functionality and follows the existing pattern.
Line range hint
441-504: LGTM: Updated handler functions with consistent response handling.The changes in
handleDelegation,handleDepositAndDelegate, andhandleOperatorAssociationare consistent with the new response handling mechanism. The empty responses are appropriate as the client chain should not expect a response for these operations.src/interfaces/precompiles/IReward.sol (1)
22-24: Verify the correctness of parameter types and descriptions.The parameters
assetsAddressandavsIdare of typebytes calldata, and the documentation mentions they are addresses represented as bytes. Typically, addresses in Solidity are of typeaddress. If these parameters are indeed addresses, consider changing their types toaddressfor clarity and type safety. If they are intended to be byte arrays (e.g., cross-chain identifiers), ensure the documentation accurately reflects their purpose.Run the following script to check where and how these parameters are used elsewhere in the codebase:
test/foundry/unit/RewardVault.t.sol (2)
133-141: Tests for access control cover necessary functionsThe
testOnlyGatewayModifierfunction appropriately tests access control fordeposit,withdraw, andunlockRewardfunctions, ensuring that only thegatewaycan call them.
21-157: Overall, the test suite is comprehensive and well-structuredThe tests effectively cover the main functionalities of the
RewardVaultcontract, and the use of Forge's testing utilities is appropriate. The code is clean and follows good practices.script/2_DeployBoth.s.sol (3)
62-66: Improve clarity of comments when deploying implementation contracts.Consider rephrasing the comments to enhance readability.
As previously suggested:
-/// deploy vault implementation contract, capsule implementation contract, reward vault implementation contract -/// that has logics called by proxy +/// Deploy implementation contracts for Vault, ExoCapsule, and RewardVault, which contain the logic called by proxies
68-71: Improve clarity of comments when deploying beacon contracts.Consider rephrasing the comments to enhance readability.
As previously suggested:
-/// deploy the vault beacon, capsule beacon, reward vault beacon that store the implementation contract address +/// Deploy UpgradeableBeacon contracts for Vault, ExoCapsule, and RewardVault to store implementation addresses
122-122: Verify that all references toclaimRewardMockhave been updated torewardMock.Ensure that there are no remaining references to
claimRewardMockin the codebase.Run the following script to check for any remaining references:
✅ Verification successful
All references to
claimRewardMockhave been successfully updated torewardMock.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any usages of `claimRewardMock` in the codebase. # Test: Search for `claimRewardMock`. Expect: No matches found. rg --type solidity 'claimRewardMock'Length of output: 69
Script:
#!/bin/bash # Description: Search for any usages of `claimRewardMock` in Solidity files. # Test: Search for `claimRewardMock` in .sol files. Expect: No matches found. rg 'claimRewardMock' --glob '*.sol'Length of output: 35
Co-authored-by: Max <[email protected]>
|
this PR breaks the backwards compatibility for |
|
After discussing with @MaxMustermann2 , I would submit a new PR to compare the storage layout against deployed contracts and fix the current break |
Description
closes: #78
Rewards are distinct from principal in some aspects, so we should design and implement a separated and single reward vault for reward custody.
Contents
Interfaces
IBaseRestakingController.sol:claim=>withdrawPrincipalsubmitReward,claimRewardFromExocoreandwithdrawRewardIILSTRestakingController.sol:withdrawPrincipalFromExocore=>claimPrincipalFromExocorewithdrawRewardFromExocoreIRewardVault.solIVault.sol:updateWIthdrawableBalance=>unlockPrincipalstorage
RewardVaultStorage.solVaultStorage.solPrincipalDeposited,PrincipalUnlockedandPrincipalWithdrawncore
RewardVault.solBaseRestakingController.sol:submitReward,claimRewardFromExocoreandwithdrawRewardclaim=>withdrawPrincipalBootstrap.sol:withdrawPrincipalFromExocore=>claimPrincipalFromExocoreClientChainGateway.sol:LSTRestakingController.sol:withdrawRewardFromExocoretest
Vaultcontract andRewardVaultContractsubmitRewardandwithdrawRewardSummary by CodeRabbit
Summary by CodeRabbit
New Features
RewardVaultfor managing reward deposits and withdrawals.IBaseRestakingControllerinterface with new reward-related methods.Bug Fixes
Documentation
ClientChainGateway,Vault, andLSTRestakingController.Tests
RewardVaultandVaultcontracts, ensuring functionality for deposits, withdrawals, and reward management.