generate go bindings in one package per contract/library #270
Conversation
WalkthroughThe pull request introduces significant changes to multiple Go packages related to Ethereum smart contracts. Key updates include renaming packages to better reflect their functionality, restructuring bindings for various contracts, and enhancing the generation of Go bindings from ABI files. The Changes
Possibly related PRs
Suggested labels
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 (8)
bindings/go/ibceventsemitter/IbcEventsEmitter.go (3)
Line range hint
65-65: Ensure variable naming consistencyThe variable
IbcEventsEmitterABIis deprecated but still present. Verify if it is still needed or if it can be safely removed to avoid confusion.Consider removing the deprecated variable if it is no longer in use:
-// IbcEventsEmitterABI is the input ABI used to generate the binding from. -// Deprecated: Use IbcEventsEmitterMetaData.ABI instead. -var IbcEventsEmitterABI = IbcEventsEmitterMetaData.ABI
Line range hint
87-89: Redundant type assertion in error handlingIn the function
NewIbcEventsEmitter, the error handling could be simplified.Consider simplifying the error return line:
- if err != nil { - return nil, err - } + if err != nil { + return nil, fmt.Errorf("failed to bind IbcEventsEmitter: %w", err) + }This provides more context if the binding fails.
Line range hint
785-788: Potential performance improvement in event iterationThe
Next()method in event iterators can be optimized by reducing redundant checks.Consider restructuring the method to avoid checking
it.failandit.donemultiple times.Makefile (1)
69-69: Unnecessary use ofexit 1aftergo buildThe command
go build ./... || exit 1is redundant becausego buildwill exit with a non-zero status on failure.You can simplify the command:
- go build ./... || exit 1; \ + go build ./...; \bindings/go/protocounterparty/ProtoCounterparty.go (1)
Line range hint
23-24: Empty ABI in metadataThe
ABIfield inProtoCounterpartyMetaDatais empty. This might cause runtime errors when attempting to bind contract methods.Ensure that the ABI is correctly defined. If the contract does not have any methods, consider whether the binding is necessary.
bindings/go/iclientupdates/IClientUpdates.go (2)
Line range hint
47-50: Error handling in constructor functionsIn
NewIClientUpdates, errors frombindIClientUpdatesare returned directly. Consider adding context to the error message for better debugging.Modify the error return to include additional context:
- if err != nil { - return nil, err - } + if err != nil { + return nil, fmt.Errorf("failed to bind IClientUpdates: %w", err) + }
Line range hint
104-111: Type assertion may cause panicsIn the
LIGHTCLIENTTYPEfunction, the type assertion may panic if the contract does not return the expected type.Add error checking to ensure safe type conversion.
Apply this diff to handle the error gracefully:
out0, ok := out[0].(uint8) + if !ok { + return 0, fmt.Errorf("unexpected type for LIGHTCLIENTTYPE output") + } return out0, errbindings/go/revertingbytesmars/RevertingBytesMars.go (1)
Line range hint
1-894: Architecture aligns well with the PR objectives.The separation of the RevertingBytesMars contract into its own package:
- Improves modularity by avoiding naming conflicts
- Makes the codebase more maintainable
- Follows Go best practices for package organization
This change successfully addresses the original issue of build errors when multiple contracts were in the same package.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
Makefile(1 hunks)bindings/go/iappstateverifier/IAppStateVerifier.go(1 hunks)bindings/go/ibceventsemitter/IbcEventsEmitter.go(1 hunks)bindings/go/ibcpacketsender/IbcPacketSender.go(1 hunks)bindings/go/iclientupdates/IClientUpdates.go(1 hunks)bindings/go/panickingmars/PanickingMars.go(1 hunks)bindings/go/protochannel/ProtoChannel.go(1 hunks)bindings/go/protocounterparty/ProtoCounterparty.go(1 hunks)bindings/go/revertingbytesmars/RevertingBytesMars.go(1 hunks)bindings/go/revertingemptymars/RevertingEmptyMars.go(1 hunks)bindings/go/revertingstringclosechannelmars/RevertingStringCloseChannelMars.go(1 hunks)bindings/go/revertingstringmars/RevertingStringMars.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- bindings/go/protochannel/ProtoChannel.go
🔇 Additional comments (15)
bindings/go/iappstateverifier/IAppStateVerifier.go (1)
4-4: Package name updated to iappstateverifier
Changing the package name to iappstateverifier aligns with the directory structure and enhances code organization.
bindings/go/panickingmars/PanickingMars.go (1)
4-4: Package name updated to panickingmars
Renaming the package to panickingmars reflects the specific functionality of the module and improves code clarity.
bindings/go/revertingemptymars/RevertingEmptyMars.go (1)
4-4: Package name updated to revertingemptymars
Updating the package name to revertingemptymars enhances alignment with the module's purpose and improves code organization.
bindings/go/revertingstringmars/RevertingStringMars.go (1)
4-4: Package name updated to revertingstringmars
Changing the package name to revertingstringmars accurately reflects the module's functionality and aids in maintaining a clear codebase.
bindings/go/revertingstringclosechannelmars/RevertingStringCloseChannelMars.go (1)
4-4: Package name updated to revertingstringclosechannelmars
Renaming the package to revertingstringclosechannelmars aligns with the module's specific functionality and improves code organization.
bindings/go/ibceventsemitter/IbcEventsEmitter.go (1)
4-4: Package name change may introduce import errors
The package name has been changed from ibcdispatcher to ibceventsemitter. Ensure that all references to this package in the codebase have been updated accordingly to prevent import errors.
Run the following script to identify any references to the old package name:
bindings/go/protocounterparty/ProtoCounterparty.go (1)
4-4: Package name change may introduce import errors
The package name has been changed from channel to protocounterparty. Ensure that all references to this package in the codebase have been updated accordingly to prevent import errors.
Run the following script to identify any references to the old package name:
bindings/go/ibcpacketsender/IbcPacketSender.go (2)
Line range hint 63-69: Check for correct contract method bindings
The method SendPacket has been defined. Ensure that the parameters and return types match the contract's actual implementation.
Double-check that the types [32]byte, []byte, and uint64 correctly correspond to the Solidity types in the contract.
4-4: Package name change requires codebase updates
The package name has been changed from ibcdispatcher to ibcpacketsender. Ensure that all references to this package have been updated to prevent import errors.
Run the following script to find references to the old package name:
bindings/go/iclientupdates/IClientUpdates.go (1)
4-4: Package name change may cause import issues
The package name has been changed from ilightclient to iclientupdates. Ensure that all references to ilightclient are updated.
Run the following script to find any outdated references:
bindings/go/revertingbytesmars/RevertingBytesMars.go (5)
4-4: LGTM: Package name follows the new convention.
The package name revertingbytesmars aligns with the PR objective of having separate packages for each contract, improving modularity and avoiding naming conflicts.
Line range hint 29-71: LGTM: Data structures are well-defined and properly mapped.
The Go structs (AckPacket, Height, IbcEndpoint, IbcPacket) are correctly generated from their Solidity counterparts with appropriate type mappings and maintain good Go naming conventions.
Line range hint 77-669: LGTM: Contract methods are well-structured and properly bound.
The contract methods are:
- Properly categorized (Caller, Transactor, Filterer)
- Well-documented with clear function signatures
- Implementing consistent error handling
Line range hint 671-894: LGTM: Event handling implementation is robust.
The OwnershipTransferred event handling:
- Follows standard Go patterns
- Implements proper iterator patterns with thread-safety
- Includes comprehensive error handling and resource cleanup
Line range hint 74-75: Verify ABI completeness and deprecation handling.
The code maintains backward compatibility by keeping the deprecated ABI variable while directing users to MetaData.ABI. Consider adding a deprecation timeline to help users plan their migration.
✅ Verification successful
Based on the results from the executed scripts, I can now provide a final response:
ABI deprecation handling is consistent with Go bindings best practices
The deprecation notice for RevertingBytesMarsABI follows the standard pattern used across all Go contract bindings in the codebase, directing users to use RevertingBytesMarsMetaData.ABI. The ABI is complete, containing all contract methods, events, and errors, including core functionality like greet, greetWithFee, and IBC-related methods.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if there are any direct uses of the deprecated ABI variable
rg -l "RevertingBytesMarsABI" --type go
Length of output: 92
Script:
#!/bin/bash
# Check the context around the ABI usage to understand the deprecation notice and completeness
rg "RevertingBytesMarsABI" -A 5 -B 5 --type go
Length of output: 13833
Script:
#!/bin/bash
# Check MetaData.ABI definition and usage
rg "MetaData.ABI" --type go -A 2 -B 2
Length of output: 18218
Script:
#!/bin/bash
# Look for contract methods and events in the ABI
ast-grep --pattern 'var MetaData = &bind.MetaData{
$$$
}'
Length of output: 61
| @@ -62,10 +62,11 @@ bindings-gen-go: build-contracts | |||
| continue; \ | |||
| fi; \ | |||
| type=$$(basename $$abi_file .abi.json); \ | |||
| pkg=$$(basename $$abi_base .sol | tr "[:upper:]" "[:lower:]"); \ | |||
| pkg=$$(basename $$type .sol | tr "[:upper:]" "[:lower:]"); \ | |||
There was a problem hiding this comment.
Correct package name derivation
In the bindings-gen-go target, the package name is derived using a transformation that may not handle all cases correctly.
Update the package name derivation to handle contract names with uppercase letters correctly.
Apply this diff to fix the issue:
- pkg=$$(basename $$type .sol | tr "[:upper:]" "[:lower:]"); \
+ pkg=$$(echo $$type | sed -E 's/([A-Z])/_\L\1/g' | sed 's/^_//'); \This change converts CamelCase names to snake_case, which is more appropriate for Go package names.
📝 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.
| pkg=$$(basename $$type .sol | tr "[:upper:]" "[:lower:]"); \ | |
| pkg=$$(echo $$type | sed -E 's/([A-Z])/_\L\1/g' | sed 's/^_//'); \ |
| mkdir -p ./bindings/go/$$pkg; \ | ||
| abigen --abi $$abi_file --pkg $$pkg --type $$type --out ./bindings/go/$$pkg/$$type.go; \ | ||
| done; \ | ||
| go build ./... || exit 1; \ |
There was a problem hiding this comment.
nit: you need the || exit 1. If go build fails, the make target will fail too.
There was a problem hiding this comment.
nit2: maybe add a message saying "running check sanity on bindings" or similar?
PR to make each contract wihtin each sol file have it's own module.
Previously, if we ran the
make bindings-gen-gocommand, it would produce go files which would not build for the mars contract, since it would declare all contracts declared in thecontracts/examples/Mars.sol(Mars, PanicingMars, etc) as part of one go module. This would produce errors if you tried to build the go bindings usinggo build ./...because it would result in doubly-declaring certain structs (e.g. AckPacket for Mars contracts). To allow our go bindings to be robust to having multiple contracts to be declared in the same sol files, we can declare each contract as part of it's own package.This should only impact users of the reverting/panicking mars (of which there are none), and
IbcDispatchergo bindingsSummary by CodeRabbit
New Features
Refactor
Bug Fixes