Feat/fetch create bridgetx logic #63
Conversation
WalkthroughThe pull request introduces modifications across multiple files in a blockchain-related project, focusing on enhancing data processing and height determination logic. Changes include refining the Changes
Sequence DiagramsequenceDiagram
participant Executor
participant Host
participant BatchSubmitter
Executor->>Host: Query Bridge Start Height
Host-->>Executor: Return Start Height
BatchSubmitter->>BatchSubmitter: Empty Oracle Data
BatchSubmitter->>BatchSubmitter: Empty Update Client Data
BatchSubmitter->>BatchSubmitter: Encode Modified Transactions
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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: 3
🔭 Outside diff range comments (1)
executor/batchsubmitter/data_conversion.go (1)
Line range hint
389-395: Check for Signature Reconstruction ErrorsWhen reconstructing signatures in the IBC client update messages, ensure that the indexing and assignment are correct to prevent panics due to out-of-range errors. Also, verify that all necessary signatures are properly filled.
Consider adding checks to ensure that
blockSigIndexis within bounds before accessingblock.Block.LastCommit.Signatures[blockSigIndex].Apply this diff to add boundary checks:
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.Block.LastCommit.Signatures) { + return fmt.Errorf("blockSigIndex out of range: %d", blockSigIndex) + } tmHeader.SignedHeader.Commit.Signatures[sigIndex] = *block.Block.LastCommit.Signatures[blockSigIndex].ToProto() } }
🧹 Nitpick comments (11)
executor/batchsubmitter/data_conversion.go (2)
50-52: Avoid Hardcoding TypeUrlsUsing hardcoded
TypeUrlstrings reduces flexibility and can lead to maintenance issues if the type URLs change in the future. Consider defining constants for theTypeUrlvalues to improve maintainability.Apply this diff to define constants for
TypeUrl:+const MsgUpdateOracleTypeUrl = "/opinit.opchild.v1.MsgUpdateOracle" ... switch msg := msgs[0].(type) { case *opchildtypes.MsgUpdateOracle: msg.Data = []byte{} case *authz.MsgExec: if len(msg.Msgs) != 1 || msg.Msgs[0].TypeUrl != MsgUpdateOracleTypeUrl { return pbb, nil } ... + oracleMsg := &opchildtypes.MsgUpdateOracle{} err = bs.node.Codec().UnpackAny(msg.Msgs[0], &oracleMsg)
122-124: Log Mismatched Chain IDs for Better DebuggingCurrently, when the
ChainIDdoes not match, the function simply continues without any logging. To aid in debugging, consider logging a warning message when theChainIDdoes not match.Apply this diff to add a log statement:
if clientMsg.SignedHeader.Header.ChainID != bs.host.ChainId() { + bs.logger.Warn("ChainID mismatch", "expected", bs.host.ChainId(), "actual", clientMsg.SignedHeader.Header.ChainID) continue }e2e/batch_reconstruction_test.go (2)
425-442: Handle Potential Pagination Issues in Validator RetrievalThe
getAllValidatorsfunction retrieves validators using pagination. Ensure that the pagination logic correctly handles cases where the total number of validators is not a multiple of theperPagevalue, and that all validators are retrieved without duplication or omission.Consider adding a check to prevent infinite loops and handle edge cases where
result.Totalmight change between pages.
444-459: Optimize Validator Conversion FunctionIn
toCmtProtoValidators, the function assignsProposerPriorityto zero for each validator. Verify whether this is intentional and if settingProposerPriorityis necessary for the correctness of the validator set.If appropriate, include comments explaining why
ProposerPriorityis set to zero for clarity.executor/batchsubmitter/common_test.go (1)
6-14: Improve import grouping for better readability.Consider grouping imports in the following order:
- Standard library
- Third-party packages
- Internal packages
Apply this diff to reorganize imports:
import ( "context" + "github.com/pkg/errors" + "go.uber.org/zap" + "go.uber.org/zap/zaptest/observer" + coretypes "github.com/cometbft/cometbft/rpc/core/types" cmttypes "github.com/cometbft/cometbft/types" + "github.com/cosmos/cosmos-sdk/codec" + sdk "github.com/cosmos/cosmos-sdk/types" + ophosttypes "github.com/initia-labs/OPinit/x/ophost/types" executortypes "github.com/initia-labs/opinit-bots/executor/types" btypes "github.com/initia-labs/opinit-bots/node/broadcaster/types" nodetypes "github.com/initia-labs/opinit-bots/node/types" "github.com/initia-labs/opinit-bots/types" - "github.com/pkg/errors" - "go.uber.org/zap" - "go.uber.org/zap/zaptest/observer" - - "github.com/cosmos/cosmos-sdk/codec" - sdk "github.com/cosmos/cosmos-sdk/types" )e2e/reconnect_node_test.go (1)
57-57: LGTM! Consider expanding test coverage.The addition of
ibc.CosmosRlyparameter aligns with the enhanced relayer implementation flexibility. The test thoroughly covers chain pause/resume scenarios.Consider adding test cases for:
- Different relayer implementations
- Edge cases like network partitions
- Concurrent chain pauses/resumes
executor/batchsubmitter/batch_submitter.go (1)
24-24: Follow Go naming conventions for ID.The method name
ChainIdshould beChainIDto follow Go's standard capitalization for "ID".-ChainId() string +ChainID() stringprovider/host/host.go (1)
119-121: Follow Go naming conventions for ID.The method name
ChainIdshould beChainIDto follow Go's standard capitalization for "ID".-func (b BaseHost) ChainId() string { +func (b BaseHost) ChainID() string { return b.cfg.ChainID }provider/host/query.go (1)
200-202: Enhance error handling and context management.While the implementation is correct, consider these improvements:
- Use
rpcclient.GetQueryContext()for consistent timeout management- Wrap errors for better context
func (b BaseHost) QueryBlock(ctx context.Context, height int64) (*coretypes.ResultBlock, error) { - return b.node.GetRPCClient().Block(ctx, &height) + ctx, cancel := rpcclient.GetQueryContext(ctx, 0) + defer cancel() + + block, err := b.node.GetRPCClient().Block(ctx, &height) + if err != nil { + return nil, errors.Wrap(err, "failed to query block") + } + return block, nil }executor/batchsubmitter/data_conversion_test.go (2)
28-176: LGTM! Well-structured test cases for oracle data handling.The test cases provide good coverage for various scenarios including empty transactions, multiple messages, and authz messages. The test structure is clear and maintainable.
Consider adding comments to explain the purpose of each test case, especially for complex scenarios like "different signatures" and "authz tx with another msg".
178-598: LGTM! Comprehensive test coverage for client data updates.The test cases thoroughly verify the client data update functionality with different signature scenarios. The mock setup and validation logic are well-implemented.
Consider extracting common test setup code (like block creation) into helper functions to reduce duplication between test cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
challenger/challenger.go(1 hunks)challenger/types/config.go(2 hunks)e2e/batch_reconstruction_test.go(9 hunks)e2e/docker.go(1 hunks)e2e/go.mod(1 hunks)e2e/helper.go(8 hunks)e2e/multiple_txs_test.go(1 hunks)e2e/reconnect_node_test.go(1 hunks)executor/batchsubmitter/batch_submitter.go(2 hunks)executor/batchsubmitter/common_test.go(3 hunks)executor/batchsubmitter/data_conversion.go(1 hunks)executor/batchsubmitter/data_conversion_test.go(1 hunks)executor/batchsubmitter/handler.go(1 hunks)executor/batchsubmitter/oracle.go(0 hunks)executor/batchsubmitter/oracle_test.go(0 hunks)executor/executor.go(1 hunks)executor/types/config.go(3 hunks)go.mod(1 hunks)node/types/config.go(2 hunks)provider/child/child.go(2 hunks)provider/host/host.go(1 hunks)provider/host/query.go(2 hunks)
💤 Files with no reviewable changes (2)
- executor/batchsubmitter/oracle_test.go
- executor/batchsubmitter/oracle.go
🔇 Additional comments (23)
executor/batchsubmitter/data_conversion.go (1)
146-150: Ensure Safe Integer OperationsThe code assumes that the validator set length is less than 65,536 and uses a 16-bit representation for the validator index. If the validator set grows beyond this size, it will cause errors. Ensure that this assumption holds true or consider using a larger data type to prevent potential overflows.
Please confirm that the validator set will not exceed 65,536 entries. If it's possible, adjust the code to handle larger validator sets.
e2e/batch_reconstruction_test.go (3)
Line range hint
40-54: Assess Impact of Reduced Trusting PeriodThe
TrustingPeriodhas been reduced from "168h" (7 days) to "1h" (1 hour) for bothl1ChainConfigandl2ChainConfig. A shorter trusting period may lead to frequent trust expiration in tests that have delays, potentially causing test flakiness.Please confirm that this reduction won't affect the stability of the tests and that all test cases complete within the new trusting period.
70-87: Ensure Test Coverage for New Relayer ImplementationsNew test cases have been added for "initia with go relayer" and "initia with hermes relayer". Ensure that these tests provide adequate coverage and that any differences between the relayer implementations are accounted for in the tests.
Good addition to increase test coverage with different relayer implementations.
Line range hint
259-405: Validate Expanded Data Filling LogicThe
fillDatafunction has been expanded to handle messages of type/ibc.core.client.v1.MsgUpdateClient. While this enhances functionality, ensure that:
- All necessary fields are accurately reconstructed.
- Potential errors during unmarshalling and marshalling are properly handled.
- The logic for filling in validator sets and commit signatures is correct.
Consider adding additional test cases to cover the new logic and verify correctness.
e2e/helper.go (4)
123-124: Register IBC Client Interfaces in Encoding ConfigurationThe addition of
ibcclienttypes.RegisterInterfacesandibctmlightclients.RegisterInterfacesin theMinitiaEncodingfunction ensures that the IBC client types are properly registered. This is necessary for the correct encoding and decoding of IBC messages.Good job updating the encoding configuration to include the necessary IBC interfaces.
137-137: Update Function Signature to Accept Relayer ImplementationThe
SetupTestfunction signature now includes therelayerImpl ibc.RelayerImplementationparameter, allowing for different relayer implementations. Ensure that all calls toSetupTestare updated accordingly and that the function handles the new parameter correctly.The change enhances flexibility in testing with different relayer implementations.
Line range hint
391-404: Set Trusting Period in Client Creation OptionsIn the
InterchainLink, theCreateClientOptsnow includesTrustingPeriodfrominitia.Config().TrustingPeriod. This ensures that the IBC clients are created with the correct trusting period. Verify that the trusting period aligns with the chain configurations and that it doesn't introduce any issues in client updates.Including the trusting period enhances consistency across the test setup.
Line range hint
568-586: Ensure Correct RPC Addresses for NodesThe RPC addresses for
L1Node,L2Node, andDANodeconfigurations useHostName()to obtain the correct hostname. Verify thatHostName()returns the expected values and that the nodes are accessible at the specified addresses.Confirm that the nodes are reachable at the constructed RPC URLs to prevent connection issues.
node/types/config.go (2)
18-20: LGTM! Well-documented field addition.The ChainID field is properly documented and essential for blockchain identification.
35-38: LGTM! Proper validation added.The validation check for empty ChainID is correctly implemented.
executor/batchsubmitter/common_test.go (2)
41-48: LGTM! Well-implemented mock method.The QueryBlock implementation properly handles both success and error cases.
50-56: LGTM! Clean and simple mock implementations.The SetBlock and ChainId methods are correctly implemented.
challenger/types/config.go (1)
116-116: LGTM! Consistent configuration updates.The ChainID field is properly propagated in both L1NodeConfig and L2NodeConfig methods.
Also applies to: 126-126
executor/batchsubmitter/batch_submitter.go (1)
26-26: LGTM! Block querying capability added.The addition of
QueryBlockmethod enhances the interface with block querying capabilities.e2e/multiple_txs_test.go (1)
62-62: LGTM!The addition of
ibc.CosmosRlyparameter toSetupTestaligns with similar updates in other test files, maintaining consistency across the test suite.challenger/challenger.go (1)
287-295: LGTM!The restructured logic for determining
l1StartHeightis more efficient. The bridge start height is now only queried whenl1StartHeightis zero, avoiding unnecessary queries.executor/types/config.go (1)
206-206: LGTM!The addition of
ChainIDfield to node configurations ensures proper chain identification across all node types and aligns with the validation requirements.Also applies to: 227-227, 248-248
provider/child/child.go (1)
18-19: LGTM!The addition of IBC client types and Tendermint light client support enhances the codec's capability to handle IBC-related functionalities.
Also applies to: 97-98
executor/executor.go (1)
235-242: Verify edge cases in the modified height determination logic.The simplified logic for querying bridge start height improves code clarity. However, please ensure this change doesn't affect edge cases where
DisableAutoSetL1Heightwas previously preventing automatic height determination.Run this script to verify the usage of
DisableAutoSetL1Heightacross the codebase:✅ Verification successful
The modified height determination logic is safe and improves code clarity
The change actually enhances the code by:
- Simplifying the logic while maintaining equivalent behavior
- Using bridge-specific height when needed (l1StartHeight=0)
- Respecting explicitly configured heights (l1StartHeight!=0)
The removal of
DisableAutoSetL1Heightcheck in this context is safe as other components (challenger) maintain their own separate logic for height determination.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other usages of DisableAutoSetL1Height to ensure the change doesn't break existing functionality rg -A 5 "DisableAutoSetL1Height"Length of output: 4665
e2e/docker.go (1)
497-497: LGTM! Improved container name handling.Using
dockerutil.CondenseHostNameis a good improvement as it ensures proper handling of container name length limits and invalid characters.go.mod (1)
16-16: LGTM! Proper dependency declaration.Moving
github.com/cosmos/ibc-go/v8from indirect to direct dependencies correctly reflects its usage in the codebase.e2e/go.mod (2)
104-104: Verify IBC-Go version security status.Please verify that version v8.5.0 of ibc-go is free from known security vulnerabilities.
✅ Verification successful
IBC-Go v8.5.0 is free from known security vulnerabilities
The version v8.5.0 is not affected by any known security vulnerabilities. It's well above the versions affected by the two known vulnerabilities (affecting <=1.5.0 and <4.6.0 respectively).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for security advisories for ibc-go gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/cosmos/ibc-go") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 798
Line range hint
3-3: Verify Go version compatibility.The specified Go version 1.22.5 is newer than the current stable release (1.22.0 as of January 2025). This might cause compatibility issues in different environments.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary by CodeRabbit
Based on the provided raw summary, here are the release notes:
New Features
Dependency Updates
Improvements