perf: change parameter types of hasOracle, isOracleOnline, chainName to bytes32 #898
Conversation
WalkthroughThis pull request introduces significant changes to the crosschain and bridge fee oracle infrastructure, primarily focusing on modifying how chain identifiers are handled. The changes involve transitioning from string-based chain representations to byte32 (fixed-size byte array) representations across multiple contracts, interfaces, and implementation files. The most notable modifications include removing the Changes
Sequence DiagramsequenceDiagram
participant Client
participant Contract
participant Router
Client->>Contract: Call hasOracle(bytes32 chain, address)
Contract->>Router: Verify chain route
Router-->>Contract: Return route status
Contract-->>Client: Return oracle presence
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
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
🧹 Nitpick comments (4)
precompiles/crosschain/has_oracle.go (1)
45-49: Update error message to use string representation.The error message should use the string representation of the chain for better readability.
- return nil, fmt.Errorf("chain not support: %s", args.Chain) + return nil, fmt.Errorf("chain not support: %s", chainName)precompiles/crosschain/is_oracle_online.go (1)
45-49: Update error message to use string representation.The error message should use the string representation of the chain for better readability.
- return nil, fmt.Errorf("chain not support: %s", args.Chain) + return nil, fmt.Errorf("chain not support: %s", chainName)precompiles/crosschain/is_oracle_online_test.go (2)
33-33: Verify error handling for MustStrToByte32.The test uses
contract.MustStrToByte32which likely panics on conversion failure. Consider:
- Adding test cases for chain names that would fail conversion
- Using a non-panicking version for better error handling
Consider adding these test cases:
{ name: "chain name too long", malleate: func() (contract.IsOracleOnlineArgs, error) { longChainName := strings.Repeat("a", 33) // 33 bytes return contract.IsOracleOnlineArgs{ Chain: contract.MustStrToByte32(longChainName), ExternalAddress: helpers.GenHexAddress(), }, fmt.Errorf("chain name too long") }, result: false, }, { name: "chain name with special characters", malleate: func() (contract.IsOracleOnlineArgs, error) { return contract.IsOracleOnlineArgs{ Chain: contract.MustStrToByte32("chain\x00name"), ExternalAddress: helpers.GenHexAddress(), }, fmt.Errorf("invalid characters in chain name") }, result: false, }Also applies to: 44-44, 54-54, 64-64
Line range hint
1-89: Consider adding test coverage for bytes32 padding.The test suite would benefit from explicit verification of how chain names are padded to 32 bytes, especially for:
- Names shorter than 32 bytes
- Names exactly 32 bytes
- UTF-8 encoded names
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
contract/bridge_fee_oracle.sol.go(1 hunks)contract/crosschain.go(1 hunks)contract/icrosschain.sol.go(4 hunks)precompiles/crosschain/has_oracle.go(1 hunks)precompiles/crosschain/has_oracle_test.go(3 hunks)precompiles/crosschain/is_oracle_online.go(1 hunks)precompiles/crosschain/is_oracle_online_test.go(4 hunks)solidity/contracts/bridge/BridgeFeeOracle.sol(1 hunks)solidity/contracts/bridge/IBridgeOracle.sol(1 hunks)solidity/contracts/test/BridgeFeeQuoteTest.sol(1 hunks)solidity/contracts/test/CrosschainTest.sol(2 hunks)solidity/test/bridge_fee_quote.ts(1 hunks)tests/contract/crosschain_test.sol.go(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (20)
tests/contract/crosschain_test.sol.go (5)
34-35: Updated ABI and Bin to reflect contract changesThe
ABIandBinfields inCrosschainTestMetaDatahave been updated to match the modified contract methods, specifically changing parameter types fromstringtobytes32. This ensures that the Go bindings accurately represent the updated Solidity contract interface.
Line range hint
267-275: UpdatedHasOraclemethod to accept[32]bytefor_chainThe
HasOraclemethod inCrosschainTestCallernow correctly accepts_chainas a[32]bytetype, aligning with the updated Solidity function that usesbytes32. This change ensures type consistency between the Go bindings and the Solidity contract.
284-294: UpdatedHasOraclemethod inCrosschainTestSessionto use[32]byteThe
HasOraclemethod inCrosschainTestSessionhas been updated to accept_chainas[32]byte, matching the contract changes. This ensures that sessions utilizing this method are consistent with the new parameter type.
Line range hint
298-309: UpdatedIsOracleOnlinemethod to accept[32]bytefor_chainThe
IsOracleOnlinemethod inCrosschainTestCallernow accepts_chainas[32]byte, ensuring consistency with the updated Solidity contract and maintaining type safety in the Go bindings.
315-318: UpdatedIsOracleOnlineinCrosschainTestSessionto use[32]byteThe
IsOracleOnlinemethod inCrosschainTestSessionhas been updated to accept_chainas[32]byte, aligning with the contract's interface changes.solidity/contracts/bridge/IBridgeOracle.sol (2)
14-16: Updated_chainparameter type tobytes32inisOracleOnlineThe
_chainparameter inisOracleOnlinehas been updated fromstring memorytobytes32. Verify that all implementations and callers of this interface method have been updated to usebytes32for the_chainparameter.Please run the script provided in the previous comment to ensure consistency across all implementations.
9-11: Updated_chainparameter type tobytes32inhasOracleThe
_chainparameter inhasOraclehas been changed fromstring memorytobytes32, enhancing efficiency and consistency in handling chain identifiers. Ensure that all implementations ofIBridgeOracleand any contracts invoking this function have updated their implementations and calls accordingly.Run the following script to verify that all implementations of
IBridgeOraclehave updated the_chainparameter tobytes32:solidity/contracts/test/BridgeFeeQuoteTest.sol (4)
12-12: Changed mapping key type tobytes32fororacleStatusThe mapping
oracleStatusnow usesbytes32as the key type for_chainName, improving efficiency and consistency in representing chain identifiers. Ensure that any code interacting with this mapping is updated to usebytes32keys.
15-19: Updated_chainNameparameter tobytes32insetOracleThe
_chainNameparameter insetOraclehas been changed fromstring memorytobytes32. This change enhances performance but may require updates to any code calling this function to ensure the correct data type is passed.As a reminder, ensure that any calls to
setOracleare updated to passbytes32for_chainName.
23-27: Updated_chainNameparameter tobytes32inhasOracleThe
_chainNameparameter inhasOraclehas been updated tobytes32. Verify that any code calling this function usesbytes32for the chain name to maintain consistency.
Line range hint
30-34: Updated_chainNameparameter tobytes32inisOracleOnlineThe
_chainNameparameter inisOracleOnlinenow usesbytes32instead ofstring memory. Ensure that callers of this function have been updated accordingly to pass the correct parameter type.contract/crosschain.go (1)
95-95: LGTM! Type changes align with PR objective.The change from
stringtocommon.Hashfor theChainfield in both structs is consistent with the PR's goal of using bytes32 for chain identifiers.Also applies to: 107-107
precompiles/crosschain/has_oracle.go (1)
38-38: LGTM! Parameter rename improves clarity.Renaming the parameter from
contracttovmContracthelps avoid confusion with the package name.precompiles/crosschain/has_oracle_test.go (1)
33-33: LGTM! Test cases properly updated.The test cases have been correctly updated to use
contract.MustStrToByte32for converting chain names to bytes32, maintaining consistency with the new type system.Also applies to: 43-43, 53-53
solidity/contracts/test/CrosschainTest.sol (2)
Line range hint
105-111: LGTM! Parameter type change looks good.The change from
stringtobytes32for the_chainparameter aligns with the performance improvement goal.
Line range hint
116-122: LGTM! Parameter type change looks good.The change from
stringtobytes32for the_chainparameter aligns with the performance improvement goal.solidity/contracts/bridge/BridgeFeeOracle.sol (1)
69-71: LGTM! Direct bytes32 usage improves performance.The removal of string conversion and direct usage of bytes32 for chain names reduces gas costs while maintaining the same functionality.
Also applies to: 74-75
solidity/test/bridge_fee_quote.ts (1)
38-38: LGTM! Test correctly uses encoded bytes32.The test has been properly updated to use the encoded chain name, maintaining test coverage for the new parameter type.
contract/icrosschain.sol.go (1)
214-217: LGTM! Generated bindings correctly reflect the interface changes.The Go bindings have been properly updated to match the Solidity contract changes:
- Function signatures updated to use [32]byte instead of string
- Method IDs updated to reflect the new parameter types
- Documentation updated to match the changes
Also applies to: 231-234, 238-241, 245-248, 262-265, 269-272
contract/bridge_fee_oracle.sol.go (1)
34-34: Verify the impact of removing Bytes32ToString function.The ABI shows that chain name parameters are now using
bytes32instead ofstring. While this change can improve gas efficiency by avoiding dynamic memory allocation for strings, ensure that:
- All chain names fit within 32 bytes
- Proper conversion utilities are available for external systems interacting with these bytes32 parameters
✅ Verification successful
Chain names are compatible with bytes32 format
All chain names found in the codebase are well under 32 bytes in length. The contract includes built-in validation (ChainNameInvalid error) and the codebase has consistent error handling for invalid chain names. The change from string to bytes32 is safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if any chain names in the codebase exceed 32 bytes rg -l '[^"]*"[^"]{33,}".*chain' | while read -r file; do echo "File with potentially long chain names: $file" rg '[^"]*"[^"]{33,}".*chain' "$file" doneLength of output: 34846
Script:
#!/bin/bash # Search for chain name string literals and constants rg -g '!*.pb.go' -g '!*.pulsar.go' -g '!*.md' -g '!*.json' '"[^"]*chain[^"]*"' --no-filename | sort -u # Also check for chain name constants ast-grep --pattern 'const $_ = $chain_name'Length of output: 44878
Summary by CodeRabbit
Breaking Changes
stringtobytes32across multiple contracts and interfaces.bytes32ToStringfunction fromBridgeFeeOraclecontract.Code Improvements
Deprecation
CrosschainTestABIandCrosschainTestBin.