Skip to content

feat: Allow fetching rollkitmngr state in rollkit-migrate command for attester sets #175

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

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Jul 3, 2025

The rollkit-migrate command 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:

expected exactly one validator in the last validators, found N

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:

  • Smart Sequencer Detection: Attempts to query rollkitmngr state to identify the correct sequencer
  • Robust Database Access: Tries multiple key formats to maximize compatibility with various Cosmos SDK storage patterns
  • Backward Compatibility: Falls back to original single-validator logic when rollkitmngr state is unavailable
  • Validation: Ensures the sequencer from rollkitmngr state exists in the validator set
  • Clear Error Messages: Provides detailed error information for troubleshooting

Key Changes

  1. Enhanced createRollkitMigrationGenesis(): Added conditional logic to query rollkitmngr state for multiple validators
  2. New getSequencerFromRollkitMngrState(): Handles database access and sequencer extraction with multiple key format strategies
  3. Comprehensive Testing: Added test coverage for all scenarios including edge cases and error conditions

Example Usage

# Now works for chains with attester sets
rollkit-migrate

# Fallback behavior for single validator chains (unchanged)
rollkit-migrate

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.

@Copilot Copilot AI changed the title [WIP] Allow fetching rollkitmngr state in rollkit-migrate command Allow fetching rollkitmngr state in rollkit-migrate command for attester sets Jul 3, 2025
@Copilot Copilot AI requested a review from julienrbrt July 3, 2025 13:17
Copilot finished work on behalf of julienrbrt July 3, 2025 13:17
// Set up encoding config
encCfg := testutil.MakeTestEncodingConfig(rollkitmngr.AppModuleBasic{})

// Try multiple key formats to find the sequencer data
Copy link
Member

@julienrbrt julienrbrt Jul 3, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 julienrbrt changed the title Allow fetching rollkitmngr state in rollkit-migrate command for attester sets feat: Allow fetching rollkitmngr state in rollkit-migrate command for attester sets Jul 3, 2025
@Copilot Copilot AI requested a review from julienrbrt July 3, 2025 13:44
Copilot finished work on behalf of julienrbrt July 3, 2025 13:44
return nil, fmt.Errorf("sequencer not found in rollkitmngr state")
}

// Store it in the context's store so collections can find it
Copy link
Member

Choose a reason for hiding this comment

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

consider the store is read only. DO NOT CHANGE THE APP STORE. you should solely get info from it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@Copilot Copilot AI requested a review from julienrbrt July 3, 2025 14:23
Copilot finished work on behalf of julienrbrt July 3, 2025 14:23
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 {
Copy link
Member

Choose a reason for hiding this comment

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

can we error joins those errors? as it will be lost otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copilot finished work on behalf of julienrbrt July 3, 2025 15:38
@Copilot Copilot AI requested a review from julienrbrt July 3, 2025 15:38
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.

Allow fetching rollkitmngr state in rollkit-migrate command
2 participants