Start: setup multiple attesters #177
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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 (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @alpe, 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 lays the groundwork for supporting multiple attesters within the system by introducing a new attester simulation tool and a robust local testing framework. It also includes necessary dependency updates and modifications to the core logic to accommodate a multi-validator setup.
Highlights
- Attester Tool Introduction: I've introduced a new
attesterCLI tool (hack/attester/main.go) designed to simulate multiple attesters on a Rollkit chain. This tool can join the attester set and submit 'fake' attestations (precommits) at the end of each epoch by polling for new blocks and signing votes using a provided mnemonic and private validator key. - Local Test Environment Setup: A comprehensive suite of new
hackscripts has been added to streamline the setup of a local development and testing environment. This includes scripts for downloading necessary binaries (download.sh), initializing and running a local Gaia chain (init-gaia.sh), setting up and starting the Rollkit example app (run_gmd.sh), and configuring IBC relayers (both Hermes viaibc-connection-hermes.shand Cosmos Relayer viaibc-connection-rly.sh). Anics20-token-transfer.shscript is also included to test cross-chain token transfers. - Multi-Attester Support: The core logic in
pkg/adapter/adapter.gohas been modified to remove the previous restriction that enforced a single validator. This change, along with the new attester tool, enables the system to support and test scenarios with multiple attesters/validators. - Dependency Updates: I've updated the
go.modandgo.sumfiles to bringgithub.com/rollkit/rollkitand its submodules to a newer commit, along with numerous other indirect Go module dependencies. Some outdated indirect dependencies have also been removed as a result of these updates.
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.
Code Review
The pull request includes changes to set up multiple attesters. It updates dependencies in go.mod and go.sum, adds new files for an attester client (hack/attester/), and modifies the adapter and server start logic. The most significant changes involve setting up the attester client and adjusting the validator set configuration. The review identifies a critical issue with the addition of a panic statement and medium severity issues with commented out code and debugging statements.
| if len(cmtGenDoc.Validators) == 0 { | ||
| panic("no .validators in genesis") |
There was a problem hiding this comment.
Adding a panic if there are no validators in the genesis doc is a critical change. While it prevents the system from starting with an invalid configuration, panics should generally be avoided in production code. A more graceful error handling mechanism should be implemented, such as returning an error that can be handled by the calling function.
| if len(cmtGenDoc.Validators) == 0 { | |
| panic("no .validators in genesis") | |
| if len(cmtGenDoc.Validators) == 0 { | |
| return nil | |
| } |
| //if n := len(nValSet.Validators); n != 1 { | ||
| // err := fmt.Errorf("expected exactly one validator but got %d", n) | ||
| // return nil, 0, err | ||
| //} |
There was a problem hiding this comment.
This check for the number of validators being exactly one is commented out. While it might be acceptable for testing, it's crucial to ensure that the validator set is correctly configured in a production environment. Leaving this check disabled could lead to unexpected behavior if the validator set deviates from the expected size. This check should be re-enabled or a more robust validation strategy should be implemented.
| //if n := len(nValSet.Validators); n != 1 { | |
| // err := fmt.Errorf("expected exactly one validator but got %d", n) | |
| // return nil, 0, err | |
| //} | |
| //if n := len(nValSet.Validators); n != 1 { | |
| // err := fmt.Errorf("expected exactly one validator but got %d", n) | |
| // return nil, 0, err | |
| //} |
| if err != nil { | ||
| return "", fmt.Errorf("getting account: %w", err) | ||
| } | ||
| fmt.Printf("+++ chainid: %s, GetAccountNumber: %d\n", chainID, account.GetAccountNumber()) |
There was a problem hiding this comment.
This Printf statement is likely for debugging purposes and should be removed before merging to avoid unnecessary output in production. Logging should be handled using a proper logging library.
No description provided.