Skip to content

Conversation

@julienrbrt
Copy link
Member

Overview

Partial revert of #2839 to keep the db dir the same.

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @julienrbrt, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses an inconsistency introduced by a prior change (PR #2839) regarding default database directory naming. By reverting specific database name suffixes in the NewDefaultKVStore calls across EVM and gRPC components, it ensures that the database paths maintain a consistent *-single suffix, thereby preventing unexpected directory structures and maintaining system stability.

Highlights

  • Database Naming Revert: The pull request partially reverts a previous change (PR chore!: rename evm-single and grpc-single #2839) to restore the original naming convention for default database directories, ensuring consistency across the application.
  • EVM Command Database Name: The database name used when creating the Key-Value store for EVM-related commands (rollback and run) has been changed from 'evm' back to 'evm-single'.
  • gRPC Command Database Name: Similarly, the database name for the gRPC command's Key-Value store has been reverted from 'evgrpc' to 'grpc-single'.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 1, 2025

The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedDec 1, 2025, 8:22 PM

@claude
Copy link
Contributor

claude bot commented Dec 1, 2025

Claude finished @julienrbrt's task —— View job


Code Review Complete ✓

Todo List

  • Read changed files and understand modifications
  • Examine git diff for exact changes
  • Perform comprehensive code review
  • Post review feedback

Summary

This PR successfully addresses a database naming inconsistency introduced in PR #2839 by reverting database names to their original "-single" suffix convention. The latest commit also implements Gemini Code Assist's suggestion to use constants instead of magic strings, which is a solid improvement.

Review Findings

Code Quality & Architecture

Strengths:

  • The use of constants (evmDbName, grpcDbName, testDbName) eliminates magic strings and improves maintainability
  • Consistent pattern applied across all three apps (evm, grpc, testapp)
  • Constants are properly scoped at package level
  • Follows Go naming conventions

Observations:

  • The EVM app now has a shared constant evmDbName used in both run.go (line 31) and rollback.go (line 41), which is the correct approach
  • The gRPC app defines grpcDbName in the existing const block alongside FlagGrpcExecutorURL, maintaining good organization
  • The testapp defines testDbName at package level, consistent with the pattern

Error Handling & Resilience

No changes to error handling logic. Existing patterns are maintained:

  • Database creation errors properly propagated
  • Deferred close with error logging in rollback commands
  • Context usage is appropriate

Security

No security concerns introduced:

  • Database names are non-sensitive configuration values
  • No credential exposure or input validation issues
  • No changes to authentication/authorization logic

Performance & Resource Efficiency

No performance impact:

  • Changes are purely cosmetic (string literals → constants)
  • No runtime overhead introduced
  • Database initialization logic unchanged

Testing & Reliability

Observations:

  • CI checks are passing (buf-check, codecov)
  • Code coverage remains stable (64.57%, -0.07% is negligible)
  • No test changes required as functionality is identical

Recommendation:
While not strictly necessary for this PR, consider adding integration tests that verify the database directory naming if they don't already exist. This would prevent future regressions of this issue.

Documentation & Developer Experience

Strengths:

Minor Suggestion:
Consider adding a brief comment above each constant explaining why the "-single" suffix is used, especially for future developers unfamiliar with the context:

// evmDbName is the database name for the single-sequencer EVM application.
// The "-single" suffix maintains consistency with the original naming convention.
const evmDbName = "evm-single"

However, this is optional and not critical given the straightforward nature of the code.


Overall Assessment

Verdict: ✅ LGTM (Looks Good To Me)

This is a well-executed fix that:

  1. Correctly reverts the database naming to maintain consistency
  2. Improves code quality by extracting constants (addressing Gemini's feedback)
  3. Maintains all existing functionality without introducing risks
  4. Follows idiomatic Go patterns

The changes are minimal, focused, and address the stated problem effectively. The author's "ack" responses to Gemini's suggestions show good collaboration, and the implementation follows through properly.

No blocking issues identified. The PR is ready to merge.


Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request reverts the default database directory names for the EVM and gRPC applications. The changes are simple and align with the goal of the PR. I've added a couple of suggestions to replace the hardcoded database names with constants. This will improve code maintainability and readability.

@codecov
Copy link

codecov bot commented Dec 1, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.59%. Comparing base (b5f5640) to head (698adaf).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2897      +/-   ##
==========================================
- Coverage   64.64%   64.59%   -0.06%     
==========================================
  Files          81       81              
  Lines        7374     7374              
==========================================
- Hits         4767     4763       -4     
- Misses       2065     2069       +4     
  Partials      542      542              
Flag Coverage Δ
combined 64.59% <ø> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

thank you!

@tac0turtle tac0turtle merged commit 2b5a1df into main Dec 2, 2025
31 of 32 checks passed
@tac0turtle tac0turtle deleted the julien/db branch December 2, 2025 09:49
alpe added a commit that referenced this pull request Dec 3, 2025
* main:
  build(deps): Bump mdast-util-to-hast from 13.2.0 to 13.2.1 in /docs in the npm_and_yarn group across 1 directory (#2900)
  refactor(block): centralize timeout in client (#2903)
  build(deps): Bump the all-go group across 2 directories with 3 updates (#2898)
  chore: bump default timeout (#2902)
  fix: revert default db (#2897)
  refactor: remove obsolete // +build tag (#2899)
  fix:da visualiser namespace  (#2895)
  refactor: omit unnecessary reassignment (#2892)
  build(deps): Bump the all-go group across 5 directories with 6 updates (#2881)
  chore: fix inconsistent method name in retryWithBackoffOnPayloadStatus comment (#2889)
  fix: ensure consistent network ID usage in P2P subscriber (#2884)
  build(deps): Bump golangci/golangci-lint-action from 9.0.0 to 9.1.0 (#2885)
  build(deps): Bump actions/checkout from 5 to 6 (#2886)
alpe added a commit that referenced this pull request Dec 8, 2025
* main:
  refactor(sequencers): persist prepended batch (#2907)
  feat(evm): add force inclusion command (#2888)
  feat: DA client, remove interface part 1: copy subset of types needed for the client using blob rpc. (#2905)
  feat: forced inclusion (#2797)
  fix: fix and cleanup metrics (sequencers + block) (#2904)
  build(deps): Bump mdast-util-to-hast from 13.2.0 to 13.2.1 in /docs in the npm_and_yarn group across 1 directory (#2900)
  refactor(block): centralize timeout in client (#2903)
  build(deps): Bump the all-go group across 2 directories with 3 updates (#2898)
  chore: bump default timeout (#2902)
  fix: revert default db (#2897)
  refactor: remove obsolete // +build tag (#2899)
  fix:da visualiser namespace  (#2895)
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.

3 participants