update executor options for solana client controller #120
Conversation
WalkthroughThe changes in this pull request focus on enhancing the response handling and messaging logic within the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/storage/ExocoreGatewayStorage.sol (2)
22-24: LGTM! Consider enhancing the documentation.The constant declaration is correct and uses appropriate type (uint32) and visibility (internal). The value 30_168 matches the LayerZero chain ID for Solana mainnet.
Consider expanding the NatSpec comment to be more descriptive:
- /// @dev the solana mainnet chain id + /// @dev The LayerZero chain ID for Solana mainnet (30_168)
26-28: LGTM! Consider enhancing the documentation.The constant declaration is correct and consistent with the mainnet chain ID declaration.
Consider expanding the NatSpec comment to be more descriptive:
- /// @dev the solana devnet chain id + /// @dev The LayerZero chain ID for Solana devnet (40_168)src/core/ExocoreGateway.sol (1)
528-539: Consider using named constants for gas limits and valuesWhile the implementation correctly handles different message options for Solana and non-Solana chains, the gas limits and values should be defined as named constants for better maintainability and clarity.
Consider defining these constants at the contract level:
+ /// @dev Gas limit for destination chain execution + uint256 private constant DESTINATION_GAS_LIMIT = /* current value */; + /// @dev Message value for destination chain execution + uint256 private constant DESTINATION_MSG_VALUE = /* current value */; + /// @dev Message value for Solana whitelist token operations + uint256 private constant SOLANA_MSG_VALUE = /* current value */;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/core/ExocoreGateway.sol (4 hunks)
- src/storage/ExocoreGatewayStorage.sol (1 hunks)
🔇 Additional comments (3)
src/storage/ExocoreGatewayStorage.sol (1)
30-31: 🛠️ Refactor suggestionVerify the hardcoded message value and improve naming.
The constant declaration has a few potential improvements:
- The constant name SOLANA_MSG_VALUE is generic while the comment suggests it's specific to token whitelisting
- The hardcoded value (3,000,000) needs verification to ensure it's sufficient for token whitelisting operations
Consider these improvements:
- /// @dev the msg.value for send addTokenWhiteList message - uint128 internal constant SOLANA_MSG_VALUE = 3_000_000; + /// @dev The required msg.value (in wei) for sending addTokenWhiteList messages to Solana chains + /// This covers the gas costs for token whitelisting operations + uint128 internal constant SOLANA_WHITELIST_TOKEN_MSG_VALUE = 3_000_000;Let's verify the value is sufficient:
✅ Verification successful
Based on the evidence gathered, I can now provide a final response:
The SOLANA_MSG_VALUE constant is correctly set for token whitelisting operations
The code analysis shows that:
- The constant is specifically used in
ExocoreGateway.solfor token whitelisting operations via LayerZero messaging- It's only applied when the source chain is Solana (either devnet or mainnet) and the action is
REQUEST_ADD_WHITELIST_TOKEN- The value is used as the executor gas limit for LayerZero cross-chain message execution
- The naming suggestion in the review comment would not improve clarity as the current name and documentation already clearly indicate its purpose
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any existing token whitelisting operations or gas estimations rg -l 'addWhitelistToken|whitelist.*token' --type sol # Look for any gas-related constants or configurations rg -i 'gas.*constant|msg\.value' --type solLength of output: 157
Script:
#!/bin/bash # Search for any existing token whitelisting operations or gas estimations # Using ripgrep without file type filter since .sol extension isn't recognized rg -l 'addWhitelistToken|whitelist.*token' # Look for any gas-related constants or configurations rg -i 'gas.*constant|msg\.value' # Find all Solidity files to analyze their content fd '\.sol$'Length of output: 6816
Script:
#!/bin/bash # Let's examine the specific usage of SOLANA_MSG_VALUE rg -A 5 -B 5 'SOLANA_MSG_VALUE' # Check for any token whitelisting related functions or tests rg -A 10 'addWhitelistToken|addTokenWhiteList'Length of output: 107584
src/core/ExocoreGateway.sol (2)
357-361: LGTM: Enhanced response handling for Solana networksThe implementation correctly provides additional context (token and staker) in responses for Solana networks while maintaining compatibility with other chains.
426-432: LGTM: Consistent response handling for Solana networksThe implementation maintains consistency with the LST transfer pattern, providing additional context for Solana networks.
|
Linting failed. Please check the logs. |
|
The build has failed. Please check the logs. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/core/ExocoreGateway.sol (4 hunks)
🔇 Additional comments (2)
src/core/ExocoreGateway.sol (2)
357-361: LGTM! Response handling for Solana chains.The implementation correctly adds additional context (token and staker/avsOrWithdrawer) in responses for Solana chains while maintaining backward compatibility for other chains.
Also applies to: 426-432
578-597: LGTM! Well-structured helper function.The
_buildOptionsfunction effectively extracts the duplicated options-building logic, improving code maintainability and following the DRY principle. The implementation is consistent with the original logic and well-documented.
magj2006
force-pushed
the
solana-client-controller
branch
from
5d5ca4c to
430cd28
Compare
magj2006
force-pushed
the
solana-client-controller
branch
from
430cd28 to
b1640df
Compare
Added, please review again. |
magj2006
force-pushed
the
solana-client-controller
branch
2 times, most recently
from
5ea5cd9 to
8b18c40
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/core/ExocoreGateway.sol (1)
549-568: Consider renaming parameter for consistency.
The parameter srcChainId should be renamed to chainId to match the function's documentation.
Apply this diff to improve parameter naming consistency:
- function _buildOptions(uint32 srcChainId, Action act) private pure returns (bytes memory) {
+ function _buildOptions(uint32 chainId, Action act) private pure returns (bytes memory) {
bytes memory options = OptionsBuilder.newOptions();
- if (srcChainId == SOLANA_DEVNET_CHAIN_ID || srcChainId == SOLANA_MAINNET_CHAIN_ID) {
+ if (chainId == SOLANA_DEVNET_CHAIN_ID || chainId == SOLANA_MAINNET_CHAIN_ID) {🛑 Comments failed to post (2)
test/foundry/unit/ExocoreGateway.t.sol (2)
119-128:
⚠️ Potential issuePotential endpoint misconfiguration for Solana client
In line 119, the
exocoreLzEndpointis setting the destination endpoint forsolanaClientGatewaytoclientLzEndpoint, which is intended for the EVM-compatible client chain. This could lead to incorrect communication between the Exocore and Solana gateways. ThesolanaClientGatewayshould be paired withsolanaClientLzEndpointto ensure proper endpoint mapping.Apply this diff to correct the endpoint configuration:
exocoreLzEndpoint.setDestLzEndpoint(address(clientGateway), address(clientLzEndpoint)); exocoreGateway.registerOrUpdateClientChain( clientChainId, address(clientGateway).toBytes32(), 20, "clientChain", "EVM compatible client chain", "secp256k1" ); -exocoreLzEndpoint.setDestLzEndpoint(address(solanaClientGateway), address(clientLzEndpoint)); +exocoreLzEndpoint.setDestLzEndpoint(address(solanaClientGateway), address(solanaClientLzEndpoint)); exocoreGateway.registerOrUpdateClientChain( solanaClientChainId, address(solanaClientGateway).toBytes32(), 20, "solanaClientChain", "Non-EVM compatible client chain", "ed25519" );Committable suggestion was skipped due to low confidence.
140-168: 🛠️ Refactor suggestion
Refactor
generateUIDfunction to eliminate code duplicationThe
generateUIDfunction contains duplicated logic for handling EVM and non-EVM chains. Refactoring the function to reduce duplication will enhance maintainability and readability.Apply this diff to refactor the function:
function generateUID(uint64 nonce, bool fromClientChainToExocore) internal view returns (bytes32 uid) { uid = generateUID(nonce, fromClientChainToExocore, true); } -function generateUID(uint64 nonce, bool fromClientChainToExocore, bool isEVM) internal view returns (bytes32 uid) { - if (fromClientChainToExocore) { - if (isEVM) { - uid = GUID.generate( - nonce, clientChainId, address(clientGateway), exocoreChainId, address(exocoreGateway).toBytes32() - ); - } else { - uid = GUID.generate( - nonce, - solanaClientChainId, - address(solanaClientGateway), - exocoreChainId, - address(exocoreGateway).toBytes32() - ); - } - } else { - if (isEVM) { - uid = GUID.generate( - nonce, exocoreChainId, address(exocoreGateway), clientChainId, address(clientGateway).toBytes32() - ); - } else { - uid = GUID.generate( - nonce, - exocoreChainId, - address(exocoreGateway), - solanaClientChainId, - address(solanaClientGateway).toBytes32() - ); - } - } -} +function generateUID(uint64 nonce, bool fromClientChainToExocore, bool isEVM) internal view returns (bytes32 uid) { + uint16 srcChainId; + address srcGateway; + uint16 dstChainId; + bytes32 dstGateway; + + if (fromClientChainToExocore) { + srcChainId = isEVM ? clientChainId : solanaClientChainId; + srcGateway = isEVM ? address(clientGateway) : address(solanaClientGateway); + dstChainId = exocoreChainId; + dstGateway = address(exocoreGateway).toBytes32(); + } else { + srcChainId = exocoreChainId; + srcGateway = address(exocoreGateway); + dstChainId = isEVM ? clientChainId : solanaClientChainId; + dstGateway = isEVM ? address(clientGateway).toBytes32() : address(solanaClientGateway).toBytes32(); + } + uid = GUID.generate(nonce, srcChainId, srcGateway, dstChainId, dstGateway); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.function generateUID(uint64 nonce, bool fromClientChainToExocore) internal view returns (bytes32 uid) { uid = generateUID(nonce, fromClientChainToExocore, true); } function generateUID(uint64 nonce, bool fromClientChainToExocore, bool isEVM) internal view returns (bytes32 uid) { uint16 srcChainId; address srcGateway; uint16 dstChainId; bytes32 dstGateway; if (fromClientChainToExocore) { srcChainId = isEVM ? clientChainId : solanaClientChainId; srcGateway = isEVM ? address(clientGateway) : address(solanaClientGateway); dstChainId = exocoreChainId; dstGateway = address(exocoreGateway).toBytes32(); } else { srcChainId = exocoreChainId; srcGateway = address(exocoreGateway); dstChainId = isEVM ? clientChainId : solanaClientChainId; dstGateway = isEVM ? address(clientGateway).toBytes32() : address(solanaClientGateway).toBytes32(); } uid = GUID.generate(nonce, srcChainId, srcGateway, dstChainId, dstGateway); }
magj2006
force-pushed
the
solana-client-controller
branch
2 times, most recently
from
b5afc5d to
85638a2
Compare
magj2006
force-pushed
the
solana-client-controller
branch
from
85638a2 to
27043e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
src/storage/ExocoreGatewayStorage.sol (1)
142-148: Add @return tag to NatSpec documentation.
The function implementation is clean and correct. Consider adding an @return tag to the NatSpec documentation to describe the return value:
/**
* @dev return true if chain is either Solana devnet or Solana mainnet
* @param srcChainId remote Chain Id
+ * @return bool true if the chain ID matches Solana networks, false otherwise
*/src/core/ExocoreGateway.sol (2)
549-568: Consider improving the options building function documentation.
While the function implementation is solid, the documentation could be more detailed about the specific options being set and their implications.
Update the function documentation:
- /// @dev Builds options for interchain messages based on chain and action
- /// @param srcChainId The source chain ID
- /// @param act The action being performed
- /// @return options The built options
+ /// @dev Builds LayerZero options for interchain messages based on chain type and action
+ /// @param srcChainId The source chain ID
+ /// @param act The action being performed
+ /// @return options The built options containing:
+ /// - Ordered execution option (except for Solana chains)
+ /// - Gas limit and value for message execution
+ /// Note: Solana chains use a different message value for whitelist token actions561-564: Consider using named constants for better code readability.
The conditional expression for determining the message value could be more readable with named constants.
Consider this improvement:
+ /// @dev Message value multiplier for Solana whitelist token operations
+ uint128 private constant SOLANA_WHITELIST_MULTIPLIER = 2;
function _buildOptions(uint32 srcChainId, Action act) private pure returns (bytes memory) {
// ...
- uint128 value = _isSolana(srcChainId) && act == Action.REQUEST_ADD_WHITELIST_TOKEN
- ? SOLANA_WHITELIST_TOKEN_MSG_VALUE
- : DESTINATION_MSG_VALUE;
+ uint128 value = (_isSolana(srcChainId) && act == Action.REQUEST_ADD_WHITELIST_TOKEN)
+ ? DESTINATION_MSG_VALUE * SOLANA_WHITELIST_MULTIPLIER
+ : DESTINATION_MSG_VALUE;📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/core/ExocoreGateway.sol (4 hunks)
- src/storage/ExocoreGatewayStorage.sol (2 hunks)
- test/foundry/unit/ExocoreGateway.t.sol (8 hunks)
🔇 Additional comments (6)
src/storage/ExocoreGatewayStorage.sol (1)
22-31: Verify Solana chain IDs and message value.
The constants are well-defined and follow best practices. However, please verify:
- Are these the official Solana chain IDs used by LayerZero?
- Is the
SOLANA_WHITELIST_TOKEN_MSG_VALUEof 3,000,000 sufficient for gas costs?
✅ Verification successful
Let me search for more specific patterns to verify these constants.
Based on the search results, I can now generate the final response:
Chain IDs and message value constants are correctly implemented and consistently used.
The verification shows:
- Chain IDs (30168, 40168) are consistently used across the codebase in
ExocoreGatewayStorage.solandExocoreGateway.sol SOLANA_WHITELIST_TOKEN_MSG_VALUE(3,000,000) is properly referenced in the token whitelisting logic inExocoreGateway.sol- Constants are used in appropriate contexts for Solana chain identification and message value determination
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other occurrences of these chain IDs in the codebase
# to ensure consistency
rg -g '!*.md' -g '!*.json' '30168|40168|3000000'
Length of output: 986
Script:
#!/bin/bash
# Search for Solana chain ID declarations or references
rg -g '*.sol' 'SOLANA.*CHAIN_ID|chainId.*[34]0168'
# Search for any LayerZero endpoint configurations or chain mappings
rg -g '*.sol' 'LayerZero.*endpoint|chainId.*mapping'
# Search for any msg.value constants related to Solana
rg -g '*.sol' 'msg\.value.*3.*000.*000|WHITELIST.*MSG.*VALUE'
Length of output: 2278
src/core/ExocoreGateway.sol (2)
528-530: LGTM: Clean refactoring of options building logic.
The extraction of options building logic into a separate function improves code organization and reusability.
Also applies to: 540-545
556-559: Verify the impact of unordered execution for Solana chains.
The code explicitly disables ordered execution for Solana chains. This could have implications for transaction ordering and consistency.
Let's verify the impact:
✅ Verification successful
Based on the extensive codebase analysis, I can now provide a definitive response about the impact of unordered execution for Solana chains:
Solana unordered execution is an intentional and handled limitation
The code explicitly handles this LayerZero limitation for Solana chains by:
-
Defining specific Solana chain IDs in
ExocoreGatewayStorage.sol:SOLANA_MAINNET_CHAIN_ID = 30_168SOLANA_DEVNET_CHAIN_ID = 40_168
-
Implementing proper message value handling for Solana chains:
- Uses
SOLANA_WHITELIST_TOKEN_MSG_VALUE = 3_000_000for Solana transactions - Has specific response payload formatting for Solana chains in
ExocoreGateway.sol
- Uses
-
The unordered execution is compensated by including necessary context in the messages:
- Solana responses include additional data (nonce, token, staker info)
- This ensures transaction consistency without relying on ordered execution
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any dependencies on ordered execution in Solana-related code
rg -l "ordered.*execution.*solana" --type sol
# Search for any comments or documentation about Solana execution ordering
rg -i "solana.*order" --type sol
Length of output: 138
Script:
#!/bin/bash
# Search for any dependencies on ordered execution in Solana-related code
rg -l "ordered.*execution.*solana"
# Search for any comments or documentation about Solana execution ordering
rg -i "solana.*order"
# Search for other occurrences of Solana-related code and execution options
rg -i "solana" -A 3 -B 3
# Look for LayerZero related configurations and options
rg -i "layerzero|lz.*option" -A 3 -B 3
Length of output: 242614
test/foundry/unit/ExocoreGateway.t.sol (3)
42-47: LGTM: SetUp contract properly initialized with Solana client configuration.
The setup correctly initializes Solana-specific test components including gateway, endpoint, and chain ID. The chain ID 40_168 appears to be the correct identifier for Solana.
Also applies to: 52-52, 72-73, 97-97
565-565: LGTM: Proper fee calculation for Solana transactions.
The implementation correctly calculates native fees for Solana transactions by constructing the appropriate message structure.
Also applies to: 574-576
630-646: Verify decimal precision for Solana token.
The test correctly adds whitelist token support for Solana, but there's a notable difference in decimal precision (9 for Solana vs 18 for EVM). Ensure this matches Solana's SPL token standard.
✅ Verification successful
Based on the code review and search results, I can now generate the final response:
Decimal precision of 9 for Solana token is correct and verified.
The test is correctly using 9 decimals for Solana token which aligns with Solana's SPL token standard. This is further supported by the implementation where special handling for Solana chains (both mainnet 30168 and devnet 40168) is present in the codebase, showing awareness of Solana's specific requirements. The TVL limit in the test is also appropriately scaled with 1e9 (9 decimals) instead of 1e18 used for EVM chains.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the decimal precision used in other Solana token tests
rg -A 2 "decimals.*9.*Solana"
Length of output: 31
Script:
#!/bin/bash
# Let's search for any Solana token decimal patterns in the codebase
rg -A 5 "solanaClientChainId.*decimals|solana.*decimals|spl.*decimals" -i
# Also search for any token decimal constants or configurations
ast-grep --pattern 'decimals = $_;'
Length of output: 113
Script:
#!/bin/bash
# Let's also check the contract implementation for decimal handling
fd "ExocoreGateway" --exec cat {}
Length of output: 96786
Script:
#!/bin/bash
# Let's search for any decimal-related constants or configurations for Solana
rg -A 2 "SOLANA|decimal.*9|9.*decimal" --type sol
# Also search for any Solana token-related tests or configurations
rg -A 5 "solana.*token|spl.*token" --type sol
Length of output: 155
Script:
#!/bin/bash
# Let's search for any decimal-related constants or configurations for Solana without file type restriction
rg -A 2 "SOLANA|decimal.*9|9.*decimal"
# Also search for any Solana token-related tests or configurations
rg -A 5 "solana.*token|spl.*token"
Length of output: 2315
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
test/foundry/unit/ExocoreGateway.t.sol (2)
52-52: Document the significance of the Solana chain ID.
Consider adding a comment explaining why the specific value 40_168 was chosen for solanaClientChainId. This helps other developers understand the significance of this magic number.
622-638: Enhance test coverage for Solana whitelist token.
While the test validates the basic flow, consider adding assertions to verify:
- The token was actually whitelisted
- The correct decimals (9) were stored
- The metadata was properly set
Example assertion:
// Add after line 637
assertEq(exocoreGateway.getWhitelistedToken(solanaClientChainId, bytes32(bytes20(address(restakeToken)))).decimals, 9);📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/core/ExocoreGateway.sol (4 hunks)
- test/foundry/unit/ExocoreGateway.t.sol (8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/ExocoreGateway.sol
🔇 Additional comments (1)
test/foundry/unit/ExocoreGateway.t.sol (1)
140-154: LGTM! Well-structured function overloading.
The refactored generateUID function is well-organized with:
- Clear separation of concerns between EVM and Solana clients
- Reduced nesting through early returns
- Improved readability with concise conditional logic
magj2006
force-pushed
the
solana-client-controller
branch
from
6cb7622 to
f5fbb86
Compare
update for #119 :
add definition of Solana devnet & mainnet ChinaID
set unordered options when sending message to Solana Chain
set value when calling addWhitelistToken to add token on Solana Chain
Summary by CodeRabbit
New Features
Bug Fixes
Documentation