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

fix: L2L1 config for tracing and counting RPC #1909

Conversation

letypequividelespoubelles
Copy link
Collaborator

No description provided.

@letypequividelespoubelles letypequividelespoubelles linked an issue Mar 14, 2025 that may be closed by this pull request

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request addresses a configuration issue in the GenerateLineCountsV2 service by passing the required L1L2 bridge configuration instead of using a hardcoded default. Key changes include:

  • Updating the constructor of GenerateLineCountsV2 to accept a LineaL1L2BridgeSharedConfiguration.
  • Modifying LineCountsEndpointServicePlugin to retrieve and forward the shared configuration.
  • Marking related CLI options as required in LineaL1L2BridgeSharedCliOptions to enforce proper configuration input.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
plugins/src/main/java/net/consensys/linea/plugins/rpc/linecounts/LineCountsEndpointServicePlugin.java Modified to retrieve and pass the shared configuration to GenerateLineCountsV2.
arithmetization/src/main/java/net/consensys/linea/plugins/config/LineaL1L2BridgeSharedCliOptions.java Updated CLI options to be required for both contract and topic parameters.
arithmetization/src/main/java/net/consensys/linea/plugins/rpc/linecounts/GenerateLineCountsV2.java Updated the constructor and field initialization to handle the injected shared configuration.
Comments suppressed due to low confidence (1)

arithmetization/src/main/java/net/consensys/linea/plugins/config/LineaL1L2BridgeSharedCliOptions.java:39

  • The option 'l1l2BridgeContract' is marked as required, yet it is assigned a default value of Address.ZERO. Consider removing the default value to enforce explicit configuration via the CLI.
private Address l1l2BridgeContract = Address.ZERO;
Copy link
Collaborator

@DavePearce DavePearce left a comment

Choose a reason for hiding this comment

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

LGTM. I guess it means whoever starts the plugins will need to provide the CLI arguments now? I guess whoever that is will figure it out!

@DavePearce
Copy link
Collaborator

Also you are sure not necessary for GenerateConflatedTracesV2? I would personally do that as well unless there is a good reason not to.

Choose a reason for hiding this comment

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

Pull Request Overview

This PR ensures that the L1L2 bridge configuration is required and properly injected into all line count and trace generation functionalities.

  • Enforces required CLI options for the L1L2 bridge configuration.
  • Updates instantiation of GenerateLineCountsV2, GenerateConflatedTracesV2, and ConflatedCountTracesV2 to use the injected L1L2 bridge configuration.
  • Adjusts the corresponding service plugins to pass the new configuration parameter.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
arithmetization/src/main/java/net/consensys/linea/plugins/config/LineaL1L2BridgeSharedCliOptions.java Marked L1L2 bridge CLI options as required.
plugins/src/main/java/net/consensys/linea/plugins/rpc/linecounts/LineCountsEndpointServicePlugin.java Updated GenerateLineCountsV2 instantiation to include L1L2 configuration retrieval.
arithmetization/src/main/java/net/consensys/linea/plugins/rpc/tracegeneration/GenerateConflatedTracesV2.java Injected L1L2 configuration in class constructor via @requiredargsconstructor.
arithmetization/src/main/java/net/consensys/linea/plugins/rpc/linecounts/GenerateLineCountsV2.java Removed legacy constructor and now using the injected L1L2 configuration for tracer instantiation.
arithmetization/src/main/java/net/consensys/linea/plugins/rpc/batchlinecount/ConflatedCountTracesV2.java Updated constructor to include the L1L2 configuration dependency.
plugins/src/main/java/net/consensys/linea/plugins/rpc/tracegeneration/TracesEndpointServicePlugin.java Adjusted method instantiation by passing the L1L2 configuration.
plugins/src/main/java/net/consensys/linea/plugins/rpc/batchlinecount/ConflatedLineCountsEndpointServicePlugin.java Updated service registration to include the L1L2 configuration.
@letypequividelespoubelles letypequividelespoubelles changed the title fix: generateLineCountsV2 needs L2L1 config fix: L2L1 config for tracing and counting RPC Mar 17, 2025
@amkCha amkCha self-requested a review March 17, 2025 15:29
@letypequividelespoubelles letypequividelespoubelles merged commit 3310b92 into arith-dev Mar 17, 2025
7 checks passed
@letypequividelespoubelles letypequividelespoubelles deleted the 1908-fix-l2l1-config-for-generatelinecountsv2 branch March 17, 2025 16:03
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.

fix L2L1 config for GenerateLineCountsV2
3 participants