-
Notifications
You must be signed in to change notification settings - Fork 2
build(deps): use tagged versions #180
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
Conversation
WalkthroughThis change updates the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Adapter
Caller->>Adapter: Call GetExecutionMode()
Note right of Adapter: Method removed, no longer available
Caller-->>Caller: Handle absence or use alternative logic
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 primarily focuses on updating the project's Go module dependencies. The most significant change is the migration of rollkit
modules from using pseudo-versions to a stable tagged release, v1.0.0-beta.1
. Additionally, it includes a comprehensive refresh of various other indirect dependencies to their latest versions and a cleanup of unused dependencies, aiming to improve overall dependency clarity and maintainability.
Highlights
- Core Dependency Update: The primary change involves updating the
github.com/rollkit/rollkit
module and its sub-modules (core
,da
,sequencers/single
) from commit-hash-based pseudo-versions (e.g.,v0.14.2-0.20250630141726-fa03f78d8121
) to a stable, taggedv1.0.0-beta.1
version. This aligns with the PR's intent to use tagged versions for better dependency management. - General Dependency Refresh: Numerous other indirect Go module dependencies have been updated to their latest versions. This includes updates to
github.com/fsnotify/fsnotify
,github.com/ipfs/boxo
, severalgithub.com/libp2p
components (go-libp2p-kad-dht
,go-libp2p-kbucket
,go-libp2p-routing-helpers
),github.com/prometheus
components (client_model
,procfs
),github.com/spf13
libraries (afero
,viper
),gonum.org/v1/gonum
,google.golang.org/genproto
, andlukechampine.com/blake3
. - Dependency Cleanup: Several indirect dependencies that are no longer explicitly required or have been subsumed by other updates have been removed from
go.mod
. Examples includegithub.com/hashicorp/errwrap
,github.com/ipfs/go-log
,github.com/opentracing/opentracing-go
,github.com/sagikazarmark/slog-shim
, andgopkg.in/ini.v1
. - New Indirect Dependency: A new indirect dependency,
github.com/go-viper/mapstructure/v2 v2.3.0
, has been introduced.
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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
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 issue 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 is currently in preview and 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 to provide feedback.
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
-
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. ↩
There was a problem hiding this 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 updates several dependencies to their tagged versions, most notably rollkit
to v1.0.0-beta.1
. While using tagged versions is a good practice for dependency management, the major version bump for rollkit
from a pre-v1.0 version to a v1.0 beta release is a significant change that could introduce breaking API changes and potential instability. I've left a comment regarding this potential issue. Please ensure that the new versions are compatible or that necessary code adaptations are made.
There was a problem hiding this 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
🧹 Nitpick comments (2)
go.mod (2)
34-37
: Drop the extra Rollkit sub-module pins unless they are strictly required
github.com/rollkit/rollkit
already transitively exposes thecore
,da
, andsequencers/single
modules. Pinning them explicitly clutters the file and risks accidental version skew if a future tag is bumped only in one place.- github.com/rollkit/rollkit v1.0.0-beta.1 - github.com/rollkit/rollkit/core v1.0.0-beta.1 - github.com/rollkit/rollkit/da v1.0.0-beta.1 - github.com/rollkit/rollkit/sequencers/single v1.0.0-beta.1 + github.com/rollkit/rollkit v1.0.0-beta.1Run
go mod tidy
afterwards; if the three sub-modules come back, it means they are required explicitly somewhere in the codebase and should stay.
280-280
:locafero
0.4 → 0.7 is a semver-major – check any direct useThe API surface changed (Context-aware I/O). Grep the repo to be safe.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sum
is excluded by!**/*.sum
📒 Files selected for processing (1)
go.mod
(9 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: julienrbrt
PR: rollkit/go-execution-abci#81
File: .github/workflows/integration_test.yml:28-30
Timestamp: 2025-05-07T12:20:24.290Z
Learning: For the rollkit/go-execution-abci repository, the maintainer prefers using the latest version of Ignite CLI rather than pinning to a specific version during the current development phase. Version tags will be added later when Rollkit has a tag.
go.mod (1)
Learnt from: julienrbrt
PR: rollkit/go-execution-abci#81
File: .github/workflows/integration_test.yml:28-30
Timestamp: 2025-05-07T12:20:24.290Z
Learning: For the rollkit/go-execution-abci repository, the maintainer prefers using the latest version of Ignite CLI rather than pinning to a specific version during the current development phase. Version tags will be added later when Rollkit has a tag.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test / Run Unit Tests
- GitHub Check: lint / golangci-lint
- GitHub Check: Test with Rollkit Chain
- GitHub Check: Test Migration from Cosmos SDK to Rollkit
🔇 Additional comments (9)
go.mod (9)
128-128
:fsnotify
1.7 → 1.8 upgrade looks safeNo breaking API changes were introduced in the v1.8.0 release.
174-174
:boxo
minor upgrade LGTMNo action required.
205-205
:miekg/dns
patch bump LGTM
244-244
: go-toml/v2 patch bump LGTM
269-271
: Prometheus client & procfs bumps – verify custom metrics still registerThe client model now rejects some non-UTF8 label names; make sure nothing breaks at startup.
287-290
: Afero & Viper were upgraded together – compile once before mergingViper ≥1.20 expects the new Afero features; this pairing should build, but run
go vet ./...
to catch any mismatch.
331-332
: Genproto jump to a November-2024 snapshot – ensure it matches gRPC 1.73This is usually fine, yet occasionally introduces renamed proto options.
337-337
: Blake3 patch bump LGTM
137-137
: Confirm double mapstructure dependenciesYour go.mod now lists both
- github.com/go-viper/mapstructure/v2 v2.3.0 // indirect
- github.com/mitchellh/mapstructure v1.5.0 // indirect
No direct imports or usages of mapstructure were found in the codebase, so these are coming in via transitive dependencies (v2 from go-viper, v1 from another module). To determine if you can safely drop the v1 entry, run:
go mod why -m github.com/mitchellh/mapstructureOnce you’ve identified which dependency requires v1, you can update or remove it—and then drop the v1 line from go.mod if it’s truly unused.
There was a problem hiding this 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/integration_test.yml
(5 hunks).github/workflows/migration_test.yml
(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: julienrbrt
PR: rollkit/go-execution-abci#81
File: .github/workflows/integration_test.yml:28-30
Timestamp: 2025-05-07T12:20:24.290Z
Learning: For the rollkit/go-execution-abci repository, the maintainer prefers using the latest version of Ignite CLI rather than pinning to a specific version during the current development phase. Version tags will be added later when Rollkit has a tag.
Learnt from: julienrbrt
PR: rollkit/go-execution-abci#114
File: modules/rollkitmngr/keeper/migration.go:13-14
Timestamp: 2025-05-30T09:40:24.076Z
Learning: In modules/rollkitmngr/keeper/migration.go, the IBCSmoothingFactor variable is intentionally mutable (not a constant) to allow different chains to modify the smoothing period for IBC migration based on their specific requirements.
.github/workflows/migration_test.yml (4)
Learnt from: julienrbrt
PR: rollkit/go-execution-abci#81
File: .github/workflows/integration_test.yml:28-30
Timestamp: 2025-05-07T12:20:24.290Z
Learning: For the rollkit/go-execution-abci repository, the maintainer prefers using the latest version of Ignite CLI rather than pinning to a specific version during the current development phase. Version tags will be added later when Rollkit has a tag.
Learnt from: julienrbrt
PR: rollkit/go-execution-abci#114
File: modules/rollkitmngr/keeper/migration.go:13-14
Timestamp: 2025-05-30T09:40:24.076Z
Learning: In modules/rollkitmngr/keeper/migration.go, the IBCSmoothingFactor variable is intentionally mutable (not a constant) to allow different chains to modify the smoothing period for IBC migration based on their specific requirements.
Learnt from: julienrbrt
PR: rollkit/go-execution-abci#114
File: modules/rollkitmngr/keeper/migration.go:18-46
Timestamp: 2025-05-30T09:43:01.102Z
Learning: In the rollkitmngr module migration functions, input validation for migration data parameters (like checking if Sequencer.ConsensusPubkey is nil) is not needed according to the project maintainer's preference.
Learnt from: julienrbrt
PR: rollkit/go-execution-abci#113
File: server/migration_cmd.go:231-231
Timestamp: 2025-06-20T13:20:04.030Z
Learning: In the rollkit migration command (server/migration_cmd.go), InitialHeight is correctly set to LastBlockHeight because the Rollkit chain is technically a new chain that starts where the CometBFT chain ended, ensuring continuity during migration.
.github/workflows/integration_test.yml (3)
Learnt from: julienrbrt
PR: rollkit/go-execution-abci#81
File: .github/workflows/integration_test.yml:28-30
Timestamp: 2025-05-07T12:20:24.290Z
Learning: For the rollkit/go-execution-abci repository, the maintainer prefers using the latest version of Ignite CLI rather than pinning to a specific version during the current development phase. Version tags will be added later when Rollkit has a tag.
Learnt from: julienrbrt
PR: rollkit/go-execution-abci#114
File: modules/rollkitmngr/keeper/migration.go:13-14
Timestamp: 2025-05-30T09:40:24.076Z
Learning: In modules/rollkitmngr/keeper/migration.go, the IBCSmoothingFactor variable is intentionally mutable (not a constant) to allow different chains to modify the smoothing period for IBC migration based on their specific requirements.
Learnt from: julienrbrt
PR: rollkit/go-execution-abci#113
File: server/migration_cmd.go:231-231
Timestamp: 2025-06-20T13:20:04.030Z
Learning: In the rollkit migration command (server/migration_cmd.go), InitialHeight is correctly set to LastBlockHeight because the Rollkit chain is technically a new chain that starts where the CometBFT chain ended, ensuring continuity during migration.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test / Run Unit Tests
- GitHub Check: lint / golangci-lint
- GitHub Check: Test with Rollkit Chain
- GitHub Check: Test Migration from Cosmos SDK to Rollkit
🔇 Additional comments (3)
.github/workflows/integration_test.yml (2)
74-76
: Good switch togo mod edit …@$ROLLKIT_VERSION
Pinning the module via
go mod edit -replace
cleanly avoids the brittlego get $(jq …)
dance. 👍
362-383
: Retry loop is clear and boundedThe rewritten wait-loop (30 attempts × 3 s) is readable, bounded, and correctly flips
IBC_FOUND
to break the loop – looks good..github/workflows/migration_test.yml (1)
180-182
: Module pinning change LGTMConsistent with the rest of the repository and the move to tagged releases.
Overview
Summary by CodeRabbit