feat: fill MsgUpdateClient data #17
Conversation
WalkthroughThe pull request introduces significant enhancements to the rollup synchronization process, focusing on improving data handling, client updates, and validator management. The changes span multiple files across the project, including modifications to synchronization logic, provider interfaces, and protocol buffer definitions. The updates introduce new methods for fetching validators, processing blockchain transactions, and defining message structures for Inter-Blockchain Communication (IBC) client updates. Changes
Sequence DiagramsequenceDiagram
participant RS as RollupSyncer
participant L1 as L1Provider
participant Block as Block
participant Tx as Transactions
RS->>Block: fillData(ctx, block)
Block->>Tx: Iterate through transactions
Tx-->>RS: Process MsgUpdateOracle
Tx-->>RS: Process MsgExec
Tx-->>RS: Process MsgUpdateClient
RS->>L1: getAllValidators(ctx, height)
L1-->>RS: Return validators
RS->>RS: Update ValidatorSet and TrustedValidators
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
rollupsync/oracle.go (1)
147-153: Clarify the use of magic number '2' in signature length check.The condition
len(signature.Signature) == 2in the loop may not be self-explanatory. Consider defining a constant with a meaningful name or adding a comment to explain why a length of '2' indicates that the signature needs to be filled.Example:
const placeholderSignatureLength = 2 // Indicates signature needs to be filled for sigIndex, signature := range tmHeader.SignedHeader.Commit.Signatures { if len(signature.Signature) == placeholderSignatureLength { // Fill signature ... } }proto/ibc/core/client/v1/client.proto (1)
18-19: Clean up or enable commented gogoproto options.The gogoproto options are currently commented out. Either:
- Remove them if they're not needed, or
- Enable them if they're required for the intended functionality
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
proto/buf.lockis excluded by!**/*.lockproto/ibc/core/client/v1/client.pb.gois excluded by!**/*.pb.goproto/ibc/core/client/v1/tx.pb.gois excluded by!**/*.pb.goproto/ibc/lightclients/tendermint/v1/tendermint.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (7)
proto/buf.yaml(1 hunks)proto/ibc/core/client/v1/client.proto(1 hunks)proto/ibc/core/client/v1/tx.proto(1 hunks)proto/ibc/lightclients/tendermint/v1/tendermint.proto(1 hunks)rollupsync/batch.go(1 hunks)rollupsync/oracle.go(3 hunks)rollupsync/provider/initia.go(2 hunks)
🧰 Additional context used
🪛 buf (1.47.2)
proto/ibc/lightclients/tendermint/v1/tendermint.proto
7-7: import "tendermint/types/validator.proto": file does not exist
(COMPILE)
proto/ibc/core/client/v1/client.proto
3-3: Files with package "ibc.core.client.v1" must be within a directory "ibc/core/client/v1" relative to root but were in directory "proto/ibc/core/client/v1".
(PACKAGE_DIRECTORY_MATCH)
proto/ibc/core/client/v1/tx.proto
7-7: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
🪛 golangci-lint (1.62.2)
rollupsync/oracle.go
9-9: import 'cosmossdk.io/api/cosmos/authz/v1beta1' is not allowed from list 'main'
(depguard)
14-14: ST1019: package "github.com/cometbft/cometbft/types" is being imported more than once
(stylecheck)
15-15: ST1019(related information): other import of "github.com/cometbft/cometbft/types"
(stylecheck)
🔇 Additional comments (6)
rollupsync/oracle.go (1)
9-9: Verify the import of 'cosmossdk.io/api/cosmos/authz/v1beta1'.The import on line 9 may not be allowed according to the project's dependency rules, as indicated by the static analysis tool (
depguard). Please confirm whether importing'cosmossdk.io/api/cosmos/authz/v1beta1'complies with the project's guidelines.🧰 Tools
🪛 golangci-lint (1.62.2)
9-9: import 'cosmossdk.io/api/cosmos/authz/v1beta1' is not allowed from list 'main'
(depguard)
rollupsync/batch.go (2)
229-229: Confirm the method name change from 'fillOracleData' to 'fillData'.The method
fillOracleDatahas been renamed tofillData. Ensure that all references tofillOracleDatahave been updated throughout the codebase to prevent undefined function errors.
229-229: Handle errors returned by 'fillData' method appropriately.Previously, errors from
fillOracleDatawere logged and the loop continued. Now, errors fromfillDatawill cause the function to return immediately. Verify if this change in error handling behavior is intentional and appropriate.Consider whether you want to continue processing subsequent blocks even if
fillDatafails for one block, or if the current behavior (exiting on error) is desired.proto/buf.yaml (1)
5-5: Verify cosmos-proto dependency availability.The addition of the cosmos-proto dependency is appropriate for the IBC message definitions.
proto/ibc/core/client/v1/client.proto (1)
17-25: LGTM! Well-documented Height message.The Height message is well-structured with clear documentation explaining the purpose of each field and the rationale behind the revision number/height design.
proto/ibc/lightclients/tendermint/v1/tendermint.proto (1)
24-30: LGTM! Well-structured Header message.The Header message is well-designed with:
- Clear documentation explaining the purpose and verification process
- Appropriate use of gogoproto options for embedding and nullable fields
- Logical field ordering and types
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rollupsync/oracle.go (1)
78-93: Enhance error messages for better debugging.Consider wrapping errors with more context to aid in debugging:
err := updateClientMsg.Unmarshal(anyMsg.Value) if err != nil { - return err + return errors.Join(errors.New("failed to unmarshal MsgUpdateClient"), err) } if updateClientMsg.ClientMessage.TypeUrl != "/ibc.lightclients.tendermint.v1.Header" { continue } err = tmHeader.Unmarshal(updateClientMsg.ClientMessage.Value) if err != nil { - return err + return errors.Join(errors.New("failed to unmarshal tendermint header"), err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
proto/ibc/core/client/v1/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (2)
proto/ibc/core/client/v1/tx.proto(1 hunks)rollupsync/oracle.go(3 hunks)
🧰 Additional context used
📓 Learnings (1)
rollupsync/oracle.go (2)
Learnt from: beer-1
PR: initia-labs/cometbft#17
File: rollupsync/oracle.go:14-15
Timestamp: 2025-01-22T06:18:19.334Z
Learning: In the CometBFT codebase, when importing `github.com/cometbft/cometbft/types` in the `rollupsync` package, use the `cmttypes` alias consistently to maintain uniformity with the rest of the package.
Learnt from: beer-1
PR: initia-labs/cometbft#17
File: rollupsync/oracle.go:14-15
Timestamp: 2025-01-22T06:18:19.334Z
Learning: Avoid duplicate imports of the same package with different aliases in Go files. Use a single consistent import with an appropriate alias if needed.
🪛 golangci-lint (1.62.2)
rollupsync/oracle.go
9-9: import 'cosmossdk.io/api/cosmos/authz/v1beta1' is not allowed from list 'main'
(depguard)
🪛 buf (1.47.2)
proto/ibc/core/client/v1/tx.proto
3-3: Files with package "ibc.core.client.v1" must be within a directory "ibc/core/client/v1" relative to root but were in directory "proto/ibc/core/client/v1".
(PACKAGE_DIRECTORY_MATCH)
🔇 Additional comments (7)
proto/ibc/core/client/v1/tx.proto (3)
1-5: LGTM!The package declaration and go_package option are correctly configured.
🧰 Tools
🪛 buf (1.47.2)
3-3: Files with package "ibc.core.client.v1" must be within a directory "ibc/core/client/v1" relative to root but were in directory "proto/ibc/core/client/v1".
(PACKAGE_DIRECTORY_MATCH)
20-21: LGTM!The empty response message is appropriate when no response data is needed.
23-26: LGTM!The CustomAny message implementation is appropriate based on the team's decision to use a custom implementation instead of google.protobuf.Any.
rollupsync/oracle.go (4)
197-228: LGTM!The helper functions are well-implemented with proper error handling, retries, and clean type conversions.
4-18: Verify the use of unauthorized import.The import
cosmossdk.io/api/cosmos/authz/v1beta1is flagged as unauthorized. Please verify if this import is allowed in your dependency configuration.🧰 Tools
🪛 golangci-lint (1.62.2)
9-9: import 'cosmossdk.io/api/cosmos/authz/v1beta1' is not allowed from list 'main'
(depguard)
95-114: 🛠️ Refactor suggestionAdd validation for proposer address comparison.
The proposer address comparison should include validation to prevent potential panics:
for _, val := range cmtValidators { - if bytes.Equal(val.Address, tmHeader.SignedHeader.Header.ProposerAddress) { + if val != nil && val.Address != nil && + tmHeader.SignedHeader != nil && + tmHeader.SignedHeader.Header != nil && + tmHeader.SignedHeader.Header.ProposerAddress != nil && + bytes.Equal(val.Address, tmHeader.SignedHeader.Header.ProposerAddress) { tmHeader.ValidatorSet.Proposer = val + break } }Likely invalid or redundant comment.
139-153:⚠️ Potential issueAdd bounds checking for signature index.
The signature index calculation needs bounds checking to prevent potential panic:
for sigIndex, signature := range tmHeader.SignedHeader.Commit.Signatures { if len(signature.Signature) == 2 { // fill signature blockSigIndex := int(signature.Signature[0]) + int(signature.Signature[1])<<8 + if blockSigIndex < 0 || blockSigIndex >= len(block.LastCommit.Signatures) { + return fmt.Errorf("invalid signature index: %d (max: %d)", + blockSigIndex, len(block.LastCommit.Signatures)-1) + } tmHeader.SignedHeader.Commit.Signatures[sigIndex] = *block.LastCommit.Signatures[blockSigIndex].ToProto() } }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
proto/ibc/core/client/v1/tx.proto(1 hunks)rollupsync/batch_data.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
rollupsync/batch_data.go
9-9: import 'cosmossdk.io/api/cosmos/authz/v1beta1' is not allowed from list 'main'
(depguard)
🪛 buf (1.47.2)
proto/ibc/core/client/v1/tx.proto
3-3: Files with package "ibc.core.client.v1" must be within a directory "ibc/core/client/v1" relative to root but were in directory "proto/ibc/core/client/v1".
(PACKAGE_DIRECTORY_MATCH)
🔇 Additional comments (2)
rollupsync/batch_data.go (1)
38-38: Verify compatibility of 'errors.Join' with Go versionThe
errors.Joinfunction is used to combine multiple errors at lines 38, 45, 65, 72, 77, 156, 161, and 169. This function was introduced in Go 1.20. Please confirm that the project is using Go 1.20 or later. If not, consider using alternative error wrapping methods likefmt.Errorfwith%worerrors.Wrapto maintain compatibility.Also applies to: 45-45, 65-65, 72-72, 77-77, 156-156, 161-161, 169-169
proto/ibc/core/client/v1/tx.proto (1)
21-24: Consider usinggoogle.protobuf.Anyinstead ofCustomAnyThe
CustomAnymessage duplicates the functionality ofgoogle.protobuf.Any, which is already imported. Unless there is a specific reason for this custom implementation, it is advisable to use the standardgoogle.protobuf.Anytype for better compatibility and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
rollupsync/batch_data.go (1)
9-9:⚠️ Potential issueRemove or replace the disallowed import.
The import of
cosmossdk.io/api/cosmos/authz/v1beta1is not allowed according to the project's dependency guidelines.🧰 Tools
🪛 golangci-lint (1.62.2)
9-9: import 'cosmossdk.io/api/cosmos/authz/v1beta1' is not allowed from list 'main'
(depguard)
🧹 Nitpick comments (5)
rollupsync/types/batch_info.go (1)
19-21: Add validation for chain type values.The function should validate the input to ensure it's a known chain type value.
Consider this implementation:
- // BatchChainTypeToString converts BatchInfo_ChainType to string batch chain type + // BatchChainTypeToString converts BatchInfo_ChainType to its string representation. + // Returns "UNKNOWN" for invalid chain types. - func BatchChainTypeToString(chainType ophostv1.BatchInfo_ChainType) string { - return chainType.String() + func BatchChainTypeToString(chainType ophostv1.BatchInfo_ChainType) string { + if _, ok := ophostv1.BatchInfo_ChainType_name[int32(chainType)]; !ok { + return "UNKNOWN" + } + return chainType.String() + }proto/tmibc/core/client/v1/tx.proto (1)
21-24: Consider using google.protobuf.Any instead of CustomAny.The
CustomAnymessage appears to duplicate the functionality ofgoogle.protobuf.Any. Unless there's a specific reason for a custom implementation, using the standardgoogle.protobuf.Anywould be more maintainable.-message CustomAny { - string type_url = 1; - bytes value = 2; -} +import "google/protobuf/any.proto"; + // Update the field in MsgUpdateClient - CustomAny client_message = 2; + google.protobuf.Any client_message = 2;proto/tmibc/core/client/v1/client.proto (1)
18-19: Remove or uncomment the gogoproto options.The commented out gogoproto options should either be removed if not needed or uncommented if they are required.
- // option (gogoproto.goproto_getters) = false; - // option (gogoproto.goproto_stringer) = false; + option (gogoproto.goproto_getters) = false; + option (gogoproto.goproto_stringer) = false;rollupsync/batch_data.go (2)
176-195: Enhance retry mechanism with max attempts limit.The retry mechanism in
fetchOracleTxcontinues indefinitely until context cancellation. Consider adding a maximum retry limit to prevent infinite loops.func (rs *RollupSyncer) fetchOracleTx(ctx context.Context, height int64) ([]byte, error) { ticker := time.NewTicker(time.Duration(rs.cfg.FetchInterval) * time.Millisecond) defer ticker.Stop() retry := 0 + maxRetries := 10 // or from config for { select { case <-ctx.Done(): return nil, ctx.Err() case <-ticker.C: + if retry >= maxRetries { + return nil, fmt.Errorf("max retries (%d) exceeded fetching oracle tx at height %d", maxRetries, height) + } oracleTx, err := rs.l1Provider.GetOracleTx(ctx, height) if err != nil { rs.logger.Error("failed to fetch oracle tx", "height", height, "retry", retry, "error", err.Error())
20-174: Refactor the message handling switch case into separate methods.The
fillDatamethod is quite long and handles multiple message types. Consider breaking it down into separate methods for each message type to improve readability and maintainability.Example refactor:
func (rs *RollupSyncer) fillData(ctx context.Context, block *types.Block) error { for i, txBytes := range block.Txs { raw, body, err := provider.UnmarshalCosmosTx(txBytes) if err != nil { return err } for _, anyMsg := range body.Messages { - switch anyMsg.TypeUrl { - case "/opinit.opchild.v1.MsgUpdateOracle": - // ... oracle handling ... - case "/cosmos.authz.v1beta1.MsgExec": - // ... authz handling ... - case "/ibc.core.client.v1.MsgUpdateClient": - // ... client update handling ... - default: - continue - } + if err := rs.handleMessage(ctx, anyMsg); err != nil { + return err + } } // ... rest of the method ... } } + +func (rs *RollupSyncer) handleMessage(ctx context.Context, anyMsg *anypb.Any) error { + switch anyMsg.TypeUrl { + case "/opinit.opchild.v1.MsgUpdateOracle": + return rs.handleOracleUpdate(ctx, anyMsg) + case "/cosmos.authz.v1beta1.MsgExec": + return rs.handleAuthzExec(ctx, anyMsg) + case "/ibc.core.client.v1.MsgUpdateClient": + return rs.handleClientUpdate(ctx, anyMsg) + default: + return nil + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
proto/tmibc/core/client/v1/client.pb.gois excluded by!**/*.pb.goproto/tmibc/core/client/v1/tx.pb.gois excluded by!**/*.pb.goproto/tmibc/lightclients/tendermint/v1/tendermint.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (5)
proto/tmibc/core/client/v1/client.proto(1 hunks)proto/tmibc/core/client/v1/tx.proto(1 hunks)proto/tmibc/lightclients/tendermint/v1/tendermint.proto(1 hunks)rollupsync/batch_data.go(1 hunks)rollupsync/types/batch_info.go(1 hunks)
🧰 Additional context used
🪛 buf (1.47.2)
proto/tmibc/lightclients/tendermint/v1/tendermint.proto
7-7: import "tendermint/types/validator.proto": file does not exist
(COMPILE)
proto/tmibc/core/client/v1/client.proto
3-3: Files with package "tmibc.core.client.v1" must be within a directory "tmibc/core/client/v1" relative to root but were in directory "proto/tmibc/core/client/v1".
(PACKAGE_DIRECTORY_MATCH)
proto/tmibc/core/client/v1/tx.proto
3-3: Files with package "tmibc.core.client.v1" must be within a directory "tmibc/core/client/v1" relative to root but were in directory "proto/tmibc/core/client/v1".
(PACKAGE_DIRECTORY_MATCH)
🪛 golangci-lint (1.62.2)
rollupsync/batch_data.go
9-9: import 'cosmossdk.io/api/cosmos/authz/v1beta1' is not allowed from list 'main'
(depguard)
🔇 Additional comments (3)
rollupsync/types/batch_info.go (1)
14-21: Verify the impact of chain type format changes.The removal of the chain type prefix and changes to these functions might affect:
- Stored data that uses the old format
- API consumers expecting the old format
- Callers that don't handle potential "UNKNOWN" values
Run this script to analyze the impact:
✅ Verification successful
The chain type format changes are safe to proceed with.
The string values from these functions are only used internally for:
- Logging messages (human readable)
- Case-insensitive config matching
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of these functions and potential format dependencies echo "Finding direct function calls..." rg -l "BatchChainTypeFromString|BatchChainTypeToString" echo "Finding potential chain type string literals..." rg -l '"CHAIN_TYPE_|chain_type_' echo "Finding potential API endpoints handling chain types..." rg -l 'ChainType.*json:|chainType.*json:'Length of output: 517
Script:
#!/bin/bash # Get context around the usage of these functions echo "=== Usage in batch.go ===" rg -C 3 "BatchChainTypeFromString|BatchChainTypeToString" rollupsync/batch.go echo -e "\n=== Usage in provider/batch.go ===" rg -C 3 "BatchChainTypeFromString|BatchChainTypeToString" rollupsync/provider/batch.go echo -e "\n=== Looking for related test files ===" fd -g "*test.go" -x rg -l "BatchChainType" echo -e "\n=== Looking for chain type related constants ===" rg -i "const.*chain.*type|type.*chain|iota.*chain"Length of output: 46596
proto/tmibc/core/client/v1/tx.proto (1)
1-5: Adjust the package directory structure to match the proto package.The package
tmibc.core.client.v1should be within a directorytmibc/core/client/v1relative to root, but it's currently inproto/tmibc/core/client/v1.Run this command to verify the package structure:
🧰 Tools
🪛 buf (1.47.2)
3-3: Files with package "tmibc.core.client.v1" must be within a directory "tmibc/core/client/v1" relative to root but were in directory "proto/tmibc/core/client/v1".
(PACKAGE_DIRECTORY_MATCH)
proto/tmibc/lightclients/tendermint/v1/tendermint.proto (1)
7-7:⚠️ Potential issueFix the missing import for validator.proto.
The import "tendermint/types/validator.proto" is missing. This will cause compilation failures.
Run this command to verify the import path:
🧰 Tools
🪛 buf (1.47.2)
7-7: import "tendermint/types/validator.proto": file does not exist
(COMPILE)
Summary by CodeRabbit
New Features
Improvements
Technical Updates