feat: remove reward vault deployment in initialization #143
Conversation
|
Warning Rate limit exceeded@adu-web3 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 51 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces changes to the Changes
Sequence DiagramsequenceDiagram
participant Owner
participant ClientChainGateway
participant RewardVault
Owner->>ClientChainGateway: deployRewardVault()
activate ClientChainGateway
ClientChainGateway-->>ClientChainGateway: Check owner and paused status
ClientChainGateway->>RewardVault: Create using Create2
ClientChainGateway->>ClientChainGateway: Emit RewardVaultCreated event
deactivate ClientChainGateway
Possibly related PRs
Suggested reviewers
Poem
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 (
|
|
✅ The Forge CI workflow has completed successfully. Check the workflow run for details. (bf5b2ad) |
|
✅ The Solhint workflow has completed successfully. Check the workflow run for details. (bf5b2ad) |
|
✅ The Slither Analysis workflow has completed successfully. Check the workflow run for details. (bf5b2ad) |
adu-web3
requested review from
MaxMustermann2,
magj2006,
bwhour and
cloud8little
|
✅ The Compare Storage Layouts workflow has completed successfully. Check the workflow run for details. (bf5b2ad) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/core/ClientChainGateway.sol (1)
Line range hint
149-159: LGTM! The newdeployRewardVaultfunction looks well-structured.The function has proper access controls and follows best practices for proxy deployment. The use of Create2 with a fixed salt ensures deterministic address generation, which is important for cross-chain verification.
Consider adding:
- A check to prevent redeployment if the reward vault already exists
- Documentation explaining the expected deployment workflow
function deployRewardVault() external onlyOwner whenNotPaused { + if (address(rewardVault) != address(0)) { + revert("RewardVault already deployed"); + } rewardVault = IRewardVault( Create2.deploy( 0, bytes32(bytes("REWARD_VAULT")), // for clarity, this BEACON_PROXY is not related to beacon chain // but rather it is the bytecode for the beacon proxy upgrade pattern. abi.encodePacked(BEACON_PROXY_BYTECODE.getBytecode(), abi.encode(address(REWARD_VAULT_BEACON), "")) ) ); rewardVault.initialize(address(this)); emit RewardVaultCreated(address(rewardVault)); }test/foundry/unit/Bootstrap.t.sol (1)
Line range hint
1007-1019: Consider adding edge case tests for withdrawNonBeaconChainETHFromCapsule.The function's error cases could be more thoroughly tested. Consider adding tests for:
- Zero address recipient
- Zero withdrawal amount
+ function test27_WithdrawNonBeaconChainETHFromCapsule_ZeroRecipient() public { + _enableNativeRestaking(); + vm.startPrank(addrs[0]); + bootstrap.createExoCapsule(); + + uint256 withdrawAmount = 1 ether; + vm.expectRevert(Errors.ZeroAddress.selector); + bootstrap.withdrawNonBeaconChainETHFromCapsule(payable(address(0)), withdrawAmount); + vm.stopPrank(); + } + + function test27_WithdrawNonBeaconChainETHFromCapsule_ZeroAmount() public { + _enableNativeRestaking(); + vm.startPrank(addrs[0]); + bootstrap.createExoCapsule(); + + address payable recipient = payable(addrs[1]); + vm.expectRevert(Errors.ZeroValue.selector); + bootstrap.withdrawNonBeaconChainETHFromCapsule(recipient, 0); + vm.stopPrank(); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/core/ClientChainGateway.sol(1 hunks)test/foundry/BootstrapDepositNST.t.sol(1 hunks)test/foundry/ExocoreDeployer.t.sol(1 hunks)test/foundry/unit/Bootstrap.t.sol(1 hunks)test/foundry/unit/ExoCapsule.t.sol(2 hunks)
🧰 Additional context used
📓 Learnings (2)
test/foundry/BootstrapDepositNST.t.sol (1)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#121
File: test/foundry/BootstrapDepositNST.t.sol:285-300
Timestamp: 2024-11-12T10:03:05.536Z
Learning: In `test/foundry/BootstrapDepositNST.t.sol`, adu-web3 prefers to use `vm.etch` and direct storage manipulation in tests, instead of deploying the capsule contract with the desired state or using Solidity inheritance and overrides.
test/foundry/ExocoreDeployer.t.sol (1)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#102
File: src/core/ClientChainGateway.sol:166-181
Timestamp: 2024-11-12T10:03:21.957Z
Learning: In the Exocore smart contracts, only a single `RewardVault` is deployed during initialization when the transparent proxy is created. Therefore, concerns about `Create2` salt uniqueness, address collisions, and front-running are not applicable in this context.
🔇 Additional comments (6)
test/foundry/BootstrapDepositNST.t.sol (1)
74-74: LGTM! Setting chain ID ensures consistent test environment.The addition of
vm.chainId(1)ensures that the capsule implementation uses the correct network constants during testing.test/foundry/unit/ExoCapsule.t.sol (2)
51-51: LGTM! Chain ID configuration added to DepositSetup.Setting chain ID to 1 ensures proper network constant initialization for deposit tests.
360-361: LGTM! Chain ID configuration added to WithdrawalSetup.Setting chain ID to 1 ensures proper network constant initialization for withdrawal tests.
test/foundry/ExocoreDeployer.t.sol (1)
386-389: LGTM! Updated reward vault validation aligns with the new deployment flow.The assertion correctly verifies that the reward vault is not deployed during initialization, which is consistent with the architectural change to deploy the reward vault separately.
test/foundry/unit/Bootstrap.t.sol (2)
88-88: LGTM! Well-documented chain ID configuration.The addition of
vm.chainId(1)is properly documented and necessary for the capsule implementation to use default network constants.
Line range hint
1-1146: Excellent test implementation with comprehensive coverage!The test file demonstrates high-quality testing practices:
- Consistent and clear test naming convention
- Well-organized test structure
- Proper use of test utilities and helpers
- Thorough state verification
Co-authored-by: Max <82761650+MaxMustermann2@users.noreply.github.com>
Description
The logics of deploying reward vault in
ClientChainGateway.initializefunction would cause out-of-gas error when upgrading Bootstrap to ClientChainGateway, so we just remove this part of logics and makedeployRewardVaulta standalone function instead of an internal function since reward vault is currently not working currently.I don't totally remove the rewardvault state from current contract storage just to make things like keeping storage layout compatible easier.
Summary by CodeRabbit
Release Notes
New Features
deployRewardVaultfunction with access control and event emission in theClientChainGatewaycontract.Tests
Bug Fixes
ExocoreDeployerto require an empty reward vault during contract initialization.