Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

indexer: remove bindings #10223

Merged
merged 2 commits into from
Apr 23, 2024
Merged

indexer: remove bindings #10223

merged 2 commits into from
Apr 23, 2024

Conversation

tynes
Copy link
Contributor

@tynes tynes commented Apr 19, 2024

Description

Removes the dependency on the op-bindings/bindings package.
This is to improve devex in the monorepo and reduce CI time
as maintaining the bindings autogenerated in each PR just
doesn't scale. Now each service is responsible for their own
bindings. In the future, we can work towards releases of
the bindings when contracts are released.

Follows:

@tynes tynes requested a review from a team as a code owner April 19, 2024 02:37
Copy link
Contributor

coderabbitai bot commented Apr 19, 2024

Walkthrough

Walkthrough

The recent updates involve restructuring import paths in various test and contract processor files within the ethereum-optimism/optimism repository, transitioning from op-bindings/bindings to indexer/bindings. Additionally, a new file l2tol1messagepasser.go has been added to interact with the L2ToL1MessagePasser Ethereum contract.

Changes

Files Change Summary
l2tol1messagepasser.go Provides functionalities for the L2ToL1MessagePasser Ethereum contract, including deployment, function interactions, and event handling.
Various e2e test files Updated import paths from op-bindings/bindings to indexer/bindings.
Various contract processor files Updated import paths from op-bindings/bindings to indexer/bindings.

Recent Review Details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits Files that changed from the base of the PR and between c956771 and a40d1c7.
Files selected for processing (18)
  • indexer/bindings/crossdomainmessenger.go (1 hunks)
  • indexer/bindings/l1crossdomainmessenger.go (1 hunks)
  • indexer/bindings/l2crossdomainmessenger.go (1 hunks)
  • indexer/bindings/l2outputoracle.go (1 hunks)
  • indexer/bindings/l2tol1messagepasser.go (1 hunks)
  • indexer/bindings/standardbridge.go (1 hunks)
  • indexer/e2e_tests/bridge_messages_e2e_test.go (1 hunks)
  • indexer/e2e_tests/bridge_transactions_e2e_test.go (1 hunks)
  • indexer/e2e_tests/bridge_transfers_e2e_test.go (1 hunks)
  • indexer/e2e_tests/etl_e2e_test.go (2 hunks)
  • indexer/e2e_tests/reorg_e2e_test.go (1 hunks)
  • indexer/e2e_tests/utils/cross_domain_messages.go (1 hunks)
  • indexer/e2e_tests/utils/deposits.go (1 hunks)
  • indexer/processors/contracts/cross_domain_messenger.go (1 hunks)
  • indexer/processors/contracts/l2_to_l1_message_passer.go (1 hunks)
  • indexer/processors/contracts/legacy_standard_bridge.go (1 hunks)
  • indexer/processors/contracts/optimism_portal.go (1 hunks)
  • indexer/processors/contracts/standard_bridge.go (1 hunks)
Files not summarized due to errors (5)
  • indexer/bindings/crossdomainmessenger.go: Error: Message exceeds token limit
  • indexer/bindings/l1crossdomainmessenger.go: Error: Message exceeds token limit
  • indexer/bindings/l2crossdomainmessenger.go: Error: Message exceeds token limit
  • indexer/bindings/l2outputoracle.go: Error: Message exceeds token limit
  • indexer/bindings/standardbridge.go: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (13)
  • indexer/bindings/l2tol1messagepasser.go
  • indexer/e2e_tests/bridge_messages_e2e_test.go
  • indexer/e2e_tests/bridge_transactions_e2e_test.go
  • indexer/e2e_tests/bridge_transfers_e2e_test.go
  • indexer/e2e_tests/etl_e2e_test.go
  • indexer/e2e_tests/reorg_e2e_test.go
  • indexer/e2e_tests/utils/cross_domain_messages.go
  • indexer/e2e_tests/utils/deposits.go
  • indexer/processors/contracts/cross_domain_messenger.go
  • indexer/processors/contracts/l2_to_l1_message_passer.go
  • indexer/processors/contracts/legacy_standard_bridge.go
  • indexer/processors/contracts/optimism_portal.go
  • indexer/processors/contracts/standard_bridge.go
Additional comments not posted (27)
indexer/bindings/standardbridge.go (13)

1-2: The file header correctly indicates that this is a generated file and should not be manually edited.


6-17: Imports are well-organized and relevant to Ethereum contract interactions. Ensure all imports are used to avoid unnecessary dependencies.


19-29: Reference imports are used to suppress errors if they are not otherwise used. This is a good practice to ensure that the compiler does not optimize away these imports.


31-34: The metadata for the StandardBridge contract is correctly defined and encapsulated within a variable. This is crucial for ABI interactions.


36-38: The deprecation notice for StandardBridgeABI is clear. It's good practice to guide developers towards using the newer StandardBridgeMetaData.ABI.


40-45: The struct StandardBridge is well-defined, separating concerns into caller, transactor, and filterer. This enhances readability and maintainability.


99-106: The constructor function NewStandardBridge is correctly implemented to handle potential errors and return a structured type. This is crucial for safe contract interactions.


136-142: The bindStandardBridge function correctly parses the ABI and binds the contract, handling errors appropriately. This is a critical function for initializing contract interactions.


148-150: The Call method in StandardBridgeRaw is implemented to handle generic contract calls. Ensure that error handling is robust and that the method is used correctly in context.


185-196: The MESSENGER method is a simple wrapper around a contract call. It correctly handles the conversion of the output to the expected type. Ensure that the contract method name and the expected return type align with the contract's actual implementation.


368-373: The BridgeERC20 method correctly constructs a transaction with the appropriate parameters. Ensure that the method's usage complies with the expected business logic and security practices, especially since it involves token transfers.


494-499: The Receive method is a special case handling the contract's receive function. It's implemented correctly to disallow calldata, which is a requirement for the receive function in Solidity.


593-616: The FilterERC20BridgeFinalized function is well-implemented to filter logs based on indexed parameters. It's important to ensure that the event signature matches the actual event in the Solidity contract.

indexer/bindings/crossdomainmessenger.go (5)

31-34: Check the ABI string for correctness.

The ABI string in CrossDomainMessengerMetaData is crucial for the correct interaction with the smart contract. Verify that this ABI string accurately reflects the current version of the smart contract it is meant to interact with.


136-142: Validate ABI parsing.

In the function bindCrossDomainMessenger, the ABI string is parsed. Verify that the ABI string is correct and that the parsing handles all expected cases without errors.


182-196: Check consistency of contract method bindings.

For each method binding like MESSAGEVERSION, ensure that the method name and expected return types match the actual smart contract. This is crucial for ensuring that the bindings work correctly at runtime.


720-808: Ensure event filters are set up correctly.

For event filtering functions like FilterFailedRelayedMessage, ensure that the event name and filter parameters are correctly configured. This is important for the correct operation of event listening in the application.


937-947: Check log filtering setup.

In FilterInitialized, ensure that the logs are filtered based on the correct event signatures and that the iterator setup is correct. This is crucial for applications that depend on event logs for processing.

indexer/bindings/l2outputoracle.go (1)

211-225: Verify the existence of the 'CHALLENGER' method in the ABI.

The verification process did not find the 'CHALLENGER' method in the ABI files of the smart contract. This suggests that the method binding in the L2OutputOracleCaller may not correspond to any method in the smart contract, which could lead to runtime errors when interacting with the contract. It is recommended to review the smart contract and update the Go code accordingly.

  • Check and update the ABI files to ensure they include the 'CHALLENGER' method if it is supposed to exist.
  • Review the L2OutputOracleCaller Go code to ensure it correctly binds to existing smart contract methods.
indexer/bindings/l2crossdomainmessenger.go (4)

6-17: Imports are correctly organized and necessary for Ethereum contract interactions.


45-60: Deployment function correctly handles errors and returns comprehensive contract deployment details.


62-82: Type definitions are well-structured and correctly separate concerns between calling, transacting, and event filtering.


166-202: Method implementations for contract interactions are correctly handling errors and follow best practices for smart contract interactions.

indexer/bindings/l1crossdomainmessenger.go (4)

47-59: Ensure error handling is robust in contract deployment.

Verify that all potential error paths are adequately handled and that any errors provide clear, actionable information. This is crucial in deployment functions to prevent silent failures.


207-218: Check consistency in error handling.

Ensure that the error handling strategy is consistent across all contract method bindings. This function returns a new instance of the type on error, which might not be consistent with other methods.


824-843: Review transactional function for potential reentrancy issues.

Given that this function is a transactional mutator, review it carefully for reentrancy vulnerabilities, especially since it interacts with external contracts.


960-975: Ensure event filters are correctly set up.

Double-check that the event filtering setup correctly captures all intended events and that there are no typos in event names or parameters that could cause events to be missed.


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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 as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

@tynes tynes marked this pull request as draft April 19, 2024 03:04
@tynes
Copy link
Contributor Author

tynes commented Apr 19, 2024

need to fix ci

@hamdiallam
Copy link
Contributor

LGTM

tynes added 2 commits April 22, 2024 22:54
Removes the dependency on the `op-bindings/bindings` package.
This is to improve devex in the monorepo and reduce CI time
as maintaining the bindings autogenerated in each PR just
doesn't scale. Now each service is responsible for their own
bindings. In the future, we can work towards releases of
the bindings when contracts are released.

Follows:
- #10213
- #10218
- #10221
@tynes tynes force-pushed the feat/indexer-bindings branch from a1029c2 to a40d1c7 Compare April 23, 2024 04:55
@tynes tynes marked this pull request as ready for review April 23, 2024 04:56
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Out of diff range and nitpick comments (11)
indexer/bindings/crossdomainmessenger.go (4)

1-2: Ensure generated code comments are accurate.

The file header comments state that this is a generated file and should not be edited manually. Ensure that the generation process is documented and that any manual changes are avoided or properly managed through the generation scripts.


37-38: Consider removing deprecated code.

The variable CrossDomainMessengerABI is marked as deprecated. If it is no longer used in the project, consider removing it to clean up the codebase.


99-106: Ensure error handling is robust.

In the constructor function NewCrossDomainMessenger, ensure that the error returned is handled or logged appropriately in the calling code. This is crucial for debugging deployment issues.


678-697: Review transaction methods for correct parameters and error handling.

In transaction methods like RelayMessage, ensure that the parameters passed and the transaction options are correctly handled. Also, check that errors from the transaction are handled or propagated appropriately.

indexer/bindings/l2outputoracle.go (1)

44-50: Deprecation of L2OutputOracleABI and L2OutputOracleBin should be clearly documented.

Consider adding detailed comments explaining why these variables are deprecated and what should be used instead. This will help developers understand the changes and transition smoothly.

indexer/bindings/l2crossdomainmessenger.go (2)

1-3: Ensure generated code is not manually edited to maintain integrity.


38-43: Consider removing deprecated variables if they are no longer used in the codebase to clean up the code.

indexer/bindings/l1crossdomainmessenger.go (4)

38-38: Deprecation notice should be more prominent.

Consider using a more noticeable method, such as logging a warning when deprecated variables are accessed, to ensure that developers are aware of the deprecation when using these variables.


159-164: Potential improvement in error handling.

Consider adding more specific error messages related to ABI parsing failures to aid in debugging issues related to contract interactions.


866-885: Optimize gas usage in transactional functions.

Review the function to ensure it's optimized for gas usage, particularly since it involves multiple contract interactions which can be costly.


1232-1237: Document the event structure clearly.

Add detailed comments describing each field in the event structure to improve code readability and maintainability, especially for complex structures.

@tynes
Copy link
Contributor Author

tynes commented Apr 23, 2024

@hamdiallam this is ready for review now

@tynes tynes enabled auto-merge April 23, 2024 05:13
@tynes tynes added this pull request to the merge queue Apr 23, 2024
Merged via the queue into develop with commit 08f3dbe Apr 23, 2024
72 checks passed
@tynes tynes deleted the feat/indexer-bindings branch April 23, 2024 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants