refactor: separate NST deposit/withdrawal from LST deposit/withdrawal #96
Conversation
WalkthroughThe pull request introduces significant changes across multiple smart contract files, primarily focusing on restructuring action handling and enhancing error management. A new Changes
Assessment against linked issues
Possibly related PRs
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 (
|
adu-web3
force-pushed
the
update-balance
branch
from
68fba84 to
bf1ceff
Compare
adu-web3
requested review from
magj2006,
bwhour,
cloud8little and
MaxMustermann2
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
Outside diff range and nitpick comments (8)
src/core/ClientChainGateway.sol (1)
18-18: Clarify the purpose of importingActionor remove the unused import.The
Actionenum is imported fromGatewayStorage.sol, but it is not used in the provided code changes. Unused imports can clutter the code and make it harder to maintain.Please clarify the purpose of importing
Action. If it is not used, consider removing the import:-import {Action} from "../storage/GatewayStorage.sol";src/core/NativeRestakingController.sol (1)
8-9: Nitpick: Remove the empty line between the import statements.The empty line between the import statements is inconsistent with the rest of the file. Consider removing it to maintain consistent formatting.
- import {Action} from "../storage/GatewayStorage.sol";test/foundry/DepositWithdrawPrinciple.t.sol (1)
90-90: Approve with a minor suggestion.The LST deposit test function follows the expected workflow and validates the emitted events correctly. Consider adding a comment to explain the purpose of updating the
lastlyUpdatedPrincipalBalancevariable for better clarity.Also applies to: 112-112, 119-128
test/foundry/unit/Bootstrap.t.sol (1)
Line range hint
1162-1184: Add assertions to verify the deposit success.The test function
test02_Deposit_Successtests the successful deposit scenario with a whitelisted token. However, it lacks assertions to verify that the deposit was indeed successful.Consider adding assertions to check the following after the deposit:
- The balance of the depositor should decrease by the deposited amount.
- The total deposit amount for the depositor and token should increase by the deposited amount.
- The vault's balance should increase by the deposited amount.
src/storage/GatewayStorage.sol (1)
7-21: Ensure consistent naming in theActionenumWhile most enum values use the
REQUEST_prefix, theRESPONDaction does not follow this pattern. For consistency, consider renamingRESPONDtoREQUEST_RESPONDorRESPONSEto align with the naming convention.test/mocks/AssetsMock.sol (2)
29-30: Improve error messages for clarityConsider rephrasing the error messages to enhance clarity:
- Line 29: "only support LST" → "only LST tokens are supported"
- Line 30: "the token is not registered before" → "the token is not registered"
Apply this diff to update the error messages:
- require(bytes32(assetsAddress) != bytes32(bytes20(VIRTUAL_STAKED_ETH_ADDRESS)), "only support LST"); + require(bytes32(assetsAddress) != bytes32(bytes20(VIRTUAL_STAKED_ETH_ADDRESS)), "only LST tokens are supported"); - require(isRegisteredToken[clientChainLzId][assetsAddress], "the token is not registered before"); + require(isRegisteredToken[clientChainLzId][assetsAddress], "the token is not registered");
51-73: Add validation foropAmountinwithdrawLSTTo prevent zero or negative withdrawal amounts, add a check to ensure
opAmountis greater than zero.Apply this diff to include the validation:
function withdrawLST( uint32 clientChainLzId, bytes calldata assetsAddress, bytes calldata withdrawer, uint256 opAmount ) external returns (bool success, uint256 latestAssetState) { require(assetsAddress.length == 32, "invalid asset address"); require(withdrawer.length == 32, "invalid staker address"); + require(opAmount > 0, "opAmount must be greater than zero"); if (bytes32(assetsAddress) == bytes32(bytes20(VIRTUAL_STAKED_ETH_ADDRESS))) { return (false, 0); } if (!isRegisteredToken[clientChainLzId][assetsAddress]) { return (false, 0); } if (opAmount > principalBalances[clientChainLzId][assetsAddress][withdrawer]) { return (false, 0); } principalBalances[clientChainLzId][assetsAddress][withdrawer] -= opAmount; return (true, principalBalances[clientChainLzId][assetsAddress][withdrawer]); }src/libraries/Errors.sol (1)
245-247: Parameter Naming: Renameselector_toselectorIn the error
PrecompileCallFailed(bytes4 selector_, bytes reason);, the parameter nameselector_ends with an underscore. Unless there is a naming conflict, consider renaming it toselectorfor consistency with other parameter names.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (43)
- script/11_SetPeers.s.sol (2 hunks)
- script/13_DepositValidator.s.sol (2 hunks)
- script/3_Setup.s.sol (2 hunks)
- script/4_Deposit.s.sol (2 hunks)
- script/5_Withdraw.s.sol (2 hunks)
- script/TestPrecompileErrorFixed.s.sol (3 hunks)
- script/TestPrecompileErrorFixed_Deploy.s.sol (1 hunks)
- src/core/BaseRestakingController.sol (4 hunks)
- src/core/Bootstrap.sol (4 hunks)
- src/core/BootstrapLzReceiver.sol (3 hunks)
- src/core/ClientChainGateway.sol (2 hunks)
- src/core/ClientGatewayLzReceiver.sol (5 hunks)
- src/core/ExoCapsule.sol (0 hunks)
- src/core/ExocoreGateway.sol (5 hunks)
- src/core/LSTRestakingController.sol (5 hunks)
- src/core/NativeRestakingController.sol (4 hunks)
- src/core/Vault.sol (0 hunks)
- src/interfaces/IExoCapsule.sol (0 hunks)
- src/interfaces/IVault.sol (0 hunks)
- src/interfaces/precompiles/IAssets.sol (2 hunks)
- src/interfaces/precompiles/IClaimReward.sol (1 hunks)
- src/interfaces/precompiles/IDelegation.sol (4 hunks)
- src/libraries/ActionAttributes.sol (1 hunks)
- src/libraries/Errors.sol (6 hunks)
- src/storage/BootstrapStorage.sol (3 hunks)
- src/storage/ClientChainGatewayStorage.sol (3 hunks)
- src/storage/ExoCapsuleStorage.sol (0 hunks)
- src/storage/ExocoreGatewayStorage.sol (2 hunks)
- src/storage/GatewayStorage.sol (2 hunks)
- src/storage/VaultStorage.sol (0 hunks)
- test/foundry/Delegation.t.sol (8 hunks)
- test/foundry/DepositThenDelegateTo.t.sol (9 hunks)
- test/foundry/DepositWithdrawPrinciple.t.sol (27 hunks)
- test/foundry/ExocoreDeployer.t.sol (7 hunks)
- test/foundry/WithdrawReward.t.sol (7 hunks)
- test/foundry/unit/Bootstrap.t.sol (2 hunks)
- test/foundry/unit/ClientChainGateway.t.sol (6 hunks)
- test/foundry/unit/ExocoreGateway.t.sol (13 hunks)
- test/mocks/AssetsMock.sol (2 hunks)
- test/mocks/ClaimRewardMock.sol (1 hunks)
- test/mocks/DelegationMock.sol (5 hunks)
- test/mocks/ExocoreGatewayMock.sol (10 hunks)
- test/mocks/PrecompileCallerMock.sol (1 hunks)
Files not reviewed due to no reviewable changes (6)
- src/core/ExoCapsule.sol
- src/core/Vault.sol
- src/interfaces/IExoCapsule.sol
- src/interfaces/IVault.sol
- src/storage/ExoCapsuleStorage.sol
- src/storage/VaultStorage.sol
Files skipped from review due to trivial changes (1)
- src/interfaces/precompiles/IClaimReward.sol
Additional comments not posted (150)
test/mocks/ClaimRewardMock.sol (1)
7-12: LGTM! The parameter type change is a good optimization.Changing the parameter types from
bytes memorytobytes calldataforassetsAddressandwithdraweris a gas optimization technique in Solidity.
calldataallows read-only function parameters to be passed by reference directly from the call data, which is cheaper than copying them to memory. This can lead to gas savings when calling the function, especially for larger byte arrays.Since the function logic and behavior remain unchanged, with the same validation checks and return values, this change has no negative impact and can be safely approved.
test/mocks/PrecompileCallerMock.sol (1)
15-15: Verify the existence and signature of thedepositLSTfunction.This change in the mock contract suggests that the
ASSETS_CONTRACTnow has adepositLSTfunction instead ofdepositTo. Please ensure that:
- The
depositLSTfunction exists in theASSETS_CONTRACTinterface (IAssets).- The parameters passed to
depositLSTin this mock are correct and match its signature.Run the following script to verify the existence and signature of
depositLST:Verification successful
Verification successful:
depositLSTfunction exists in IAssets interfaceThe
depositLSTfunction has been confirmed to exist in theIAssetsinterface (src/interfaces/precompiles/IAssets.sol) and is correctly used in the mock contract (test/mocks/PrecompileCallerMock.sol). This verifies that the change in the mock contract reflects a valid function in the interface.However, please manually verify:
- The parameters of the
depositLSTfunction insrc/interfaces/precompiles/IAssets.solmatch those used intest/mocks/PrecompileCallerMock.sol.- The implementation in
test/mocks/AssetsMock.solis consistent with the interface and mock usage.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and signature of the `depositLST` function in the `IAssets` interface. # Test: Search for the function declaration. Expect: A match with the correct signature. rg --type solidity -A 5 $'function\s+depositLST\s*\(' # Test: Search for the function selector. Expect: A match in the mock contract. rg --type solidity -A 5 $'ASSETS_CONTRACT\.depositLST\.selector'Length of output: 186
Script:
#!/bin/bash # Description: Verify the existence and signature of the `depositLST` function and its usage in the mock contract. # Find Solidity files and search for the depositLST function declaration echo "Searching for depositLST function declaration:" fd -e sol | xargs rg -n "function\s+depositLST\s*\(" # Search for the usage of depositLST selector in the mock contract echo -e "\nSearching for depositLST selector usage in mock contract:" rg -n "ASSETS_CONTRACT\.depositLST\.selector" test/mocks/PrecompileCallerMock.sol # Search for IAssets interface definition echo -e "\nSearching for IAssets interface definition:" fd -e sol | xargs rg -n "interface\s+IAssets"Length of output: 762
script/11_SetPeers.s.sol (2)
5-5: LGTM!The import statement change is consistent with the list of alterations and improves code readability by directly importing the
Actionenum.
54-54: Verify thenativeFeecalculation change.The change in the argument passed to
abi.encodePackedis consistent with the import statement change and looks good.However, the
nativeFeecalculation change is not mentioned in the list of alterations or PR objectives. Please verify if this change aligns with the PR objectives or linked issues and explain its implications on the transaction cost for marking a bootstrap.src/core/BootstrapLzReceiver.sol (3)
29-29: LGTM!The changes to use the
Errorslibrary for handlingUnexpectedSourceChainandUnsupportedRequesterrors are good improvements. They promote consistency, maintainability, and clarity in the error handling logic of the contract.Also applies to: 38-38
42-42: Looks good!Updating the revert statement to use the
Errorslibrary for handlingRequestOrResponseExecuteFailederror is a positive change. It aligns with the goal of using theErrorslibrary for structured and consistent error handling throughout the contract.
8-8: Verify the usage of the importedActiontype.The
Actiontype is imported fromGatewayStorage.sol, but its usage remains unchanged in the provided code. Please ensure that the imported type is actually used in the contract. If not, consider removing the import to keep the code clean and maintainable.Run the following script to verify the usage of
Action:Verification successful
Verified: The
Actionimport is correctly used in the contract.The
Actiontype, imported fromGatewayStorage.sol, is indeed used in theBootstrapLzReceiver.solcontract. It's utilized to create anActioninstance from the payload and then compared against a specific action (Action.RESPOND). This usage justifies the import statement, and no changes are necessary.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the imported `Action` type in the contract. # Test: Search for the usage of `Action` in the contract. Expect: At least one occurrence. rg --type solidity -w $'Action' $(fd --type f --extension sol --regex '.*BootstrapLzReceiver\.sol')Length of output: 164
Script:
#!/bin/bash # Description: Verify the usage of the imported `Action` type in the BootstrapLzReceiver.sol contract. # Find the exact path of the BootstrapLzReceiver.sol file file_path=$(fd --type f --extension sol --regex '.*BootstrapLzReceiver\.sol') # Search for 'Action' in the file with 2 lines of context before and after rg -C 2 '\bAction\b' "$file_path" # If no results, check if the file exists and show its content if [ $? -ne 0 ]; then echo "File content:" cat "$file_path" fiLength of output: 747
script/TestPrecompileErrorFixed_Deploy.s.sol (1)
8-8: Verify the necessity of importingAction.The import statement has been modified to include
Actionalong withGatewayStorage. However, there doesn't seem to be any direct usage ofActionin the rest of the script.Please ensure that
Actionis actually required for the script's functionality. If it's not being used, consider removing the import to maintain code clarity and avoid unnecessary dependencies.To verify the usage of
Action, run the following script:Verification successful
Keep the
Actionimport for consistency and potential use.While the
Actionenum isn't directly used in the visible part of the TestPrecompileErrorFixed_Deploy.s.sol script, its import is still valid for the following reasons:
Actionis part of theGatewayStorageimport, so it's indirectly imported anyway.Actionis widely used across the project, suggesting it might be needed for type consistency or in inherited contracts.- The import might be anticipating future use in this deployment script.
You can keep the import as is for consistency with other files and potential future use. However, if you prefer to minimize imports, you could remove
Actionfrom the explicit import since it's already included viaGatewayStorage.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if `Action` is used in the script. # Test: Search for the usage of `Action`. Expect: No occurrences. rg --type solidity -w Action script/TestPrecompileErrorFixed_Deploy.s.solLength of output: 108
Script:
#!/bin/bash # Description: Verify file existence, content, and usage of 'Action' # Test 1: Check if the file exists and show its content echo "File content:" cat script/TestPrecompileErrorFixed_Deploy.s.sol # Test 2: Search for 'Action' in the specific file echo -e "\nSearching for 'Action' in TestPrecompileErrorFixed_Deploy.s.sol:" grep -n "Action" script/TestPrecompileErrorFixed_Deploy.s.sol # Test 3: Search for 'Action' in all .sol files echo -e "\nSearching for 'Action' in all .sol files:" find . -name "*.sol" -type f | xargs grep -n "Action"Length of output: 30927
src/libraries/ActionAttributes.sol (7)
1-4: LGTM!The SPDX license identifier, Solidity version pragma, and import statement are correctly specified.
6-23: LGTM!The
ActionAttributeslibrary is correctly defined with appropriate constants for message lengths, operation type bitmaps, and message length masking/shifting.
24-57: LGTM!The
getAttributesfunction is well-structured and correctly determines the attributes and message length based on the inputAction. The bitwise operations used to combine the attributes and message length are appropriate.
59-61: LGTM!The
isLSTfunction correctly checks if the given action is associated with LST by callinggetAttributesand using a bitwise AND operation.
63-65: LGTM!The
isNSTfunction correctly checks if the given action is associated with NST by callinggetAttributesand using a bitwise AND operation.
67-81: LGTM!The
isWithdrawal,isPrincipal,isReward, andgetMessageLengthfunctions are correctly implemented. They callgetAttributesand use appropriate bitwise operations to check the corresponding attributes or extract the message length.
83-83: LGTM!The closing brace is correctly placed to end the
ActionAttributeslibrary definition.src/interfaces/precompiles/IDelegation.sol (4)
28-33: LGTM!The function renaming and parameter type changes are good improvements:
- The new function name
delegateis more concise and clear.- Using
calldatainstead ofmemoryfor read-only function parameters is more gas-efficient.These changes enhance the code quality without altering the core functionality.
49-54: LGTM!The function renaming and parameter type changes are consistent with the improvements made to the
delegatefunction:
- The new function name
undelegateis more concise and clear.- Using
calldatainstead ofmemoryfor read-only function parameters is more gas-efficient.These changes enhance the code quality and maintain consistency within the interface.
66-66: LGTM!The parameter type changes in the
associateOperatorWithStakerfunction are a good improvement:
- Using
calldatainstead ofmemoryfor read-only function parameters is more gas-efficient.This change enhances the code quality without altering the core functionality.
76-78: LGTM!The parameter type change in the
dissociateOperatorFromStakerfunction is a good improvement:
- Using
calldatainstead ofmemoryfor read-only function parameters is more gas-efficient.This change enhances the code quality and maintains consistency with the other function parameter type changes in the interface.
src/core/LSTRestakingController.sol (4)
40-41: LGTM!The changes to the
_processRequestcall in thedepositfunction are appropriate:
- Using
Action.REQUEST_DEPOSIT_LSTcorrectly indicates that the deposit is a must-succeed action.- Passing an empty bytes argument is suitable since no response is expected for a must-succeed action.
61-62: LGTM!The changes to the
_processRequestcall in thewithdrawPrincipalFromExocorefunction are appropriate:
- Using
Action.REQUEST_WITHDRAW_LSTcorrectly indicates a withdrawal of LST tokens.- The comment rightly emphasizes the importance of checking the response to unlock the principal for later claims.
77-78: LGTM!The changes to the
_processRequestcall in thewithdrawRewardFromExocorefunction are appropriate:
- Using
Action.REQUEST_CLAIM_REWARDcorrectly indicates a reward claim operation.- The comment rightly emphasizes the importance of checking the response to unlock the reward for later claims.
98-100: LGTM!The changes to the
_processRequestcall in thedepositThenDelegateTofunction are appropriate:
- Retaining the
Action.REQUEST_DEPOSIT_THEN_DELEGATE_TOaction type correctly indicates a combined deposit and delegation operation.- Passing an empty bytes argument is suitable since no response is expected for must-succeed actions.
- The comment aptly justifies not checking the response, as both deposit and delegation are treated as must-succeed actions.
script/5_Withdraw.s.sol (2)
8-8: LGTM!The import change is a good refactoring that improves code readability and maintainability by directly importing the
Actionenum fromGatewayStorage. This change aligns with the PR objective of separating NST and LST messages during deposit and withdrawal operations.
60-60: Verify the usage of the new action across the codebase.The change from
GatewayStorage.Action.REQUEST_WITHDRAW_PRINCIPAL_FROM_EXOCOREtoAction.REQUEST_WITHDRAW_LSTaligns with the PR objective of separating NST and LST messages during deposit and withdrawal operations. This change indicates a focus on a different withdrawal mechanism, which may have implications for the overall functionality and flow of the contract.Please ensure that the new withdrawal mechanism is properly implemented and thoroughly tested to avoid any potential issues or bugs.
Run the following script to verify the usage of the new action across the codebase:
Verification successful
Verification successful: New action consistently implemented across the codebase
The change from
GatewayStorage.Action.REQUEST_WITHDRAW_PRINCIPAL_FROM_EXOCOREtoAction.REQUEST_WITHDRAW_LSThas been successfully implemented across the codebase. The new action is consistently used in various parts of the project, including:
- Scripts (e.g., script/5_Withdraw.s.sol)
- Core contracts (e.g., src/core/ExocoreGateway.sol, src/core/LSTRestakingController.sol)
- Libraries (e.g., src/libraries/ActionAttributes.sol)
- Test files (e.g., test/foundry/DepositWithdrawPrinciple.t.sol)
- Mock contracts (e.g., test/mocks/ExocoreGatewayMock.sol)
This change aligns with the PR objective of separating NST and LST messages during deposit and withdrawal operations. The consistent implementation suggests that the new withdrawal mechanism for LST has been properly integrated into the system.
To ensure the reliability of this change, please make sure to thoroughly test all affected components, particularly focusing on the LST withdrawal process and its interactions with other parts of the system.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new action `Action.REQUEST_WITHDRAW_LST` across the codebase. # Test 1: Search for the usage of the old action. Expect: No occurrences. rg --type solidity $'GatewayStorage\.Action\.REQUEST_WITHDRAW_PRINCIPAL_FROM_EXOCORE' # Test 2: Search for the usage of the new action. Expect: Only valid occurrences. rg --type solidity -A 5 $'Action\.REQUEST_WITHDRAW_LST'Length of output: 209
Script:
#!/bin/bash # Description: Verify the usage of the new action `Action.REQUEST_WITHDRAW_LST` across the codebase. # Find all Solidity files solidity_files=$(fd -e sol) # Test 1: Search for the usage of the old action. Expect: No occurrences. echo "Searching for old action:" echo "$solidity_files" | xargs grep -n "GatewayStorage\.Action\.REQUEST_WITHDRAW_PRINCIPAL_FROM_EXOCORE" || echo "No occurrences found (as expected)" # Test 2: Search for the usage of the new action. Expect: Only valid occurrences. echo -e "\nSearching for new action:" echo "$solidity_files" | xargs grep -n -A 5 "Action\.REQUEST_WITHDRAW_LST"Length of output: 19958
script/4_Deposit.s.sol (2)
60-60: Verify the handling logic for theREQUEST_DEPOSIT_LSTaction.The change from
REQUEST_DEPOSITtoREQUEST_DEPOSIT_LSTaligns with the PR objective of separating NST and LST deposit messages.Please ensure that the
REQUEST_DEPOSIT_LSTaction is correctly handled in theClientChainGatewayandExocoreGatewaycontracts.Run the following script to verify the action handling:
#!/bin/bash # Description: Verify the handling logic for the `REQUEST_DEPOSIT_LST` action. # Test 1: Search for the action handling in the `ClientChainGateway` contract. Expect: A `case` statement for `Action.REQUEST_DEPOSIT_LST`. rg --type solidity -A 10 $'case Action.REQUEST_DEPOSIT_LST' ./src/ClientChainGateway.sol # Test 2: Search for the action handling in the `ExocoreGateway` contract. Expect: A `case` statement for `Action.REQUEST_DEPOSIT_LST`. rg --type solidity -A 10 $'case Action.REQUEST_DEPOSIT_LST' ./src/ExocoreGateway.sol
7-7: Verify theActionenum definition.The import statement change looks good. It improves code readability by directly importing the
Actionenum.Please ensure that the
Actionenum is correctly defined in theGatewayStoragecontract.Run the following script to verify the enum definition:
Verification successful
Verification successful:
Actionenum is correctly defined.The
Actionenum is properly defined insrc/storage/GatewayStorage.solwith the expected values, includingREQUEST_DEPOSIT_LST. The import statement inscript/4_Deposit.s.solis correct and consistent with its usage across the project.
- The
Actionenum is defined insrc/storage/GatewayStorage.sol(line 7)- It includes the values: REQUEST_DEPOSIT_LST, REQUEST_DEPOSIT_NST, REQUEST_WITHDRAW_LST, REQUEST_WITHDRAW_NST, REQUEST_CLAIM_REWARD
- The import statement is used consistently in multiple files throughout the project
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the `Action` enum definition in the `GatewayStorage` contract. # Test: Search for the enum definition. Expect: The `Action` enum with `REQUEST_DEPOSIT_LST` value. rg --type solidity -A 5 $'enum Action' ./src/storage/GatewayStorage.solLength of output: 241
Script:
#!/bin/bash # Description: Verify the `Action` enum definition in Solidity files # Search for Solidity files and grep for the Action enum definition fd -e sol | xargs grep -n -A 5 "enum Action" # Search for usage of Action enum in case statements fd -e sol | xargs grep -n "case Action\." # Search for import statements related to Action fd -e sol | xargs grep -n "import.*Action"Length of output: 3532
test/mocks/DelegationMock.sol (6)
30-35: LGTM!The function renaming and parameter type changes improve readability and efficiency. The changes align with the PR objective of separating NST and LST messages during deposit and withdrawal operations.
52-57: LGTM!Similar to the
delegatefunction, the renaming and parameter type changes in theundelegatefunction improve readability and efficiency. The changes align with the PR objective.
77-77: LGTM!The parameter type changes in the
associateOperatorWithStakerfunction improve efficiency by usingcalldatainstead ofmemoryfor external function calls.
92-95: LGTM!The parameter type change in the
dissociateOperatorFromStakerfunction improves efficiency by usingcalldatainstead ofmemoryfor thestakerparameter in external function calls.
108-108: LGTM!The parameter type change in the
getDelegateAmountfunction improves efficiency by usingcalldatainstead ofmemoryfor theoperatorparameter.
116-116: LGTM!The parameter type change in the
getAssociatedOperatorfunction improves efficiency by usingcalldatainstead ofmemoryfor thestakerparameter in external function calls.src/core/BaseRestakingController.sol (4)
9-9: LGTM!The import statement is correct and necessary for using the
Actionenum within the contract.
44-46: LGTM!The code segment correctly handles the withdrawal of the native staking token (NST) from the associated
ExoCapsulecontract. It uses theVIRTUAL_NST_ADDRESSconstant to identify the NST token and calls thewithdrawfunction on the retrievedExoCapsulecontract instance to transfer the specifiedamountto therecipient.
67-67: LGTM!The code segments correctly call the
_processRequestfunction to process the delegation and undelegation requests. TheactionArgsparameter is constructed by encoding the necessary values, and theencodedRequestis passed as an emptybytesarray.Passing an empty
encodedRequestsuggests that these requests do not expect a response that needs to be cached. This simplifies the request processing logic and reduces the overhead of managing request states.Also applies to: 82-82
86-96: LGTM!The code segment correctly implements the conditional registration of requests based on the length of the
encodedRequest. If theencodedRequestis not empty, it means the request expects a response and needs to be cached.The
_registeredRequestsand_registeredRequestActionsmappings are used to store theencodedRequestand correspondingactionassociated with therequestNonce. This allows the contract to track and manage requests that expect a response.The conditional registration provides flexibility in handling different types of requests and optimizes the storage usage by only registering requests that require caching.
script/TestPrecompileErrorFixed.s.sol (3)
11-11: LGTM!The change to the import statement is correct and allows for more concise references to action types within the contract.
97-97: Verify the action type change in the codebase.The change from
GatewayStorage.Action.REQUEST_WITHDRAW_PRINCIPAL_FROM_EXOCOREtoAction.REQUEST_WITHDRAW_LSTis correct. However, ensure that all references to this action have been updated to match the new type.Run the following script to verify the action usage:
Verification successful
Action type change successfully verified across the codebase
The change from
GatewayStorage.Action.REQUEST_WITHDRAW_PRINCIPAL_FROM_EXOCOREtoAction.REQUEST_WITHDRAW_LSThas been consistently implemented throughout the codebase. No instances of the old action type were found, and the new action type is used appropriately in various contexts, including:
- Function selector mappings
- Message processing
- Event emissions
- Test cases and mock implementations
This verification confirms that the action type change has been applied thoroughly and consistently.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to `GatewayStorage.Action.REQUEST_WITHDRAW_PRINCIPAL_FROM_EXOCORE` have been updated to `Action.REQUEST_WITHDRAW_LST`. # Test: Search for the old action usage. Expect: No occurrences. rg --type solidity $'GatewayStorage\.Action\.REQUEST_WITHDRAW_PRINCIPAL_FROM_EXOCORE' # Test: Search for the new action usage. Expect: Only occurrences of the new type. rg --type solidity -A 5 $'Action\.REQUEST_WITHDRAW_LST'Length of output: 209
Script:
#!/bin/bash # Description: Verify all references to `GatewayStorage.Action.REQUEST_WITHDRAW_PRINCIPAL_FROM_EXOCORE` have been updated to `Action.REQUEST_WITHDRAW_LST`. # Test: Search for the old action usage. Expect: No occurrences. rg -g '*.sol' 'GatewayStorage\.Action\.REQUEST_WITHDRAW_PRINCIPAL_FROM_EXOCORE' # Test: Search for the new action usage. Expect: Only occurrences of the new type. rg -g '*.sol' -A 5 'Action\.REQUEST_WITHDRAW_LST'Length of output: 10151
76-76: Verify the action type change in the codebase.The change from
GatewayStorage.Action.REQUEST_DEPOSITtoAction.REQUEST_DEPOSIT_LSTis correct. However, ensure that all references to this action have been updated to match the new type.Run the following script to verify the action usage:
Verification successful
Verification of action type change completed successfully.
The change from
GatewayStorage.Action.REQUEST_DEPOSITtoAction.REQUEST_DEPOSIT_LSThas been correctly implemented and is consistent throughout the codebase. Key findings:
- The old action type
GatewayStorage.Action.REQUEST_DEPOSITis no longer present in any Solidity files.- The new action type
Action.REQUEST_DEPOSIT_LSTis used consistently in relevant parts of the codebase, including tests, core logic, and deployment scripts.- Related action types like
REQUEST_DEPOSIT_NSTandREQUEST_DEPOSIT_THEN_DELEGATE_TOare also present, indicating a comprehensive update to the action system.The change appears to be part of a broader refactoring or feature update, and has been implemented thoroughly across the project.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to `GatewayStorage.Action.REQUEST_DEPOSIT` have been updated to `Action.REQUEST_DEPOSIT_LST`. # Test: Search for the old action usage. Expect: No occurrences. rg --type solidity $'GatewayStorage\.Action\.REQUEST_DEPOSIT' # Test: Search for the new action usage. Expect: Only occurrences of the new type. rg --type solidity -A 5 $'Action\.REQUEST_DEPOSIT_LST'Length of output: 184
Script:
#!/bin/bash # Description: Verify all references to `GatewayStorage.Action.REQUEST_DEPOSIT` have been updated to `Action.REQUEST_DEPOSIT_LST`. # Find all Solidity files echo "Solidity files in the repository:" fd -e sol # Search for the old action usage. Expect: No occurrences. echo -e "\nSearching for old action usage (GatewayStorage.Action.REQUEST_DEPOSIT):" fd -e sol | xargs rg 'GatewayStorage\.Action\.REQUEST_DEPOSIT' # Search for the new action usage. Expect: Only occurrences of the new type. echo -e "\nSearching for new action usage (Action.REQUEST_DEPOSIT_LST):" fd -e sol | xargs rg -A 5 'Action\.REQUEST_DEPOSIT_LST' # Search for any remaining references to REQUEST_DEPOSIT echo -e "\nSearching for any remaining references to REQUEST_DEPOSIT:" fd -e sol | xargs rg 'REQUEST_DEPOSIT'Length of output: 18295
test/foundry/WithdrawReward.t.sol (10)
4-4: LGTM!The import statement is correct and the imported types are likely used in the test contract.
16-16: LGTM!The
ClaimRewardResultevent declaration is correct and consistent with the AI-generated summary.
39-39: LGTM!The usage of the
Action.REQUEST_CLAIM_REWARDconstant is correct and consistent with the AI-generated summary.
57-57: LGTM!The
MessageSentevent expectation is correctly updated to use theAction.REQUEST_CLAIM_REWARDconstant, which is consistent with the AI-generated summary.
67-75: LGTM!The
withdrawResponsePayloadconstruction and theClaimRewardResultevent expectation are correctly updated to use theAction.RESPONDconstant and the new event name and parameters, which is consistent with the AI-generated summary.
87-87: LGTM!The
MessageSentevent expectation is correctly updated to use theAction.RESPONDconstant, which is consistent with the AI-generated summary.
90-90: LGTM!The
MessageExecutedevent expectation is correctly updated to use theAction.REQUEST_CLAIM_REWARDconstant, which is consistent with the AI-generated summary.
107-107: LGTM!The
ResponseProcessedevent expectation is correctly updated to use theAction.REQUEST_CLAIM_REWARDconstant, which is consistent with the AI-generated summary.
110-110: LGTM!The
MessageExecutedevent expectation is correctly updated to use theAction.RESPONDconstant, which is consistent with the AI-generated summary.
1-3: Skipped reviewing the remaining code.The remaining code in the file does not have any annotated changes, so it can be assumed to be unchanged. Reviewing unchanged code is not necessary for this review.
Also applies to: 5-15, 17-38, 40-56, 58-66, 76-86, 88-89, 91-106, 108-109, 111-123
script/13_DepositValidator.s.sol (2)
8-8: LGTM!The change to use destructured import syntax for importing the
Actionenum along withGatewayStorageis a good improvement. It allows direct usage of theActionenum in the code, which improves readability and suggests that the enum will be used for deposit-related actions.
77-77: Excellent change to align with the PR objective!The modification of the first argument in the
msg_construction fromGatewayStorage.Action.REQUEST_DEPOSITtoAction.REQUEST_DEPOSIT_LSTis a great change that aligns perfectly with the PR objective of separating NST (Native Staking Token) and LST (Liquid Staking Token) deposit/withdrawal messages.By using
Action.REQUEST_DEPOSIT_LST, it clearly indicates that this deposit action is specifically for LST, helping to distinguish it from NST deposit actions. This change enhances the clarity and maintainability of the code.src/storage/ClientChainGatewayStorage.sol (3)
6-7: LGTM!The import statement for the
Errorslibrary is syntactically correct and the relative import path appears to be valid.
79-83: LGTM!The
ResponseProcessedevent declaration is syntactically correct and the indexed parameters provide clear semantics about the purpose of the event. The change fromRequestFinishedtoResponseProcessedbetter reflects the event's purpose without altering the contract's functionality or event consumption.
116-116: LGTM!The reference to the
Errorslibrary'sCapsuleNotExist()error is correct and consistent with the previous custom error. Centralizing error definitions in theErrorslibrary can improve maintainability and consistency across the contract's error handling without causing any issues in the contract's functionality.src/interfaces/precompiles/IAssets.sol (4)
26-31: LGTM!The changes to the
depositLSTfunction improve clarity and gas efficiency:
- The function name clearly indicates that it is for depositing LSTs.
- Using
calldataforassetsAddressandstakerAddressparameters is more gas-efficient compared tomemory.The functionality remains the same, with the changes enhancing the code quality.
43-48: New function for depositing NSTs looks good!The addition of the
depositNSTfunction enhances the contract's capability to handle different asset types:
- The function is specifically designed for depositing NSTs, complementing the existing
depositLSTfunction.- The inclusion of
validatorPubkeyas a parameter indicates that this function is tailored for handling NSTs.- The use of
calldatafor parameters follows best practices for gas optimization.The new function is a valuable addition to the contract's functionality.
58-63: LGTM!The changes to the
withdrawLSTfunction improve clarity and gas efficiency:
- The function name clearly indicates that it is for withdrawing LSTs.
- Using
calldataforassetsAddressandwithdrawAddressparameters is more gas-efficient compared tomemory.The functionality remains the same, with the changes enhancing the code quality.
73-78: New function for withdrawing NSTs looks good!The addition of the
withdrawNSTfunction enhances the contract's capability to handle different asset types:
- The function is specifically designed for withdrawing NSTs, complementing the existing
withdrawLSTfunction.- The inclusion of
validatorPubkeyas a parameter indicates that this function is tailored for handling NSTs.- The use of
calldatafor parameters follows best practices for gas optimization.The new function is a valuable addition to the contract's functionality.
src/core/ClientChainGateway.sol (1)
132-132: The change looks good, but please clarify the broader context and implications.The conditional check has been updated from
VIRTUAL_STAKED_ETH_ADDRESStoVIRTUAL_NST_ADDRESS, which aligns with the error messageNoTvlLimitForNativeRestaking. This suggests a shift in the token being referenced for the TVL limit logic, potentially impacting how the contract handles native restaking.To better understand the full impact of this change, please provide more context on:
- The difference between
VIRTUAL_STAKED_ETH_ADDRESSandVIRTUAL_NST_ADDRESS.- How this change affects the contract's handling of native restaking.
Also, consider adding comments to explain the purpose and implications of this change for future maintainers.
To verify the usage of
VIRTUAL_NST_ADDRESS, run the following script:Verification successful
The change from VIRTUAL_STAKED_ETH_ADDRESS to VIRTUAL_NST_ADDRESS is correct and consistent.
The update to use
VIRTUAL_NST_ADDRESSinstead ofVIRTUAL_STAKED_ETH_ADDRESSin the core contracts is part of a broader refactoring to support native staking scenarios. The functionality associated with this constant remains the same, used to identify and handle native staked ETH differently from other tokens.However, for improved consistency and maintainability:
- Consider updating remaining instances of
VIRTUAL_STAKED_ETH_ADDRESSin test files and scripts to useVIRTUAL_NST_ADDRESS.- Add a comment explaining the purpose of
VIRTUAL_NST_ADDRESSwhere it's defined, for better code readability.Example comment:
// VIRTUAL_NST_ADDRESS represents the special address for native staked ETH address internal constant VIRTUAL_NST_ADDRESS = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `VIRTUAL_NST_ADDRESS` in the codebase. # Test: Search for the usage of `VIRTUAL_NST_ADDRESS`. Expect: Relevant usage in the codebase. rg --type solidity -A 5 $'VIRTUAL_NST_ADDRESS'Length of output: 78
Script:
#!/bin/bash # Description: Search for VIRTUAL_NST_ADDRESS and VIRTUAL_STAKED_ETH_ADDRESS in Solidity files # Search for VIRTUAL_NST_ADDRESS echo "Searching for VIRTUAL_NST_ADDRESS:" fd -e sol | xargs grep -n -C 3 "VIRTUAL_NST_ADDRESS" echo -e "\n------------------------\n" # Search for VIRTUAL_STAKED_ETH_ADDRESS echo "Searching for VIRTUAL_STAKED_ETH_ADDRESS:" fd -e sol | xargs grep -n -C 3 "VIRTUAL_STAKED_ETH_ADDRESS"Length of output: 33162
script/3_Setup.s.sol (2)
3-3: LGTM!The import statement is updated correctly to align with the addition of
enum ActioninGatewayStorage. This change enhances readability and maintainability by directly importing the required entities.
124-124: LGTM!The usage of
Action.REQUEST_ADD_WHITELIST_TOKENis updated correctly to align with the updated import statement. This change enhances readability and maintainability by directly using the importedActionenum, while preserving the functionality of quoting the native fee for adding whitelist tokens.src/core/NativeRestakingController.sol (3)
36-36: LGTM!The change from
VIRTUAL_STAKED_ETH_ADDRESStoVIRTUAL_NST_ADDRESSaligns with the new token model for restaking operations as mentioned in the PR summary. The code change looks good.
105-109: Looks good!The code changes align with the PR objectives:
- Including the validator's public key in the action arguments facilitates sending it to Exocore for recording purposes.
- Updating the action name to
REQUEST_DEPOSIT_NSTreflects the new token type being used for deposits.- Simplifying the encoded request to an empty byte array indicates that the deposit action is now a must-succeed operation.
The implementation looks correct and the changes make sense in the context of the PR.
128-133: Changes look good!The code changes align with the PR objectives:
- Including the validator's public key in the action arguments facilitates sending it to Exocore for recording purposes.
- Updating the action name to
REQUEST_WITHDRAW_NSTreflects the new token type being used for withdrawals.- Referencing
VIRTUAL_NST_ADDRESSin the encoded request indicates that the withdrawal process has been aligned with the new token model.The implementation looks correct and the changes make sense in the context of the PR.
test/foundry/Delegation.t.sol (12)
6-6: LGTM!The refactoring to import
ActionandGatewayStoragedirectly from theGatewayStoragemodule improves code readability and maintainability.
28-34: LGTM!The new
DelegationRequestevent structure consolidates the success status, token, delegator, operator, and amount into a single event for both delegation and undelegation actions. This change enhances clarity and reduces redundancy in event handling.
Line range hint
95-100: LGTM!The code changes to use the
Actionenum directly are consistent with the refactoring and look good.
116-116: LGTM!The
MessageSentevent emission update to use theAction.REQUEST_DELEGATE_TOenum value is consistent with the refactoring and looks good.
139-146: LGTM!The replacement of the
DelegateResultevent emission with the newDelegationRequestevent emission is consistent with the consolidated event structure and looks good.
149-149: LGTM!The
MessageExecutedevent emission update to use theAction.REQUEST_DELEGATE_TOenum value is consistent with the refactoring and looks good.
Line range hint
180-185: LGTM!The code changes to use the
Actionenum directly are consistent with the refactoring and look good.
201-201: LGTM!The
MessageSentevent emission update to use theAction.REQUEST_UNDELEGATE_FROMenum value is consistent with the refactoring and looks good.
224-231: LGTM!The replacement of the
UndelegateResultevent emission with the newDelegationRequestevent emission is consistent with the consolidated event structure and looks good.
234-234: LGTM!The
MessageExecutedevent emission update to use theAction.REQUEST_UNDELEGATE_FROMenum value is consistent with the refactoring and looks good.
139-146: This code segment has already been reviewed earlier. Please refer to the previous comments.
224-231: This code segment has already been reviewed earlier. Please refer to the previous comments.test/foundry/unit/ClientChainGateway.t.sol (5)
28-28: LGTM!The changes to the import statement and the
MessageSentevent declaration improve modularity and readability by directly using theActionenum instead of referencing it through theGatewayStoragecontract.Also applies to: 71-71
Line range hint
1-1:
Line range hint
1-1:
335-335: LGTM!Referencing the
CapsuleNotExisterror selector from the centralizedErrorslibrary improves maintainability and reduces dependencies on specific storage contracts.
378-378: LGTM!The changes in this contract improve consistency and maintainability by:
- Utilizing the
Actionenum directly in message encoding, reducing dependencies on theGatewayStoragecontract.- Referencing the
VaultNotExisterror selector from the centralizedErrorslibrary, reducing dependencies on specific storage contracts.Also applies to: 387-387, 397-397
src/storage/BootstrapStorage.sol (3)
129-130: LGTM!The introduction of the
VIRTUAL_NST_ADDRESSconstant to represent the virtual address for the native staking token is a clear and concise addition.
324-324: LGTM!Replacing the custom error
VaultNotExistwith the namespaced error referenceErrors.VaultNotExist()improves code organization and maintainability by centralizing error definitions.
338-340: LGTM!The added check to revert if the
underlyingTokenis equal toVIRTUAL_NST_ADDRESSwhen deploying a vault is a good safety measure. It prevents accidentally deploying a vault for the virtual native staking token address, which could lead to unexpected behavior or errors.test/foundry/ExocoreDeployer.t.sol (5)
22-22: LGTM!Importing
Actiondirectly alongsideGatewayStorageis a good way to reduce verbosity when referencingActionlater in the code.
111-115: Approve the event signature changes.The modifications to the
MessageSentandRequestFinishedevents are good changes:
- Directly referencing
Actionin theMessageSentevent is consistent with the updated import statement and enhances clarity.- Renaming
RequestFinishedtoResponseProcessedsuggests a shift in focus from a request-centric to a response-centric model. This indicates a broader conceptual change in how the contract processes messages, possibly indicating a more robust handling of responses.
Line range hint
186-201: LGTM!The changes in the
setUpfunction are good:
- Directly using
Action.REQUEST_ADD_WHITELIST_TOKENin the payload construction enhances readability.- The modifications in the
emitstatements ensure that all references to actions are consistent and clear.
Line range hint
224-236: LGTM!The updated
emitstatements are consistent with the previous changes and ensure clarity and consistency throughout the contract.
469-477: Approve the new utility functions.The addition of the
_addressToBytesand_getPrincipalBalancefunctions is a good enhancement to the contract's functionality:
- These utility methods can be reused in various contexts within the contract, promoting code reusability and maintainability.
- The functions are properly implemented, follow the correct syntax, and are appropriately marked as
internal.test/foundry/unit/ExocoreGateway.t.sol (11)
55-55: LGTM!The
MessageSentevent has been updated to use theActiontype, which is consistent with the goal of streamlining the naming convention across the codebase.
186-186: LGTM!The
AssociateOperatorResultandDissociateOperatorResultevents have been consolidated into a singleAssociationResultevent, which improves code maintainability and enhances semantic clarity with the addition of theisAssociateparameter.
208-208: LGTM!The
REQUEST_WITHDRAW_LSTaction has been updated to use theActiontype, which is consistent with the goal of streamlining the naming convention across the codebase.
224-224: LGTM!The
REQUEST_ASSOCIATE_OPERATORaction has been updated to use theActiontype, and the emitted event has been updated toAssociationResultwith theisAssociateparameter set totrue, which is consistent with the goal of streamlining the naming convention and enhancing semantic clarity across the codebase.Also applies to: 227-227
246-246: LGTM!The
REQUEST_ASSOCIATE_OPERATORaction has been updated to use theActiontype, and the emitted event has been updated toAssociationResultwith theisAssociateparameter set totrue, which is consistent with the goal of streamlining the naming convention and enhancing semantic clarity across the codebase.Also applies to: 249-249
274-274: LGTM!The
REQUEST_ASSOCIATE_OPERATORaction has been updated to use theActiontype, and the emitted event has been updated toAssociationResultwith theisAssociateparameter set totrue, which is consistent with the goal of streamlining the naming convention and enhancing semantic clarity across the codebase.Also applies to: 288-288
311-311: LGTM!The
REQUEST_DISSOCIATE_OPERATORaction has been updated to use theActiontype, and the emitted event has been updated toAssociationResultwith theisAssociateparameter set tofalse, which is consistent with the goal of streamlining the naming convention and enhancing semantic clarity across the codebase.Also applies to: 314-314
330-330: LGTM!The
REQUEST_DISSOCIATE_OPERATORaction has been updated to use theActiontype, and the emitted event has been updated toAssociationResultwith theisAssociateparameter set tofalse, which is consistent with the goal of streamlining the naming convention and enhancing semantic clarity across the codebase.Also applies to: 333-333
575-575: LGTM!The emitted event has been updated to use the
Action.REQUEST_ADD_WHITELIST_TOKENaction, which is consistent with the goal of streamlining the naming convention across the codebase.
615-615: LGTM!The emitted event has been updated to use the
Action.REQUEST_ADD_WHITELIST_TOKENaction, which is consistent with the goal of streamlining the naming convention across the codebase.
773-773: LGTM!The
nativeFeecalculation has been updated to use theAction.REQUEST_MARK_BOOTSTRAPaction, which is consistent with the goal of streamlining the naming convention across the codebase.src/core/Bootstrap.sol (5)
25-25: LGTM!The import statement for the
BootstrapStoragecontract is correctly added.
27-27: LGTM!The import statement for the
Actionenum from theGatewayStoragecontract is correctly added.
218-218: LGTM!The check to skip deploying a vault for the
VIRTUAL_NST_ADDRESStoken is correctly added.
239-239: LGTM!The check to prevent updating the TVL limit for the
VIRTUAL_NST_ADDRESStoken is correctly added.
439-439: LGTM!The revert statement with the
Errors.NotYetSupported()error is correctly added to indicate that thewithdrawRewardFromExocorefunction is not yet supported.test/foundry/DepositWithdrawPrinciple.t.sol (9)
55-61: LGTM!The test function follows a clear and concise testing flow for LST deposit and withdrawal using LayerZero. The use of helper functions improves readability, and the assertions ensure the correctness of the balances at each step.
Also applies to: 66-73
150-150: LGTM!The LST withdrawal test function follows the expected workflow and validates the emitted events correctly. The
lastlyUpdatedPrincipalBalanceis updated to keep track of the principal balance.Also applies to: 169-169, 179-191, 204-206, 208-208, 222-222, 224-224
259-265: LGTM!The test function follows a clear and concise testing flow for native deposit and withdrawal. The use of helper functions improves readability, and the assertions ensure the correctness of the balances at each step.
Also applies to: 284-290
309-309: LGTM!The native deposit test function follows the expected workflow and validates the emitted events correctly. The
lastlyUpdatedPrincipalBalanceis updated to keep track of the principal balance.Also applies to: 325-325, 336-347
Line range hint
362-391: LGTM!The function performs the necessary setup steps before a native deposit, ensuring the block environment states are properly simulated and the capsule is attached to the withdrawal credentials for compatibility.
438-441: LGTM!The native withdrawal test function follows the expected workflow and validates the emitted events correctly. The
lastlyUpdatedPrincipalBalanceis updated to keep track of the principal balance.Also applies to: 458-458, 469-482, 495-495, 499-499, 511-511, 514-514
Line range hint
525-546: LGTM!The function performs the necessary setup steps to simulate the block environment for a native withdrawal, loading the required data from a JSON file and mocking the
beaconOracle.timestampToBlockRootcall.
565-573: LGTM!The test function follows a comprehensive testing flow to validate the deposit TVL limit behavior. It performs a series of deposit and withdrawal operations, checking the consumed TVL and TVL limit values at each step. The test cases cover various scenarios, ensuring the correctness of the TVL limit implementation.
Also applies to: 585-587, 609-615, 620-621, 624-630, 640-640, 677-683, 687-687, 691-697, 708-708, 738-746, 753-755
23-29: LGTM!The new
LSTTransferandNSTTransferevent declarations provide clear and structured information about LST and NST transfers. The inclusion of theisDepositflag allows distinguishing between deposits and withdrawals, and thesuccessflag indicates the status of the transfer operation. The indexedtokenandaccountparameters enable efficient event filtering.test/foundry/unit/Bootstrap.t.sol (13)
Line range hint
1007-1038: Test looks good!The test function
test01_AddWhitelistTokenis well-structured and covers the happy path scenario of adding a new token to the whitelist. It makes appropriate assertions to verify the expected behavior.
Line range hint
1040-1049: Test looks good!The test function
test01_AddWhitelistToken_AlreadyExistscorrectly tests the scenario when trying to add a token that already exists in the whitelist. It expects the transaction to revert with the appropriate error.
Line range hint
1051-1067: Test looks good!The test function
test01_AddWhitelistToken_NoVaultForNativeETHcorrectly verifies that no vault is deployed for the native ETH token when it is added to the whitelist. The test function is well-structured and makes appropriate assertions.
Line range hint
1069-1119: Test looks good!The test function
test02_Depositthoroughly tests the deposit functionality of theBootstrapcontract. It distributes tokens to addresses, makes deposits, and checks the balances, deposit amounts, and depositor counts. The test function is well-structured and covers various scenarios.
Line range hint
1121-1130: Test looks good!The test function
test02_Deposit_WithoutApprovalcorrectly tests the scenario of making a deposit without approving the vault. It expects the transaction to revert, which is the desired behavior.
Line range hint
1132-1141: Test looks good!The test function
test02_Deposit_ZeroBalancecorrectly tests the scenario of making a deposit with zero balance. It expects the transaction to revert, which is the desired behavior.
Line range hint
1143-1160: Test looks good!The test function
test02_Deposit_NonWhitelistedTokencorrectly tests the scenario of making a deposit with a non-whitelisted token. It expects the transaction to revert with the appropriate error message.
Line range hint
1186-1203: Test looks good!The test function
test02_Deposit_MoreThanTvlcorrectly tests the scenario of making a deposit that exceeds the TVL limit. It reduces the TVL limit and expects the transaction to revert with the appropriate error.
Line range hint
1205-1270: Test looks good!The test function
test02_Deposit_ReduceTvlWithdrawthoroughly tests the scenario of reducing the TVL limit and making deposits and withdrawals. It verifies that the TVL limit is enforced correctly and deposits are not allowed when the TVL is reached. The test function covers various cases and makes appropriate assertions.
Line range hint
1272-1301: Test looks good!The test function
test03_RegisterValidatorthoroughly tests the validator registration functionality of theBootstrapcontract. It registers multiple validators and checks the validator count, mappings, and struct values. The test function is well-structured and covers various scenarios.
Line range hint
1303-1317: Test looks good!The test function
test03_RegisterValidator_EthAlreadyRegisteredcorrectly tests the scenario of registering a validator with an Ethereum address that is already registered. It expects the transaction to revert with the appropriate error.
Line range hint
1319-1333: Test looks good!The test function
test03_RegisterValidator_ExoAlreadyRegisteredcorrectly tests the scenario of registering a validator with an Exocore address that is already registered. It expects the transaction to revert with the appropriate error.
Line range hint
1335-1349: Test looks good!The test function
test03_RegisterValidator_ConsensusKeyInUsecorrectly tests the scenario of registering a validator with a consensus key that is already in use. It expects the transaction to revert with the appropriate error.src/storage/GatewayStorage.sol (2)
4-4: Good use of centralized error handling withErrorslibraryImporting the
Errorslibrary promotes consistent and maintainable error management throughout the codebase.
86-86: Use of centralized error handling improves maintainabilityReplacing inline revert statements with
Errors.UnexpectedInboundNonceenhances consistency and readability in error handling.src/storage/ExocoreGatewayStorage.sol (1)
108-126: Message Length Validation Function Implemented CorrectlyThe
_validateMessageLengthfunction effectively validates the message length based on the action type, ensures unsupported actions are handled, and provides specific error messages. The implementation appears correct and robust.test/mocks/AssetsMock.sol (3)
10-10: Declaration ofinValidatorSetmapping is appropriateThe mapping is correctly defined to track whether a staker is part of a validator set.
21-26: FunctiondepositLSTis correctly definedThe function signature and parameters are appropriate for handling LST deposits.
29-29: Verify address comparison logicEnsure that the comparison between
assetsAddressandVIRTUAL_STAKED_ETH_ADDRESSfunctions as intended. The casting and comparison may lead to unexpected results ifassetsAddressis not properly formatted. Consider verifying thatassetsAddressis consistently encoded.To verify the address comparison logic, run the following script:
Verification successful
Address comparison logic is correctly implemented
The address comparison logic in
AssetsMock.solis implemented correctly and consistently. The code ensures that:
assetsAddressis 32 bytes long before comparison.- The comparison uses
bytes32(assetsAddress) != bytes32(bytes20(VIRTUAL_STAKED_ETH_ADDRESS)).- The casting of
VIRTUAL_STAKED_ETH_ADDRESStobytes20and then tobytes32is a standard practice for address comparisons in Solidity.This implementation effectively excludes the virtual staked ETH address from being deposited as a Liquid Staking Token (LST), which aligns with the intended behavior.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the address comparison logic is consistent. # Test: Search for all usages of `assetsAddress` to ensure consistent formatting. rg --type solidity -A 3 -B 3 'assetsAddress'Length of output: 77
Script:
#!/bin/bash # 1. Display the content of AssetsMock.sol echo "Content of AssetsMock.sol:" cat test/mocks/AssetsMock.sol # 2. Search for 'assetsAddress' in all .sol files echo -e "\n\nOccurrences of 'assetsAddress' in .sol files:" rg -t sol 'assetsAddress' -A 3 -B 3 # 3. Look for similar address comparison patterns echo -e "\n\nSimilar address comparison patterns in .sol files:" rg -t sol 'require\(.*==.*\)' -A 3 -B 3Length of output: 6397
src/core/ClientGatewayLzReceiver.sol (2)
89-93: Ensure all action types are handled in_handleResponseThe current implementation checks for
isPrincipal()andisReward(), but if therequestActdoes not match these, it reverts withUnsupportedResponse. Ensure that all valid action types are considered, or validate that only these two are expected.Please confirm that only
isPrincipal()andisReward()action types are valid in this context. If there are additional action types, they should be handled appropriately.
180-182: ValidateADD_TOKEN_WHITELIST_REQUEST_LENGTHcorrectlyThe check for
payload.length != ADD_TOKEN_WHITELIST_REQUEST_LENGTHshould ensure thatADD_TOKEN_WHITELIST_REQUEST_LENGTHis defined and matches the expected payload size.Run the following script to confirm that
ADD_TOKEN_WHITELIST_REQUEST_LENGTHis correctly defined and used consistently:Verification successful
ADD_TOKEN_WHITELIST_REQUEST_LENGTH is correctly defined and used
The constant ADD_TOKEN_WHITELIST_REQUEST_LENGTH is properly defined in src/storage/ClientChainGatewayStorage.sol as 48 bytes, which correctly represents the expected payload structure (bytes32 token + uint128 tvlLimit). It is then appropriately used in src/core/ClientGatewayLzReceiver.sol to validate the payload length before processing the request.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that ADD_TOKEN_WHITELIST_REQUEST_LENGTH matches the expected payload size. # Test: Search for the definition and usage of ADD_TOKEN_WHITELIST_REQUEST_LENGTH. # Expect: Definition and usage should align with the payload structure. rg --type solidity 'ADD_TOKEN_WHITELIST_REQUEST_LENGTH'Length of output: 88
Script:
#!/bin/bash # Search for ADD_TOKEN_WHITELIST_REQUEST_LENGTH definition and usage rg -C 5 'ADD_TOKEN_WHITELIST_REQUEST_LENGTH' # Search for comments or code related to the payload structure rg -C 5 'payload.*length|payload.*structure'Length of output: 110713
src/libraries/Errors.sol (1)
4-4: Import Statement Properly Adds 'Action'The import of
ActionfromGatewayStorage.solis necessary for the newly added error types that use theActionenum.src/core/ExocoreGateway.sol (8)
303-303: Ensure_validateMessageLengthproperly checksmessagelengthThe
_validateMessageLengthfunction should adequately verify thatmessagehas the minimum required length before further processing. This prevents potential out-of-bounds errors when accessingmessage[0]or slicingmessage.
372-373: Ensure payload length is sufficient before slicing inhandleNSTTransferSimilar to
handleLSTTransfer,handleNSTTransferslicespayloadwithout a length check. Verify thatpayload.length >= 96to prevent errors.+ require(payload.length >= 96, "Payload too short in handleNSTTransfer"); bytes calldata validatorPubkey = payload[:32]; bytes calldata staker = payload[32:64];
427-438: Ensure payload length is sufficient before slicing inhandleDelegationBefore slicing
payloadto extracttoken,staker,operator, andamount, check thatpayload.length >= 138to avoid out-of-bounds errors.+ require(payload.length >= 138, "Payload too short in handleDelegation"); // use memory to avoid stack too deep bytes memory token = payload[:32];
453-466: Ensure payload length is sufficient before slicing inhandleDepositAndDelegateVerify that
payload.lengthis adequate before slicing inhandleDepositAndDelegate. This prevents potential errors due to insufficient payload size.+ require(payload.length >= 138, "Payload too short in handleDepositAndDelegate"); // use memory to avoid stack too deep bytes memory token = payload[:32];
480-489: Ensure payload length is sufficient before slicing inhandleOperatorAssociationAdd a check to ensure
payload.lengthis appropriate before slicing inhandleOperatorAssociation, especially when accessingoperatordata.bool success; + require(payload.length >= 32, "Payload too short in handleOperatorAssociation"); bytes calldata staker = payload[:32];
80-89: Verify action mappings in_initializeWhitelistFunctionSelectorsEnsure that each
Actionis correctly mapped to its corresponding handler. This is crucial for the proper routing of actions to their handlers and the overall functionality of the gateway.Run the following script to confirm that all actions have the appropriate mappings:
Verification successful
Action mappings in
_initializeWhitelistFunctionSelectorsare correctThe verification process has confirmed that all Actions are correctly mapped to their corresponding handlers in the
ExocoreGateway.solfile. Each mapping has been checked and found to be accurate, ensuring proper routing of actions to their respective handlers. This verification addresses the concern raised in the original review comment.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all Actions are mapped to function selectors. # Expected Actions and their handlers declare -A expected_mappings=( ["REQUEST_DEPOSIT_LST"]="handleLSTTransfer" ["REQUEST_WITHDRAW_LST"]="handleLSTTransfer" ["REQUEST_DEPOSIT_NST"]="handleNSTTransfer" ["REQUEST_WITHDRAW_NST"]="handleNSTTransfer" ["REQUEST_CLAIM_REWARD"]="handleRewardOperation" ["REQUEST_DELEGATE_TO"]="handleDelegation" ["REQUEST_UNDELEGATE_FROM"]="handleDelegation" ["REQUEST_DEPOSIT_THEN_DELEGATE_TO"]="handleDepositAndDelegate" ["REQUEST_ASSOCIATE_OPERATOR"]="handleOperatorAssociation" ["REQUEST_DISSOCIATE_OPERATOR"]="handleOperatorAssociation" ) for action in "${!expected_mappings[@]}"; do handler=${expected_mappings[$action]} grep_pattern="_whiteListFunctionSelectors\[Action\.$action\] = this\.$handler\.selector;" if ! grep -q "$grep_pattern" src/core/ExocoreGateway.sol; then echo "Mapping for $action to $handler is missing or incorrect." else echo "Mapping for $action to $handler is correct." fi doneLength of output: 5407
289-289: Improve error handling in_registerOrUpdateClientChainThe function currently reverts with a custom error if
successis false but doesn't provide information about the underlying cause. Consider including the failure reason fromASSETS_CONTRACT.registerOrUpdateClientChainto aid in debugging.Update the code to capture and use the failure reason:
(bool success, bool updated) = ASSETS_CONTRACT.registerOrUpdateClientChain( clientChainId, addressLength, name, metaInfo, signatureType ); - if (!success) { + if (!success || !updated) { revert Errors.RegisterClientChainToExocoreFailed(clientChainId); }Likely invalid or redundant comment.
6-6: Remove duplicate import ofErrors.solThe
Errors.sollibrary is imported twice in this contract, which is unnecessary and may cause confusion or compiler warnings. Removing the duplicate import will clean up the code.Apply this diff to fix the issue:
... ---Likely invalid or redundant comment.
test/mocks/ExocoreGatewayMock.sol (5)
81-86: Zero address check ininitializefunction is appropriateThe inclusion of a zero address check for the
owner_parameter in theinitializefunction ensures that the contract owner cannot be set to the zero address, which could lead to loss of control over contract functions.
119-121: Consider restricting access tomarkBootstrapfunctionThe
markBootstrapfunction is publicly accessible, allowing any user to send a bootstrap request to a chain. Confirm if this open accessibility is intended. If not, consider adding access control modifiers such asonlyOwneror a role-based check to restrict this function to authorized entities.Also applies to: 126-127
173-176: Verified client chain ID before setting peerAdding
_validateClientChainIdRegistered(clientChainId)ensures that only registered client chain IDs can have a peer set. This enhances security by preventing unauthorized or unregistered chains from being configured.
312-332: Verify security of dynamic function calls in_lzReceiveThe
_lzReceivefunction usesaddress(this).callto invoke handlers based on function selectors retrieved from_whiteListFunctionSelectors. Ensure that this mechanism is secure against potential exploits, such as unauthorized function calls or reentrancy attacks. All callable functions should be carefully audited to prevent unintended side effects.
400-418: Confirm non-reverting behavior on reward claim failure inhandleRewardOperationIn
handleRewardOperation, the function does not revert ifCLAIM_REWARD_CONTRACT.claimRewardfails; instead, it includes the success status in the response. Verify that this approach aligns with the intended contract logic and that failure scenarios are adequately handled and communicated to the client chain.
|
Does this also close #57 ? |
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (15)
src/libraries/Errors.sol (3)
126-138: Approve new errors with a suggestion for improvementThe new errors
VaultDoesNotExistandRequestOrResponseExecuteFailedare well-defined and provide clear error handling. However, theNotYetSupportederror could benefit from additional context.Consider adding a parameter to
NotYetSupportedto provide more context about which operation is not yet supported. For example:-error NotYetSupported(); +error NotYetSupported(string operation);This would make debugging and error handling more informative.
246-250: Approve DepositRequestShouldNotFail error with a suggestionThe
DepositRequestShouldNotFailerror is well-documented and provides crucial information for debugging failed deposit requests. The inclusion ofsrcChainIdandlzNonceparameters is particularly useful for tracing issues across chains.Consider renaming the error to better reflect its critical nature:
-error DepositRequestShouldNotFail(uint32 srcChainId, uint64 lzNonce); +error CriticalDepositRequestFailure(uint32 srcChainId, uint64 lzNonce);This name more clearly communicates the severity of the error to developers.
266-270: Approve new whitelist token errors with suggestionsThe
InvalidWhitelistTokensInputandWhitelistTokensListTooLongerrors are clear additions that will help in managing whitelist token operations. They align with the PR objectives for improving token management across chains.Consider adding parameters to these errors to provide more context:
-error InvalidWhitelistTokensInput(); +error InvalidWhitelistTokensInput(uint256 expectedLength, uint256 actualLength); -error WhitelistTokensListTooLong(); +error WhitelistTokensListTooLong(uint256 maxLength, uint256 actualLength);These additions would make debugging and error handling more informative.
src/storage/BootstrapStorage.sol (2)
324-324: LGTM: Improved error handling in _getVaultThe switch to
Errors.VaultDoesNotExist()represents a positive move towards centralized error handling, which can enhance code consistency and maintainability. This approach aligns with best practices for error management in Solidity.Consider adding a custom error message to provide more context, e.g.,
Errors.VaultDoesNotExist(token). This would help in debugging by immediately identifying which token's vault is missing.
338-340: LGTM: Prevention of vault deployment for NSTThe addition of this check aligns with the PR's objective to handle Native Staking Tokens (NST) differently. Preventing vault deployment for NST is a logical step, as native tokens typically don't require a separate vault.
Consider making the error message more specific, e.g.,
Errors.CannotDeployVaultForNST(). This would provide clearer context about why the vault deployment is forbidden specifically for the native staking token.test/mocks/ExocoreGatewayMock.sol (10)
109-117: LGTM: Added pause and unpause functionsThe addition of
pauseandunpausefunctions provides important contract management capabilities. TheonlyOwnermodifier ensures that only the contract owner can call these functions, which is appropriate for such critical operations.Consider emitting events when the contract is paused or unpaused for better transparency and easier off-chain tracking. For example:
function pause() external onlyOwner { _pause(); emit ContractPaused(msg.sender); } function unpause() external onlyOwner { _unpause(); emit ContractUnpaused(msg.sender); }Don't forget to define these events in your contract.
119-134: LGTM: Added markBootstrap functionalityThe
markBootstrapfunction and its internal implementation_markBootstrapprovide a mechanism to retry bootstrap if it fails on a destination chain. The use ofwhenNotPausedandnonReentrantmodifiers in the public function is appropriate for security.Consider adding error handling for the
_sendInterchainMsgcall in the_markBootstrapfunction. While it's internal, it's good practice to handle potential failures, especially in cross-chain communications. For example:function _markBootstrap(uint32 chainIndex) internal { try { _sendInterchainMsg(chainIndex, Action.REQUEST_MARK_BOOTSTRAP, "", false); emit BootstrapRequestSent(chainIndex); } catch Error(string memory reason) { emit BootstrapRequestFailed(chainIndex, reason); } catch (bytes memory lowLevelData) { emit BootstrapRequestFailed(chainIndex, "Low level error"); } }Don't forget to define the
BootstrapRequestFailedevent in your contract.
242-273: LGTM: Added operator association management functionsThe new functions
associateOperatorWithEVMStakeranddissociateOperatorFromEVMStakerprovide important functionality for managing operator associations. They are properly restricted with thewhenNotPausedmodifier and use custom errors consistently with the rest of the contract.Consider emitting events when an operator is associated or dissociated for better transparency and easier off-chain tracking. For example:
function associateOperatorWithEVMStaker(uint32 clientChainId, string calldata operator) external whenNotPaused isValidBech32Address(operator) { bytes memory staker = abi.encodePacked(bytes32(bytes20(msg.sender))); bool success = DELEGATION_CONTRACT.associateOperatorWithStaker(clientChainId, staker, bytes(operator)); if (!success) { revert Errors.AssociateOperatorFailed(clientChainId, msg.sender, operator); } emit OperatorAssociated(clientChainId, msg.sender, operator); } function dissociateOperatorFromEVMStaker(uint32 clientChainId) external whenNotPaused { bytes memory staker = abi.encodePacked(bytes32(bytes20(msg.sender))); bool success = DELEGATION_CONTRACT.dissociateOperatorFromStaker(clientChainId, staker); if (!success) { revert Errors.DissociateOperatorFailed(clientChainId, msg.sender); } emit OperatorDissociated(clientChainId, msg.sender); }Don't forget to define these events in your contract.
312-336: LGTM: Improved _lzReceive function with enhanced security and error handlingThe changes in the
_lzReceivefunction represent significant improvements:
- Custom errors are now used, which is more gas-efficient.
- Additional checks for message length and supported requests enhance security.
- The use of a low-level call allows for dynamic execution of handler functions.
These modifications improve the function's efficiency, security, and flexibility.
Consider providing more detailed error information when the low-level call fails. Instead of just returning the raw
responseOrReason, you could decode it if it's an error message. For example:if (!success) { if (responseOrReason.length > 0) { // Try to decode the error message string memory errorMsg; try abi.decode(responseOrReason, (string)) returns (string memory _errorMsg) { errorMsg = _errorMsg; } catch { errorMsg = "Unknown error"; } revert Errors.RequestOrResponseExecuteFailed(act, _origin.nonce, errorMsg); } else { revert Errors.RequestOrResponseExecuteFailed(act, _origin.nonce, "Empty error"); } }This would provide more meaningful error messages when the execution fails.
338-366: LGTM: Added handleLSTTransfer function for LST operationsThe new
handleLSTTransferfunction effectively manages Liquid Staking Token (LST) transfers, supporting both deposit and withdrawal operations. It appropriately uses the ASSETS_CONTRACT for transfer operations, emits relevant events, and sends interchain messages with the results.Key points:
- Proper handling of different actions (deposit/withdrawal).
- Use of custom errors for failure cases.
- Restricted to be called only from the contract itself (onlyCalledFromThis modifier).
Consider adding error handling for the
_sendInterchainMsgcall at the end of the function. While it's unlikely to fail, it's good practice to handle potential errors in cross-chain communications. For example:bytes memory response = abi.encodePacked(lzNonce, success, updatedBalance); try { _sendInterchainMsg(srcChainId, Action.RESPOND, response, true); } catch Error(string memory reason) { emit InterchainMessageFailed(srcChainId, Action.RESPOND, reason); } catch (bytes memory lowLevelData) { emit InterchainMessageFailed(srcChainId, Action.RESPOND, "Low level error"); }Don't forget to define the
InterchainMessageFailedevent in your contract.
369-397: LGTM: Added handleNSTTransfer function for NST operationsThe new
handleNSTTransferfunction effectively manages Native Staking Token (NST) transfers, supporting both deposit and withdrawal operations. It's structurally similar tohandleLSTTransferbut appropriately uses different ASSETS_CONTRACT functions and includes the validator's public key in the payload.Key points:
- Proper handling of different actions (deposit/withdrawal).
- Use of custom errors for failure cases.
- Restricted to be called only from the contract itself (onlyCalledFromThis modifier).
As with
handleLSTTransfer, consider adding error handling for the_sendInterchainMsgcall at the end of the function. For example:bytes memory response = abi.encodePacked(lzNonce, success, updatedBalance); try { _sendInterchainMsg(srcChainId, Action.RESPOND, response, true); } catch Error(string memory reason) { emit InterchainMessageFailed(srcChainId, Action.RESPOND, reason); } catch (bytes memory lowLevelData) { emit InterchainMessageFailed(srcChainId, Action.RESPOND, "Low level error"); }This ensures that any potential errors in cross-chain communication are properly handled and logged.
400-418: LGTM: Added handleRewardOperation function for reward claimsThe new
handleRewardOperationfunction effectively manages reward claim operations. It appropriately uses the CLAIM_REWARD_CONTRACT for the actual claim operation, emits relevant events, and sends interchain messages with the results.Key points:
- Proper handling of the reward claim process.
- Use of custom errors implicitly through the CLAIM_REWARD_CONTRACT.
- Restricted to be called only from the contract itself (onlyCalledFromThis modifier).
As with previous functions, consider adding error handling for the
_sendInterchainMsgcall at the end of the function. For example:bytes memory response = abi.encodePacked(lzNonce, success, updatedBalance); try { _sendInterchainMsg(srcChainId, Action.RESPOND, response, true); } catch Error(string memory reason) { emit InterchainMessageFailed(srcChainId, Action.RESPOND, reason); } catch (bytes memory lowLevelData) { emit InterchainMessageFailed(srcChainId, Action.RESPOND, "Low level error"); }This ensures that any potential errors in cross-chain communication are properly handled and logged.
421-446: LGTM: Added handleDelegation function for delegation operationsThe new
handleDelegationfunction effectively manages delegation and undelegation operations. It appropriately uses the DELEGATION_CONTRACT for the actual delegation operations, emits relevant events, and sends interchain messages with the results.Key points:
- Proper handling of different actions (delegate/undelegate).
- Use of custom errors implicitly through the DELEGATION_CONTRACT.
- Restricted to be called only from the contract itself (onlyCalledFromThis modifier).
As with previous functions, consider adding error handling for the
_sendInterchainMsgcall at the end of the function. For example:bytes memory response = abi.encodePacked(lzNonce, accepted); try { _sendInterchainMsg(srcChainId, Action.RESPOND, response, true); } catch Error(string memory reason) { emit InterchainMessageFailed(srcChainId, Action.RESPOND, reason); } catch (bytes memory lowLevelData) { emit InterchainMessageFailed(srcChainId, Action.RESPOND, "Low level error"); }This ensures that any potential errors in cross-chain communication are properly handled and logged.
449-473: LGTM: Added handleDepositAndDelegate function for combined operationsThe new
handleDepositAndDelegatefunction effectively manages a combined deposit and delegate operation. It appropriately uses both ASSETS_CONTRACT and DELEGATION_CONTRACT, emits relevant events for both deposit and delegation operations, and sends an interchain message with the results.Key points:
- Proper handling of the combined deposit and delegate operation.
- Use of custom errors for failure cases.
- Restricted to be called only from the contract itself (onlyCalledFromThis modifier).
As with previous functions, consider adding error handling for the
_sendInterchainMsgcall at the end of the function. For example:bytes memory response = abi.encodePacked(lzNonce, accepted, updatedBalance); try { _sendInterchainMsg(srcChainId, Action.RESPOND, response, true); } catch Error(string memory reason) { emit InterchainMessageFailed(srcChainId, Action.RESPOND, reason); } catch (bytes memory lowLevelData) { emit InterchainMessageFailed(srcChainId, Action.RESPOND, "Low level error"); }This ensures that any potential errors in cross-chain communication are properly handled and logged.
Line range hint
1-538: Overall assessment: Significant improvements and additions to ExocoreGatewayMockThis update to the ExocoreGatewayMock contract introduces several important improvements and new functionalities:
- Enhanced error handling using custom errors throughout the contract, improving gas efficiency and error reporting.
- New handler functions for various operations (LST/NST transfers, rewards, delegations, operator associations), expanding the contract's capabilities.
- Improved security measures, including the use of ReentrancyGuard and additional checks in critical functions.
- Consistent use of access control modifiers (onlyOwner, onlyCalledFromThis, whenNotPaused) across new and modified functions.
The changes are well-structured and maintain consistency with the existing code. The new functionalities align with the contract's purpose as a mock for the ExocoreGateway.
Consider implementing the suggested improvements in error handling for cross-chain message sending (
_sendInterchainMsg) in the various handler functions. This would further enhance the contract's robustness and error reporting capabilities.As the contract grows in complexity, consider breaking it down into smaller, more focused contracts using inheritance or a modular design pattern. This could improve maintainability and make it easier to test individual components.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- docs/native_deposit_workflow.wsd (1 hunks)
- src/core/ExocoreGateway.sol (5 hunks)
- src/libraries/Errors.sol (6 hunks)
- src/storage/BootstrapStorage.sol (3 hunks)
- src/storage/ClientChainGatewayStorage.sol (3 hunks)
- src/storage/ExocoreGatewayStorage.sol (2 hunks)
- test/foundry/unit/ClientChainGateway.t.sol (6 hunks)
- test/mocks/ExocoreGatewayMock.sol (10 hunks)
Files skipped from review due to trivial changes (1)
- docs/native_deposit_workflow.wsd
Files skipped from review as they are similar to previous changes (2)
- src/storage/ClientChainGatewayStorage.sol
- test/foundry/unit/ClientChainGateway.t.sol
Additional context used
Learnings (2)
src/core/ExocoreGateway.sol (2)
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#96 File: src/core/ExocoreGateway.sol:305-306 Timestamp: 2024-09-23T09:39:44.021Z Learning: Within `ExocoreGateway.sol`, the `_validateMessageLength` function ensures that `message` has sufficient length, so additional length checks after calling `_validateMessageLength` are unnecessary.Learnt from: MaxMustermann2 PR: ExocoreNetwork/exocore-contracts#96 File: src/core/ExocoreGateway.sol:340-341 Timestamp: 2024-09-22T10:48:21.801Z Learning: In `ExocoreGateway`, the payload length is validated in `_lzReceive`, so additional checks in functions like `handleLSTTransfer` are unnecessary.test/mocks/ExocoreGatewayMock.sol (2)
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#96 File: test/mocks/ExocoreGatewayMock.sol:338-366 Timestamp: 2024-09-23T09:39:05.445Z Learning: In the `ExocoreGateway`, deposit operations are intended to be must-succeed and should revert on failure, while withdrawal failures are acceptable and should not trigger a revert.Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#96 File: test/mocks/ExocoreGatewayMock.sol:144-149 Timestamp: 2024-09-23T09:40:29.126Z Learning: When reviewing mock contracts, optimizations for error handling are not necessary.
Additional comments not posted (25)
src/storage/ExocoreGatewayStorage.sol (8)
4-6: LGTM: New imports align with PR objectivesThe new imports for ActionAttributes, Errors, and Action libraries are appropriate for the changes made in the contract. They support the handling of different message types and improved error handling, which aligns with the PR objectives.
13-13: LGTM: ActionAttributes usage supports new message handlingThe addition of
using ActionAttributes for Action;is consistent with the new imports and supports the new message handling approach mentioned in the PR objectives.
46-52: LGTM: Improved ClaimRewardResult eventThe updated
ClaimRewardResultevent now includes a 'success' parameter, which enhances the informativeness of the event. This change is consistent with the PR's goal of improving clarity in operations.
54-61: LGTM: New LSTTransfer event aligns with PR objectivesThe new
LSTTransferevent is well-structured and provides comprehensive information about Liquid Staking Token transfers. It includes parameters for deposit/withdraw operations, success status, token, staker, and amount. This event aligns perfectly with the PR objective of separating LST operations and enhances the contract's ability to track and report on these transfers.
64-71: LGTM: New NSTTransfer event supports NST operations and oracle functionalityThe new
NSTTransferevent is well-designed and crucial for tracking Native Staking Token transfers. It includes essential parameters such as deposit/withdraw flag, success status, validator public key, staker, and amount. This event aligns perfectly with the PR objective of separating NST operations and supports the oracle module's functionality by including the validator public key. The structure is consistent with best practices and complements the LSTTransfer event.
74-87: LGTM: Consolidated DelegationRequest event improves efficiencyThe updated
DelegationRequestevent efficiently consolidates both delegate and undelegate operations into a single event. It provides comprehensive information about delegation requests, including the operation type, acceptance status, token, delegator, operator, and amount. This consolidation simplifies event handling and aligns with the PR's goal of improving code organization and efficiency.
103-121: LGTM: New _validateMessageLength function enhances message handling robustnessThe new
_validateMessageLengthfunction is a valuable addition that enhances the robustness of message handling in the contract. It effectively uses the Action enum and ActionAttributes library to validate message lengths based on action types. The function includes appropriate error handling for invalid message lengths and unsupported requests, consistent with the new Errors library. This implementation aligns well with the PR objective of improving code organization and supporting different message types.
Line range hint
1-122: Overall: Excellent refactoring that aligns with PR objectivesThe changes in this file successfully achieve the PR's objectives of separating NST and LST operations, improving code organization, and enhancing message handling. Key improvements include:
- New events (
LSTTransferandNSTTransfer) that clearly distinguish between LST and NST operations.- Updated
DelegationRequestevent that consolidates delegation-related operations.- New
_validateMessageLengthfunction that improves message handling robustness.- Appropriate use of new libraries (ActionAttributes, Errors) to support these changes.
These modifications contribute to a more modular, clear, and efficient contract structure, which will facilitate the updating of beacon chain balances by the oracle module as intended.
src/libraries/Errors.sol (8)
Line range hint
4-57: LGTM: New errors for cross-chain communicationThe new import and error definitions (UnsupportedRequest, UnexpectedSourceChain, UnexpectedInboundNonce) are well-documented and align with the PR objectives. They provide clear error handling for cross-chain communication scenarios, which is crucial for the separation of NST and LST messages.
159-160: LGTM: CapsuleDoesNotExist errorThe
CapsuleDoesNotExisterror is a clear and concise addition that aligns with the library's naming conventions and the PR objectives. It will be useful for handling ExoCapsule-related operations.
169-186: LGTM: New errors for response handling and whitelist operationsThe new errors (
UnsupportedResponse,UnexpectedResponse,DepositShouldNotFailOnExocore, andInvalidAddWhitelistTokensRequest) are well-documented and provide clear error handling for various scenarios. They align with the PR objectives, particularly in improving the handling of cross-chain responses and deposit operations.
188-191: LGTM: WithdrawFailedOnExocore eventThe
WithdrawFailedOnExocoreevent is a valuable addition for tracking and debugging withdrawal failures. It's well-documented and uses indexed parameters appropriately, which will facilitate efficient event filtering and monitoring of withdrawal issues on the Exocore side.
238-245: LGTM: New errors for precompile calls and message validationThe
PrecompileCallFailedandInvalidMessageLengtherrors are well-defined and provide clear error handling for precompile call failures and message length validation, respectively. These additions will improve the robustness of cross-chain message processing in the Exocore system.
252-254: LGTM: RegisterClientChainToExocoreFailed errorThe
RegisterClientChainToExocoreFailederror is a clear and useful addition for handling failed client chain registrations. The inclusion of theclientChainIdparameter will greatly assist in debugging and troubleshooting cross-chain registration issues.
256-264: LGTM: New errors for whitelist token operationsThe
AddWhitelistTokenFailedandUpdateWhitelistTokenFailederrors are well-defined and provide clear error handling for whitelist token operations. The inclusion ofclientChainIdandtokenparameters will be valuable for debugging cross-chain token management issues. These additions align well with the PR objectives for separating NST and LST operations.
Line range hint
1-301: LGTM: Comprehensive error handling for Exocore operationsThe
Errors.solfile has been updated with a comprehensive set of new errors that align well with the PR objectives. The additions provide robust error handling for cross-chain operations, token management, and ExoCapsule interactions. The file maintains a consistent structure and naming convention, making it easy to navigate and understand.The new errors and event cover various scenarios that may occur during the separation of NST and LST operations, which will greatly improve the system's ability to handle and report issues. This comprehensive approach to error handling will enhance the overall reliability and debuggability of the Exocore system.
src/storage/BootstrapStorage.sol (2)
129-130: LGTM: Addition of VIRTUAL_NST_ADDRESS constantThe introduction of
VIRTUAL_NST_ADDRESSaligns with the PR's objective to separate Native Staking Token (NST) operations. Using a virtual address (0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE) for native tokens is a common and recognized practice in Ethereum-based systems, enhancing the contract's ability to handle native token operations consistently.
Line range hint
1-365: Summary: Improvements in NST handling and error managementThe changes in this file successfully contribute to the PR's objective of separating Native Staking Token (NST) operations from Liquid Staking Token (LST) operations. The introduction of
VIRTUAL_NST_ADDRESS, prevention of vault deployment for NST, and improved error handling enhance the contract's ability to manage different token types more effectively.These modifications improve code quality, maintainability, and align with best practices in Solidity development. The suggested minor improvements in error messaging could further enhance debugging and code clarity.
test/mocks/ExocoreGatewayMock.sol (6)
4-4: LGTM: Import statement for ActionThe addition of this import statement is appropriate for the new functionality using the Action enum.
81-92: LGTM: Improved initialization with custom error and ReentrancyGuardThe changes in the
initializefunction are positive:
- Using a custom error (
Errors.ZeroAddress()) for the zero address check is more gas-efficient.- The addition of
__ReentrancyGuard_init_unchained()enhances the contract's security against reentrancy attacks.These modifications improve both efficiency and security.
95-106: LGTM: Updated whitelist function selectorsThe
_initializeWhitelistFunctionSelectorsfunction has been expanded to include selectors for new operations such as LST and NST transfers, reward operations, delegation, and operator association. These additions are consistent with the new functions implemented in the contract and are necessary for the gateway's functionality.
Line range hint
135-240: LGTM: Improved error handling and security in client chain and token management functionsThe changes in
registerOrUpdateClientChain,setPeer,addWhitelistToken, andupdateWhitelistTokenfunctions represent significant improvements:
- Custom errors are now used instead of require statements, which is more gas-efficient.
- The
setPeerfunction now includes a check for registered client chain ID, enhancing security.- Error handling in
addWhitelistTokenandupdateWhitelistTokenis improved with custom errors.These modifications enhance the contract's efficiency, readability, and security while maintaining the core functionality of each function.
Line range hint
275-309: LGTM: Added internal functions for client chain validation and managementThe new internal functions
_validateClientChainIdRegisteredand_registerOrUpdateClientChainprovide important utility for client chain management:
_validateClientChainIdRegisteredensures that a given client chain ID is registered, using the ASSETS_CONTRACT._registerOrUpdateClientChainhandles the registration or update of a client chain, also using the ASSETS_CONTRACT.Both functions use custom errors consistently with the rest of the contract and properly handle success and failure cases. These additions enhance the contract's ability to manage client chains effectively and securely.
476-498: LGTM: Added handleOperatorAssociation function for operator managementThe new
handleOperatorAssociationfunction effectively manages the association and dissociation of operators. It appropriately uses the DELEGATION_CONTRACT for the actual operations and emits an event with the result.Key points:
- Proper handling of different actions (associate/dissociate).
- Use of custom errors implicitly through the DELEGATION_CONTRACT.
- Restricted to be called only from the contract itself (onlyCalledFromThis modifier).
- Consistent with other handler functions in the contract.
This function enhances the contract's ability to manage operator associations effectively and securely.
src/core/ExocoreGateway.sol (1)
372-374: Correct the extraction ofvalidatorPubkey; expected length may be incorrectIn the
handleNSTTransferfunction,validatorPubkeyis extracted usingpayload[:32]. However, Ethereum BLS validator public keys are typically 48 bytes in length. Extracting only 32 bytes may result in incomplete or invalid public keys.Ensure that the
validatorPubkeyis correctly extracted with the appropriate length. If the public key is 48 bytes, adjust the slicing accordingly:- bytes calldata validatorPubkey = payload[:32]; - bytes calldata staker = payload[32:64]; - uint256 amount = uint256(bytes32(payload[64:96])); + bytes calldata validatorPubkey = payload[:48]; + bytes calldata staker = payload[48:80]; + uint256 amount = uint256(bytes32(payload[80:112]));Make sure to update all subsequent offsets and ensure that the payload being sent matches this structure. This adjustment will correctly parse the
validatorPubkey,staker, andamountfrom the payload.Skipped due to learnings
Learnt from: bwhour PR: ExocoreNetwork/exocore-contracts#96 File: test/mocks/AssetsMock.sol:75-90 Timestamp: 2024-09-21T06:01:54.422Z Learning: In the `withdrawNST` function of `AssetsMock.sol`, the `validatorPubkey.length` should be 32 bytes, not 48 bytes.
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
src/storage/ExocoreGatewayStorage.sol (1)
104-122: LGTM: Robust message length validationThe
_validateMessageLengthfunction provides a centralized and robust way to validate message lengths. It effectively uses the new Action enum and ActionAttributes library, with appropriate error handling using custom errors from the Errors library. This aligns well with the PR objective of improving message validation.Consider adding a comment explaining the significance of the
expectedLength == 0check:if (expectedLength == 0) { + // expectedLength of 0 indicates an unsupported action revert Errors.UnsupportedRequest(action); }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- docs/native_deposit_workflow.wsd (1 hunks)
- src/core/ExocoreGateway.sol (5 hunks)
- src/libraries/Errors.sol (6 hunks)
- src/storage/BootstrapStorage.sol (3 hunks)
- src/storage/ClientChainGatewayStorage.sol (3 hunks)
- src/storage/ExocoreGatewayStorage.sol (2 hunks)
- test/foundry/unit/ClientChainGateway.t.sol (6 hunks)
- test/mocks/ExocoreGatewayMock.sol (10 hunks)
Files skipped from review as they are similar to previous changes (4)
- docs/native_deposit_workflow.wsd
- src/storage/BootstrapStorage.sol
- src/storage/ClientChainGatewayStorage.sol
- test/foundry/unit/ClientChainGateway.t.sol
Additional context used
Learnings (2)
src/core/ExocoreGateway.sol (4)
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#96 File: src/core/ExocoreGateway.sol:313-315 Timestamp: 2024-09-23T10:03:22.440Z Learning: When using `address(this).call(...)`, `msg.sender` becomes `address(this)`, so functions with the `onlyCalledFromThis` modifier will function correctly.Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#96 File: src/core/ExocoreGateway.sol:425-438 Timestamp: 2024-09-23T10:01:38.842Z Learning: In the `handleDelegation` function of `ExocoreGateway.sol`, the `operator` variable is converted from a string, so casting from `bytes` to `string` is safe because `operator` will always contain valid UTF-8 encoded characters.Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#96 File: src/core/ExocoreGateway.sol:305-306 Timestamp: 2024-09-23T09:39:44.021Z Learning: Within `ExocoreGateway.sol`, the `_validateMessageLength` function ensures that `message` has sufficient length, so additional length checks after calling `_validateMessageLength` are unnecessary.Learnt from: MaxMustermann2 PR: ExocoreNetwork/exocore-contracts#96 File: src/core/ExocoreGateway.sol:340-341 Timestamp: 2024-09-22T10:48:21.801Z Learning: In `ExocoreGateway`, the payload length is validated in `_lzReceive`, so additional checks in functions like `handleLSTTransfer` are unnecessary.test/mocks/ExocoreGatewayMock.sol (2)
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#96 File: test/mocks/ExocoreGatewayMock.sol:338-366 Timestamp: 2024-09-23T09:39:05.445Z Learning: In the `ExocoreGateway`, deposit operations are intended to be must-succeed and should revert on failure, while withdrawal failures are acceptable and should not trigger a revert.Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#96 File: test/mocks/ExocoreGatewayMock.sol:144-149 Timestamp: 2024-09-23T09:40:29.126Z Learning: When reviewing mock contracts, optimizations for error handling are not necessary.
Additional comments not posted (31)
src/storage/ExocoreGatewayStorage.sol (7)
4-6: LGTM: New imports align with contract changesThe new imports for
ActionAttributes,Errors, andActionlibraries are appropriate and necessary for the new functionality introduced in the contract.
13-13: LGTM: Appropriate library usageThe
using ActionAttributes for Action;statement is consistent with the new imports and supports the new message length validation functionality.
46-52: LGTM: Well-defined ClaimRewardResult eventThe
ClaimRewardResultevent is well-structured and provides necessary information for tracking reward withdrawals. The parameter descriptions are clear and accurate.
54-61: LGTM: Well-structured LSTTransfer eventThe
LSTTransferevent is well-designed and aligns with the PR objective of separating LST operations. It efficiently handles both deposit and withdrawal operations, providing clear and accurate parameter descriptions.
64-71: LGTM: Well-defined NSTTransfer eventThe
NSTTransferevent is well-structured and aligns with the PR objective of separating NST operations. The inclusion of the validator public key supports the requirement mentioned in the PR objectives. The parameter descriptions are clear and accurate.
74-88: LGTM: Comprehensive DelegationRequest eventThe
DelegationRequestevent is well-designed and provides comprehensive information about delegation requests. It efficiently handles both delegate and undelegate operations, with clear and accurate parameter descriptions.
Line range hint
1-123: Overall: Excellent refactoring and improvementsThis refactoring successfully separates NST and LST operations, improves message validation, and supports sending validator public keys to Exocore. The new events and internal function are well-designed and align perfectly with the PR objectives. The code is clean, well-documented, and follows best practices.
Great job on this refactoring effort!
src/libraries/Errors.sol (15)
4-5: LGTM: Import statement for ActionThe import statement for
ActionfromGatewayStorage.solis correctly added and necessary for the new error definitions.
46-49: LGTM: UnsupportedRequest errorThe
UnsupportedRequesterror is well-defined and properly documented. It follows the established pattern for error definitions in this library.
50-53: LGTM: UnexpectedSourceChain errorThe
UnexpectedSourceChainerror is well-defined and properly documented. The use ofuint32for the chain ID is appropriate.
54-57: LGTM: UnexpectedInboundNonce errorThe
UnexpectedInboundNonceerror is well-defined and properly documented. The use ofuint64for nonces is appropriate and provides a good balance between range and gas efficiency.
126-127: LGTM: VaultDoesNotExist errorThe
VaultDoesNotExisterror is concise and follows the established naming convention. It's consistent with similar errors in the library.
129-130: LGTM: NotYetSupported errorThe
NotYetSupportederror is concise and self-explanatory. It's a useful error for features that are planned but not yet implemented.
132-138: LGTM: RequestOrResponseExecuteFailed errorThe
RequestOrResponseExecuteFailederror is well-defined and thoroughly documented. The use ofbytesfor the reason parameter allows for flexible and detailed error messages.
159-160: LGTM: CapsuleDoesNotExist errorThe
CapsuleDoesNotExisterror is concise and follows the established naming convention. It's consistent with theVaultDoesNotExisterror, maintaining a coherent error handling approach.
169-176: LGTM: UnsupportedResponse and UnexpectedResponse errorsBoth
UnsupportedResponseandUnexpectedResponseerrors are well-defined and properly documented. They complement the existingUnsupportedRequesterror, providing a symmetrical approach to error handling for both requests and responses.
178-186: LGTM: DepositShouldNotFailOnExocore and InvalidAddWhitelistTokensRequest errorsBoth
DepositShouldNotFailOnExocoreandInvalidAddWhitelistTokensRequesterrors are well-defined and properly documented. TheInvalidAddWhitelistTokensRequesterror includes both expected and actual lengths, which is particularly helpful for debugging purposes.
188-191: LGTM: WithdrawFailedOnExocore eventThe
WithdrawFailedOnExocoreevent is well-defined and properly documented. The use of indexed parameters fortokenandwithdrawerallows for efficient event filtering, which is a good practice.
238-250: LGTM: PrecompileCallFailed, InvalidMessageLength, and DepositRequestShouldNotFail errorsAll three errors (
PrecompileCallFailed,InvalidMessageLength, andDepositRequestShouldNotFail) are well-defined and properly documented. ThePrecompileCallFailederror includes both the selector and reason, which is particularly helpful for debugging. The comment indicating thatDepositRequestShouldNotFailis a critical error is valuable information for error handling.
252-264: LGTM: Client chain and whitelist token operation errorsThe errors
RegisterClientChainToExocoreFailed,AddWhitelistTokenFailed, andUpdateWhitelistTokenFailedare well-defined and properly documented. They provide specific error cases for client chain registration and whitelist token operations, which will be helpful for debugging and error handling in these processes.
266-270: LGTM: Whitelist token input validation errorsThe errors
InvalidWhitelistTokensInputandWhitelistTokensListTooLongare well-defined and properly documented. They provide specific error cases for input validation of whitelist tokens, which will be helpful for ensuring the integrity of whitelist token operations.
Line range hint
1-301: Overall assessment: Excellent additions to the Errors libraryThe new errors and event added to the
Errors.sollibrary are well-defined, properly documented, and consistent with the existing code style. These additions enhance the error handling capabilities of the Exocore contracts by providing more specific and detailed error information. This will significantly aid in debugging and development processes.Some notable improvements:
- Consistent naming conventions and documentation styles.
- Appropriate use of parameter types (e.g.,
uint32for chain IDs,uint64for nonces).- Inclusion of both generic (e.g.,
NotYetSupported) and specific (e.g.,DepositShouldNotFailOnExocore) error types.- Addition of the
WithdrawFailedOnExocoreevent with indexed parameters for efficient filtering.These changes contribute to a more robust and developer-friendly error handling system for the Exocore contracts.
src/core/ExocoreGateway.sol (5)
6-11: Imports are correctly added and organizedThe new imports for
Errors,Action, and the precompile interfaces are appropriately added, ensuring that the contract has access to the necessary external contracts and libraries.
80-89: Whitelist function selectors are properly initializedThe mapping of action enums to their respective handler function selectors in
_initializeWhitelistFunctionSelectorsis correctly implemented, allowing for accurate dispatching of incoming messages based on the action type.
295-324: Message handling logic is well-structured and secureThe
_lzReceivefunction accurately processes incoming messages, validates them, and dispatches to the appropriate handlers usingaddress(this).call(). The use ofonlyCalledFromThismodifier ensures internal security when invoking handler functions.
425-438: Efficient handling of delegation requestsThe
handleDelegationfunction correctly processes delegation and undelegation requests, safely castingoperatorfrombytestostring. This is acceptable as per prior learning thatoperatorcontains valid UTF-8 encoded characters.
478-492: Potential mismatch in operator data slicingIn
handleOperatorAssociation, theoperatoris sliced frompayload[32:74], resulting in 42 bytes. Ensure that this length aligns with the expected size of theoperatordata. Be cautious of potential issues ifoperatoris of variable length.Run the following script to verify the expected operator length:
This script extracts the end index of the operator slice to confirm it matches the expected length.
Verification successful
Operator data slicing length is correct
The operator slice length of 42 bytes in
handleOperatorAssociationaligns with the expected operator data size.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the length of operator data used in handleOperatorAssociation. # Test: Ensure operator data length is consistent. # Expect: Operator data should be of expected length. rg --type solidity 'handleOperatorAssociation' -A 10 | rg 'operator = payload\[32:([0-9]+)\];' -o -r '$1'Length of output: 138
Script:
#!/bin/bash # Description: Check the length of operator data used in handleOperatorAssociation. # Test: Ensure operator data length is consistent. # Expect: Operator data should be of expected length. rg 'handleOperatorAssociation' -A 10 -g '*.sol' | rg 'operator = payload\[32:([0-9]+)\];' -o -r '$1'Length of output: 106
test/mocks/ExocoreGatewayMock.sol (4)
84-86: Good addition: Zero address check forowner_ininitializeAdding a check to prevent a zero address for
owner_in theinitializefunction enhances the contract's security during initialization.
97-106: Function selectors are correctly mapped to actionsThe initialization of the
_whiteListFunctionSelectorsmapping correctly associates eachActionwith its corresponding handler function, ensuring that incoming messages are handled appropriately.
320-320: Improved robustness with message length validationIncluding
_validateMessageLength(message);in the_lzReceivefunction enhances the robustness by ensuring that the message meets expected length requirements before processing.
255-256: Verify correct encoding ofstakeraddressPlease verify that the staker's address is correctly encoded when calling
DELEGATION_CONTRACT.associateOperatorWithStakerandDELEGATION_CONTRACT.dissociateOperatorFromStaker. The use ofabi.encodePacked(bytes32(bytes20(msg.sender)))may introduce unnecessary padding. Consider usingabi.encodePacked(msg.sender)if the receiving function expects the address in this format.Run the following script to check for consistent staker address encoding:
Also applies to: 268-269
adu-web3
force-pushed
the
update-balance
branch
from
8a86625 to
3a52726
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (9)
src/storage/GatewayStorage.sol (2)
7-21: ExpandedActionenum enhances functionality.The
Actionenum has been significantly expanded to include a wider range of operations, including separate actions for NST and LST deposits/withdrawals, reward claiming, delegation, and system management. This aligns well with the PR objectives and suggests a more comprehensive approach to handling various operations within the system.Consider adding inline documentation for each enum value to clarify their specific purposes and use cases. This would greatly enhance the readability and maintainability of the code. For example:
enum Action { /// @notice Request to deposit Liquid Staking Tokens REQUEST_DEPOSIT_LST, /// @notice Request to deposit Native Staking Tokens REQUEST_DEPOSIT_NST, // ... (continue for each enum value) }
Missing payload length validation after removal of
_validatePayloadLength.The removal of the
_validatePayloadLengthfunction appears to leave the contract without necessary payload length checks, potentially compromising security. Ensure that payload validation is properly implemented elsewhere in the contract to maintain robust security measures.Analysis chain
Line range hint
1-90: Clarification needed on payload validation after removal of_validatePayloadLength.The summary indicates that the
_validatePayloadLengthfunction has been removed. This function was likely responsible for ensuring that incoming payloads met certain length requirements, which is an important security measure.Could you please clarify how payload validation is now being handled? It's crucial to ensure that this security check hasn't been overlooked. If it's been moved to another part of the codebase, please provide details on its new location and implementation.
To help verify this, let's search for any new payload validation logic:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for new payload validation logic # Test 1: Look for functions or methods that might be handling payload validation echo "Searching for potential payload validation logic:" rg "function.*payload" --type solidity rg "function.*validate" --type solidity # Test 2: Check for any length checks that might be related to payload validation echo "Checking for length validations:" rg "\.length\s*(==|!=|>|<|>=|<=)" --type solidityLength of output: 417
Script:
#!/bin/bash # Description: Search for new payload validation logic in Solidity files # Test 1: Look for functions or methods that might be handling payload validation echo "Searching for potential payload validation logic in .sol files:" rg "function.*payload" --glob "*.sol" rg "function.*validate" --glob "*.sol" # Test 2: Check for any length checks that might be related to payload validation echo "Checking for length validations in .sol files:" rg "\.length\s*(==|!=|>|<|>=|<=)" --glob "*.sol"Length of output: 14023
Script:
#!/bin/bash # Description: Further search for payload length validation logic in Solidity files # Test 1: Look for functions that explicitly validate payload length echo "Searching for explicit payload length validation functions in .sol files:" rg "function.*validate.*payload.*length" --glob "*.sol" rg "function.*check.*payload.*length" --glob "*.sol" # Test 2: Look for modifiers that might handle payload validation echo "Searching for modifiers handling payload validation in .sol files:" rg "modifier.*validate.*payload" --glob "*.sol" # Test 3: Search for utility libraries or contracts that might contain payload validation logic echo "Searching for utility contracts or libraries related to payload validation in .sol files:" rg "library.*.*payload" --glob "*.sol" rg "contract.*.*payload" --glob "*.sol"Length of output: 944
src/storage/ExocoreGatewayStorage.sol (1)
104-122: Robust message length validation with a suggestionThe new
_validateMessageLengthfunction significantly enhances the contract's robustness by validating incoming messages based on their action type. The use of custom errors improves gas efficiency and error reporting. The function's design, leveraging theActionAttributeslibrary, is well-structured and maintainable.Suggestion for improvement:
Consider adding a custom error for the case when
message.length < 1to provide more specific error information. This could be implemented as follows:function _validateMessageLength(bytes calldata message) internal pure { if (message.length < 1) { - revert Errors.InvalidMessageLength(); + revert Errors.EmptyMessage(); } // ... rest of the function }This change would require adding a new custom error
EmptyMessage()to theErrorslibrary.src/core/ClientGatewayLzReceiver.sol (5)
31-40: New nextNonce function implementationThe addition of the
nextNoncefunction is a good implementation that correctly calculates the next nonce. Thevirtualmodifier allows for potential customization in derived contracts.Consider adding a brief comment explaining the purpose of this function and why it's overriding the base implementation.
/// @inheritdoc OAppReceiverUpgradeable +/// @dev Overrides the base implementation to provide custom nonce calculation function nextNonce(uint32 srcEid, bytes32 sender) public view virtual override(OAppReceiverUpgradeable) returns (uint64) { return inboundNonce[srcEid][sender] + 1; }
45-67: Improved _lzReceive function with better error handling and action processingThe refactoring of the
_lzReceivefunction significantly improves its clarity and robustness. The use of theErrorslibrary enhances error handling consistency, and the explicit handling of different action types improves the separation of concerns.Consider adding a comment explaining the purpose of the
_whiteListFunctionSelectorsmapping and how it's used to determine the appropriate function to call for each action type.Action act = Action(uint8(message[0])); bytes calldata payload = message[1:]; if (act == Action.RESPOND) { _handleResponse(message); } else { + // _whiteListFunctionSelectors maps actions to their corresponding function selectors bytes4 selector_ = _whiteListFunctionSelectors[act]; if (selector_ == bytes4(0)) { revert Errors.UnsupportedRequest(act); } (bool success, bytes memory reason) = address(this).call(abi.encodePacked(selector_, abi.encode(payload))); if (!success) { revert Errors.RequestOrResponseExecuteFailed(act, _origin.nonce, reason); } }
Line range hint
71-100: Improved _handleResponse function with better modularity and withdrawal handlingThe refactoring of the
_handleResponsefunction significantly improves its clarity and modularity. The function now focuses explicitly on withdrawal actions and uses helper functions for updating balances, which enhances code organization. The separate handling of principal and reward withdrawals improves the function's flexibility.Consider adding a comment explaining the purpose of the
_getCachedRequestForResponsefunction and how it relates to the overall request-response flow.+// Retrieve the cached request details for the incoming response (uint64 requestId, Action requestAct, bytes memory cachedRequest) = _getCachedRequestForResponse(response); if (!requestAct.isWithdrawal()) { revert Errors.UnsupportedResponse(requestAct); } bool requestSuccess = _isSuccess(response); if (requestSuccess) { (address token, address staker, uint256 amount) = _decodeCachedRequest(cachedRequest); if (requestAct.isPrincipal()) { _updatePrincipalWithdrawableBalance(token, staker, amount); } else if (requestAct.isReward()) { _updateRewardWithdrawableBalance(token, staker, amount); } else { revert Errors.UnsupportedResponse(requestAct); } }
129-135: Simplified _decodeCachedRequest functionThe
_decodeCachedRequestfunction has been simplified, improving its readability and efficiency. The direct use ofabi.decodeto extract all values at once is a good practice.Consider adding a comment to explain the expected structure of the cached request, as this function now assumes a specific format.
function _decodeCachedRequest(bytes memory cachedRequest) internal pure returns (address token, address staker, uint256 amount) { + // Cached request structure: (address token, address staker, uint256 amount) return abi.decode(cachedRequest, (address, address, uint256)); }
146-171: New helper functions for updating withdrawable balancesThe addition of
_updatePrincipalWithdrawableBalanceand_updateRewardWithdrawableBalancefunctions improves code modularity and readability. These functions provide a clear separation of concerns for handling different types of withdrawals (principal and reward) and interact appropriately with the relevant contracts (ExoCapsule or Vault).Consider adding error handling for the case where an invalid token address is provided, especially in the
_updatePrincipalWithdrawableBalancefunction.function _updatePrincipalWithdrawableBalance(address token, address staker, uint256 amount) internal { if (token == VIRTUAL_NST_ADDRESS) { IExoCapsule capsule = _getCapsule(staker); capsule.updateWithdrawableBalance(amount); - } else { + } else if (token != address(0)) { IVault vault = _getVault(token); vault.updateWithdrawableBalance(staker, amount, 0); + } else { + revert Errors.InvalidTokenAddress(); } }test/mocks/ExocoreGatewayMock.sol (1)
524-524: Nitpick: Inconsistent parameter naming inquotefunctionThe parameter
srcChainidshould be renamed tosrcChainIdto maintain consistency with the naming convention used throughout the codebase.Apply this diff to correct the parameter name:
- function quote(uint32 srcChainid, bytes calldata _message) public view returns (uint256 nativeFee) { + function quote(uint32 srcChainId, bytes calldata _message) public view returns (uint256 nativeFee) {
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (44)
- docs/native_deposit_workflow.wsd (1 hunks)
- script/11_SetPeers.s.sol (2 hunks)
- script/13_DepositValidator.s.sol (2 hunks)
- script/3_Setup.s.sol (2 hunks)
- script/4_Deposit.s.sol (2 hunks)
- script/5_Withdraw.s.sol (2 hunks)
- script/TestPrecompileErrorFixed.s.sol (3 hunks)
- script/TestPrecompileErrorFixed_Deploy.s.sol (1 hunks)
- src/core/BaseRestakingController.sol (4 hunks)
- src/core/Bootstrap.sol (4 hunks)
- src/core/BootstrapLzReceiver.sol (3 hunks)
- src/core/ClientChainGateway.sol (2 hunks)
- src/core/ClientGatewayLzReceiver.sol (5 hunks)
- src/core/ExoCapsule.sol (0 hunks)
- src/core/ExocoreGateway.sol (5 hunks)
- src/core/LSTRestakingController.sol (5 hunks)
- src/core/NativeRestakingController.sol (4 hunks)
- src/core/Vault.sol (0 hunks)
- src/interfaces/IExoCapsule.sol (0 hunks)
- src/interfaces/IVault.sol (0 hunks)
- src/interfaces/precompiles/IAssets.sol (2 hunks)
- src/interfaces/precompiles/IClaimReward.sol (1 hunks)
- src/interfaces/precompiles/IDelegation.sol (4 hunks)
- src/libraries/ActionAttributes.sol (1 hunks)
- src/libraries/Errors.sol (6 hunks)
- src/storage/BootstrapStorage.sol (3 hunks)
- src/storage/ClientChainGatewayStorage.sol (3 hunks)
- src/storage/ExoCapsuleStorage.sol (0 hunks)
- src/storage/ExocoreGatewayStorage.sol (2 hunks)
- src/storage/GatewayStorage.sol (2 hunks)
- src/storage/VaultStorage.sol (0 hunks)
- test/foundry/Delegation.t.sol (8 hunks)
- test/foundry/DepositThenDelegateTo.t.sol (9 hunks)
- test/foundry/DepositWithdrawPrinciple.t.sol (27 hunks)
- test/foundry/ExocoreDeployer.t.sol (7 hunks)
- test/foundry/WithdrawReward.t.sol (7 hunks)
- test/foundry/unit/Bootstrap.t.sol (2 hunks)
- test/foundry/unit/ClientChainGateway.t.sol (6 hunks)
- test/foundry/unit/ExocoreGateway.t.sol (13 hunks)
- test/mocks/AssetsMock.sol (2 hunks)
- test/mocks/ClaimRewardMock.sol (1 hunks)
- test/mocks/DelegationMock.sol (5 hunks)
- test/mocks/ExocoreGatewayMock.sol (10 hunks)
- test/mocks/PrecompileCallerMock.sol (1 hunks)
Files not reviewed due to no reviewable changes (6)
- src/core/ExoCapsule.sol
- src/core/Vault.sol
- src/interfaces/IExoCapsule.sol
- src/interfaces/IVault.sol
- src/storage/ExoCapsuleStorage.sol
- src/storage/VaultStorage.sol
Files skipped from review as they are similar to previous changes (30)
- docs/native_deposit_workflow.wsd
- script/11_SetPeers.s.sol
- script/13_DepositValidator.s.sol
- script/3_Setup.s.sol
- script/4_Deposit.s.sol
- script/5_Withdraw.s.sol
- script/TestPrecompileErrorFixed.s.sol
- script/TestPrecompileErrorFixed_Deploy.s.sol
- src/core/BaseRestakingController.sol
- src/core/Bootstrap.sol
- src/core/BootstrapLzReceiver.sol
- src/core/ClientChainGateway.sol
- src/core/LSTRestakingController.sol
- src/core/NativeRestakingController.sol
- src/interfaces/precompiles/IAssets.sol
- src/interfaces/precompiles/IClaimReward.sol
- src/interfaces/precompiles/IDelegation.sol
- src/libraries/ActionAttributes.sol
- src/libraries/Errors.sol
- src/storage/ClientChainGatewayStorage.sol
- test/foundry/Delegation.t.sol
- test/foundry/DepositWithdrawPrinciple.t.sol
- test/foundry/ExocoreDeployer.t.sol
- test/foundry/WithdrawReward.t.sol
- test/foundry/unit/Bootstrap.t.sol
- test/foundry/unit/ClientChainGateway.t.sol
- test/foundry/unit/ExocoreGateway.t.sol
- test/mocks/ClaimRewardMock.sol
- test/mocks/DelegationMock.sol
- test/mocks/PrecompileCallerMock.sol
Additional context used
Learnings (5)
src/core/ClientGatewayLzReceiver.sol (2)
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#96 File: src/core/ClientGatewayLzReceiver.sol:85-93 Timestamp: 2024-09-23T09:34:52.523Z Learning: In the `ClientGatewayLzReceiver` contract's `_handleResponse` function, the `ResponseProcessed` event includes a boolean indicating whether the request was successfully executed on Exocore, so explicit handling of failure cases within `_handleResponse` is unnecessary.Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#96 File: src/core/ClientGatewayLzReceiver.sol:62-64 Timestamp: 2024-09-23T09:37:05.474Z Learning: In the `ClientGatewayLzReceiver` contract, using `address(this).call` to invoke internal functions is acceptable, as it benefits general message handling and does not increase gas consumption.src/core/ExocoreGateway.sol (5)
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#96 File: src/core/ExocoreGateway.sol:459-466 Timestamp: 2024-09-25T02:26:07.302Z Learning: In the `ExocoreGateway` contract, when delegate/undelegate operations fail in `handleDelegation` and `handleDepositAndDelegate`, we should consume the request, emit the event, and not revert.Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#96 File: src/core/ExocoreGateway.sol:313-315 Timestamp: 2024-09-23T10:03:22.440Z Learning: When using `address(this).call(...)`, `msg.sender` becomes `address(this)`, so functions with the `onlyCalledFromThis` modifier will function correctly.Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#96 File: src/core/ExocoreGateway.sol:425-438 Timestamp: 2024-09-23T10:01:38.842Z Learning: In the `handleDelegation` function of `ExocoreGateway.sol`, the `operator` variable is converted from a string, so casting from `bytes` to `string` is safe because `operator` will always contain valid UTF-8 encoded characters.Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#96 File: src/core/ExocoreGateway.sol:305-306 Timestamp: 2024-09-23T09:39:44.021Z Learning: Within `ExocoreGateway.sol`, the `_validateMessageLength` function ensures that `message` has sufficient length, so additional length checks after calling `_validateMessageLength` are unnecessary.Learnt from: MaxMustermann2 PR: ExocoreNetwork/exocore-contracts#96 File: src/core/ExocoreGateway.sol:340-341 Timestamp: 2024-09-22T10:48:21.801Z Learning: In `ExocoreGateway`, the payload length is validated in `_lzReceive`, so additional checks in functions like `handleLSTTransfer` are unnecessary.test/foundry/DepositThenDelegateTo.t.sol (2)
Learnt from: MaxMustermann2 PR: ExocoreNetwork/exocore-contracts#96 File: test/foundry/DepositThenDelegateTo.t.sol:235-236 Timestamp: 2024-09-22T10:51:25.359Z Learning: In Foundry tests, when using `vm.expectEmit`, you should also include the manual `emit` statement to specify the expected event parameters.Learnt from: MaxMustermann2 PR: ExocoreNetwork/exocore-contracts#96 File: test/foundry/DepositThenDelegateTo.t.sol:163-165 Timestamp: 2024-09-22T10:51:00.426Z Learning: In Foundry tests, after calling `vm.expectEmit()`, it's correct to specify the expected event using `emit EventName(...)`.test/mocks/AssetsMock.sol (2)
Learnt from: bwhour PR: ExocoreNetwork/exocore-contracts#96 File: test/mocks/AssetsMock.sol:75-90 Timestamp: 2024-09-21T06:01:54.422Z Learning: In the `withdrawNST` function of `AssetsMock.sol`, the `validatorPubkey.length` should be 32 bytes, not 48 bytes.Learnt from: bwhour PR: ExocoreNetwork/exocore-contracts#96 File: test/mocks/AssetsMock.sol:37-49 Timestamp: 2024-09-21T06:01:19.718Z Learning: In the `depositNST` and `withdrawNST` functions, the `validatorPubkey` has a length of 32 bytes.test/mocks/ExocoreGatewayMock.sol (3)
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#96 File: test/mocks/ExocoreGatewayMock.sol:523-523 Timestamp: 2024-09-25T02:23:30.105Z Learning: In mock contracts like `ExocoreGatewayMock`, it's acceptable to leave function parameters as `memory` without optimizing to `calldata` for gas savings.Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#96 File: test/mocks/ExocoreGatewayMock.sol:338-366 Timestamp: 2024-09-23T09:39:05.445Z Learning: In the `ExocoreGateway`, deposit operations are intended to be must-succeed and should revert on failure, while withdrawal failures are acceptable and should not trigger a revert.Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#96 File: test/mocks/ExocoreGatewayMock.sol:144-149 Timestamp: 2024-09-23T09:40:29.126Z Learning: When reviewing mock contracts, optimizations for error handling are not necessary.
Additional comments not posted (44)
src/storage/GatewayStorage.sol (1)
4-4: Centralized error handling introduced.The import of the
Errorslibrary indicates a shift towards centralized error handling. This change is consistent with the removal of individual error definitions noted in the summary and should lead to more consistent error management across the codebase.src/storage/ExocoreGatewayStorage.sol (5)
4-6: Improved code organization with new importsThe addition of these imports suggests a well-structured approach to code organization:
ActionAttributeslibrary for action-related functionalityErrorsfor centralized error handlingActionandGatewayStoragefor base storage contract integrationThis refactoring enhances modularity and maintainability.
13-13: Appropriate use of ActionAttributes libraryThe addition of
using ActionAttributes for Action;is consistent with the new imports and allows for clean usage ofActionAttributeslibrary functions withActiontype variables. This enhances code readability and maintainability.
52-52: Well-structured ClaimRewardResult eventThe new
ClaimRewardResultevent provides comprehensive information about reward withdrawal operations. The use of indexed parameters forsuccess,token, andwithdrawerallows for efficient event filtering. This event structure is consistent with other events in the contract and enhances the contract's observability.
54-61: Efficient LSTTransfer event for deposit and withdrawal operationsThe new
LSTTransferevent efficiently consolidates both deposit and withdrawal operations for Liquid Staking Tokens (LST). The use of theisDepositboolean parameter allows for a single event to handle both scenarios, reducing code duplication. The event structure, including indexed parameters, is consistent with other events in the contract and provides comprehensive information about LST transfers.
64-71: Comprehensive NSTTransfer event for Native Staking Token operationsThe new
NSTTransferevent effectively captures Native Staking Token (NST) transfer operations. It follows a similar structure to theLSTTransferevent, maintaining consistency in the contract. The notable inclusion of thevalidatorPubkeyparameter is particularly relevant for NST operations, providing crucial information for tracking and auditing purposes. This event enhances the contract's ability to monitor and log NST-specific activities.test/mocks/AssetsMock.sol (6)
10-10: LGTM: New mapping for tracking validator set membershipThe addition of the
inValidatorSetmapping is appropriate for tracking whether a staker is part of a validator set. This aligns with the PR objectives to separate NST and LST operations.
21-34: LGTM: Improved LST deposit handlingThe changes to the
depositLSTfunction (previouslydepositTo) enhance its functionality and align with the PR objectives:
- The renaming clarifies the function's purpose.
- New requirements prevent NST deposits and ensure the token is registered.
- Input validation has been improved.
These changes effectively separate LST operations from NST operations.
37-49: LGTM: New function for NST depositsThe
depositNSTfunction is a good addition that aligns with the PR objectives:
- It correctly handles NST deposits using the
VIRTUAL_STAKED_ETH_ADDRESS.- It updates both
principalBalancesandinValidatorSetappropriately.- The implementation separates NST operations from LST operations as intended.
Note: While there's no validation for
validatorPubkeylength, this is acceptable for a mock contract used in testing.
51-72: LGTM: Enhanced LST withdrawal handlingThe changes to the
withdrawLSTfunction (previouslywithdrawPrincipal) improve its functionality and align with the PR objectives:
- The renaming clarifies the function's purpose.
- New checks prevent NST withdrawals and ensure the token is registered.
- The balance check is now performed before the withdrawal, which is a good practice.
These changes effectively separate LST operations from NST operations and improve the overall robustness of the function.
75-90: LGTM: New function for NST withdrawalsThe
withdrawNSTfunction is a valuable addition that aligns with the PR objectives:
- It correctly handles NST withdrawals using the
VIRTUAL_STAKED_ETH_ADDRESS.- It updates both
principalBalancesandinValidatorSetappropriately.- The implementation separates NST operations from LST operations as intended.
Note: While there's no validation for
validatorPubkeylength, this is acceptable for a mock contract used in testing.
Line range hint
1-91: Overall: Well-implemented changes for NST/LST separationThe changes in this mock contract effectively achieve the PR objectives of separating NST deposit/withdrawal from LST deposit/withdrawal. The new functions and modifications to existing ones provide a clear distinction between NST and LST operations, which will be beneficial for testing purposes.
Key improvements:
- Introduction of
inValidatorSetmapping for tracking validator set membership.- Clear separation of LST and NST operations in deposit and withdrawal functions.
- Consistent use of
VIRTUAL_STAKED_ETH_ADDRESSfor NST operations.The implementation is appropriate for a mock contract used in testing, striking a balance between functionality and simplicity.
src/core/ClientGatewayLzReceiver.sol (3)
6-10: Improved code organization with new importsThe addition of new imports, particularly
ActionAttributes,Action, andErrors, indicates a positive move towards better code organization and modularization. The use of anErrorslibrary suggests a centralized approach to error handling, which can improve consistency and maintainability across the codebase.
21-21: Improved code readability with ActionAttributesThe addition of the
using ActionAttributes for Action;directive is a good practice. It allows for more concise and readable code when working withActionenums by enabling the use of helper functions directly onActionvariables.
180-182: Consistent error handling in afterReceiveAddWhitelistTokenRequestThe use of the
Errorslibrary for handling invalid message length in theafterReceiveAddWhitelistTokenRequestfunction is a good change. It improves consistency in error reporting across the contract and aligns with the overall error handling strategy.test/foundry/DepositThenDelegateTo.t.sol (7)
6-6: Improved import statementThe import statement has been updated to include the
Actionenum along withGatewayStorage. This change is beneficial as it allows direct use of theActionenum in the test file, improving code readability and potentially reducing compilation time by importing only what's necessary.
25-34: Enhanced event structures for improved clarityThe new
LSTTransferandDelegationRequestevents provide more detailed and flexible representations of deposit, withdrawal, and delegation operations. The inclusion of boolean flags (isDepositandisDelegate) allows these events to cover multiple scenarios, reducing code duplication. Theacceptedstatus inDelegationRequestadds valuable information about the outcome of delegation attempts.These changes enhance the test's ability to verify complex scenarios and improve the overall clarity of the contract's behavior.
113-113: Simplified Action enum usageThe direct use of
Action.REQUEST_DEPOSIT_THEN_DELEGATE_TOinstead ofGatewayStorage.Action.REQUEST_DEPOSIT_THEN_DELEGATE_TOimproves code readability and is consistent with the updated import statement. This change reduces verbosity without sacrificing clarity.
136-137: Consistent use of Action enumThe
MessageSentevent emission now usesAction.REQUEST_DEPOSIT_THEN_DELEGATE_TOdirectly, maintaining consistency with the earlier changes. This approach ensures uniform usage of theActionenum throughout the test file, enhancing code consistency and readability.
Line range hint
152-218: Improved test function for successful request executionThe
_testRequestExecutionSuccessfunction (formerly_testResponse) has been significantly enhanced:
- The new name clearly indicates the function's purpose, improving code readability.
- Event expectations for
LSTTransferandDelegationRequesthave been added, aligning with the new event structures and providing more comprehensive testing.- The
MessageExecutedevent now uses theActionenum directly, maintaining consistency with earlier changes.These improvements enhance the test's ability to verify the correct behavior of successful deposit and delegation operations.
220-288: Comprehensive testing for request execution failure scenarioThe new
_testRequestExecutionFailurefunction is a valuable addition to the test suite:
- It simulates a scenario where the deposit succeeds but the delegation fails, covering an important edge case.
- The function uses
vm.mockCallto simulate the delegation failure, demonstrating good use of Foundry's mocking capabilities.- It verifies the correct emission of events and state changes in the failure scenario.
- The use of
vm.clearMockedCallsat the end ensures that the test environment is cleaned up, preventing potential interference with other tests.This function significantly enhances the robustness of the test suite by covering failure scenarios, which is crucial for ensuring the contract behaves correctly under all conditions.
Line range hint
1-288: Overall improvement in test structure and coverageThe changes made to this test file represent a significant improvement in both structure and coverage:
- The new event structures (
LSTTransferandDelegationRequest) provide more detailed and flexible representations of the contract's operations.- The addition of the
_testRequestExecutionFailurefunction enhances the test suite's robustness by covering failure scenarios.- Consistent use of the
Actionenum throughout the file improves readability and maintainability.- The renaming of test functions to more descriptive names (e.g.,
_testRequestExecutionSuccess) enhances code clarity.These changes collectively contribute to a more comprehensive and maintainable test suite, which is crucial for ensuring the reliability of the contract under various scenarios.
src/storage/BootstrapStorage.sol (3)
129-131: LGTM: Addition of VIRTUAL_NST_ADDRESS constantThe addition of the
VIRTUAL_NST_ADDRESSconstant is a good practice for handling native staking tokens alongside ERC20 tokens. The chosen address (0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE) is a widely recognized convention for representing native tokens in smart contracts.
324-324: LGTM: Improved error handling in _getVault functionThe replacement of the custom error
VaultNotExistwithErrors.VaultDoesNotExist()indicates a shift towards a centralized error handling system. This change enhances consistency across the contract and makes error management more maintainable.
338-340: LGTM: Prevention of vault deployment for native staking tokenThe addition of a check to prevent vault deployment for
VIRTUAL_NST_ADDRESSis a good safeguard. It ensures that no vault is created for the native staking token, which is typically handled differently from ERC20 tokens. This change is consistent with the introduction of theVIRTUAL_NST_ADDRESSconstant and maintains the contract's logical integrity.src/core/ExocoreGateway.sol (7)
80-89: Function selectors are correctly initializedThe
_initializeWhitelistFunctionSelectorsfunction correctly maps actions to their respective handler functions, ensuring proper routing of requests.
295-324: Message handling in_lzReceiveis correctly implementedThe
_lzReceivefunction appropriately validates the message length, decodes the action, calls the correct handler using the whitelist, and manages responses. Error handling is correctly implemented, ensuring that invalid requests are reverted.
335-356:handleLSTTransfercorrectly processes LST transfersThe
handleLSTTransferfunction correctly handles deposit and withdrawal of LST tokens. It appropriately distinguishes between deposit and withdrawal actions, interacts with theASSETS_CONTRACT, and emits theLSTTransferevent.
359-388:handleNSTTransfercorrectly processes NST transfersThe
handleNSTTransferfunction accurately processes deposit and withdrawal of NST tokens, interacting with theASSETS_CONTRACTand emitting theNSTTransferevent. The logic mirrors that ofhandleLSTTransfer, ensuring consistency.
425-438:handleDelegationhandles delegation requests appropriatelyThe
handleDelegationfunction correctly processes both delegation and undelegation actions. It interacts with theDELEGATION_CONTRACTand emits theDelegationRequestevent with the expected parameters.
453-466:handleDepositAndDelegatecorrectly executes combined operationsThe
handleDepositAndDelegatefunction properly performs both deposit and delegation operations in sequence. It correctly handles the success of the deposit and proceeds to delegate, emitting appropriate events. This aligns with the intended behavior as per previous discussions.
480-492:handleOperatorAssociationoperates correctlyThe
handleOperatorAssociationfunction effectively handles the associating and dissociating of operators based on the action type. It interacts with theDELEGATION_CONTRACT, handles success flags appropriately, and emits theAssociationResultevent.test/mocks/ExocoreGatewayMock.sol (12)
81-86: Validation added ininitializeenhances securityThe addition of a zero address check for
owner_and the use ofReentrancyGuardinitialization strengthen the contract's resilience against misconfiguration and reentrancy attacks.
95-106: Function selectors initialized correctly in_initializeWhitelistFunctionSelectorsThe mapping of action types to their respective handler functions ensures proper message routing and handling of various operations.
109-114: Addition ofpauseandunpausefunctions for contract controlImplementing these functions allows the contract owner to manage the contract's operational state effectively, enhancing control during maintenance or emergency situations.
242-258: Implementation of operator association inassociateOperatorWithEVMStakerThe function correctly encodes the staker's address and interfaces with the
DELEGATION_CONTRACTto associate an operator. Input validation and error handling are appropriately managed.
268-273: Implementation of operator dissociation indissociateOperatorFromEVMStakerThe function properly handles the dissociation of an operator from a staker, ensuring that the staker's address is correctly encoded and error conditions are handled.
322-333: Robust message handling in_lzReceiveThe function accurately extracts the action from incoming messages and dispatches them to the correct handler functions. It includes necessary validations and error handling for unsupported requests and call failures.
338-366: Correct handling of LST transfers inhandleLSTTransferThe function processes both deposit and withdrawal of Liquid Staking Tokens accurately, updating balances and emitting events as expected. The error handling aligns with the intended contract behavior for deposit operations.
369-397: Correct handling of NST transfers inhandleNSTTransferSimilar to LST handling, this function manages deposits and withdrawals of Native Staking Tokens appropriately, with consistent balance updates and event emissions.
400-418: Effective reward operation handling inhandleRewardOperationThe function successfully interfaces with the
CLAIM_REWARD_CONTRACTto process reward claims, ensuring that balances are updated and results are communicated back.
421-446: Proper delegation handling inhandleDelegationThe function accurately differentiates between delegation and undelegation actions, invoking the correct methods and handling responses appropriately.
449-473: Efficient combined operations inhandleDepositAndDelegateBy combining deposit and delegation into a single function, the contract ensures atomic execution of these related actions, maintaining consistency and efficiency.
476-498: Handling operator association requests inhandleOperatorAssociationThe function properly manages both association and dissociation of operators without the need for response messages, simplifying the interchain communication flow.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
docs/native_deposit_workflow.wsd (1)
61-61: Approved: Improved error message clarityThe change from "CapsuleNotExist" to "CapsuleDoesNotExist" improves the clarity and grammatical correctness of the error message. This enhancement contributes to better code readability and maintainability.
Consider reviewing and updating similar error messages throughout the codebase to ensure consistent grammatical correctness. This will further improve the overall code quality and developer experience.
docs/client-chain-contracts-design.md (1)
14-19: Approve changes with minor suggestions for improvementThe additions of Bootstrap, ExoCapsule, and NativeRestakingController, along with the reordering of components, significantly enhance the documentation's completeness and structure. The brief descriptions for each component provide a clear overview of their roles in the system.
Consider the following minor improvements:
- Line 14: Change "bootstraping" to "bootstrapping".
- Line 19: Change "It provide functions" to "It provides functions".
- Consider adding a blank line between each numbered item for improved readability.
🧰 Tools
LanguageTool
[uncategorized] ~14-~14: Loose punctuation mark.
Context: ... contracts architecture: 1.Bootstrap: The contract is used for bootstraping t...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~15-~15: Loose punctuation mark.
Context: ...e Exocore chain. 2.ClientChainGateway: This is the entry point where client ch...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~16-~16: Loose punctuation mark.
Context: ...om the Exocore validator set. 3.Vault: This is where user funds are taken into...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~17-~17: Loose punctuation mark.
Context: ...oneVault. 4.LSTRestakingController: The controller is responsible for manag...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~18-~18: Loose punctuation mark.
Context: ...mented by theGateway. 5.ExoCapsule: The contract is used as the withdrawal ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~19-~19: Loose punctuation mark.
Context: ...contract. 6.NativeRestakingController: The controller is responsible for manag...(UNLIKELY_OPENING_PUNCTUATION)
[grammar] ~19-~19: After ‘It’, use the third-person verb form “provides”.
Context: ...for managing multipleExoCapsules. It provide functions for Ethereum native restaking...(IT_VBZ)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
🔇 Files ignored due to path filters (6)
docs/delegate.drawio.svgis excluded by!**/*.svgdocs/delegate.svgis excluded by!**/*.svgdocs/deposit.drawio.svgis excluded by!**/*.svgdocs/deposit.svgis excluded by!**/*.svgdocs/withdraw.drawio.svgis excluded by!**/*.svgdocs/withdraw.svgis excluded by!**/*.svg
📒 Files selected for processing (2)
- docs/client-chain-contracts-design.md (2 hunks)
- docs/native_deposit_workflow.wsd (2 hunks)
🧰 Additional context used
LanguageTool
docs/client-chain-contracts-design.md
[uncategorized] ~14-~14: Loose punctuation mark.
Context: ... contracts architecture: 1.Bootstrap: The contract is used for bootstraping t...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~15-~15: Loose punctuation mark.
Context: ...e Exocore chain. 2.ClientChainGateway: This is the entry point where client ch...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~16-~16: Loose punctuation mark.
Context: ...om the Exocore validator set. 3.Vault: This is where user funds are taken into...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~17-~17: Loose punctuation mark.
Context: ...oneVault. 4.LSTRestakingController: The controller is responsible for manag...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~18-~18: Loose punctuation mark.
Context: ...mented by theGateway. 5.ExoCapsule: The contract is used as the withdrawal ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~19-~19: Loose punctuation mark.
Context: ...contract. 6.NativeRestakingController: The controller is responsible for manag...(UNLIKELY_OPENING_PUNCTUATION)
[grammar] ~19-~19: After ‘It’, use the third-person verb form “provides”.
Context: ...for managing multipleExoCapsules. It provide functions for Ethereum native restaking...(IT_VBZ)
🔇 Additional comments not posted (5)
docs/native_deposit_workflow.wsd (3)
38-39: Approved: Method names updated to reflect NST-specific operationsThe changes in method names from
requestDepositTotohandleNSTTransferanddepositTotodepositNSTalign well with the PR objective of separating Native Staking Token (NST) operations from Liquid Staking Token (LST) operations. These new method names are more specific and clearly indicate their purpose in handling NST transfers.
47-47: Approved: Optimized response handling with event emissionThe change from sending a response back to ExocoreL0Endpoint to emitting an NSTTransfer event aligns with the optimization goal mentioned in Issue #3. This modification potentially reduces gas costs by eliminating unnecessary responses and simplifies the workflow. It's a good step towards optimizing the LayerZero relaying fees.
Line range hint
1-139: Summary: UML diagram updates accurately reflect PR objectivesThe changes made to this UML diagram effectively represent the separation of Native Staking Token (NST) and Liquid Staking Token (LST) operations, optimize the workflow by reducing unnecessary responses, and improve error messaging. These modifications align well with the PR objectives and address concerns raised in the linked issues.
To ensure consistency across the codebase, please run the following script to check for any remaining instances of "CapsuleNotExist" that might need updating:
docs/client-chain-contracts-design.md (2)
Line range hint
1-266: Summary of documentation changesThe changes to this documentation file significantly improve its completeness and clarity. The addition of new components (Bootstrap, ExoCapsule, and NativeRestakingController) and the reordering of existing ones provide a more comprehensive overview of the system architecture. The simplification of the delegation workflow is a notable improvement that should enhance user experience.
While the changes are generally positive, there are a few areas that could benefit from minor refinements:
- Some grammatical improvements in the overview section.
- Clarification on error handling in the new delegation workflow.
Overall, these changes represent a substantial improvement to the documentation and should help developers better understand the system's architecture and workflows.
🧰 Tools
LanguageTool
[uncategorized] ~14-~14: Loose punctuation mark.
Context: ... contracts architecture: 1.Bootstrap: The contract is used for bootstraping t...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~15-~15: Loose punctuation mark.
Context: ...e Exocore chain. 2.ClientChainGateway: This is the entry point where client ch...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~16-~16: Loose punctuation mark.
Context: ...om the Exocore validator set. 3.Vault: This is where user funds are taken into...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~17-~17: Loose punctuation mark.
Context: ...oneVault. 4.LSTRestakingController: The controller is responsible for manag...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~18-~18: Loose punctuation mark.
Context: ...mented by theGateway. 5.ExoCapsule: The contract is used as the withdrawal ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~19-~19: Loose punctuation mark.
Context: ...contract. 6.NativeRestakingController: The controller is responsible for manag...(UNLIKELY_OPENING_PUNCTUATION)
[grammar] ~19-~19: After ‘It’, use the third-person verb form “provides”.
Context: ...for managing multipleExoCapsules. It provide functions for Ethereum native restaking...(IT_VBZ)
266-266: Approve simplified delegation workflow with a request for clarificationThe simplification of the delegation workflow to a single transaction improves the user experience and potentially enhances efficiency. The use of events for success/failure notification is a common and effective pattern in blockchain systems.
Could you please clarify the error handling mechanism in this new workflow? Specifically:
- How will users be notified of failures if there's no direct response from Exocore?
- Are there any plans to implement a way for the client-side application to listen for and interpret the events emitted by ExocoreGateway?
To verify the event emission, we can run the following script:
✅ Verification successful
Delegation event emissions verified successfully
The
ExocoreGatewaycontract emits events such asDelegationRequestandAssociationResultwith success indicators, effectively communicating the status of delegation operations without requiring a direct response.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the event emission in ExocoreGateway for delegation status # Test: Search for event emission related to delegation status ast-grep --lang solidity --pattern 'emit $_(string $_, bool $_)'Length of output: 183
Script:
#!/bin/bash # Description: Verify the event emission in ExocoreGateway for delegation status # Find Solidity files related to ExocoreGateway and search for 'emit' statements fd -e solidity | grep -i 'ExocoreGateway' | xargs rg 'emit\s+\w+\s*\('Length of output: 17828
Description
closes: #65
closes: #3
closes: #57
closes: #77
To enable updating beacon chain balance by oracle module, we should send the validator pubkey to Exocore to be recorded. So we should separate NST(native staking token) message from LST message when doing deposit/withdrawal from client chain to Exocore
This PR involves relatively big refactoring to support adding more message types and message handlers, as well as making the code better organized. For reviewer's convenience, I would list the scope and list of tasks for this pr:
Scope
The refactoring mainly happens to:
ClientGatewayLzReceiver: refactor thehandleResponsefunction to simplify the process, since only withdrawal actions need response from Exocore after we remove the principal balances and reward balances fromVaultExocoreGateway: refactor the originalrequestXXXmessage handlers ashandleXXXto remove redundant logics(though there are still some repeated logics) and add new message handlers to support NST deposit/withdrawal, and also remove thetry/catchwrapper when calling precompiles, because:a.
try/catchis actually not working since we cannot catch the revert caused by precompileb. precompiled contracts are not outside contracts and we can easily avoid revert happening by not returning errors for precompile handlers
NativeRestakingController: encode validator's pubkey into the message when doing deposit/withdrawal for native restakingVaultandExoCapsule: remove theprincipalBalancesandrewardBalancesstates since we have decided to not storing them on client chain side and they are not used as constraints in any scenario like depoist/withdraw/delegate/undelegateActionAttributes: use bitmap to represent each action's attributes and message length, so that it might be less error prone and more explicitIAssetsandIDelegationprecompiles: add new interfaces to support deposit/withdraw NST with validator's pubkey, and revise some function names to make them more conciseTargeted Tasks
principalBalancesandrewardBalancesfromVaultStorageandExoCapsuleStorage, and remove corresponding update functions from interfaces, vault/exocapsule implementations and even caller contracts(BootstrapandClientGatewayLzReceiver)ActionAttributesto represent action attributes as bitmaps and optimize the redundant functionsSummary by CodeRabbit
Release Notes
New Features
Bootstrap,ExoCapsule, andNativeRestakingControllerto enhance the Exocore system.Bug Fixes
Errorslibrary, providing clearer feedback for various scenarios.Refactor
Documentation