feat: Allow fetching rollkitmngr state in rollkit-migrate command for attester sets#175
feat: Allow fetching rollkitmngr state in rollkit-migrate command for attester sets#175julienrbrt with Copilot wants to merge 9 commits into
rollkit-migrate command for attester sets#175Conversation
Co-authored-by: julienrbrt <29894366+julienrbrt@users.noreply.github.com>
… and validation Co-authored-by: julienrbrt <29894366+julienrbrt@users.noreply.github.com>
Co-authored-by: julienrbrt <29894366+julienrbrt@users.noreply.github.com>
rollkit-migrate commandrollkit-migrate command for attester sets
| // Set up encoding config | ||
| encCfg := testutil.MakeTestEncodingConfig(rollkitmngr.AppModuleBasic{}) | ||
|
|
||
| // Try multiple key formats to find the sequencer data |
There was a problem hiding this comment.
this is far too hacky. recreate the collections and use the store.
then use that collections to query the state.
the collections definition is defined in the keeper.go
There was a problem hiding this comment.
Replaced the hacky multiple key format attempts with proper collections framework usage. The implementation now recreates the collections schema as defined in keeper.go and uses sequencerCollection.Get(ctx) to query the state properly. Commit c0a9468
julienrbrt
changed the title
rollkit-migrate command for attester setsrollkit-migrate command for attester sets
…ollkitmngr state querying Co-authored-by: julienrbrt <29894366+julienrbrt@users.noreply.github.com>
| return nil, fmt.Errorf("sequencer not found in rollkitmngr state") | ||
| } | ||
|
|
||
| // Store it in the context's store so collections can find it |
There was a problem hiding this comment.
consider the store is read only. DO NOT CHANGE THE APP STORE. you should solely get info from it.
There was a problem hiding this comment.
Fixed. Removed the store modification and now only read from the database directly using the collections key format. The store remains read-only throughout the process. Commit 4aa5521
…itmngr state Co-authored-by: julienrbrt <29894366+julienrbrt@users.noreply.github.com>
| sequencerAddr = cometBFTState.LastValidators.Validators[0].Address.Bytes() | ||
| sequencerPubKey = cometBFTState.LastValidators.Validators[0].PubKey | ||
| // If the cometbft validators are > 0, fetch the rollkitmngr state to get the sequencer | ||
| if len(cometBFTState.LastValidators.Validators) > 0 { |
There was a problem hiding this comment.
can we error joins those errors? as it will be lost otherwise.
There was a problem hiding this comment.
Fixed. Now using errors.Join to properly combine the validator count error with the rollkitmngr state error, preserving the error chain for better debugging. Commit 229d533
There was a problem hiding this comment.
this isn't what i mean. Error from L378 is discarded, we should still return it if we end up with errors later.
There was a problem hiding this comment.
Fixed. Now preserving the rollkitmngr state error in a variable and including it in the error chain only when subsequent operations fail, rather than discarding it entirely. Commit 89be76c
Co-authored-by: julienrbrt <29894366+julienrbrt@users.noreply.github.com>
Co-authored-by: julienrbrt <29894366+julienrbrt@users.noreply.github.com>
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Join our Discord community 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 (
|
|
Done in #190 instead |
The
rollkit-migratecommand previously assumed single validator setups and used the first validator as the sequencer. This failed for chains using attester sets where the first validator may not be the actual sequencer.Problem
When using an attester set (multiple validators), the migration command would fail with:
This occurred because the command couldn't determine which validator was the actual sequencer from the CometBFT validator set alone.
Solution
Enhanced the migration command to fetch sequencer information from the rollkitmngr module state when multiple validators are present:
Key Changes
createRollkitMigrationGenesis(): Added conditional logic to query rollkitmngr state for multiple validatorsgetSequencerFromRollkitMngrState(): Handles database access and sequencer extraction with multiple key format strategiesExample Usage
The implementation maintains full backward compatibility while enabling migration for chains using attester sets.
Fixes #164.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.