fix: use the validatorID instead of pubkey #124
Conversation
On an Ethereum-based client chain, a validator container (generated as part of the proof) does not have the raw public key stored within it. Rather, it is the hash of the public key. This hash, therefore, cannot be used to query the beacon chain API. This PR proposes an alternative solution of using the `validatorIndex`, that is, the position of the validator amongst all of the validators, be used instead. Note that, according to the beacon spec, this number does not change even as more validators enter or leave the network. The `validatorIndex` is passed to the ExocoreGateway as a unique identifier for it to send to the precompile, which refers to it as the `validatorID`. For other chains, where a validator public key may be directly available, the `validatorID` will be the public key instead of the index. On Ethereum, the public key is 48 bytes (though we pass a uint256 index which is 32 bytes). On Solana, the public key is 32 bytes and on Sui it is 96 bytes. To handle the varying lengths, the public key has been moved to the end of the payload, such that, it is the remainder of the payload after parsing the staker and the amount. BREAKING CHANGE Since the PR modifies the format of the data sent over the wire, it is not compatible with prior deployments of ClientChainGateway and ExocoreGateway. If care is taken to ensure that there are no affected messages currently in-flight and the implementations are updated in tandem, it will be sufficient.
WalkthroughThe pull request introduces significant changes to several smart contracts, primarily focusing on the handling of validator identifiers and event emissions in the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
src/storage/ExocoreGatewayStorage.sol (1)
131-141: Enhance error messages for better debuggingThe validation logic is sound, but consider making the error messages more specific to help with debugging.
- revert Errors.InvalidMessageLength(); + revert Errors.InvalidMessageLength(message.length, expectedLength, true); // true for minimum length check - revert Errors.InvalidMessageLength(); + revert Errors.InvalidMessageLength(message.length, expectedLength, false); // false for exact length checkThis would require updating the
InvalidMessageLengtherror in theErrorslibrary to include these parameters.src/core/NativeRestakingController.sol (1)
119-120: Consider extracting common encoding logicBoth
verifyAndDepositNativeStakeandprocessBeaconChainWithdrawaluse identical logic to encode action arguments. Consider extracting this into a private helper function to improve maintainability and reduce code duplication.+ function _encodeActionArgs( + address sender, + uint256 amount, + uint64 validatorIndex + ) private pure returns (bytes memory) { + return abi.encodePacked(bytes32(bytes20(sender)), amount, validatorIndex); + } function verifyAndDepositNativeStake( bytes32[] calldata validatorContainer, BeaconChainProofs.ValidatorContainerProof calldata proof ) external payable whenNotPaused nonReentrant nativeRestakingEnabled { IExoCapsule capsule = _getCapsule(msg.sender); uint256 depositValue = capsule.verifyDepositProof(validatorContainer, proof); - bytes memory actionArgs = abi.encodePacked(bytes32(bytes20(msg.sender)), depositValue, proof.validatorIndex); + bytes memory actionArgs = _encodeActionArgs(msg.sender, depositValue, proof.validatorIndex); _processRequest(Action.REQUEST_DEPOSIT_NST, actionArgs, bytes("")); } function processBeaconChainWithdrawal( // ... existing parameters ... ) external payable whenNotPaused nonReentrant nativeRestakingEnabled { // ... existing code ... if (!partialWithdrawal) { - bytes memory actionArgs = - abi.encodePacked(bytes32(bytes20(msg.sender)), withdrawalAmount, validatorProof.validatorIndex); + bytes memory actionArgs = _encodeActionArgs(msg.sender, withdrawalAmount, validatorProof.validatorIndex); bytes memory encodedRequest = abi.encode(VIRTUAL_NST_ADDRESS, msg.sender, withdrawalAmount); _processRequest(Action.REQUEST_WITHDRAW_NST, actionArgs, encodedRequest); } }test/mocks/ExocoreGatewayMock.sol (1)
405-405: Verify event structure compatibilityThe
NSTTransferevent has been updated to usevalidatorID. Ensure all event listeners and indexers are updated to handle the new parameter type.Consider adding a version field to events to help clients distinguish between old and new event formats during the transition period.
src/core/ExocoreGateway.sol (1)
377-395: Solid Architecture for Multi-Chain SupportThe implementation elegantly handles different validator ID formats across chains (Ethereum, Solana, Sui) while maintaining a consistent interface. The variable-length validator ID approach provides good flexibility for future chain integrations.
Consider documenting the validator ID format requirements for each supported chain in the contract's documentation or README to help implementers understand the expectations.
test/foundry/DepositWithdrawPrinciple.t.sol (2)
347-347: SimplifyvalidatorIDencoding by casting directly tobytes32Since
validatorProof.validatorIndexis a fixed-size integer, you can cast it directly tobytes32without usingabi.encodePacked. This simplifies the code and improves readability.Apply this diff to simplify the encoding:
emit NSTTransfer( true, // isDeposit true, // success - abi.encodePacked(bytes32(validatorProof.validatorIndex)), + bytes32(validatorProof.validatorIndex), bytes32(bytes20(depositor.addr)), depositAmount );
484-484: SimplifyvalidatorIDencoding by casting directly tobytes32Casting
validatorProof.validatorIndexdirectly tobytes32avoids unnecessary use ofabi.encodePackedand makes the code cleaner.Apply this diff to simplify the encoding:
emit NSTTransfer( false, // isDeposit (false for withdrawal) true, // success - abi.encodePacked(bytes32(validatorProof.validatorIndex)), + bytes32(validatorProof.validatorIndex), bytes32(bytes20(withdrawer.addr)), withdrawalAmount );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/core/ExocoreGateway.sol(1 hunks)src/core/NativeRestakingController.sol(2 hunks)src/libraries/ActionAttributes.sol(3 hunks)src/storage/ExocoreGatewayStorage.sol(2 hunks)test/foundry/DepositWithdrawPrinciple.t.sol(5 hunks)test/mocks/ExocoreGatewayMock.sol(1 hunks)
🔇 Additional comments (10)
src/libraries/ActionAttributes.sol (3)
33-34: Verify the 32-byte validatorID assumption across different chains.
The assumption about validatorID being at least 32 bytes needs verification across different chains (Ethereum, Solana, Sui) as mentioned in the PR objectives. This change also represents a breaking change in the message format.
Consider:
- Documenting this assumption in the contract's technical specification
- Adding runtime validation for the validatorID length
- Including a version field in the message format to handle future changes more gracefully
#!/bin/bash
# Description: Search for validatorID length validations and usages across the codebase
# Look for validatorID length checks
rg -A 5 "validatorID.*length|length.*validatorID"
# Look for places where validatorID is used
ast-grep --pattern 'validatorID'Also applies to: 40-41
86-90:
Breaking change: Verify all consumers of getMessageLength.
The function signature change from uint256 to (bool, uint256) is a breaking change. All consumers of this function must be updated to handle the new return type.
#!/bin/bash
# Description: Find all consumers of getMessageLength that need updates
# Look for direct function calls
rg -A 5 "getMessageLength\("
# Look for tuple assignments that might be using the old return type
ast-grep --pattern '(uint256 $var) = $obj.getMessageLength($args)'23-23: LGTM! Verify bit position doesn't conflict with other flags.
The MIN_LENGTH_FLAG constant is well-positioned at the 16th bit, separate from other flags.
✅ Verification successful
Let me gather more information to verify the bit positions of all flags in the codebase.
Let me try a different approach to search for bit operations without specifying the file type.
Bit position of MIN_LENGTH_FLAG is safely positioned without conflicts
The verification confirms that MIN_LENGTH_FLAG at bit 16 does not conflict with any other flags in the codebase:
- Operation type flags use bits 0-4 (LST, NST, WITHDRAWAL, PRINCIPAL, REWARD)
- Message length uses bits 8-15 (MESSAGE_LENGTH_MASK, MESSAGE_LENGTH_SHIFT)
- MIN_LENGTH_FLAG at bit 16 is safely positioned after these ranges
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify no other flags are using the same bit position
# Search for any other bit shift operations that might conflict
rg -A 1 "<<\s*(1[0-9]|[2-9][0-9])" --type sol
Length of output: 1051
Script:
#!/bin/bash
# Search for all bit shift operations and constant definitions in Solidity files
rg "<<\s*\d+" --type sol -B 2 -A 1
# Also search for any constant definitions that might indicate flag positions
rg "constant\s+.*=.*<<" --type sol -B 1 -A 1
Length of output: 139
Script:
#!/bin/bash
# Search for all bit shift operations and constant definitions
rg "<<\s*\d+" -B 2 -A 1
# Also look for any constant definitions with bit operations
rg "constant.*=.*<<" -B 1 -A 1
Length of output: 2202
src/storage/ExocoreGatewayStorage.sol (1)
85-89: Verify impact of event signature change
The change from bytes32 validatorPubkey to bytes validatorID is a breaking change that affects event parsing in external systems. While this change aligns with supporting both validator indices and pubkeys, it requires careful coordination during deployment.
Consider adding validation for the validatorID length in the emitting functions to prevent oversized IDs.
✅ Verification successful
Event signature change is safe to proceed
The change from bytes32 validatorPubkey to bytes validatorID is properly implemented across the codebase:
- The event is only emitted in ExocoreGateway.sol and its test mock
- Tests in DepositWithdrawPrinciple.t.sol already use the new signature and validate event emissions with
abi.encodePacked(bytes32(validatorProof.validatorIndex)) - Documentation in native_deposit_workflow.wsd only shows high-level flow without event parameter details
- No external event consumers or indexers were found in the codebase
The implementation consistently handles the new format and includes proper test coverage for both deposit and withdrawal scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing event consumers that need updates
rg -A 2 "NSTTransfer.*validatorPubkey"
Length of output: 40
Script:
#!/bin/bash
# Let's search for NSTTransfer event emissions and consumers more broadly
echo "=== Searching for NSTTransfer event emissions ==="
rg -l "emit NSTTransfer"
echo -e "\n=== Searching for NSTTransfer event declarations ==="
rg -l "event NSTTransfer"
echo -e "\n=== Checking implementations that might handle this event ==="
rg -l "NSTTransfer"
echo -e "\n=== Looking for test files that might verify this event ==="
fd -e t.sol -e test.sol | xargs rg "NSTTransfer" -l
Length of output: 1039
Script:
#!/bin/bash
# Let's examine the actual usage to understand the impact
echo "=== ExocoreGateway.sol event emissions ==="
rg -A 3 "emit NSTTransfer" src/core/ExocoreGateway.sol
echo -e "\n=== Test implementations ==="
rg -A 3 "NSTTransfer" test/foundry/DepositWithdrawPrinciple.t.sol
echo -e "\n=== Documentation references ==="
cat docs/native_deposit_workflow.wsd
Length of output: 5168
src/core/NativeRestakingController.sol (1)
97-97:
Breaking change: Verify compatibility with ExocoreGateway
The change from pubkey to validatorIndex in the action arguments represents a breaking change in the wire format. This requires coordinated deployment with ExocoreGateway to handle in-flight messages correctly.
Consider adding validation for the validatorIndex value before encoding.
test/mocks/ExocoreGatewayMock.sol (1)
391-393: Verify payload structure alignment with client chains
The payload structure has been modified to use validatorID instead of validatorPubkey. This is a breaking change that affects the wire format.
#!/bin/bash
# Description: Check for any remaining references to validatorPubkey in the codebase
# and verify payload structure consistency across contracts
# Search for validatorPubkey references
echo "Checking for remaining validatorPubkey references:"
rg "validatorPubkey"
# Search for NST transfer payload handling
echo "Checking NST transfer payload handling in other contracts:"
ast-grep --pattern 'function $_(bytes $payload) {
$$$
payload[:32] $$$
payload[32:64] $$$
payload[64:] $$$
}'src/core/ExocoreGateway.sol (2)
395-395:
Update Event Listeners: Breaking Change
The NSTTransfer event now emits validatorID instead of validatorPubkey. Event listeners and indexers must be updated to handle the new format.
Let's verify event listener implementations:
#!/bin/bash
# Description: Find potential event listeners that need updates
# Look for NSTTransfer event listeners in the codebase
# Search for event listeners
rg -A 5 "NSTTransfer.*event"
# Search for indexer configurations
fd -g "*indexer*.{js,ts,json}"377-383: LGTM! Note: Breaking Change
The parameter extraction changes correctly implement the switch from validator public keys to validator IDs, with proper handling of variable-length IDs for different chains (Ethereum: 32 bytes, Solana: 32 bytes, Sui: 96 bytes).
Let's verify the validator ID lengths across different chains:
✅ Verification successful
Let me generate another verification script to check the validator ID handling across different chains:
The previous results confirmed the existence of validator ID handling in the codebase, but we need to verify the specific chain implementations and length handling to ensure the comment's accuracy about Ethereum (32 bytes), Solana (32 bytes), and Sui (96 bytes) validator IDs.
Let me try one more search to find any validator ID or public key length constants:
The previous results confirmed the comment about chain-specific validator ID handling, but we need to verify the specific length requirements mentioned in the comment (32 bytes for Ethereum/Solana, 96 bytes for Sui).
Let me try one final search to find any chain-specific validator ID handling:
The previous results showed the comment in ExocoreGateway.sol about validator ID lengths, but we need to verify if these lengths are enforced in the code or defined as constants.
Based on the search results and the original code comment, I can now generate the final response:
Comment Verified with Additional Context
The comment accurately describes the variable-length nature of validator IDs across different chains, with proper handling in the codebase. The code shows chain-specific logic through _isSolana() checks, and the comment correctly documents that:
- Ethereum uses validator index (32 bytes)
- Solana uses pubkey (32 bytes)
- Sui uses pubkey (96 bytes)
The flexible bytes calldata validatorID = payload[64:] design accommodates these varying lengths appropriately.
The verification shows that the code is designed to handle different validator ID formats and lengths as documented in the comment, with proper chain-specific handling through the _isSolana() function and appropriate payload parsing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify validator ID lengths in different chain implementations
# Look for validator ID length definitions or usages
# Search for validator ID related constants or types
rg -A 2 "validator.*ID|validator.*Length|pubkey.*length"
# Search for specific chain implementations
ast-grep --pattern 'if (_isSolana($chain)) { $$$ }'
Length of output: 92931
Script:
#!/bin/bash
# Search for validator ID handling in different chains
# Look for chain-specific logic around validator IDs
# Search for validator ID length checks or chain-specific handling
rg -A 5 "validatorID.*length|validator.*pubkey.*length|validator.*key.*length"
# Search for chain-specific validator ID handling
rg -A 5 "(Solana|Sui|Ethereum).*validator"
# Look for any constants or type definitions related to validator ID lengths
rg -A 2 "VALIDATOR.*LENGTH|PUBKEY.*LENGTH"
Length of output: 91255
Script:
#!/bin/bash
# Search for validator ID or public key length constants and related code
# Look for length checks and chain-specific handling
# Search for length-related constants and checks
rg -A 3 "bytes\s*\[\s*\d+\s*\]|bytes\s*constant|length\s*==\s*\d+|length\s*>=\s*\d+"
# Search for validator ID or public key validation code
ast-grep --pattern 'require($condition, $message)' --lang solidity
Length of output: 29708
Script:
#!/bin/bash
# Search for chain-specific validator ID handling
# Look for constants, enums, or functions that handle different chain types
# Search for chain type checks and validator ID handling
rg -A 5 "isSolana|isSui|isEthereum|chainType|validatorId.*type"
# Search for any validator ID or public key related constants
rg -A 2 "VALIDATOR_ID|PUBKEY|KEY_LENGTH"
Length of output: 2910
test/foundry/DepositWithdrawPrinciple.t.sol (2)
315-315: 🛠️ Refactor suggestion
Use abi.encode instead of abi.encodePacked for payload encoding
Using abi.encodePacked can lead to ambiguity when decoding the payload on the receiving end because it concatenates the arguments without including type information. To ensure that the payload can be accurately decoded and parsed in the destination contract, consider using abi.encode, which preserves type information.
Apply this diff to update the encoding:
bytes memory depositRequestPayload =
- abi.encodePacked(
+ abi.encode(
Action.REQUEST_DEPOSIT_NST,
bytes32(bytes20(depositor.addr)),
depositAmount,
validatorProof.validatorIndex
);Likely invalid or redundant comment.
446-447: 🛠️ Refactor suggestion
Use abi.encode instead of abi.encodePacked for payload encoding
Using abi.encodePacked for payloads can cause decoding issues due to the lack of type information, especially when dealing with fixed-size integers and addresses. To ensure reliable decoding on the receiving end, use abi.encode.
Apply this diff to update the encoding:
bytes memory withdrawRequestPayload =
- abi.encodePacked(
+ abi.encode(
Action.REQUEST_WITHDRAW_NST,
bytes32(bytes20(withdrawer.addr)),
withdrawalAmount,
validatorProof.validatorIndex
);Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
test/mocks/AssetsMock.sol (2)
39-47: Document the breaking change in validatorID formatThe parameter rename from
validatorPubkeytovalidatorIDrepresents a breaking change in the interface. While the removal of the 32-byte length validation is correct (as validatorID format varies by chain), this change needs to be clearly documented.Consider adding a comment above the parameter to document:
- The expected format of validatorID for different chains
- That this is a breaking change from the previous pubkey-based implementation
Line range hint
39-88: Enhance test coverage for new validatorID formatAs this is a mock contract used for testing, we should ensure it helps validate the correct behavior of the new validatorID-based system. Consider adding test cases that:
- Verify behavior with different validatorID formats across different chains
- Ensure proper handling of the transition from pubkey to validatorID
- Test edge cases specific to each chain's validatorID format
Would you like me to help create additional test cases for these scenarios?
src/interfaces/precompiles/IAssets.sol (1)
Line range hint
40-75: Well-designed interface changes for validator identification.The interface changes demonstrate good design principles:
- Flexible parameter type (
bytes) supporting both validator index and pubkey formats- Consistent parameter naming and documentation across related functions
- Clear documentation of the dual-purpose nature of the identifier
However, given this is a breaking change, consider:
- Version bumping the interface (e.g.,
IAssetsV2)- Adding migration documentation
- Implementing a deprecation period if possible
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/interfaces/precompiles/IAssets.sol(2 hunks)test/mocks/AssetsMock.sol(3 hunks)
🧰 Additional context used
📓 Learnings (1)
test/mocks/AssetsMock.sol (4)
Learnt from: bwhour
PR: ExocoreNetwork/exocore-contracts#96
File: test/mocks/AssetsMock.sol:37-49
Timestamp: 2024-11-12T10:03:05.536Z
Learning: In the `depositNST` and `withdrawNST` functions, the `validatorPubkey` has a length of 32 bytes.
Learnt from: bwhour
PR: ExocoreNetwork/exocore-contracts#96
File: test/mocks/AssetsMock.sol:37-49
Timestamp: 2024-11-12T10:03:05.536Z
Learning: In the `depositNST` and `withdrawNST` functions, the `validatorPubkey` has a length of 32 bytes.
Learnt from: bwhour
PR: ExocoreNetwork/exocore-contracts#96
File: test/mocks/AssetsMock.sol:75-90
Timestamp: 2024-11-12T10:03:21.957Z
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:75-90
Timestamp: 2024-11-12T10:03:21.957Z
Learning: In the `withdrawNST` function of `AssetsMock.sol`, the `validatorPubkey.length` should be 32 bytes, not 48 bytes.
🔇 Additional comments (5)
test/mocks/AssetsMock.sol (1)
Line range hint 77-88: Verify deposit-withdraw validator ID matching
The change to use validatorID affects both deposit and withdraw operations. We should verify that a validator can only be withdrawn using the same ID format that was used for deposit.
Let's verify the contract's test coverage for this scenario:
src/interfaces/precompiles/IAssets.sol (4)
40-42: LGTM! Clear and accurate parameter documentation.
The documentation clearly explains that validatorID can represent either an index (as uint256 in bytes32) or a pubkey, which aligns well with the PR's objective of supporting both formats.
70-72: LGTM! Documentation maintains consistency.
The parameter documentation matches the depositNST function, maintaining a consistent interface description throughout the contract.
75-75: Verify integration impacts of this breaking change.
The parameter rename from validatorPubkey to validatorID in withdrawNST represents another breaking change that will affect existing integrations.
#!/bin/bash
# Search for existing calls to withdrawNST to identify affected integrations
rg -l "withdrawNST" --type solidity
# Search for potential test files that need updating
rg -l "validatorPubkey.*withdrawNST" --type solidity45-45: Verify integration impacts of this breaking change.
The parameter rename from validatorPubkey to validatorID represents a breaking change that will affect existing integrations.
|
✅ The Solhint workflow has completed successfully. Check the workflow run for details. (3a7c8aa) |
|
✅ The Forge CI workflow has completed successfully. Check the workflow run for details. (3a7c8aa) |
|
✅ The Slither Analysis workflow has completed successfully. Check the workflow run for details. (3a7c8aa) |
|
✅ The Compare Storage Layouts workflow has completed successfully. Check the workflow run for details. (3a7c8aa) |
In all functions, accept the call to IETH_POS.stake, the pubkey is not used directly. Instead it is the hashed public key.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/libraries/ValidatorContainer.sol (1)
Line range hint
1-73: Implementation maintains robustness while improving clarity.The changes to this library are well-scoped and necessary for accurate representation of validator identification. The core functionality remains intact while terminology has been clarified. The unchanged portions of the library (merkleization, epoch handling, etc.) continue to work correctly with these changes.
Consider adding:
- A version constant to track breaking changes
- Migration documentation for users of this library
- Clear documentation about the breaking changes in the library's header
src/storage/ExoCapsuleStorage.sol (1)
70-71: Consider enhancing the documentation for index mapping.While the rename to
pubkeyHashis good, consider expanding the documentation to explain:
- The relationship between validator indices and pubkey hashes
- The immutability of indices as mentioned in the PR objectives
- /// @dev Mapping of validator index to their corresponding pubkey hash. + /// @dev Mapping of validator index to their corresponding pubkey hash. + /// @dev Validator indices are immutable and remain constant even as validators + /// join or leave the network, as per the beacon chain specification. mapping(uint256 index => bytes32 pubkeyHash) internal _capsuleValidatorsByIndex;src/core/ExoCapsule.sol (1)
Line range hint
165-196: Consider caching the pubkeyHash value.The implementation correctly uses
pubkeyHashfor validator identification. However, we're callinggetPubkeyHash()multiple times on the same container.Consider caching the value to save gas:
function verifyDepositProof( bytes32[] calldata validatorContainer, BeaconChainProofs.ValidatorContainerProof calldata proof ) external onlyGateway returns (uint256 depositAmount) { bytes32 validatorPubkeyHash = validatorContainer.getPubkeyHash(); bytes32 withdrawalCredentials = validatorContainer.getWithdrawalCredentials(); - Validator storage validator = _capsuleValidators[validatorPubkeyHash]; + Validator storage validator = _capsuleValidators[validatorPubkeyHash]; if (!validatorContainer.verifyValidatorContainerBasic()) { - revert InvalidValidatorContainer(validatorPubkeyHash); + revert InvalidValidatorContainer(validatorPubkeyHash); }Also applies to: 206-243
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/core/ExoCapsule.sol(9 hunks)src/libraries/ValidatorContainer.sol(2 hunks)src/storage/ExoCapsuleStorage.sol(1 hunks)
🔇 Additional comments (7)
src/libraries/ValidatorContainer.sol (2)
29-31:
Breaking change: Function rename requires coordinated deployment.
The renaming of getPubkey to getPubkeyHash is a breaking change that will affect all contracts using this library. This aligns with the PR objective of clarifying that we're working with public key hashes rather than raw public keys.
Consider the following recommendations:
- Ensure all dependent contracts are updated simultaneously
- Consider adding a version identifier to the library to prevent mixing of old and new implementations
- Document the breaking change in the library's interface documentation
Let's identify all contracts that need to be updated:
8-8: Documentation accurately reflects the field's purpose.
The change from pubkey to pubkeyHash in the documentation better represents that this field stores the hash of the validator's BLS12-381 public key, not the raw key itself.
Let's verify that all dependent contracts have been updated to handle the pubkey hash correctly:
✅ Verification successful
Based on the search results, I can now generate the final response:
Documentation change is accurate and all dependent code is properly aligned
The change from pubkey to pubkeyHash in the documentation accurately reflects the field's purpose, and the codebase consistently uses this terminology. The search results show that:
- The ValidatorContainer library and ExoCapsule contract consistently use
pubkeyHashfor validator identification - All error messages and events properly reference
pubkeyHash - Storage mappings are correctly defined using
pubkeyHashas keys - Related functions like
getPubkeyHash()maintain consistent terminology
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to validator pubkey that might need updating
rg -i "pubkey|public.?key" --type solidity
Length of output: 77
Script:
#!/bin/bash
# Search for any remaining references to validator pubkey that might need updating
# Using ripgrep without file type filter since .sol extension might not be recognized
rg -i "pubkey|public.?key"
Length of output: 103749
Script:
#!/bin/bash
# Search for files with .sol extension and then search for pubkey/pubkeyHash references
find . -name "*.sol" -exec grep -i "pubkey\|public.?key" {} \;
Length of output: 10180
src/storage/ExoCapsuleStorage.sol (1)
67-68: LGTM! Explicit naming improves clarity.
The rename from pubkey to pubkeyHash better reflects that the mapping stores hashes of validator public keys, which aligns with the PR's objective of improving validator identification handling.
Let's verify the consistent usage of this mapping across the codebase:
✅ Verification successful
Rename to pubkeyHash is consistent and correctly implemented
The verification shows the mapping rename is properly implemented and used consistently:
- All references to
_capsuleValidatorsmapping usepubkeyHashas the key - The mapping is used correctly in core functions like
getRegisteredValidatorByPubkeyand deposit handling - Related mapping
_capsuleValidatorsByIndexalso consistently usespubkeyHashterminology - No legacy references to
pubkeymapping were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old mapping name
# and verify consistent hash usage
# Search for any remaining references to old mapping name
rg -l "pubkey.*=>.*Validator"
# Search for direct pubkey usage to ensure we're consistently using hashes
ast-grep --pattern 'mapping($_ pubkey => $_)'
Length of output: 113
Script:
#!/bin/bash
# Let's check the actual usage of this mapping in the codebase
# to ensure the rename is consistent with its usage
# Check for any references to _capsuleValidators mapping
rg "_capsuleValidators" -A 3 -B 3
# Also check for any pubkey-related function parameters or variables
# to ensure we're consistently using pubkeyHash terminology
ast-grep --pattern 'function $_($_ pubkey$_)'
ast-grep --pattern 'function $_($_ pubkeyHash$_)'
Length of output: 5012
src/core/ExoCapsule.sol (4)
69-96: LGTM! Consistent error declarations.
The error declarations have been consistently updated to use pubkeyHash terminology, maintaining clarity about the nature of the identifier being used.
315-322: LGTM! Consistent getter function update.
The getter function has been properly updated to use pubkeyHash terminology while maintaining its original functionality.
38-52: LGTM! Note: Breaking changes in event signatures.
The renaming from pubkey to pubkeyHash in events accurately reflects that we're dealing with hashed public keys. However, this is a breaking change that will affect event listeners and indexers.
Let's verify if there are any external services or contracts listening to these events:
✅ Verification successful
Let me search for any test files or external contracts that might be listening to these events to ensure we have a complete picture.
Let me try one more search pattern to look for any event expectations in tests:
Let me check the test files that import ExoCapsule to see if they're testing these events:
Based on the search results from all the scripts executed:
- The events
PartialWithdrawalRedeemedandFullWithdrawalRedeemedare only emitted within the ExoCapsule contract itself. - No external contracts or services are listening to these events.
- No test files are explicitly verifying these events.
- The events are only used for logging purposes within the contract to track withdrawal operations.
Breaking change in event signatures is safe to proceed with.
The renaming from pubkey to pubkeyHash in the events is safe as there are no external dependencies or contracts relying on these event signatures. The events are used purely for logging and tracking purposes within the contract itself.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for contracts or tests that might be listening to these events
rg -A 2 "PartialWithdrawalRedeemed|FullWithdrawalRedeemed"
Length of output: 1187
Script:
#!/bin/bash
# Search for test files that might be verifying these events
rg -l "PartialWithdrawalRedeemed|FullWithdrawalRedeemed" "test/"
# Search for any interface definitions or external contract references
ast-grep --pattern 'interface $_$ {
$$$
event PartialWithdrawalRedeemed($_$);
$$$
}'
ast-grep --pattern 'interface $_$ {
$$$
event FullWithdrawalRedeemed($_$);
$$$
}'
Length of output: 247
Script:
#!/bin/bash
# Search for test files that might be expecting these events
rg -A 3 "expectEmit|expect.*event.*PartialWithdrawalRedeemed|expect.*event.*FullWithdrawalRedeemed" "test/"
# Also search for any imports of ExoCapsule in other contracts
rg -l "import.*ExoCapsule"
Length of output: 44190
Script:
#!/bin/bash
# Search for event testing in ExoCapsule tests
rg -A 3 "PartialWithdrawalRedeemed|FullWithdrawalRedeemed" "test/foundry/unit/ExoCapsule.t.sol"
Length of output: 95
Line range hint 1-424: Ensure comprehensive test coverage for breaking changes.
The changes look good overall, but given the breaking changes in event signatures and function parameters, we should ensure comprehensive test coverage for:
- Event emissions with new parameter names
- Error cases with new parameter names
- Integration tests with external systems that might be affected by these changes
Let's check the test coverage:
On an Ethereum-based client chain, a validator container (generated as part of the proof) does not have the raw public key stored within it. Rather, it is the hash of the public key. This hash, therefore, cannot be used to query the beacon chain API.
This PR proposes an alternative solution of using the
validatorIndex, that is, the position of the validator amongst all of the validators, be used instead. Note that, according to the beacon spec, this number does not change even as more validators enter or leave the network.The
validatorIndexis passed to the ExocoreGateway as a unique identifier for it to send to the precompile, which refers to it as thevalidatorID. For other chains, where a validator public key may be directly available, thevalidatorIDwill be the public key instead of the index. On Ethereum, the public key is 48 bytes (though we pass a uint256 index which is 32 bytes). On Solana, the public key is 32 bytes and on Sui it is 96 bytes. To handle the varying lengths, the public key has been moved to the end of the payload, such that, it is the remainder of the payload after parsing the staker and the amount.BREAKING CHANGE
Since the PR modifies the format of the data sent over the wire, it is not compatible with prior deployments of ClientChainGateway and ExocoreGateway. If care is taken to ensure that there are no affected messages currently in-flight and the implementations are updated in tandem, it will be sufficient.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests