refactor(modules): replace sequencer by rollkitmngr #114
Conversation
WalkthroughThis change removes the old "sequencer" module and introduces a new "rollkitmngr" (Rollkit Manager) module, including new keeper logic, gRPC queries, migration mechanisms, and Protocol Buffers definitions. The update expands the migration process to support attesters, updates documentation, and refactors all relevant code, keys, and interfaces to use the new module name and structure. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI/REST
participant AppModule
participant Keeper
participant StakingKeeper
User->>CLI/REST: Submit MigrateToRollkit or EditAttesters tx
CLI/REST->>AppModule: Route tx to rollkitmngr MsgServer
AppModule->>Keeper: Validate and store migration/attesters
Keeper->>StakingKeeper: Validate attester/validator info
Keeper-->>AppModule: Acknowledge or error
User->>CLI/REST: Query IsMigrating / Sequencer / Attesters
CLI/REST->>AppModule: Route query to rollkitmngr QueryServer
AppModule->>Keeper: Fetch migration/attester/sequencer info
Keeper->>StakingKeeper: Fetch validator set (if needed)
Keeper-->>AppModule: Return query result
AppModule-->>CLI/REST: Return response to user
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 ✨ Finishing Touches
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 (
|
julienrbrt
changed the title
julienrbrt
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (12)
modules/rollkitmngr/types/sequencer.go (3)
32-44: Consider refactoring to reduce code duplication.The
TmConsPublicKeymethod forAttesteris functionally correct and necessary. However, it's identical to theSequencer.TmConsPublicKeymethod, creating code duplication.Consider extracting this logic into a shared helper function:
+// tmConsPublicKeyFromAny converts a consensus public key from Any type to CometBFT public key. +func tmConsPublicKeyFromAny(consensusPubkey *codectypes.Any) (cmtprotocrypto.PublicKey, error) { + pk, ok := consensusPubkey.GetCachedValue().(cryptotypes.PubKey) + if !ok { + return cmtprotocrypto.PublicKey{}, errors.Wrapf(sdkerrors.ErrInvalidType, "expecting cryptotypes.PubKey, got %T", pk) + } + + tmPk, err := cryptocodec.ToCmtProtoPublicKey(pk) + if err != nil { + return cmtprotocrypto.PublicKey{}, err + } + return tmPk, nil +} + // TmConsPublicKey converts the Sequencer's ConsensusPubkey to a CometBFT PublicKey type. func (att Attester) TmConsPublicKey() (cmtprotocrypto.PublicKey, error) { - pk, ok := att.ConsensusPubkey.GetCachedValue().(cryptotypes.PubKey) - if !ok { - return cmtprotocrypto.PublicKey{}, errors.Wrapf(sdkerrors.ErrInvalidType, "expecting cryptotypes.PubKey, got %T", pk) - } - - tmPk, err := cryptocodec.ToCmtProtoPublicKey(pk) - if err != nil { - return cmtprotocrypto.PublicKey{}, err - } - return tmPk, nil + return tmConsPublicKeyFromAny(att.ConsensusPubkey) }Then apply the same refactor to the
Sequencer.TmConsPublicKeymethod.
32-32: Fix comment inconsistency.The comment incorrectly refers to "Sequencer's ConsensusPubkey" when it should refer to "Attester's ConsensusPubkey".
-// TmConsPublicKey converts the Sequencer's ConsensusPubkey to a CometBFT PublicKey type. +// TmConsPublicKey converts the Attester's ConsensusPubkey to a CometBFT PublicKey type.
46-50: Code duplication in UnpackInterfaces method.Similar to the
TmConsPublicKeymethod, thisUnpackInterfacesimplementation is identical to theSequencerversion, creating unnecessary duplication.Consider creating a shared helper function for interface unpacking as well:
+// unpackConsensusPubkey unpacks a consensus public key from Any type. +func unpackConsensusPubkey(unpacker codectypes.AnyUnpacker, consensusPubkey *codectypes.Any) error { + var pk cryptotypes.PubKey + return unpacker.UnpackAny(consensusPubkey, &pk) +} + // UnpackInterfaces implements UnpackInterfacesMessage.UnpackInterfaces func (att Attester) UnpackInterfaces(unpacker codectypes.AnyUnpacker) error { - var pk cryptotypes.PubKey - return unpacker.UnpackAny(att.ConsensusPubkey, &pk) + return unpackConsensusPubkey(unpacker, att.ConsensusPubkey) }modules/README.md (1)
13-19: Consider rewording to avoid repetitive sentence structure.The documentation is accurate and comprehensive. However, there's a minor style issue with consecutive sentences starting with "Additionally."
-Additionally, it has additional queries to get the sequencer information, and the attesters information. +It provides queries to get the sequencer information and the attesters information. -Additionally, when added to a CometBFT chain, the `rollkitmngr` module will handle the switch from a CometBFT validator set to a Rollkit sequencer at a given height. +When added to a CometBFT chain, the `rollkitmngr` module will handle the switch from a CometBFT validator set to a Rollkit sequencer at a given height.🧰 Tools
🪛 LanguageTool
[style] ~18-~18: Two successive sentences begin with the same adverb. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...mation, and the attesters information. Additionally, when added to a CometBFT chain, the `r...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
modules/proto/rollkitsdk/rollkitmngr/v1/tx.proto (1)
32-35: Fix grammatical error in comment.The comment has a grammatical error - "trigger" should be "triggers" to match the singular subject "block height".
- // block height that trigger the rollkit migration. + // block height that triggers the rollkit migration.modules/proto/rollkitsdk/rollkitmngr/v1/types.proto (1)
13-15: Fix grammatical error in comment.Same grammatical error as in tx.proto - "trigger" should be "triggers".
- // block height that trigger the rollkit migration. + // block height that triggers the rollkit migration.modules/rollkitmngr/keeper/grpc_query.go (1)
17-17: Update comment to reflect module name change.The comment still references "sequencer query server" but this is now the rollkitmngr module.
-// NewQueryServer creates a new instance of the sequencer query server. +// NewQueryServer creates a new instance of the rollkitmngr query server.modules/rollkitmngr/keeper/msg_server.go (2)
20-20: Update comment to reflect module name change.The comment still references "sequencer MsgServer" but this is now the rollkitmngr module.
-// NewMsgServerImpl returns an implementation of sequencer MsgServer interface. +// NewMsgServerImpl returns an implementation of rollkitmngr MsgServer interface.
54-54: Complete the EditAttesters implementation.The method currently only validates attesters but doesn't actually edit them. The TODO comment and empty implementation suggest this is incomplete.
Would you like me to help implement the logic to update the attesters after validation, or open an issue to track this incomplete functionality?
Also applies to: 78-80
modules/rollkitmngr/keeper/keeper.go (2)
29-29: Update comment to reflect module name change.The comment still references "sequencer Keeper" but this is now the rollkitmngr module.
-// NewKeeper creates a new sequencer Keeper instance. +// NewKeeper creates a new rollkitmngr Keeper instance.
113-114: Consider assigning to blank identifier for clarity.The GetKVStore result is not used. Assign to blank identifier to make the intent clearer.
ms := sdkCtx.MultiStore() - ms.GetKVStore(ibcStoreKey) + _ = ms.GetKVStore(ibcStoreKey)modules/rollkitmngr/keeper/migration.go (1)
107-111: Implement the migrateOver function or remove it.The
migrateOverfunction is declared but not implemented. This could cause runtime panics if called.Consider either:
- Implementing the gradual migration logic for IBC compatibility
- Removing the function if it's not needed yet
- Adding a feature flag to prevent accidental calls
Would you like me to help implement the gradual migration logic or suggest an alternative approach?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
modules/rollkitmngr/module/module.pb.gois excluded by!**/*.pb.gomodules/rollkitmngr/types/query.pb.gois excluded by!**/*.pb.gomodules/rollkitmngr/types/query.pb.gw.gois excluded by!**/*.pb.gw.gomodules/rollkitmngr/types/tx.pb.gois excluded by!**/*.pb.gomodules/rollkitmngr/types/types.pb.gois excluded by!**/*.pb.gomodules/sequencer/types/query.pb.gois excluded by!**/*.pb.gomodules/sequencer/types/query.pb.gw.gois excluded by!**/*.pb.gw.gomodules/sequencer/types/tx.pb.gois excluded by!**/*.pb.gomodules/sequencer/types/types.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (28)
Makefile(1 hunks)go.mod(1 hunks)modules/README.md(2 hunks)modules/proto/rollkitsdk/rollkitmngr/module/v1/module.proto(1 hunks)modules/proto/rollkitsdk/rollkitmngr/v1/query.proto(1 hunks)modules/proto/rollkitsdk/rollkitmngr/v1/tx.proto(1 hunks)modules/proto/rollkitsdk/rollkitmngr/v1/types.proto(1 hunks)modules/proto/rollkitsdk/sequencer/v1/query.proto(0 hunks)modules/proto/rollkitsdk/sequencer/v1/tx.proto(0 hunks)modules/proto/rollkitsdk/sequencer/v1/types.proto(0 hunks)modules/rollkitmngr/autocli.go(2 hunks)modules/rollkitmngr/depinject.go(2 hunks)modules/rollkitmngr/keeper/abci.go(1 hunks)modules/rollkitmngr/keeper/grpc_query.go(1 hunks)modules/rollkitmngr/keeper/keeper.go(1 hunks)modules/rollkitmngr/keeper/migration.go(1 hunks)modules/rollkitmngr/keeper/msg_server.go(1 hunks)modules/rollkitmngr/module.go(4 hunks)modules/rollkitmngr/types/codec.go(1 hunks)modules/rollkitmngr/types/expected_keepers.go(1 hunks)modules/rollkitmngr/types/keys.go(1 hunks)modules/rollkitmngr/types/sequencer.go(2 hunks)modules/sequencer/keeper/abci.go(0 hunks)modules/sequencer/keeper/grpc_query.go(0 hunks)modules/sequencer/keeper/keeper.go(0 hunks)modules/sequencer/keeper/migration.go(0 hunks)modules/sequencer/keeper/msg_server.go(0 hunks)modules/sequencer/types/keys.go(0 hunks)
💤 Files with no reviewable changes (9)
- modules/sequencer/types/keys.go
- modules/proto/rollkitsdk/sequencer/v1/types.proto
- modules/sequencer/keeper/abci.go
- modules/proto/rollkitsdk/sequencer/v1/tx.proto
- modules/sequencer/keeper/keeper.go
- modules/sequencer/keeper/msg_server.go
- modules/sequencer/keeper/grpc_query.go
- modules/sequencer/keeper/migration.go
- modules/proto/rollkitsdk/sequencer/v1/query.proto
🧰 Additional context used
🧬 Code Graph Analysis (7)
modules/rollkitmngr/module.go (1)
modules/rollkitmngr/types/keys.go (1)
ModuleName(4-4)
modules/rollkitmngr/types/codec.go (1)
modules/rollkitmngr/types/tx.pb.go (6)
MsgMigrateToRollkit(35-47)MsgMigrateToRollkit(51-51)MsgMigrateToRollkit(52-54)MsgEditAttesters(149-155)MsgEditAttesters(159-159)MsgEditAttesters(160-162)
modules/rollkitmngr/keeper/abci.go (1)
modules/rollkitmngr/keeper/keeper.go (1)
Keeper(16-27)
modules/rollkitmngr/types/sequencer.go (1)
modules/rollkitmngr/types/types.pb.go (3)
Attester(152-158)Attester(162-162)Attester(163-165)
modules/rollkitmngr/keeper/keeper.go (4)
modules/rollkitmngr/types/expected_keepers.go (2)
AccountKeeper(12-17)StakingKeeper(20-24)modules/rollkitmngr/types/types.pb.go (6)
Sequencer(96-102)Sequencer(106-106)Sequencer(107-109)RollkitMigration(30-39)RollkitMigration(43-43)RollkitMigration(44-46)modules/rollkitmngr/types/keys.go (3)
SequencerKey(8-8)MigrationKey(7-7)ModuleName(4-4)modules/rollkitmngr/keeper/migration.go (1)
IBCSmoothingFactor(14-14)
modules/rollkitmngr/keeper/msg_server.go (3)
modules/rollkitmngr/types/tx.pb.go (13)
MsgServer(333-338)MsgMigrateToRollkit(35-47)MsgMigrateToRollkit(51-51)MsgMigrateToRollkit(52-54)MsgMigrateToRollkitResponse(112-113)MsgMigrateToRollkitResponse(117-117)MsgMigrateToRollkitResponse(118-120)MsgEditAttesters(149-155)MsgEditAttesters(159-159)MsgEditAttesters(160-162)MsgEditAttestersResponse(206-207)MsgEditAttestersResponse(211-211)MsgEditAttestersResponse(212-214)modules/rollkitmngr/keeper/keeper.go (1)
Keeper(16-27)modules/rollkitmngr/types/types.pb.go (6)
RollkitMigration(30-39)RollkitMigration(43-43)RollkitMigration(44-46)Sequencer(96-102)Sequencer(106-106)Sequencer(107-109)
modules/rollkitmngr/keeper/grpc_query.go (3)
modules/rollkitmngr/types/query.pb.go (19)
QueryServer(405-412)QueryAttestersRequest(218-219)QueryAttestersRequest(223-223)QueryAttestersRequest(224-226)QueryAttestersResponse(255-258)QueryAttestersResponse(262-262)QueryAttestersResponse(263-265)QueryIsMigratingRequest(34-35)QueryIsMigratingRequest(39-39)QueryIsMigratingRequest(40-42)QueryIsMigratingResponse(71-78)QueryIsMigratingResponse(82-82)QueryIsMigratingResponse(83-85)QuerySequencerRequest(135-136)QuerySequencerRequest(140-140)QuerySequencerRequest(141-143)QuerySequencerResponse(172-175)QuerySequencerResponse(179-179)QuerySequencerResponse(180-182)modules/rollkitmngr/keeper/keeper.go (1)
Keeper(16-27)modules/rollkitmngr/types/types.pb.go (6)
Attester(152-158)Attester(162-162)Attester(163-165)Sequencer(96-102)Sequencer(106-106)Sequencer(107-109)
🪛 Buf (1.54.0)
modules/proto/rollkitsdk/rollkitmngr/module/v1/module.proto
4-4: import "cosmos/app/v1alpha1/module.proto": file does not exist
(COMPILE)
modules/proto/rollkitsdk/rollkitmngr/v1/types.proto
4-4: import "amino/amino.proto": file does not exist
(COMPILE)
modules/proto/rollkitsdk/rollkitmngr/v1/query.proto
4-4: import "amino/amino.proto": file does not exist
(COMPILE)
modules/proto/rollkitsdk/rollkitmngr/v1/tx.proto
4-4: import "amino/amino.proto": file does not exist
(COMPILE)
🪛 LanguageTool
modules/README.md
[style] ~18-~18: Two successive sentences begin with the same adverb. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...mation, and the attesters information. Additionally, when added to a CometBFT chain, the `r...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / Run Unit Tests
- GitHub Check: Test with Rollkit Chain
🔇 Additional comments (30)
go.mod (1)
17-17: LGTM: Appropriate dependency promotion.Moving
cosmossdk.io/storefrom indirect to direct dependency is correct, as the newrollkitmngrmodule likely uses Cosmos SDK store interfaces directly for its expanded functionality.modules/rollkitmngr/depinject.go (2)
1-1: LGTM: Consistent package rename.The package name change from
sequencertorollkitmngris properly implemented.
12-14: LGTM: Import paths correctly updated.All import paths have been consistently updated to reference the new
rollkitmngrmodule structure, maintaining the same functionality.modules/rollkitmngr/types/expected_keepers.go (1)
21-21: LGTM: Well-designed interface extension.The new
GetValidatorByConsAddrmethod is properly designed and supports the expanded attester validation functionality in the rollkitmngr module.modules/rollkitmngr/module.go (3)
1-1: LGTM: Package name consistently updated.The package rename from
sequencertorollkitmngris correctly implemented.
15-16: LGTM: Import paths properly updated.All import paths have been consistently updated to reference the new
rollkitmngrmodule structure.
47-47: LGTM: Documentation consistently updated.All function comments have been properly updated to reference the
rollkitmngrmodule instead of the oldsequencermodule, maintaining accurate documentation.Also applies to: 52-52, 62-62, 67-67
Makefile (1)
73-74: LGTM! Paths correctly updated for module rename.The Makefile changes properly reflect the refactor from
sequencertorollkitmngrmodule, updating the protobuf generation paths accordingly.modules/rollkitmngr/types/codec.go (2)
12-13: LGTM! Amino codec registrations updated correctly.The legacy amino registrations properly reflect the new message types
MsgMigrateToRollkitandMsgEditAttesterswith the correctrollkitmngr/v1namespace.
19-20: LGTM! Interface registry updated correctly.The interface registry properly registers the new message types as implementations of
sdk.Msg, maintaining consistency with the amino registrations.modules/rollkitmngr/types/keys.go (1)
1-9:✅ Verification successful
LGTM! Standard module key definitions.
The keys file follows Cosmos SDK conventions with clear naming and appropriate byte values for storage keys.
Please verify that the key values
0x13and0x14don't conflict with any existing keys in the broader codebase:
🏁 Script executed:
#!/bin/bash # Description: Check for potential key conflicts in the codebase # Expected: No other modules using the same byte values for keys echo "Searching for potential key conflicts with 0x13 and 0x14..." rg "0x13|0x14" --type go -A 2 -B 2Length of output: 7603
🏁 Script executed:
#!/bin/bash echo "Searching for literal []byte{0x13} and []byte{0x14} outside .pb.go files..." rg '\[\]byte\{0x13\}' -t go -g '!*.pb.go' rg '\[\]byte\{0x14\}' -t go -g '!*.pb.go' echo "Searching for MigrationKey and SequencerKey definitions across Go files..." rg 'MigrationKey' -t go rg 'SequencerKey' -t goLength of output: 827
MigrationKey (0x13) and SequencerKey (0x14) are unique
Ripgrep across all Go files (excluding generated *.pb.go) shows these byte values only in
- modules/rollkitmngr/types/keys.go (definitions)
- referenced in modules/rollkitmngr/keeper/keeper.go
No conflicts detected.
modules/proto/rollkitsdk/rollkitmngr/module/v1/module.proto (4)
2-2: LGTM! Package name correctly updated.The protobuf package name properly reflects the module rename from
sequencertorollkitmngr.
6-6: LGTM! Go package path updated correctly.The Go package import path correctly reflects the new module structure.
11-11: LGTM! Go import directive updated correctly.The go_import directive properly points to the new rollkitmngr module path.
4-4:❌ Incorrect review comment
Fix missing import causing compilation error.
The static analysis correctly identifies that the imported file
cosmos/app/v1alpha1/module.protois not included in the imports section, which will cause protobuf compilation to fail.The file references
cosmos.app.v1alpha1.modulein line 10 but doesn't import it. Add the missing import:import "cosmos/app/v1alpha1/module.proto"; +import "cosmos/app/v1alpha1/module.proto";Wait, I see the import is already there on line 4. Let me verify this is a real issue by checking if the path exists in the dependencies:
🏁 Script executed:
#!/bin/bash # Description: Check if the cosmos/app/v1alpha1/module.proto import exists # Expected: Find the import file in dependencies or proto files echo "Checking for cosmos/app/v1alpha1/module.proto in the codebase..." fd -e proto -x grep -l "cosmos/app/v1alpha1/module.proto" {} \; echo "Checking for any cosmos/app proto files..." fd "cosmos" --type d | head -10 echo "Checking buf dependencies..." cat modules/proto/buf.yaml 2>/dev/null || echo "No buf.yaml found" cat modules/proto/buf.mod 2>/dev/null || echo "No buf.mod found"Length of output: 1011
Import already present—no action needed
The import
cosmos/app/v1alpha1/module.protois already declared on line 4 of
modules/proto/rollkitsdk/rollkitmngr/module/v1/module.proto. There’s no missing import here, so no change is required. If you still encounter compilation errors, please ensure your Buf dependencies are properly fetched and that your build configuration is correct.Likely an incorrect or invalid review comment.
🧰 Tools
🪛 Buf (1.54.0)
4-4: import "cosmos/app/v1alpha1/module.proto": file does not exist
(COMPILE)
modules/README.md (1)
4-4: Documentation accurately reflects expanded module scope.The updated description correctly captures the enhanced functionality of the rollkitmngr module, expanding from just sequencers to include attesters and migration management.
modules/rollkitmngr/autocli.go (3)
1-1: Package and import path updates are correct.The package rename from
sequencertorollkitmngrand the corresponding import path update properly reflect the module refactor.Also applies to: 6-6
16-29: CLI commands properly reflect expanded module functionality.The new CLI commands accurately represent the enhanced capabilities:
Attesterscommand for querying attesters (new functionality)Sequencercommand (renamed from plural to singular for consistency)IsMigratingcommand for migration status (new functionality)
36-38: Authority gating for migration command is appropriate.The
MigrateToRollkitcommand is correctly marked as skipped due to authority gating, which is appropriate for governance-controlled migration operations.modules/proto/rollkitsdk/rollkitmngr/v1/query.proto (3)
1-10: Protobuf file structure follows Cosmos SDK conventions.The file header, package declaration, imports, and Go package option are correctly structured according to Cosmos SDK protobuf conventions.
🧰 Tools
🪛 Buf (1.54.0)
4-4: import "amino/amino.proto": file does not exist
(COMPILE)
12-27: Query service definition is well-structured.The
Queryservice properly defines the three RPC methods (IsMigrating,Sequencer,Attesters) with appropriate HTTP endpoint mappings. This aligns well with the CLI commands defined inautocli.go.
29-61: Request and response message types are properly defined.The message types follow protobuf best practices:
- Empty request types for simple queries
- Well-documented response fields
- Appropriate use of gogoproto and amino options for serialization
- Consistent naming conventions
modules/proto/rollkitsdk/rollkitmngr/v1/types.proto (1)
12-16:❓ Verification inconclusive
Verify intentional field numbering starting at 2.
The
RollkitMigrationmessage starts field numbering at 2, skipping field 1. This could indicate a reserved or deprecated field.
🏁 Script executed:
#!/bin/bash # Description: Check if field 1 is reserved or if this is intentional # Search for reserved fields in proto files rg -A 2 -B 2 "reserved.*1" modules/proto/ # Check git history for removed field 1 git log -p --all -S "= 1;" -- "*.proto" | grep -A 5 -B 5 "RollkitMigration" # Look for any comments about field reservation rg -i "reserved|deprecated" modules/proto/rollkitsdk/rollkitmngr/Length of output: 111
Confirm skipped field number in RollkitMigration
I didn’t find any
reserved 1;, deprecated markers, or git history indicating a removed field 1 inmodules/proto/rollkitsdk/rollkitmngr/v1/types.proto. Please verify that skipping field number 1 is intentional:• File:
modules/proto/rollkitsdk/rollkitmngr/v1/types.proto
Message:RollkitMigration(around line 12)
Current field:uint64 block_height = 2;If the omission was intentional, consider adding an explicit reservation or a comment for clarity, for example:
// Reserved for future use. // reserved 1;If not, renumber the field or restore the missing field definition.
modules/rollkitmngr/keeper/abci.go (2)
33-51: Well-implemented chain halt mechanism for migration completion.The
PreBlockerimplementation correctly:
- Checks for migration completion at exactly one block after the migration end
- Removes migration state before halting to prevent repeated halts on restart
- Provides clear error message for operators
This ensures a clean binary switch after migration completes.
53-79: Clean migration logic with appropriate IBC handling.The
EndBlockimplementation properly:
- Checks migration status and timing
- Handles both IBC-enabled and non-IBC scenarios
- Delegates to appropriate migration strategies (
migrateNowvsmigrateOver)- Includes proper error handling for state retrieval
The separation of immediate vs gradual migration based on IBC enablement is a good design choice.
modules/rollkitmngr/keeper/grpc_query.go (1)
44-53: LGTM!The IsMigrating query implementation correctly delegates to the keeper and properly constructs the response.
modules/rollkitmngr/keeper/msg_server.go (1)
25-46: LGTM!The MigrateToRollkit implementation correctly validates authority and block height, and properly handles the migration state storage.
modules/rollkitmngr/keeper/keeper.go (2)
72-76: LGTM!The Logger method correctly implements the standard Cosmos SDK module logger pattern.
91-91:✅ Verification successful
Verify IBCSmoothingFactor is properly imported.
The code references
IBCSmoothingFactorwhich is not defined in this file. According to the relevant snippets, it's defined inmigration.go.
🏁 Script executed:
#!/bin/bash # Description: Verify IBCSmoothingFactor constant is defined and accessible # Check if IBCSmoothingFactor is defined in the keeper package rg -A 2 "IBCSmoothingFactor" modules/rollkitmngr/keeper/ # Check if it's exported (starts with uppercase) ast-grep --pattern 'const IBCSmoothingFactor'Length of output: 947
No action needed: IBCSmoothingFactor is defined in the same package
TheIBCSmoothingFactorvariable is declared inmodules/rollkitmngr/keeper/migration.goas an exporteduint64, so it’s accessible inkeeper.gowithout additional imports.modules/rollkitmngr/keeper/migration.go (1)
50-74: LGTM - Sequencer migration logic is correct.The function properly handles single sequencer migration by removing all non-sequencer validators and adding/updating the sequencer with power 1.
| syntax = "proto3"; | ||
| package rollkitsdk.rollkitmngr.v1; | ||
|
|
||
| import "amino/amino.proto"; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify amino.proto import configuration.
The static analysis tool reports that amino/amino.proto file does not exist. This could be a false positive if the proto dependencies are properly configured in your build system.
Run the following script to verify the proto dependencies and build configuration:
🏁 Script executed:
#!/bin/bash
# Description: Verify the amino proto import is properly configured
# Check if amino proto files exist in common locations
fd -t f "amino.proto" --max-depth 5
# Check proto include paths in Makefile or buf configuration
if [ -f "buf.yaml" ]; then
echo "=== buf.yaml configuration ==="
cat buf.yaml
fi
if [ -f "buf.work.yaml" ]; then
echo "=== buf.work.yaml configuration ==="
cat buf.work.yaml
fi
# Check Makefile for proto generation commands
if [ -f "Makefile" ]; then
echo "=== Makefile proto-related targets ==="
grep -A 5 -B 5 "proto" Makefile | grep -E "(amino|PROTO|proto-gen)"
fiLength of output: 338
Missing amino.proto dependency
A search of the repo (and your Makefile/buf configs) did not locate amino/amino.proto. As written, this import in tx.proto will fail at build time. Please vendor or add the amino.proto file and/or configure your proto include paths so that it resolves correctly.
• File: modules/proto/rollkitsdk/rollkitmngr/v1/tx.proto line 4
- import "amino/amino.proto";
+ // Ensure this path matches a vendored or configured proto location
+ import "amino/amino.proto";Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Buf (1.54.0)
4-4: import "amino/amino.proto": file does not exist
(COMPILE)
🤖 Prompt for AI Agents
In modules/proto/rollkitsdk/rollkitmngr/v1/tx.proto at line 4, the import
statement for "amino/amino.proto" is causing build failures because the
amino.proto file is missing or not found in the proto include paths. To fix
this, either add the amino.proto file to the repository under the expected path
or update the proto include paths in your build configuration (such as buf.yaml,
buf.work.yaml, or Makefile) to correctly reference the location of amino.proto
so that the import resolves successfully during the build.
| [ (gogoproto.nullable) = false, (amino.dont_omitempty) = true ]; | ||
| } | ||
|
|
||
| // Sequencer defines a sequence of instructions to be executed. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update misleading comment for Sequencer message.
The comment "defines a sequence of instructions to be executed" is misleading. In blockchain context, a sequencer is typically an entity that orders transactions, not a sequence of instructions.
-// Sequencer defines a sequence of instructions to be executed.
+// Sequencer defines an entity responsible for ordering and producing blocks.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Sequencer defines a sequence of instructions to be executed. | |
| // Sequencer defines an entity responsible for ordering and producing blocks. |
🤖 Prompt for AI Agents
In modules/proto/rollkitsdk/rollkitmngr/v1/types.proto at line 27, update the
comment for the Sequencer message to accurately reflect its role as an entity
that orders transactions in the blockchain context, rather than describing it as
a sequence of instructions to be executed. Replace the misleading comment with
one that clearly states the Sequencer's function in ordering transactions.
| // If the sequencer and attesters are equals, then the chain is not using attesters. | ||
| func (q queryServer) Attesters(context.Context, *types.QueryAttestersRequest) (*types.QueryAttestersResponse, error) { | ||
| vals, err := q.stakingKeeper.GetLastValidators(context.Background()) |
There was a problem hiding this comment.
Use the provided context instead of context.Background().
The method receives a context parameter but ignores it, using context.Background() instead. This breaks context propagation for cancellation, deadlines, and values.
-func (q queryServer) Attesters(context.Context, *types.QueryAttestersRequest) (*types.QueryAttestersResponse, error) {
- vals, err := q.stakingKeeper.GetLastValidators(context.Background())
+func (q queryServer) Attesters(ctx context.Context, *types.QueryAttestersRequest) (*types.QueryAttestersResponse, error) {
+ vals, err := q.stakingKeeper.GetLastValidators(ctx)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // If the sequencer and attesters are equals, then the chain is not using attesters. | |
| func (q queryServer) Attesters(context.Context, *types.QueryAttestersRequest) (*types.QueryAttestersResponse, error) { | |
| vals, err := q.stakingKeeper.GetLastValidators(context.Background()) | |
| // If the sequencer and attesters are equals, then the chain is not using attesters. | |
| func (q queryServer) Attesters(ctx context.Context, *types.QueryAttestersRequest) (*types.QueryAttestersResponse, error) { | |
| vals, err := q.stakingKeeper.GetLastValidators(ctx) | |
| // … | |
| } |
🤖 Prompt for AI Agents
In modules/rollkitmngr/keeper/grpc_query.go around lines 24 to 26, the Attesters
method receives a context parameter but incorrectly uses context.Background()
when calling GetLastValidators. Replace context.Background() with the provided
context parameter to ensure proper context propagation for cancellation,
deadlines, and values.
| _, _, done := q.Keeper.IsMigrating(ctx) | ||
| if !done { | ||
| return nil, sdkerrors.ErrLogic.Wrap("sequencer is not set, migration is in progress or not started yet") | ||
| } |
There was a problem hiding this comment.
Fix the inverted migration status check logic.
The variable naming creates confusion. The third return value from IsMigrating represents whether migration is in progress, not whether it's done. The current logic is inverted.
- _, _, done := q.Keeper.IsMigrating(ctx)
- if !done {
+ _, _, migrationInProgress := q.Keeper.IsMigrating(ctx)
+ if migrationInProgress {
return nil, sdkerrors.ErrLogic.Wrap("sequencer is not set, migration is in progress or not started yet")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _, _, done := q.Keeper.IsMigrating(ctx) | |
| if !done { | |
| return nil, sdkerrors.ErrLogic.Wrap("sequencer is not set, migration is in progress or not started yet") | |
| } | |
| _, _, migrationInProgress := q.Keeper.IsMigrating(ctx) | |
| if migrationInProgress { | |
| return nil, sdkerrors.ErrLogic.Wrap("sequencer is not set, migration is in progress or not started yet") | |
| } |
🤖 Prompt for AI Agents
In modules/rollkitmngr/keeper/grpc_query.go around lines 57 to 60, the logic
checking migration status is inverted because the third return value from
IsMigrating indicates if migration is in progress, not if it is done. Rename the
variable to reflect that it means migration is ongoing (e.g., migrating), then
update the condition to return the error when migration is in progress
(migrating is true), not when it is done.
|
|
||
| attesters[attester.ConsensusPubkey.String()] = struct{}{} | ||
|
|
||
| consPub := attester.ConsensusPubkey.GetCachedValue().(cryptotypes.PubKey) |
There was a problem hiding this comment.
Add safety check for type assertion to prevent panic.
The type assertion could panic if the cached value is not a cryptotypes.PubKey. Add a safety check.
- consPub := attester.ConsensusPubkey.GetCachedValue().(cryptotypes.PubKey)
+ consPubInterface := attester.ConsensusPubkey.GetCachedValue()
+ consPub, ok := consPubInterface.(cryptotypes.PubKey)
+ if !ok {
+ return nil, sdkerrors.ErrInvalidRequest.Wrapf("invalid consensus public key type for attester %s", attester.Name)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| consPub := attester.ConsensusPubkey.GetCachedValue().(cryptotypes.PubKey) | |
| consPubInterface := attester.ConsensusPubkey.GetCachedValue() | |
| consPub, ok := consPubInterface.(cryptotypes.PubKey) | |
| if !ok { | |
| return nil, sdkerrors.ErrInvalidRequest.Wrapf( | |
| "invalid consensus public key type for attester %s", | |
| attester.Name, | |
| ) | |
| } |
🤖 Prompt for AI Agents
In modules/rollkitmngr/keeper/msg_server.go at line 68, the type assertion on
attester.ConsensusPubkey.GetCachedValue() to cryptotypes.PubKey can panic if the
value is not of that type. Modify the code to use the two-value form of type
assertion to safely check the type, handle the case where the assertion fails
(e.g., return an error or log it), and only proceed if the assertion succeeds.
| if val.GetMoniker() == "" { | ||
| return nil, sdkerrors.ErrInvalidRequest.Wrapf("no validator found for consensus public key %s", attester.ConsensusPubkey.String()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve validator existence check.
Checking for empty moniker is not a reliable way to verify validator existence, as validators can have empty monikers. The error from GetValidatorByConsAddr should be sufficient.
val, err := m.stakingKeeper.GetValidatorByConsAddr(ctx, sdk.ConsAddress(consPub.Address()))
if err != nil {
return nil, sdkerrors.ErrInvalidRequest.Wrapf("failed to get validator by consensus address %s: %v", consPub.Address(), err)
}
- if val.GetMoniker() == "" {
- return nil, sdkerrors.ErrInvalidRequest.Wrapf("no validator found for consensus public key %s", attester.ConsensusPubkey.String())
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if val.GetMoniker() == "" { | |
| return nil, sdkerrors.ErrInvalidRequest.Wrapf("no validator found for consensus public key %s", attester.ConsensusPubkey.String()) | |
| } | |
| val, err := m.stakingKeeper.GetValidatorByConsAddr(ctx, sdk.ConsAddress(consPub.Address())) | |
| if err != nil { | |
| return nil, sdkerrors.ErrInvalidRequest.Wrapf("failed to get validator by consensus address %s: %v", consPub.Address(), err) | |
| } |
🤖 Prompt for AI Agents
In modules/rollkitmngr/keeper/msg_server.go around lines 73 to 75, remove the
check for an empty moniker to verify validator existence, as it is unreliable.
Instead, rely solely on the error returned by GetValidatorByConsAddr to
determine if the validator exists, and return that error appropriately without
additional moniker checks.
| // IBCSmoothingFactor is the factor used to smooth the migration process when IBC is enabled. It determines how many blocks the migration will take. | ||
| var IBCSmoothingFactor uint64 = 30 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make IBCSmoothingFactor immutable.
The IBCSmoothingFactor should be a constant rather than a variable to prevent accidental modification at runtime.
-// IBCSmoothingFactor is the factor used to smooth the migration process when IBC is enabled. It determines how many blocks the migration will take.
-var IBCSmoothingFactor uint64 = 30
+// IBCSmoothingFactor is the factor used to smooth the migration process when IBC is enabled. It determines how many blocks the migration will take.
+const IBCSmoothingFactor uint64 = 30🤖 Prompt for AI Agents
In modules/rollkitmngr/keeper/migration.go at lines 13 to 14, change the
declaration of IBCSmoothingFactor from a variable to a constant by replacing
'var' with 'const' to make it immutable and prevent runtime modification.
| func (k Keeper) migrateNow( | ||
| ctx context.Context, | ||
| migrationData types.RollkitMigration, | ||
| lastValidatorSet []stakingtypes.Validator, | ||
| ) (initialValUpdates []abci.ValidatorUpdate, err error) { | ||
| switch len(migrationData.Attesters) { | ||
| case 0: | ||
| // no attesters, we are migrating to a single sequencer | ||
| initialValUpdates, err = migrateToSequencer(migrationData, lastValidatorSet) | ||
| if err != nil { | ||
| return nil, sdkerrors.ErrInvalidRequest.Wrapf("failed to migrate to sequencer: %v", err) | ||
| } | ||
| default: | ||
| // we are migrating the validator set to attesters | ||
| initialValUpdates, err = migrateToAttesters(migrationData, lastValidatorSet) | ||
| if err != nil { | ||
| return nil, sdkerrors.ErrInvalidRequest.Wrapf("failed to migrate to sequencer & attesters: %v", err) | ||
| } | ||
| } | ||
|
|
||
| // set new sequencer in the store | ||
| // it will be used by the rollkit migration command when using attesters | ||
| seq := migrationData.Sequencer | ||
| if err := k.Sequencer.Set(ctx, seq); err != nil { | ||
| return nil, sdkerrors.ErrInvalidRequest.Wrapf("failed to set sequencer: %v", err) | ||
| } | ||
|
|
||
| return initialValUpdates, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for migration data.
The function should validate the migration data before processing to prevent runtime errors.
func (k Keeper) migrateNow(
ctx context.Context,
migrationData types.RollkitMigration,
lastValidatorSet []stakingtypes.Validator,
) (initialValUpdates []abci.ValidatorUpdate, err error) {
+ // Validate migration data
+ if migrationData.Sequencer.ConsensusPubkey == nil {
+ return nil, sdkerrors.ErrInvalidRequest.Wrap("sequencer consensus public key cannot be nil")
+ }
+
switch len(migrationData.Attesters) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (k Keeper) migrateNow( | |
| ctx context.Context, | |
| migrationData types.RollkitMigration, | |
| lastValidatorSet []stakingtypes.Validator, | |
| ) (initialValUpdates []abci.ValidatorUpdate, err error) { | |
| switch len(migrationData.Attesters) { | |
| case 0: | |
| // no attesters, we are migrating to a single sequencer | |
| initialValUpdates, err = migrateToSequencer(migrationData, lastValidatorSet) | |
| if err != nil { | |
| return nil, sdkerrors.ErrInvalidRequest.Wrapf("failed to migrate to sequencer: %v", err) | |
| } | |
| default: | |
| // we are migrating the validator set to attesters | |
| initialValUpdates, err = migrateToAttesters(migrationData, lastValidatorSet) | |
| if err != nil { | |
| return nil, sdkerrors.ErrInvalidRequest.Wrapf("failed to migrate to sequencer & attesters: %v", err) | |
| } | |
| } | |
| // set new sequencer in the store | |
| // it will be used by the rollkit migration command when using attesters | |
| seq := migrationData.Sequencer | |
| if err := k.Sequencer.Set(ctx, seq); err != nil { | |
| return nil, sdkerrors.ErrInvalidRequest.Wrapf("failed to set sequencer: %v", err) | |
| } | |
| return initialValUpdates, nil | |
| } | |
| func (k Keeper) migrateNow( | |
| ctx context.Context, | |
| migrationData types.RollkitMigration, | |
| lastValidatorSet []stakingtypes.Validator, | |
| ) (initialValUpdates []abci.ValidatorUpdate, err error) { | |
| // Validate migration data | |
| if migrationData.Sequencer.ConsensusPubkey == nil { | |
| return nil, sdkerrors.ErrInvalidRequest.Wrap("sequencer consensus public key cannot be nil") | |
| } | |
| switch len(migrationData.Attesters) { | |
| case 0: | |
| // no attesters, we are migrating to a single sequencer | |
| initialValUpdates, err = migrateToSequencer(migrationData, lastValidatorSet) | |
| if err != nil { | |
| return nil, sdkerrors.ErrInvalidRequest.Wrapf("failed to migrate to sequencer: %v", err) | |
| } | |
| default: | |
| // we are migrating the validator set to attesters | |
| initialValUpdates, err = migrateToAttesters(migrationData, lastValidatorSet) | |
| if err != nil { | |
| return nil, sdkerrors.ErrInvalidRequest.Wrapf("failed to migrate to sequencer & attesters: %v", err) | |
| } | |
| } | |
| // set new sequencer in the store | |
| // it will be used by the rollkit migration command when using attesters | |
| seq := migrationData.Sequencer | |
| if err := k.Sequencer.Set(ctx, seq); err != nil { | |
| return nil, sdkerrors.ErrInvalidRequest.Wrapf("failed to set sequencer: %v", err) | |
| } | |
| return initialValUpdates, nil | |
| } |
🤖 Prompt for AI Agents
In modules/rollkitmngr/keeper/migration.go around lines 18 to 46, the migrateNow
function lacks validation of the migrationData input, which could lead to
runtime errors. Add checks at the start of the function to validate critical
fields of migrationData, such as ensuring that required fields like Sequencer
and Attesters are not nil or empty and that their values meet expected
constraints. If validation fails, return an appropriate error before proceeding
with migration logic.
| // migrateToAttesters migrates the chain to attesters. | ||
| // the validator set is updated to include the attesters and remove all other validators. | ||
| func migrateToAttesters( | ||
| migrationData types.RollkitMigration, | ||
| lastValidatorSet []stakingtypes.Validator, | ||
| ) (initialValUpdates []abci.ValidatorUpdate, err error) { | ||
| attesters := migrationData.Attesters | ||
| for _, attester := range attesters { | ||
| pk, err := attester.TmConsPublicKey() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| attesterUpdate := abci.ValidatorUpdate{ | ||
| PubKey: pk, | ||
| Power: 1, | ||
| } | ||
|
|
||
| for _, val := range lastValidatorSet { | ||
| powerUpdate := val.ABCIValidatorUpdateZero() | ||
| if val.ConsensusPubkey.Equal(attester.ConsensusPubkey) { | ||
| continue | ||
| } | ||
| initialValUpdates = append(initialValUpdates, powerUpdate) | ||
| } | ||
|
|
||
| initialValUpdates = append(initialValUpdates, attesterUpdate) | ||
| } | ||
|
|
||
| return initialValUpdates, nil | ||
| } |
There was a problem hiding this comment.
Fix critical bug in migrateToAttesters function.
The nested loop logic creates duplicate validator removal updates. For each attester, the function iterates through all validators and adds zero-power updates for non-matching validators, resulting in the same validator being removed multiple times.
func migrateToAttesters(
migrationData types.RollkitMigration,
lastValidatorSet []stakingtypes.Validator,
) (initialValUpdates []abci.ValidatorUpdate, err error) {
+ // First, remove all existing validators that are not attesters
+ attesterPubKeys := make(map[string]bool)
+ for _, attester := range migrationData.Attesters {
+ key := attester.ConsensusPubkey.String()
+ attesterPubKeys[key] = true
+ }
+
+ // Remove validators that are not attesters
+ for _, val := range lastValidatorSet {
+ if !attesterPubKeys[val.ConsensusPubkey.String()] {
+ powerUpdate := val.ABCIValidatorUpdateZero()
+ initialValUpdates = append(initialValUpdates, powerUpdate)
+ }
+ }
+
+ // Add attesters with power 1
- attesters := migrationData.Attesters
- for _, attester := range attesters {
+ for _, attester := range migrationData.Attesters {
pk, err := attester.TmConsPublicKey()
if err != nil {
return nil, err
}
attesterUpdate := abci.ValidatorUpdate{
PubKey: pk,
Power: 1,
}
-
- for _, val := range lastValidatorSet {
- powerUpdate := val.ABCIValidatorUpdateZero()
- if val.ConsensusPubkey.Equal(attester.ConsensusPubkey) {
- continue
- }
- initialValUpdates = append(initialValUpdates, powerUpdate)
- }
-
initialValUpdates = append(initialValUpdates, attesterUpdate)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // migrateToAttesters migrates the chain to attesters. | |
| // the validator set is updated to include the attesters and remove all other validators. | |
| func migrateToAttesters( | |
| migrationData types.RollkitMigration, | |
| lastValidatorSet []stakingtypes.Validator, | |
| ) (initialValUpdates []abci.ValidatorUpdate, err error) { | |
| attesters := migrationData.Attesters | |
| for _, attester := range attesters { | |
| pk, err := attester.TmConsPublicKey() | |
| if err != nil { | |
| return nil, err | |
| } | |
| attesterUpdate := abci.ValidatorUpdate{ | |
| PubKey: pk, | |
| Power: 1, | |
| } | |
| for _, val := range lastValidatorSet { | |
| powerUpdate := val.ABCIValidatorUpdateZero() | |
| if val.ConsensusPubkey.Equal(attester.ConsensusPubkey) { | |
| continue | |
| } | |
| initialValUpdates = append(initialValUpdates, powerUpdate) | |
| } | |
| initialValUpdates = append(initialValUpdates, attesterUpdate) | |
| } | |
| return initialValUpdates, nil | |
| } | |
| // migrateToAttesters migrates the chain to attesters. | |
| // the validator set is updated to include the attesters and remove all other validators. | |
| func migrateToAttesters( | |
| migrationData types.RollkitMigration, | |
| lastValidatorSet []stakingtypes.Validator, | |
| ) (initialValUpdates []abci.ValidatorUpdate, err error) { | |
| // First, remove all existing validators that are not attesters | |
| attesterPubKeys := make(map[string]bool) | |
| for _, attester := range migrationData.Attesters { | |
| key := attester.ConsensusPubkey.String() | |
| attesterPubKeys[key] = true | |
| } | |
| // Remove validators that are not attesters | |
| for _, val := range lastValidatorSet { | |
| if !attesterPubKeys[val.ConsensusPubkey.String()] { | |
| powerUpdate := val.ABCIValidatorUpdateZero() | |
| initialValUpdates = append(initialValUpdates, powerUpdate) | |
| } | |
| } | |
| // Add attesters with power 1 | |
| for _, attester := range migrationData.Attesters { | |
| pk, err := attester.TmConsPublicKey() | |
| if err != nil { | |
| return nil, err | |
| } | |
| attesterUpdate := abci.ValidatorUpdate{ | |
| PubKey: pk, | |
| Power: 1, | |
| } | |
| initialValUpdates = append(initialValUpdates, attesterUpdate) | |
| } | |
| return initialValUpdates, nil | |
| } |
🤖 Prompt for AI Agents
In modules/rollkitmngr/keeper/migration.go between lines 76 and 105, the
migrateToAttesters function incorrectly nests a loop that appends zero-power
updates for non-matching validators inside the attesters loop, causing duplicate
removal updates. To fix this, separate the logic: first, iterate over
lastValidatorSet once to append zero-power updates for validators not in
attesters, then append updates for all attesters. This prevents multiple
removals of the same validator.
Overview
ref: #102
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores