feat(exo-gateway): add oracleInfo #86
Conversation
ExocoreNetwork/exocore#159 adds a new `oracleInfo` parameter to the `IAssets.sol` precompile when registering tokens. This PR follows that change. Previously, multiple tokens could be registered with a single call to ExocoreGateway. However, that resulted in too many variables (and too much gas) for Solidity to handle within a single function. To that end, and keeping in mind that addition of new tokens isn't likely to be a frequent occurrence, the function's capabilities have been tempered to support only a single token within the transaction. As a side effect, this fixes ExocoreNetwork#80
WalkthroughThe changes involve a comprehensive refactor of the token whitelisting functionality across multiple contracts. The updates transition from bulk handling of tokens to a more granular approach, allowing for the addition and updating of individual tokens. This includes modifications to function signatures, error handling, and internal logic, ensuring that each token can be processed with its associated metadata and oracle information. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ExocoreGateway
participant ClientChain
User->>ExocoreGateway: addWhitelistToken(token, metadata, oracleInfo)
ExocoreGateway->>ClientChain: REQUEST_ADD_WHITELIST_TOKEN(token)
ClientChain-->>ExocoreGateway: Confirmation
ExocoreGateway-->>User: Success
Assessment against linked issues
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (19)
- script/3_Setup.s.sol (2 hunks)
- src/core/Bootstrap.sol (1 hunks)
- src/core/ClientChainGateway.sol (1 hunks)
- src/core/ClientGatewayLzReceiver.sol (1 hunks)
- src/core/ExocoreGateway.sol (2 hunks)
- src/interfaces/IExocoreGateway.sol (1 hunks)
- src/interfaces/precompiles/IAssets.sol (2 hunks)
- src/libraries/Errors.sol (2 hunks)
- src/storage/ExocoreGatewayStorage.sol (1 hunks)
- src/storage/GatewayStorage.sol (1 hunks)
- test/foundry/Delegation.t.sol (2 hunks)
- test/foundry/DepositThenDelegateTo.t.sol (2 hunks)
- test/foundry/DepositWithdrawPrinciple.t.sol (4 hunks)
- test/foundry/ExocoreDeployer.t.sol (2 hunks)
- test/foundry/WithdrawReward.t.sol (1 hunks)
- test/foundry/unit/ExocoreGateway.t.sol (2 hunks)
- test/mocks/AssetsMock.sol (1 hunks)
- test/mocks/ExocoreGatewayMock.sol (1 hunks)
- test/mocks/NonShortCircuitEndpointV2Mock.sol (1 hunks)
Files skipped from review due to trivial changes (2)
- src/storage/ExocoreGatewayStorage.sol
- test/mocks/NonShortCircuitEndpointV2Mock.sol
Additional context used
Learnings (7)
src/interfaces/IExocoreGateway.sol (1)
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#39 File: test/foundry/unit/ExocoreGateway.t.sol:360-366 Timestamp: 2024-07-02T01:45:58.683Z Learning: The `updateWhitelistedTokens` function in `ExocoreGateway` interacts solely with the trusted `ASSET_CONTRACT` precompile, mitigating the need for a reentrancy guard.script/3_Setup.s.sol (1)
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#23 File: script/3_Setup.s.sol:87-89 Timestamp: 2024-06-17T07:40:21.280Z Learning: For the `SetupScript` in `script/3_Setup.s.sol`, a single token is used intentionally for testing purposes. Multiple tokens will be handled as needed in the future.src/core/ClientGatewayLzReceiver.sol (1)
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#39 File: src/core/ClientGatewayLzReceiver.sol:0-0 Timestamp: 2024-07-01T09:41:18.389Z Learning: The `afterReceiveAddWhitelistTokensRequest` function in `ClientGatewayLzReceiver.sol` includes a check to ensure a token is not already whitelisted before proceeding with any operations like adding to the whitelist or deploying a vault.test/mocks/ExocoreGatewayMock.sol (1)
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#39 File: test/foundry/unit/ExocoreGateway.t.sol:360-366 Timestamp: 2024-07-02T01:45:58.683Z Learning: The `updateWhitelistedTokens` function in `ExocoreGateway` interacts solely with the trusted `ASSET_CONTRACT` precompile, mitigating the need for a reentrancy guard.src/core/Bootstrap.sol (3)
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#23 File: src/core/Bootstrap.sol:0-0 Timestamp: 2024-06-17T02:56:27.617Z Learning: Tokens are not removed from the whitelist in the `Bootstrap.sol`, simplifying the management of whitelisted tokens.Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#23 File: src/core/Bootstrap.sol:0-0 Timestamp: 2024-06-17T02:55:34.831Z Learning: The `removeWhitelistTokens` function was removed from the `Bootstrap.sol` due to design overhead and limited feature benefits, with a possibility of reconsideration in the future if needed.Learnt from: MaxMustermann2 PR: ExocoreNetwork/exocore-contracts#17 File: src/core/Bootstrap.sol:159-172 Timestamp: 2024-06-05T04:48:13.734Z Learning: The `addWhitelistToken` and `removeWhitelistToken` functions in `Bootstrap.sol` are intended to be called externally by the deployer, and thus are correctly set as `public`.src/core/ExocoreGateway.sol (1)
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#39 File: test/foundry/unit/ExocoreGateway.t.sol:360-366 Timestamp: 2024-07-02T01:45:58.683Z Learning: The `updateWhitelistedTokens` function in `ExocoreGateway` interacts solely with the trusted `ASSET_CONTRACT` precompile, mitigating the need for a reentrancy guard.test/foundry/unit/ExocoreGateway.t.sol (1)
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#39 File: test/foundry/unit/ExocoreGateway.t.sol:360-366 Timestamp: 2024-07-02T01:45:58.683Z Learning: The `updateWhitelistedTokens` function in `ExocoreGateway` interacts solely with the trusted `ASSET_CONTRACT` precompile, mitigating the need for a reentrancy guard.
Additional comments not posted (20)
src/interfaces/IExocoreGateway.sol (1)
44-64: Review ofaddOrUpdateWhitelistTokenFunctionThe function
addOrUpdateWhitelistTokenhas been refactored to handle only a single token per transaction, which aligns with the PR objectives to reduce complexity and gas costs. The addition of theoracleInfoparameter is a significant enhancement, allowing for richer metadata to be associated with each token.Key Observations:
- The function is now marked as
payable, which is appropriate given that it may involve sending messages that could incur gas costs.- The documentation has been updated to reflect the new functionality and parameters.
- The function ensures that the chain must be registered before adding tokens, which is a good security practice to prevent unauthorized token registration.
Suggestions:
- Ensure that all client-side and other contract interactions that previously depended on the old function are updated to accommodate these changes.
- Consider adding more detailed error handling or revert messages to provide clearer feedback in case of failures, especially related to the new
oracleInfoparameter.Overall, the changes are well-aligned with the project's goals and appear to be implemented correctly.
test/mocks/AssetsMock.sol (1)
74-75: Review ofregisterOrUpdateTokensFunction inAssetsMockThe addition of the
oracleInfoparameter to theregisterOrUpdateTokensfunction is a necessary update to align the mock with the changes in the main contract. This will ensure that tests can accurately reflect the new functionality.Key Observations:
- The function correctly requires that the chain be registered before tokens can be registered or updated, which is consistent with the main contract's behavior.
- The function returns a boolean indicating success and another indicating whether the token was updated, which is useful for testing scenarios.
Suggestions:
- Ensure that test cases are updated to pass the new
oracleInfoparameter and to verify its handling within the function.- Consider adding specific test cases to verify the behavior when
oracleInfois provided, especially edge cases such as empty strings or invalid formats.Overall, the changes are correctly implemented and should support effective testing of the new token registration functionality.
src/storage/GatewayStorage.sol (1)
18-18: Review of Constant Renaming inGatewayStorageThe renaming of the constant from
REQUEST_ADD_WHITELIST_TOKENStoREQUEST_ADD_WHITELIST_TOKENis a minor but important change that reflects the new functionality of handling a single token per transaction. This change should be reflected wherever the constant is used to ensure consistency across the codebase.Suggestions:
- Verify that all references to the old constant name are updated to the new name in the entire codebase. This includes checking event emissions, function calls, and any conditional checks that use this constant.
- Consider adding a comment or documentation to explain why the constant was renamed, especially if the historical context might be relevant for future maintenance or debugging.
Overall, the renaming is consistent with the project's goals and appears to be correctly implemented.
src/interfaces/precompiles/IAssets.sol (1)
Line range hint
68-78: Approve addition oforacleInfoparameter but verify integration.The addition of the
oracleInfoparameter to theregisterOrUpdateTokensfunction is well-documented and seems appropriate for the intended enhancements in token management. However, it's crucial to verify that this new parameter is correctly integrated across all relevant parts of the system where this function is invoked.Run the following script to verify the integration of the
oracleInfoparameter:Verification successful
Integration of
oracleInfoparameter verified successfully.The
oracleInfoparameter is correctly integrated into theregisterOrUpdateTokensfunction, both in its definition and invocation. The parameter is passed as expected, confirming that the changes are properly implemented. No further action is required.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of the `oracleInfo` parameter in all relevant parts of the system. # Test: Search for the function usage. Expect: Only occurrences with the new parameter. rg --type solidity -A 5 $'registerOrUpdateTokens'Length of output: 81
Script:
#!/bin/bash # Locate all Solidity files and search for the function usage within them. fd -e sol -x rg 'registerOrUpdateTokens' {}Length of output: 277
Script:
#!/bin/bash # Search for invocations of the function with additional context to verify parameter usage. fd -e sol -x rg 'registerOrUpdateTokens' -A 10 {}Length of output: 1393
test/foundry/WithdrawReward.t.sol (1)
74-74: Approve nonce change but verify impact on test scenarios.The change in
withdrawResponseNoncefrom2to3is a sensible adjustment to address nonce reuse issues. However, it's important to verify that this change does not introduce any unintended side effects in the test scenarios where this nonce is used.Run the following script to verify the impact of the nonce change on test scenarios:
Verification successful
Nonce change verified and correctly implemented.
The change of
withdrawResponseNoncefrom2to3inWithdrawReward.t.solis consistently applied and does not conflict with other test scenarios. The nonce is used appropriately for generating unique identifiers and emitting events, ensuring the integrity of the test logic. No further action is required.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the nonce change on test scenarios. # Test: Search for the nonce usage in test scenarios. Expect: Consistent usage of the new value. rg --type solidity -A 5 $'withdrawResponseNonce'Length of output: 80
Script:
#!/bin/bash # Description: Verify the impact of the nonce change on test scenarios by searching for its usage across all files. # Test: Search for the nonce usage in the codebase. Expect: Consistent usage of the new value. rg -A 5 'withdrawResponseNonce'Length of output: 7833
src/core/ClientChainGateway.sol (1)
72-73: Approve mapping key change but verify consistency across related functionalities.The change in the
_whiteListFunctionSelectorsmapping key fromREQUEST_ADD_WHITELIST_TOKENStoREQUEST_ADD_WHITELIST_TOKENreflects a more focused approach to handling whitelist token requests. However, it's crucial to verify that this change is consistently reflected across all related functionalities to avoid discrepancies.Run the following script to verify the consistency of the mapping key change across related functionalities:
script/3_Setup.s.sol (1)
3-3: Approved import addition.The addition of
GatewayStorageis appropriate given the new functionality for handling tokens individually, which likely requires more complex storage interactions.src/libraries/Errors.sol (1)
26-29: Approved error message enhancements.The updates to the
NonZeroValue,ZeroValue, andClientChainGatewayAlreadyWhitelistederrors improve clarity and provide necessary context, which enhances the debuggability and maintainability of the contracts.Also applies to: 126-126
test/foundry/DepositThenDelegateTo.t.sol (1)
49-49: Verify the correctness of theresponseLzNonceadjustment.The change in the
responseLzNoncefrom2to3is noted to correspond with the whitelisting of two tokens. Ensure that this adjustment accurately reflects the expected sequence of operations and does not disrupt the contract's interactions with other components.Consider adding tests or checks to confirm that the new nonce value functions as intended in all scenarios.
Also applies to: 88-88
test/foundry/Delegation.t.sol (2)
246-246: Approved change inundelegateResponseNonce.The increment in
undelegateResponseNoncefrom 3 to 4 is noted. Ensure that this change aligns with the expected sequence of operations and does not disrupt other functionalities.Run the following script to verify the impact of this nonce change on the system:
123-123: Approved change indelegateResponseNonce.The increment in
delegateResponseNoncefrom 2 to 3 is noted. Ensure that this change aligns with the expected sequence of operations and does not disrupt other functionalities.Run the following script to verify the impact of this nonce change on the system:
Verification successful
Change in
delegateResponseNonceis consistent with test logic.The increment of
delegateResponseNoncefrom 2 to 3 aligns with the test's logic, as it is used in generating response IDs and emitting events. This change appears to be part of a broader scenario to verify the correct handling of delegation responses. Ensure that this change is reflected in any related documentation or test expectations.
- Instances found in
test/foundry/Delegation.t.solconfirm the nonce's role in delegation operations.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the nonce change on delegate operations. # Test: Search for the usage of `delegateResponseNonce`. Expect: Proper handling and sequence. rg --type solidity -A 5 $'delegateResponseNonce'Length of output: 80
Script:
#!/bin/bash # Description: Verify the impact of the nonce change on delegate operations. # Correct the file type for Solidity and search for the usage of `delegateResponseNonce`. rg --type-add 'solidity:*.sol' --type solidity -A 5 'delegateResponseNonce'Length of output: 4474
test/foundry/ExocoreDeployer.t.sol (1)
139-186: Review oftest_AddWhitelistTokensFunction: Enhancements and SuggestionsThe function
test_AddWhitelistTokenshas been updated to handle new metadata and oracle information arrays. Here are some observations and suggestions:
Initialization of Arrays: The arrays
metaDatas,oracleInfos,payloads, andrequestIdsare initialized correctly. However, consider validating the input sizes if they are expected to be provided externally to ensure they match the expected length for processing.Loop Processing: Each token is processed in a loop where payloads are encoded, and request IDs are generated. This loop handles the core functionality but does not include error handling for potential failures in
addOrUpdateWhitelistToken. Consider adding try-catch blocks to handle and log errors gracefully.Event Emissions: The function emits several events which are crucial for tracking operations on the blockchain. Ensure that all necessary data is included in the events for complete traceability.
Security Considerations: The use of
abi.encodePackedand custom UID generation needs to be audited for security implications, especially around potential collisions or predictability in UIDs.Optimization Opportunities: Review the gas cost associated with the loop operations, especially the external calls and event emissions. If possible, optimize by reducing state changes or reordering operations to minimize gas usage.
Overall, the function aligns with the PR's objectives but could benefit from enhanced error handling and security audits.
src/core/Bootstrap.sol (1)
198-201: Simplified Token Whitelisting Process inBootstrapContractThe
Bootstrapcontract has been modified to always deploy a vault when a token is added to the whitelist, removing the previous check for an existing vault. This simplification enhances clarity and reduces the complexity of the code. However, it may lead to the deployment of unnecessary vaults if tokens are re-added or mistakenly handled.Suggestions:
- Audit for Reentrancy: Ensure that the new vault deployment process is protected against reentrancy attacks, especially since the vault deployment involves external calls.
- Resource Management: Monitor the deployment of vaults to prevent unnecessary resource usage. Consider implementing a mechanism to check for and reuse existing vaults if they already exist for a token, potentially reintroducing a check in a more efficient manner.
This change streamlines operations but should be carefully managed to avoid unintended consequences in resource usage and security.
test/foundry/DepositWithdrawPrinciple.t.sol (2)
199-199: Verify the impact of the nonce change for withdrawal responses.The nonce for withdrawal responses has been updated from 3 to 4. Please ensure that this change aligns with the expected sequence of transaction responses and does not disrupt the contract's interaction with the LayerZero endpoint.
114-114: Verify the impact of the nonce change for deposit responses.The nonce for deposit responses has been updated from 2 to 3. Please ensure that this change aligns with the expected sequence of transaction responses and does not disrupt the contract's interaction with the LayerZero endpoint.
test/foundry/unit/ExocoreGateway.t.sol (5)
669-682: Verify the correctness of thetest_Success_UpdateWhitelistTokenfunction implementation.The
test_Success_UpdateWhitelistTokenfunction has been updated to validate updates to a single token. Please verify that the test correctly validates the update of a single token and that the expected events are emitted as specified.Verification successful
The
test_Success_UpdateWhitelistTokenfunction is correctly implemented.The function sets up the initial state, performs the update, and checks for the expected event emission, ensuring that the update of a single token is validated correctly. No issues were found with the implementation.
- The function calls
test_Success_AddWhiteListToken()to set up the initial state.- It uses
vm.startPrankto simulate a transaction from the correct address.- It expects the
WhitelistTokenUpdatedevent to be emitted, verifying the update.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the `test_Success_UpdateWhitelistToken` function implementation. # Test: Check the test implementation. Expect: Correct validation of a single token update. rg --type solidity -A 5 $'function test_Success_UpdateWhitelistToken'Length of output: 103
Script:
#!/bin/bash # Description: Verify the correctness of the `test_Success_UpdateWhitelistToken` function implementation. # Test: Check the test implementation. Expect: Correct validation of a single token update. rg -A 10 'function test_Success_UpdateWhitelistToken'Length of output: 1015
517-525: Verify the correctness of theMESSAGE_LENGTHandnativeFeeinitialization.The
MESSAGE_LENGTHconstant has been introduced to reflect the new message structure, and thenativeFeeis initialized based on this length. Please ensure that theMESSAGE_LENGTHis correctly set to accommodate the action and token address, and verify that thenativeFeecalculation is accurate.Verification successful
Verification of
MESSAGE_LENGTHandnativeFeeInitializationThe
MESSAGE_LENGTHis correctly set to1 + 32, which accommodates the action and token address as expected. ThenativeFeeis calculated using thisMESSAGE_LENGTH, ensuring that the fee is based on the updated message structure. This confirms that the initialization logic is accurate.
MESSAGE_LENGTHis set to1 + 32.nativeFeeis calculated usingMESSAGE_LENGTH.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the `MESSAGE_LENGTH` and `nativeFee` initialization. # Test: Check the initialization logic. Expect: Correct values for `MESSAGE_LENGTH` and `nativeFee`. rg --type solidity -A 5 $'uint256 MESSAGE_LENGTH = 1 + 32'Length of output: 92
Script:
#!/bin/bash # Find all Solidity files and search for the `MESSAGE_LENGTH` initialization. fd --extension sol --exec rg 'uint256 MESSAGE_LENGTH = 1 + 32' {}Length of output: 69
Script:
#!/bin/bash # Find all Solidity files and search for any occurrences of `MESSAGE_LENGTH`. fd --extension sol --exec rg 'MESSAGE_LENGTH' {}Length of output: 208
602-616: Verify the correctness of theTokenDetailsstruct initialization.The
TokenDetailsstruct has been introduced to encapsulate the properties of a token, and thesetUpfunction initializes this struct with the properties of a token. Please verify that the struct is correctly initialized and that the properties (e.g., token address, decimals, TVL limit) are accurately set.Verification successful
The
TokenDetailsstruct is correctly initialized in thesetUpfunction.The fields in the
TokenDetailsstruct match the initialization in thesetUpfunction, ensuring that the properties are accurately set. No issues were found with the initialization.
- Struct fields:
tokenAddress,decimals,tvlLimit,name,metaData,oracleInfo.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the `TokenDetails` struct initialization. # Test: Check the struct initialization logic. Expect: Correct initialization of token properties. rg --type solidity -A 5 $'struct TokenDetails'Length of output: 80
Script:
#!/bin/bash # Search for the definition of the `TokenDetails` struct in the codebase. rg 'struct TokenDetails' -A 10Length of output: 679
530-548: Verify the correctness of theaddOrUpdateWhitelistTokenfunction implementation.The
addOrUpdateWhitelistTokenfunction has been updated to handle a single token. Please verify that the function correctly implements the addition or update of a single token and that the revert conditions (e.g., zero address token, zero TVL limit) are appropriate for the new logic.Verification successful
The
addOrUpdateWhitelistTokenfunction implementation is correct.The function correctly handles a single token with appropriate revert conditions for zero address and zero TVL limit. The access control and execution safety are ensured through the use of modifiers. The implementation aligns with the expected behavior for managing a single token.
- Files:
src/core/ExocoreGateway.soltest/mocks/ExocoreGatewayMock.solsrc/interfaces/IExocoreGateway.solScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the `addOrUpdateWhitelistToken` function implementation. # Test: Check the function implementation and revert conditions. Expect: Correct handling of a single token. rg --type solidity -A 5 $'function addOrUpdateWhitelistToken'Length of output: 95
Script:
#!/bin/bash # Locate Solidity files and search for the `addOrUpdateWhitelistToken` function. fd -e sol | xargs rg 'function addOrUpdateWhitelistToken' -A 10Length of output: 2345
560-574: Verify the correctness of thetest_Success_AddWhiteListTokenfunction implementation.The
test_Success_AddWhiteListTokenfunction has been updated to reflect the new single-token logic. Please verify that the test correctly validates the addition of a single token and that the expected events are emitted as specified.Verification successful
The
test_Success_AddWhiteListTokenfunction is correctly implemented.The function accurately tests the addition of a single token to the whitelist by setting up the environment, performing the action, and verifying the expected events. The use of
vm.startPrankandvm.stopPrankensures proper simulation of the environment. No issues were found with the implementation.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the `test_Success_AddWhiteListToken` function implementation. # Test: Check the test implementation. Expect: Correct validation of a single token addition. rg --type solidity -A 5 $'function test_Success_AddWhiteListToken'Length of output: 100
Script:
#!/bin/bash # Use ast-grep to search for the `test_Success_AddWhiteListToken` function in Solidity files. ast-grep --lang solidity --pattern $'function test_Success_AddWhiteListToken() { $$$ }'Length of output: 205
Script:
#!/bin/bash # Locate Solidity files and search for the `test_Success_AddWhiteListToken` function. fd --extension sol | xargs rg 'function test_Success_AddWhiteListToken'Length of output: 167
Script:
#!/bin/bash # Extract the full implementation of the `test_Success_AddWhiteListToken` function with additional context. rg 'function test_Success_AddWhiteListToken' -A 20 test/foundry/unit/ExocoreGateway.t.solLength of output: 1055
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- test/mocks/NonShortCircuitEndpointV2Mock.sol (1 hunks)
Files skipped from review due to trivial changes (1)
- test/mocks/NonShortCircuitEndpointV2Mock.sol
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- src/core/Bootstrap.sol (1 hunks)
- src/core/ClientGatewayLzReceiver.sol (1 hunks)
- src/libraries/Errors.sol (2 hunks)
- test/foundry/ExocoreDeployer.t.sol (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- src/core/Bootstrap.sol
- src/libraries/Errors.sol
- test/foundry/ExocoreDeployer.t.sol
Additional context used
Learnings (1)
src/core/ClientGatewayLzReceiver.sol (1)
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#39 File: src/core/ClientGatewayLzReceiver.sol:0-0 Timestamp: 2024-07-01T09:41:18.389Z Learning: The `afterReceiveAddWhitelistTokensRequest` function in `ClientGatewayLzReceiver.sol` includes a check to ensure a token is not already whitelisted before proceeding with any operations like adding to the whitelist or deploying a vault.
Additional comments not posted (1)
src/core/ClientGatewayLzReceiver.sol (1)
299-320: The refactoredafterReceiveAddWhitelistTokenRequestfunction looks good.The function has been successfully refactored to handle a single token, which aligns with the PR's objectives to simplify the process and reduce gas costs. The checks for zero address and already whitelisted tokens are appropriate and enhance the function's robustness.
The function's visibility as
publicis not a security concern because theonlyCalledFromThismodifier effectively restricts the function's access, ensuring that it can only be called internally via low-level calls.
Since only certain parameters can be updated (TVL limit and metadata), it does not make sense to use the same function for token additions and updates
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- script/3_Setup.s.sol (2 hunks)
- src/core/ExocoreGateway.sol (2 hunks)
- src/interfaces/IExocoreGateway.sol (1 hunks)
- src/interfaces/precompiles/IAssets.sol (1 hunks)
- src/storage/ExocoreGatewayStorage.sol (1 hunks)
- test/foundry/ExocoreDeployer.t.sol (2 hunks)
- test/foundry/unit/ClientChainGateway.t.sol (1 hunks)
- test/foundry/unit/ExocoreGateway.t.sol (2 hunks)
- test/mocks/AssetsMock.sol (1 hunks)
- test/mocks/ExocoreGatewayMock.sol (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- script/3_Setup.s.sol
- src/interfaces/precompiles/IAssets.sol
- src/storage/ExocoreGatewayStorage.sol
- test/foundry/ExocoreDeployer.t.sol
- test/foundry/unit/ExocoreGateway.t.sol
Additional context used
Learnings (4)
src/interfaces/IExocoreGateway.sol (2)
Learnt from: MaxMustermann2 PR: ExocoreNetwork/exocore-contracts#86 File: src/core/ExocoreGateway.sol:180-225 Timestamp: 2024-08-31T15:43:42.705Z Learning: Non-payable token registrations are not possible in the `addOrUpdateWhitelistToken` function. A fee must be provided when registering a token to send the interchain message, while no fee is required for updates.Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#39 File: test/foundry/unit/ExocoreGateway.t.sol:360-366 Timestamp: 2024-07-02T01:45:58.683Z Learning: The `updateWhitelistedTokens` function in `ExocoreGateway` interacts solely with the trusted `ASSET_CONTRACT` precompile, mitigating the need for a reentrancy guard.test/foundry/unit/ClientChainGateway.t.sol (1)
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#39 File: test/foundry/unit/ExocoreGateway.t.sol:360-366 Timestamp: 2024-07-02T01:45:58.683Z Learning: The `updateWhitelistedTokens` function in `ExocoreGateway` interacts solely with the trusted `ASSET_CONTRACT` precompile, mitigating the need for a reentrancy guard.test/mocks/ExocoreGatewayMock.sol (2)
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#39 File: test/foundry/unit/ExocoreGateway.t.sol:360-366 Timestamp: 2024-07-02T01:45:58.683Z Learning: The `updateWhitelistedTokens` function in `ExocoreGateway` interacts solely with the trusted `ASSET_CONTRACT` precompile, mitigating the need for a reentrancy guard.Learnt from: MaxMustermann2 PR: ExocoreNetwork/exocore-contracts#86 File: src/core/ExocoreGateway.sol:180-225 Timestamp: 2024-08-31T15:43:42.705Z Learning: Non-payable token registrations are not possible in the `addOrUpdateWhitelistToken` function. A fee must be provided when registering a token to send the interchain message, while no fee is required for updates.src/core/ExocoreGateway.sol (2)
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#39 File: test/foundry/unit/ExocoreGateway.t.sol:360-366 Timestamp: 2024-07-02T01:45:58.683Z Learning: The `updateWhitelistedTokens` function in `ExocoreGateway` interacts solely with the trusted `ASSET_CONTRACT` precompile, mitigating the need for a reentrancy guard.Learnt from: MaxMustermann2 PR: ExocoreNetwork/exocore-contracts#86 File: src/core/ExocoreGateway.sol:180-225 Timestamp: 2024-08-31T15:43:42.705Z Learning: Non-payable token registrations are not possible in the `addOrUpdateWhitelistToken` function. A fee must be provided when registering a token to send the interchain message, while no fee is required for updates.
Additional comments not posted (10)
src/interfaces/IExocoreGateway.sol (3)
44-63: LGTM!The code changes are approved. The function has been correctly updated to handle a single token registration per transaction, and the payable modifier has been added to handle the fee for sending the interchain message.
46-46: Address the comments from the previous review.The comments from the previous review are still valid and applicable. It would be helpful to add comments to indicate which parameters can be updated.
66-75: LGTM!The code changes are approved. The function is correctly implemented and follows the learnings. It is not payable because it does not send a cross-chain message, and no fee is required for updates. The function interacts solely with the trusted
ASSET_CONTRACTprecompile, mitigating the need for a reentrancy guard.test/mocks/AssetsMock.sol (2)
68-85: LGTM!The code changes to the
registerTokenfunction are approved. The changes align with the PR objectives and the AI-generated summary. The function correctly handles the case when the token is already registered by returning false, and registers the token if it is not already registered. The function also correctly checks if the chain is registered before registering the token.
87-97: LGTM!The code changes to the
updateTokenfunction are approved. The changes align with the PR objectives and the AI-generated summary. The function correctly handles the case when the token is not registered by returning false. The function also correctly checks if the chain is registered before updating the token.test/foundry/unit/ClientChainGateway.t.sol (2)
377-387: LGTM!The code changes reflect a shift from batch processing to individual token handling. Encoding a single token and incrementing the nonce before processing the second token enhances the control flow and ensures each request is uniquely identified.
Line range hint
390-395: LGTM!The test case correctly asserts the expected revert condition when trying to withdraw
VIRTUAL_STAKED_ETH.test/mocks/ExocoreGatewayMock.sol (2)
178-216: LGTM!The
addWhitelistTokenfunction has been successfully refactored to handle a single token, which aligns with the PR's objectives to simplify the process and improve the management of whitelist tokens. The inclusion of checks for zero address, non-zero tvlLimit, and non-empty name, metaData, and oracleInfo are appropriate and enhance the function's robustness.
219-234: LGTM!The newly added
updateWhitelistTokenfunction complements the refactoredaddWhitelistTokenfunction by providing a way to update the attributes of a single token in the whitelist. The function follows a similar structure and includes appropriate validation checks.src/core/ExocoreGateway.sol (1)
180-219: Refactoring ofaddWhitelistTokenfunction aligns with the PR's objectives.The refactoring of the
addWhitelistTokenfunction to handle a single token per transaction instead of multiple tokens aligns with the PR's objectives to reduce complexity and gas costs. The updated function signature, internal logic, and the addition of theoracleInfoparameter simplify the token registration process.Verify that the
oracleInfoparameter is being utilized correctly in theASSETS_CONTRACT.registerTokenfunction to ensure that the necessary token metadata is being stored.The learnings indicate that non-payable token registrations are not possible, and a fee must be provided when registering a token to send the interchain message. Verify that this requirement is being enforced correctly in the function.
Run the following script to verify the
oracleInfousage and the handling ofmsg.value:
|
To be merged along ExocoreNetwork/exocore#159 which may have some changes |
|
@coderabbitai review |
Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
src/core/ExocoreGateway.sol (1)
180-216: Refactoring ofaddWhitelistTokenfunction: Approved with a minor suggestionThe refactoring of the
addWhitelistTokenfunction to handle a single token registration per transaction aligns with the PR objective and simplifies the process. The parameter validations, event emission, and interchain messaging logic are implemented correctly.Suggestions:
- Consider adding a custom error for the
requirestatements to provide more context about the validation failures.- require(clientChainId != 0, "ExocoreGateway: client chain id cannot be zero"); + if (clientChainId == 0) { + revert InvalidClientChainId(); + }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/core/ExocoreGateway.sol (2 hunks)
- src/interfaces/precompiles/IAssets.sol (1 hunks)
- test/foundry/unit/ExocoreGateway.t.sol (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/interfaces/precompiles/IAssets.sol
Additional context used
Learnings (2)
src/core/ExocoreGateway.sol (2)
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#39 File: test/foundry/unit/ExocoreGateway.t.sol:360-366 Timestamp: 2024-07-02T01:45:58.683Z Learning: The `updateWhitelistedTokens` function in `ExocoreGateway` interacts solely with the trusted `ASSET_CONTRACT` precompile, mitigating the need for a reentrancy guard.Learnt from: MaxMustermann2 PR: ExocoreNetwork/exocore-contracts#86 File: src/core/ExocoreGateway.sol:180-225 Timestamp: 2024-08-31T15:43:42.705Z Learning: Non-payable token registrations are not possible in the `addOrUpdateWhitelistToken` function. A fee must be provided when registering a token to send the interchain message, while no fee is required for updates.test/foundry/unit/ExocoreGateway.t.sol (1)
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#39 File: test/foundry/unit/ExocoreGateway.t.sol:360-366 Timestamp: 2024-07-02T01:45:58.683Z Learning: The `updateWhitelistedTokens` function in `ExocoreGateway` interacts solely with the trusted `ASSET_CONTRACT` precompile, mitigating the need for a reentrancy guard.
Additional comments not posted (8)
src/core/ExocoreGateway.sol (1)
218-233: Implementation ofupdateWhitelistTokenfunction: ApprovedThe
updateWhitelistTokenfunction follows a similar structure to theaddWhitelistTokenfunction, with proper parameter validation, event emission, and error handling. The logic for updating the token'stvlLimitandmetaDatais implemented correctly.test/foundry/unit/ExocoreGateway.t.sol (7)
116-126: LGTM!The code changes are approved.
536-539: LGTM!The code changes are approved.
544-545: LGTM!The code changes are approved.
553-554: LGTM!The code changes are approved.
558-570: LGTM!The code changes are approved.
575-576: LGTM!The code changes are approved.
588-603: LGTM!The code changes are approved.
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- src/core/Bootstrap.sol (1 hunks)
- src/core/ClientChainGateway.sol (1 hunks)
- src/core/ClientGatewayLzReceiver.sol (1 hunks)
- src/core/ExocoreGateway.sol (2 hunks)
- src/interfaces/IExocoreGateway.sol (1 hunks)
- src/libraries/Errors.sol (2 hunks)
- src/storage/ExocoreGatewayStorage.sol (1 hunks)
- test/foundry/unit/ExocoreGateway.t.sol (2 hunks)
- test/mocks/ExocoreGatewayMock.sol (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- src/core/Bootstrap.sol
- src/core/ClientChainGateway.sol
- src/libraries/Errors.sol
- src/storage/ExocoreGatewayStorage.sol
- test/mocks/ExocoreGatewayMock.sol
Additional context used
Learnings (4)
src/interfaces/IExocoreGateway.sol (2)
Learnt from: MaxMustermann2 PR: ExocoreNetwork/exocore-contracts#86 File: src/core/ExocoreGateway.sol:180-225 Timestamp: 2024-08-31T15:43:42.705Z Learning: Non-payable token registrations are not possible in the `addOrUpdateWhitelistToken` function. A fee must be provided when registering a token to send the interchain message, while no fee is required for updates.Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#39 File: test/foundry/unit/ExocoreGateway.t.sol:360-366 Timestamp: 2024-07-02T01:45:58.683Z Learning: The `updateWhitelistedTokens` function in `ExocoreGateway` interacts solely with the trusted `ASSET_CONTRACT` precompile, mitigating the need for a reentrancy guard.src/core/ClientGatewayLzReceiver.sol (1)
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#39 File: src/core/ClientGatewayLzReceiver.sol:0-0 Timestamp: 2024-07-01T09:41:18.389Z Learning: The `afterReceiveAddWhitelistTokensRequest` function in `ClientGatewayLzReceiver.sol` includes a check to ensure a token is not already whitelisted before proceeding with any operations like adding to the whitelist or deploying a vault.src/core/ExocoreGateway.sol (2)
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#39 File: test/foundry/unit/ExocoreGateway.t.sol:360-366 Timestamp: 2024-07-02T01:45:58.683Z Learning: The `updateWhitelistedTokens` function in `ExocoreGateway` interacts solely with the trusted `ASSET_CONTRACT` precompile, mitigating the need for a reentrancy guard.Learnt from: MaxMustermann2 PR: ExocoreNetwork/exocore-contracts#86 File: src/core/ExocoreGateway.sol:180-225 Timestamp: 2024-08-31T15:43:42.705Z Learning: Non-payable token registrations are not possible in the `addOrUpdateWhitelistToken` function. A fee must be provided when registering a token to send the interchain message, while no fee is required for updates.test/foundry/unit/ExocoreGateway.t.sol (1)
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#39 File: test/foundry/unit/ExocoreGateway.t.sol:360-366 Timestamp: 2024-07-02T01:45:58.683Z Learning: The `updateWhitelistedTokens` function in `ExocoreGateway` interacts solely with the trusted `ASSET_CONTRACT` precompile, mitigating the need for a reentrancy guard.
Additional comments not posted (10)
src/core/ExocoreGateway.sol (1)
209-224: Review ofupdateWhitelistTokenfunction: ApprovalThe
updateWhitelistTokenfunction is implemented correctly and aligns with the expected functionality. The validation checks are appropriate, ensuring that the token address and client chain ID are not zero. The flexibility in updating the TVL limit and metadata is a positive feature, allowing for dynamic adjustments to token properties.test/foundry/unit/ExocoreGateway.t.sol (9)
Line range hint
1-127: Setup Logic Appears CorrectThe
SetUpcontract correctly initializes the testing environment, including players, mock contracts, and gateway deployment. This setup is crucial for the integrity of the tests that follow.
Line range hint
127-158: Pausing Functionality Tests Are ComprehensiveThe tests in the
Pausablecontract comprehensively check the pausing and unpausing functionality, including access control via ownership checks. This ensures that the gateway can only be paused and unpaused by authorized entities.
Line range hint
158-286: LayerZero Receive Functionality Tests Are AdequateThe
LzReceivecontract provides a robust set of tests for thelzReceivefunction, covering various scenarios including operator association, withdrawals, and error handling. These tests are crucial for ensuring secure and correct cross-chain communication.
Line range hint
286-428: Client Chain Registration and Update Tests Are ThoroughThe
RegisterOrUpdateClientChaincontract effectively tests the registration and updating of client chains, including proper handling of ownership and input validation. These tests are vital for maintaining the integrity and security of the gateway's client chain management.
Line range hint
428-490: Peer Setting Functionality Tests Are CorrectThe
SetPeercontract provides comprehensive tests for setting a peer in a client chain, ensuring that only the owner can perform this action and that errors are handled correctly when the client chain is not registered.
Line range hint
490-602: Whitelist Token Addition Tests Reflect New Single-Token LogicThe
AddWhitelistTokenscontract tests the addition of whitelist tokens with the new single-token logic. The tests cover various failure scenarios and a success scenario, ensuring that the system handles token addition correctly. This change simplifies the token management process and aligns with the PR's objectives to reduce complexity and gas costs.
609-687: Whitelist Token Update Tests Are EffectiveThe
UpdateWhitelistTokenscontract tests the updating of whitelist tokens using a struct to encapsulate token details. This approach improves data management and ensures that the system handles token updates correctly. The tests cover various failure scenarios and a success scenario, aligning with the PR's objectives to enhance clarity and efficiency in token management.
Line range hint
687-770: Operator Association Tests Are ComprehensiveThe
AssociateOperatorWithEVMStakercontract provides thorough tests for the association of operators with EVM stakers. These tests ensure that the system handles operator associations correctly across different client chains and that errors are handled appropriately.
Line range hint
770-798: Bootstrap Marking Tests Are CorrectThe
MarkBootstrapcontract tests the marking of a client chain as bootstrapped, including handling of errors. These tests ensure that the system correctly marks a client chain as bootstrapped and handles errors appropriately.
ExocoreNetwork/exocore#159 adds a new
oracleInfoparameter to theIAssets.solprecompile when registering tokens. This PR follows that change.Previously, multiple tokens could be registered with a single call to ExocoreGateway. However, that resulted in too many variables (and too much gas) for Solidity to handle within a single function. To that end, and keeping in mind that addition of new tokens isn't likely to be a frequent occurrence, the function's capabilities have been tempered to support only a single token within the transaction.
At the same time, the function to update a whitelisted token has been split from the one that adds whitelisted tokens. This is because only certain parameters (
tvlLimitandmetadata) are editable.As a side effect, this fixes #80
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests