plasma mode: Specify valid commitment type #10622
Conversation
trianglesphere
changed the title
trianglesphere
force-pushed
the
jg/specify_comm_type
branch
from
8f97679
to
16010a3
Compare
trianglesphere
requested review from
a team,
protolambda and
ajsutton
as code owners
WalkthroughWalkthroughThe changes involve introducing support for generic Plasma allocations and commitments across various components. This includes updating configuration files, environment variables, and deployment scripts to accommodate a new Changes
Assessment against linked issues
Tip New Features and ImprovementsReview SettingsIntroduced new personality profiles for code reviews. Users can now select between "Chill" and "Assertive" review tones to tailor feedback styles according to their preferences. The "Assertive" profile posts more comments and nitpicks the code more aggressively, while the "Chill" profile is more relaxed and posts fewer comments. AST-based InstructionsCodeRabbit offers customizing reviews based on the Abstract Syntax Tree (AST) pattern matching. Read more about AST-based instructions in the documentation. Community-driven AST-based RulesWe are kicking off a community-driven initiative to create and share AST-based rules. Users can now contribute their AST-based rules to detect security vulnerabilities, code smells, and anti-patterns. Please see the ast-grep-essentials repository for more information. New Static Analysis ToolsWe are continually expanding our support for static analysis tools. We have added support for Tone SettingsUsers can now customize CodeRabbit to review code in the style of their favorite characters or personalities. Here are some of our favorite examples:
Revamped Settings PageWe have redesigned the settings page for a more intuitive layout, enabling users to find and adjust settings quickly. This change was long overdue; it not only improves the user experience but also allows our development team to add more settings in the future with ease. Going forward, the changes to Miscellaneous
Recent Review DetailsConfiguration used: .coderabbit.yml Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 9
Outside diff range and nitpick comments (5)
bedrock-devnet/devnet/__init__.py (2)
Line range hint
224-224: Refactor the condition checks to use more Pythonic expressions.- if os.path.exists(l2_allocs_path) == False or DEVNET_FPAC == True: + if not os.path.exists(l2_allocs_path) or DEVNET_FPAC:
Line range hint
321-321: Remove or use the local variableeto handle exceptions appropriately.- except Exception as e: + except Exception: log.info(f'Waiting for RPC server at {url}')op-plasma/damgr.go (2)
54-55: Ensure theCommitmentTypefield is properly documented.It would be beneficial to add a comment explaining the purpose and possible values of
CommitmentTypefor future maintainability.
351-354: Clarify the handling of legacy and new plasma configurations.The logic that handles both legacy and new plasma configurations could be simplified or better documented to avoid confusion and potential bugs in future modifications.
op-node/rollup/types.go (1)
565-567: Document plasma mode support in the configuration description.It would be helpful to add a comment in the
Descriptionmethod to explain that it also reports the plasma mode configuration. This enhances the understandability of the configuration output.
There was a problem hiding this comment.
It would be really, really nice to not use magic 0 and 1 values, particularly in the json config files where there's no context about what they mean. Seems particularly important since accidentally setting it to use generic commitments is very dangerous.
If you really insist I'll approve but I won't like it. :)
|
Semgrep found 2 Variable |
trianglesphere
force-pushed
the
jg/specify_comm_type
branch
from
8b692b3
to
953128d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (5)
bedrock-devnet/devnet/__init__.py (5)
Line range hint
7-7: Remove unused import:calendar.- import calendarThe
calendarmodule is imported but not used in this file. Removing unused imports can help reduce clutter and improve code readability.
Line range hint
12-12: Remove unused import:gzip.- import gzipThe
gzipmodule is imported but not used in this file. It's a good practice to remove unused imports to keep the codebase clean and maintainable.
Line range hint
18-18: Remove unused import:devnet.log_setup.- import devnet.log_setupThe
devnet.log_setupis imported but not used in this file. Removing it will clean up the code and potentially reduce initialization overhead.
Line range hint
224-224: Simplify boolean checks.- if os.path.exists(l2_allocs_path) == False or DEVNET_FPAC == True: - # Also regenerate if FPAC. - # The FPAC flag may affect the L1 deployments addresses, which may affect the L2 genesis. - devnet_l2_allocs(paths) - else: - log.info('Re-using existing L2 allocs.') + if not os.path.exists(l2_allocs_path) or DEVNET_FPAC: + # Also regenerate if FPAC. + # The FPAC flag may affect the L1 deployments addresses, which may affect the L2 genesis. + devnet_l2_allocs(paths) + else: + log.info('Re-using existing L2 allocs.')Simplify the boolean expressions by directly using
notfor false checks and removing the comparison for true checks. This makes the code more Pythonic and easier to read.
Line range hint
321-321: Remove unused local variablee.- except Exception as e: + except Exception:The variable
eis declared but not used in the exception block. It's good practice to remove unused variables to clean up the code.
trianglesphere
force-pushed
the
jg/specify_comm_type
branch
from
953128d
to
7c2faae
Compare
![semgrep-app[bot]](https://avatars.githubusercontent.com/in/60555?s=60&v=4){kind=small}
trianglesphere
force-pushed
the
jg/specify_comm_type
branch
3 times, most recently
from
43ace75
to
3945d8f
Compare
trianglesphere
force-pushed
the
jg/specify_comm_type
branch
from
3945d8f
to
2d71169
Compare
|
@ajsutton everything got updated to use the string |
| // CommitmentType describes the binary format of the commitment. | ||
| // KeccakCommitmentType is the default commitment type for the centralized DA storage. | ||
| // KeccakCommitmentStringType is the default commitment type for the centralized DA storage. |
There was a problem hiding this comment.
Revert diff. Should be KeccakCommitmentType
* op-node: Better logging in commitment tracking * op-node: Properly handle differences in Keccak vs Generic Commitments * Use daCommitmentType in deploy config * da-server: Add option to use generic commitments * CI: Add generic commitment * op-chain-ops: Set daCommitmentType in testdata * Fix comm type check * Fix CI env var * Set proper default for daCommitmentType * Use string for daCommitmentType * Use bytes cast instead of abi.encodePacked
Description
This PR modifies the plasma mode config to support specifying the valid commitment type. This commitment type is then checked inside the op-node.
This also adds support for generic commitments to the DA server for testing. The generic commitment version of the DA server is used in a new devnet matrix test.
Tests
Adds a devnet test for generic commitment types.
Metadata