Skip to content

Comments

BindGen - local contracts refactor#8279

Merged
mslipper merged 1 commit intodevelopfrom
wyatt/bindgen/local-contract-refactor
Dec 13, 2023
Merged

BindGen - local contracts refactor#8279
mslipper merged 1 commit intodevelopfrom
wyatt/bindgen/local-contract-refactor

Conversation

@Spacesai1or
Copy link
Contributor

@Spacesai1or Spacesai1or commented Nov 27, 2023

This PR is refactoring the existing bindings generation code to better fit with an extension of the code to allow for bindings created for remotely sourced contracts (#8281). For the most part, the original code isn't modified other than breaking out code into smaller functions

@Spacesai1or
Copy link
Contributor Author

Spacesai1or commented Nov 27, 2023

@Spacesai1or Spacesai1or force-pushed the wyatt/bindgen/local-contract-refactor branch 2 times, most recently from 9f68969 to b9aed79 Compare November 27, 2023 20:31
@Spacesai1or
Copy link
Contributor Author

Currently blocked by #8269

@Spacesai1or Spacesai1or marked this pull request as draft November 27, 2023 21:32
@Spacesai1or Spacesai1or force-pushed the wyatt/bindgen/local-contract-refactor branch from b9aed79 to a36df5d Compare November 27, 2023 22:28
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 27, 2023

Walkthrough

Walkthrough

The changes involve a significant update to the Go package responsible for generating Ethereum smart contract bindings. A new bindgen tool has been introduced, replacing the previous generation command. This tool comes with new CLI flags and altered functions for error handling and logging. The Makefile has been updated to accommodate these changes. Additionally, the visibility of certain functions has been modified to be public, as indicated by the capitalization of their names.

Changes

File Path Change Summary
op-bindings/Makefile Updated build process to use new bindgen tool with additional parameters for binding generation.
op-bindings/bindgen/generator_local.go Added new types and functions for generating bindings and metadata for local contracts, with error handling and logging.
op-bindings/bindgen/main.go Introduced new main package with types and functions for setting up binding generation, including CLI flags and configuration parsing.
op-bindings/bindgen/utils.go New package with functions for reading contracts, handling artifacts, and generating Go bindings using abigen.
indexer/config/devnet.go
op-e2e/config/init.go
Renamed findMonorepoRoot to FindMonorepoRoot to make the function public.

Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

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.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • 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/coderabbit-overrides.v2.json

@Spacesai1or Spacesai1or marked this pull request as ready for review November 28, 2023 09:34
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.

Review Status

Actionable comments generated: 4

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between ec6a2bd and a36df5d.
Files ignored due to filter (1)
  • op-bindings/artifacts.json
Files selected for processing (4)
  • op-bindings/Makefile (2 hunks)
  • op-bindings/bindgen/generator_local.go (1 hunks)
  • op-bindings/bindgen/main.go (1 hunks)
  • op-bindings/bindgen/utils.go (1 hunks)
Additional comments: 5
op-bindings/Makefile (4)
  • 9-9: The all target correctly depends on bindgen-local as per the changes described.

  • 26-36: The bindgen-generate-local rule correctly invokes the Go script with the necessary parameters.

  • 24-24: The bindgen-local target correctly depends on compile and bindgen-generate-local.

  • 38-39: The mkdir target is correctly defined to ensure the necessary directory exists.

op-bindings/bindgen/utils.go (1)
  • 110-111: Ensure that the abigen tool is installed and available in the system's PATH as the genContractBindings function relies on it.

@Spacesai1or Spacesai1or marked this pull request as draft November 28, 2023 09:50
@Spacesai1or Spacesai1or marked this pull request as ready for review November 28, 2023 21:11
Copy link
Contributor

@hamdiallam hamdiallam left a comment

Choose a reason for hiding this comment

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

Looks really great! Left a few comments

@Spacesai1or Spacesai1or marked this pull request as draft December 1, 2023 19:29
@Spacesai1or Spacesai1or force-pushed the wyatt/bindgen/local-contract-refactor branch 4 times, most recently from 4c3a3a8 to c2bac37 Compare December 4, 2023 06:18
@Spacesai1or Spacesai1or marked this pull request as ready for review December 4, 2023 18:36
@Spacesai1or Spacesai1or requested review from a team as code owners December 4, 2023 18:36
@Spacesai1or Spacesai1or force-pushed the wyatt/bindgen/local-contract-refactor branch from c2bac37 to 08e676f Compare December 7, 2023 22:51
@Spacesai1or Spacesai1or force-pushed the wyatt/bindgen/local-contract-refactor branch from 08e676f to e4536f9 Compare December 8, 2023 03:25
Copy link
Contributor

@hamdiallam hamdiallam left a comment

Choose a reason for hiding this comment

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

Looks good after another pass, great code!

Mainly reviewed by just looking at the logic & not actually running it locally to verify -- i'm sure you've verified expected behavior locally

Copy link
Collaborator

@mslipper mslipper left a comment

Choose a reason for hiding this comment

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

I've verified that these changes are drop-in replacements for the existing bindings functionality, and generate the same output as the original bindings script. LGTM.

@mslipper mslipper added this pull request to the merge queue Dec 13, 2023
Merged via the queue into develop with commit 9f16e05 Dec 13, 2023
@mslipper mslipper deleted the wyatt/bindgen/local-contract-refactor branch December 13, 2023 23:09
This was referenced Dec 17, 2023
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.

5 participants