fix go bindings #264
fix go bindings #264
Conversation
WalkthroughThis pull request involves the removal of the Changes
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
src/evm/contracts/factories/CrossL2Prover__factory.ts(node:30636) [MODULE_TYPELESS_PACKAGE_JSON] Warning: Module type of file:///eslint.config.js?mtime=1732638260944 is not specified and it doesn't parse as CommonJS. Oops! Something went wrong! :( ESLint: 8.57.1 TypeError: Key "rules": Key "@typescript-eslint/quotes": Could not find "quotes" in plugin "@typescript-eslint". Did you mean "@/quotes"? 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: 0
🧹 Outside diff range and nitpick comments (1)
contracts/interfaces/ILightClient.sol (1)
57-57: LGTM! Interface simplification improves code organization.The removal of separate verifier interfaces in favor of direct method inclusion in
ILightClientis a good architectural decision that:
- Reduces interface complexity
- Maintains all necessary functionality
- Makes the contract responsibilities clearer
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
bindings/go/ilightclient/IMembershipVerifier.go(0 hunks)bindings/go/ilightclient/INonMembershipVerifier.go(0 hunks)contracts/interfaces/ILightClient.sol(2 hunks)src/evm/contracts/ILightClient.sol/IMembershipVerifier.ts(0 hunks)src/evm/contracts/ILightClient.sol/INonMembershipVerifier.ts(0 hunks)src/evm/contracts/ILightClient.sol/index.ts(0 hunks)src/evm/contracts/factories/CrossL2Prover__factory.ts(1 hunks)src/evm/contracts/factories/Dispatcher__factory.ts(1 hunks)src/evm/contracts/factories/DummyLightClient__factory.ts(1 hunks)src/evm/contracts/factories/ILightClient.sol/IMembershipVerifier__factory.ts(0 hunks)src/evm/contracts/factories/ILightClient.sol/INonMembershipVerifier__factory.ts(0 hunks)src/evm/contracts/factories/ILightClient.sol/index.ts(0 hunks)src/evm/contracts/factories/OptimisticLightClient__factory.ts(1 hunks)src/evm/contracts/factories/SequencerSoloClient__factory.ts(1 hunks)src/evm/contracts/index.ts(0 hunks)
💤 Files with no reviewable changes (9)
- bindings/go/ilightclient/IMembershipVerifier.go
- bindings/go/ilightclient/INonMembershipVerifier.go
- src/evm/contracts/ILightClient.sol/IMembershipVerifier.ts
- src/evm/contracts/ILightClient.sol/INonMembershipVerifier.ts
- src/evm/contracts/ILightClient.sol/index.ts
- src/evm/contracts/factories/ILightClient.sol/IMembershipVerifier__factory.ts
- src/evm/contracts/factories/ILightClient.sol/INonMembershipVerifier__factory.ts
- src/evm/contracts/factories/ILightClient.sol/index.ts
- src/evm/contracts/index.ts
✅ Files skipped from review due to trivial changes (4)
- src/evm/contracts/factories/CrossL2Prover__factory.ts
- src/evm/contracts/factories/DummyLightClient__factory.ts
- src/evm/contracts/factories/OptimisticLightClient__factory.ts
- src/evm/contracts/factories/SequencerSoloClient__factory.ts
🔇 Additional comments (2)
contracts/interfaces/ILightClient.sol (1)
63-73: Verify all implementations are updated with the new interface.
The interface changes require updates to all implementing contracts and bindings.
✅ Verification successful
Based on the gathered information, I can now provide the final response:
All implementations have been properly updated with the new interface methods
The verification shows that all implementations of ILightClient (OptimisticLightClient, SequencerSoloClient, and DummyLightClient) have the updated method signatures:
- All implementations include both
verifyMembershipandverifyNonMembershipwith the correctIcs23Proofparameter - No references to the old interfaces (
IMembershipVerifierorINonMembershipVerifier) were found - The method signatures match the new interface requirements across all implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all potential light client implementations and Go bindings that need updates
# Find Solidity implementations
echo "=== Solidity Implementations ==="
rg -l "contract.*implements.*ILightClient" --type solidity
# Find Go files that might need updates
echo -e "\n=== Go Files Referencing Light Client ==="
rg -l "IMembershipVerifier|INonMembershipVerifier" --type go
# Check if the old interfaces are completely removed
echo -e "\n=== Checking for remnants of old interfaces ==="
rg "interface.*(IMembershipVerifier|INonMembershipVerifier)" --type solidity
Length of output: 553
Script:
#!/bin/bash
# Let's try a different approach using fd to find Solidity files first, then grep their content
# Find all Solidity files that implement ILightClient
echo "=== Solidity Implementations ==="
fd -e sol | xargs rg -l "contract.*implements.*ILightClient"
# Find all Solidity files that might reference the old interfaces
echo -e "\n=== Files with Old Interface References ==="
fd -e sol | xargs rg -l "IMembershipVerifier|INonMembershipVerifier"
# Find all Go files that might need updates
echo -e "\n=== Go Files Referencing Light Client ==="
fd -e go | xargs rg -l "IMembershipVerifier|INonMembershipVerifier"
# Let's also check for any direct implementations of the new interface methods
echo -e "\n=== Implementation of New Methods ==="
fd -e sol | xargs rg "function verifyMembership.*Ics23Proof|function verifyNonMembership.*Ics23Proof"
Length of output: 2976
Script:
#!/bin/bash
# Let's verify the actual implementations of ILightClient and their method signatures
echo "=== Light Client Implementations ==="
# Check OptimisticLightClient implementation
echo "OptimisticLightClient:"
rg "contract OptimisticLightClient" -A 2 contracts/core/OptimisticLightClient.sol
# Check SequencerSoloClient implementation
echo -e "\nSequencerSoloClient:"
rg "contract SequencerSoloClient" -A 2 contracts/core/SequencerSoloClient.sol
# Check DummyLightClient implementation
echo -e "\nDummyLightClient:"
rg "contract DummyLightClient" -A 2 contracts/utils/DummyLightClient.sol
# Let's also verify if there are any other contracts implementing ILightClient
echo -e "\n=== Searching for other ILightClient implementations ==="
fd -e sol | xargs rg "contract.*ILightClient"
Length of output: 1863
src/evm/contracts/factories/Dispatcher__factory.ts (1)
2093-2093: Assignment of _bytecode updated correctly
The _bytecode variable has been updated appropriately with the new contract bytecode.
nicopernas
force-pushed
the
nico/fix-go-bindings
branch
from
8cc13ea to
03004d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/evm/contracts/factories/Dispatcher__factory.ts (1)
Line range hint
2143-2164: Consider enhancing library linking robustnessWhile the current implementation works correctly, consider these improvements:
- Add validation for library addresses
- Use a more robust linking mechanism than regex replacement
- Add error handling for missing library addresses
Example implementation:
static linkBytecode( linkLibraryAddresses: DispatcherLibraryAddresses ): string { + // Validate library addresses + Object.entries(linkLibraryAddresses).forEach(([name, addr]) => { + if (!addr || !addr.match(/^0x[0-9a-fA-F]{40}$/)) { + throw new Error(`Invalid library address for ${name}`); + } + }); + let linkedBytecode = _bytecode; linkedBytecode = linkedBytecode.replace( new RegExp("__\\$d825222459c46c14afb2efe0967c30e98d\\$__", "g"), linkLibraryAddresses["contracts/libs/Ibc.sol:Ibc"] .replace(/^0x/, "") .toLowerCase() ); // ... rest of the code
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
bindings/go/ilightclient/IMembershipVerifier.go(0 hunks)bindings/go/ilightclient/INonMembershipVerifier.go(0 hunks)contracts/interfaces/ILightClient.sol(2 hunks)src/evm/contracts/ILightClient.sol/IMembershipVerifier.ts(0 hunks)src/evm/contracts/ILightClient.sol/INonMembershipVerifier.ts(0 hunks)src/evm/contracts/ILightClient.sol/index.ts(0 hunks)src/evm/contracts/factories/CrossL2Prover__factory.ts(1 hunks)src/evm/contracts/factories/Dispatcher__factory.ts(1 hunks)src/evm/contracts/factories/DummyLightClient__factory.ts(1 hunks)src/evm/contracts/factories/ILightClient.sol/IMembershipVerifier__factory.ts(0 hunks)src/evm/contracts/factories/ILightClient.sol/INonMembershipVerifier__factory.ts(0 hunks)src/evm/contracts/factories/ILightClient.sol/index.ts(0 hunks)src/evm/contracts/factories/OptimisticLightClient__factory.ts(1 hunks)src/evm/contracts/factories/SequencerSoloClient__factory.ts(1 hunks)src/evm/contracts/index.ts(0 hunks)
💤 Files with no reviewable changes (9)
- bindings/go/ilightclient/IMembershipVerifier.go
- bindings/go/ilightclient/INonMembershipVerifier.go
- src/evm/contracts/ILightClient.sol/IMembershipVerifier.ts
- src/evm/contracts/ILightClient.sol/INonMembershipVerifier.ts
- src/evm/contracts/ILightClient.sol/index.ts
- src/evm/contracts/factories/ILightClient.sol/IMembershipVerifier__factory.ts
- src/evm/contracts/factories/ILightClient.sol/INonMembershipVerifier__factory.ts
- src/evm/contracts/factories/ILightClient.sol/index.ts
- src/evm/contracts/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/evm/contracts/factories/CrossL2Prover__factory.ts
- src/evm/contracts/factories/DummyLightClient__factory.ts
- src/evm/contracts/factories/OptimisticLightClient__factory.ts
- src/evm/contracts/factories/SequencerSoloClient__factory.ts
🔇 Additional comments (6)
contracts/interfaces/ILightClient.sol (3)
25-25: LGTM! Comment formatting improves readability.
The comment formatting enhancement provides better clarity about the latency considerations for the SequencerLightClient type.
45-45: LGTM! Improved documentation formatting.
The comment formatting enhancement improves the readability of the LIGHT_CLIENT_TYPE function documentation.
50-57: Architectural improvement: Interface consolidation looks good.
The consolidation of membership verification methods into ILightClient and removal of separate verifier interfaces improves the design by:
- Reducing interface complexity
- Providing clearer contract responsibilities
- Maintaining functionality while simplifying the inheritance hierarchy
Let's verify the impact of these interface changes:
✅ Verification successful
Interface changes are safe and implementations are already aligned
The verification shows that:
- No references to removed interfaces (IMembershipVerifier/INonMembershipVerifier) exist in the codebase
- Both existing implementations (SequencerSoloClient and DummyLightClient) already implement the membership verification methods
- The implementations are compatible with the consolidated interface, having proper method signatures for
verifyMembershipandverifyNonMembership - All imports and dependencies are using ILightClient directly, showing the interface consolidation is already in place
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing implementations that might be affected by the interface changes
# Search for contract implementations of the removed interfaces
echo "Checking for implementations of removed interfaces..."
rg "contract \w+ is .*IMembershipVerifier"
rg "contract \w+ is .*INonMembershipVerifier"
# Search for direct usage of the removed interfaces
echo "Checking for direct interface usage..."
rg "IMembershipVerifier\s+\w+\s*="
rg "INonMembershipVerifier\s+\w+\s*="
Length of output: 373
Script:
#!/bin/bash
# Let's try a different approach to verify the interface changes
# First, let's check if these interfaces existed before in the codebase
echo "Checking for any references to removed interfaces..."
rg -l "IMembershipVerifier|INonMembershipVerifier"
# Check for any light client implementations to understand impact
echo "Checking for light client implementations..."
rg "contract \w+ is .*ILightClient"
# Look for any imports of ILightClient to find dependent contracts
echo "Checking for ILightClient imports..."
rg "import.*ILightClient"
# Check the history of the interface file to understand changes
echo "Checking interface file history..."
git log --oneline contracts/interfaces/ILightClient.sol | head -n 5
Length of output: 2194
Script:
#!/bin/bash
# Let's examine the actual implementations to understand if they're compatible with the interface changes
# Check SequencerSoloClient implementation
echo "Examining SequencerSoloClient implementation..."
ast-grep --pattern 'contract SequencerSoloClient is ILightClient {
$$$
}'
# Check DummyLightClient implementation
echo "Examining DummyLightClient implementation..."
ast-grep --pattern 'contract DummyLightClient is ILightClient {
$$$
}'
# Look for any membership verification methods in these implementations
echo "Checking for membership verification methods..."
rg -A 5 "function verifyMembership|function verifyNonMembership" contracts/core/SequencerSoloClient.sol contracts/utils/DummyLightClient.sol
Length of output: 2609
src/evm/contracts/factories/Dispatcher__factory.ts (3)
Line range hint 2094-2204: LGTM: Factory implementation follows best practices
The Dispatcher__factory implementation:
- Correctly extends ContractFactory
- Implements proper type checking for constructor parameters
- Provides necessary static methods for interface creation and contract connection
- Follows the ethers.js factory pattern
Line range hint 2205-2209: LGTM: Type definitions are well structured
The TypeScript type definitions:
- Properly define constructor parameters
- Correctly specify library address requirements
- Follow TypeScript best practices
2093-2093: Verify bytecode matches contract changes
The bytecode has been updated, which is expected due to the removal of the membership verifier interfaces. However, we should verify that this bytecode was properly generated from the updated Solidity contract.
✅ Verification successful
Based on the analysis of the contract source and bytecode changes:
Bytecode changes are valid and consistent with interface removals
The changes in the bytecode at line 2093 are consistent with the removal of IMembershipVerifier and INonMembershipVerifier interfaces. This is supported by:
- No imports or references to these interfaces found in the current contract implementation
- Contract inheritance remains unchanged, showing only: Ownable2StepUpgradeable, UUPSUpgradeable, ReentrancyGuardUpgradeable, IDispatcher
- No membership verification related functions found in the contract
- The bytecode changes reflect the removal of these interfaces without affecting core functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the bytecode matches the contract source
# Check if the bytecode matches a fresh compilation of the contract
# Find the Dispatcher contract source file
fd -t f "Dispatcher.sol"
# Find all related interface files that were removed
rg -l "IMembershipVerifier|INonMembershipVerifier"
Length of output: 183
Script:
#!/bin/bash
# Let's analyze the contract changes and interface removals
# First, check the contract source for membership verifier references
rg -A 5 -B 5 "IMembershipVerifier|INonMembershipVerifier" contracts/core/Dispatcher.sol
# Check for any remaining imports of these interfaces
rg "import.*IMembershipVerifier|import.*INonMembershipVerifier"
# Look for any other related interface usage
rg -A 2 "membershipVerifier|nonMembershipVerifier" contracts/core/Dispatcher.sol
# Check git history for the bytecode changes
git log -p -- src/evm/contracts/factories/Dispatcher__factory.ts | grep -A 5 -B 5 '"0x60a0604052306080523480156200001557'
Length of output: 238
Script:
#!/bin/bash
# Let's check the contract changes more broadly to understand the bytecode changes
# Find and examine the Dispatcher contract content
cat contracts/core/Dispatcher.sol
# Check for any interface-related changes in the entire codebase
rg -l "interface" --type sol
# Look for any factory-related changes or patterns
rg -l "factory" --type ts
# Check the git diff for the specific file
git diff HEAD~1 src/evm/contracts/factories/Dispatcher__factory.ts
Length of output: 109873
Script:
#!/bin/bash
# Let's analyze the contract source and bytecode changes
# Find the Dispatcher contract implementation
cat contracts/core/Dispatcher.sol | grep -A 5 -B 5 "IMembershipVerifier\|INonMembershipVerifier"
# Check for any membership verification related imports
cat contracts/core/Dispatcher.sol | grep "import.*IMembershipVerifier\|import.*INonMembershipVerifier"
# Look for any membership verification related functions
cat contracts/core/Dispatcher.sol | grep -A 5 "membershipVerifier\|nonMembershipVerifier"
# Check the contract's inheritance
cat contracts/core/Dispatcher.sol | grep "contract Dispatcher is"
Length of output: 470
This PR fixes an issue introduced in
v4.0.11that would cause the go bindings to no build.Using
v4.0.11you'll get something like thisThe proposed change merges the newly added
INonMembershipVerifierandIMembershipVerifierinterfaces withILightClientSummary by CodeRabbit
New Features
ILightClientinterface by integratingverifyMembershipandverifyNonMembershipmethods directly.Bug Fixes
Chores