Feat/tx update batch info #74
Conversation
WalkthroughThis pull request adds functionality to update batch information and standardizes chain type constants across modules. A new command ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TxCmd as txUpdateBatchInfoCmd
participant Validator as validateBatchInfoArgs
participant ConfigLoader as getExecutorConfig
participant BridgeQuerier as QueryBridgeId
participant Proposer as l1ProposerAccount
participant Broadcaster as l2BroadcasterAccount
User->>TxCmd: Invoke update batch info command
TxCmd->>Validator: Validate chain type and submitter address
Validator-->>TxCmd: Return validation result
TxCmd->>ConfigLoader: Load executor configuration
ConfigLoader-->>TxCmd: Config loaded
TxCmd->>BridgeQuerier: Query bridge ID
BridgeQuerier-->>TxCmd: Bridge ID received
TxCmd->>Proposer: Retrieve proposer account
Proposer-->>TxCmd: Proposer account returned
TxCmd->>Broadcaster: Submit batch info update transaction
Broadcaster-->>TxCmd: Transaction hash returned
TxCmd-->>User: Return transaction hash
sequenceDiagram
participant Client
participant BaseHost as Provider Host
participant Ticker
Client->>BaseHost: QueryBatchInfos (with context)
loop While more pages exist
BaseHost->>Ticker: Wait for polling interval
Ticker-->>BaseHost: Tick
BaseHost->>BaseHost: Fetch next page of batch infos
end
BaseHost-->>Client: Return accumulated batch info list
Poem
Tip 🌐 Web search-backed reviews and chat
✨ 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: 0
🔭 Outside diff range comments (1)
go.mod (1)
185-187: Review GRPC Replace Directive.
The replace block forcesgoogle.golang.org/grpcto v1.65.0 despite the require block specifying v1.67.1. Please reconcile this discrepancy to avoid potential runtime conflicts.
🧹 Nitpick comments (7)
provider/host/query.go (1)
144-177: Consider immediate page fetching rather than polling with a Ticker
Using a Ticker to fetch subsequent pages might delay retrieval of complete data, especially when a large number of pages is expected. If you want faster pagination, replace the interval-based approach with immediate consecutive requests. Also be mindful of potential memory usage if the result set is large and stored in a single slice.cmd/opinitd/tx.go (1)
120-197: Missing retry mechanism for transaction broadcast
The new command effectively builds and sends a transaction to update batch info. However, if the broadcast fails (line 183), there's a TODO to handle errors. You may want to implement an exponential backoff or a manual retry prompt. Also consider providing an example usage in the command's Long description to guide users.e2e/da_chain.go (1)
70-70: Consider refactoring pagination logic.Both
QueryInitiaBatchDataandQueryCelestiaBatchDatamethods use similar pagination logic. Consider extracting this into a shared helper function to improve maintainability.+func (da *DAChain) paginateResults(ctx context.Context, query string, processPage func(txsResult *coretypes.ResultTxSearch) error) error { + page := 1 + perPage := 100 + + for { + txsResult, err := da.GetFullNode().Client.TxSearch(ctx, query, false, &page, &perPage, "asc") + if err != nil { + return err + } + + if err := processPage(txsResult); err != nil { + return err + } + + if txsResult.TotalCount <= page*100 { + break + } + page++ + } + return nil +}Also applies to: 115-115
e2e/reconnect_node_test.go (1)
67-67: Consider parameterizing wait times.The test uses hardcoded sleep durations. Consider extracting these as configurable parameters to make the test more flexible and maintainable.
+const ( + pauseWaitTime = 10 * time.Second + resumeWaitTime = 20 * time.Second +)Also applies to: 73-73, 90-90, 96-96, 114-114, 122-122
e2e/update_batch_info_test.go (2)
74-75: Consider extracting magic numbers into named constants.The test uses magic numbers for block waits (20, 3, 2). Consider extracting these into named constants to improve readability and maintainability.
+const ( + longBlockWait = 20 + shortBlockWait = 3 + minBlockWait = 2 +) -err := testutil.WaitForBlocks(ctx, 20, op.Initia, op.Minitia) +err := testutil.WaitForBlocks(ctx, longBlockWait, op.Initia, op.Minitia)Also applies to: 104-105, 127-128, 142-143, 156-157
116-117: Consider extracting timeout duration into a named constant.The timeout duration of 30 seconds should be extracted into a named constant for better maintainability.
+const opBotStopTimeout = 30 * time.Second -err = op.OP.WaitForStop(ctx, 30*time.Second) +err = op.OP.WaitForStop(ctx, opBotStopTimeout)e2e/l1_chain.go (1)
146-166: Consider making the page size configurable.The implementation uses a hardcoded page size of 100. Consider making this configurable for better flexibility.
+const defaultBatchInfoPageSize = 100 func (l1 *L1Chain) QueryBatchInfos(ctx context.Context, bridgeId uint64) ([]ophosttypes.BatchInfoWithOutput, error) { client := ophosttypes.NewQueryClient(l1.GetFullNode().GrpcConn) var batchInfos []ophosttypes.BatchInfoWithOutput var nextKey []byte for { res, err := client.BatchInfos(ctx, &ophosttypes.QueryBatchInfosRequest{BridgeId: bridgeId, Pagination: &query.PageRequest{ - Limit: 100, + Limit: defaultBatchInfoPageSize, Key: nextKey, }})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
e2e/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sum
📒 Files selected for processing (25)
cmd/opinitd/tx.go(6 hunks)e2e/batch_reconstruction_test.go(2 hunks)e2e/da_chain.go(2 hunks)e2e/docker.go(2 hunks)e2e/go.mod(6 hunks)e2e/helper.go(3 hunks)e2e/l1_chain.go(2 hunks)e2e/multiple_txs_test.go(1 hunks)e2e/opbot.go(2 hunks)e2e/reconnect_node_test.go(1 hunks)e2e/update_batch_info_test.go(1 hunks)executor/batchsubmitter/batch_info.go(1 hunks)executor/batchsubmitter/batch_info_test.go(5 hunks)executor/batchsubmitter/batch_submitter.go(2 hunks)executor/batchsubmitter/batch_test.go(4 hunks)executor/batchsubmitter/common_test.go(1 hunks)executor/batchsubmitter/handler_test.go(1 hunks)executor/executor.go(2 hunks)executor/host/batch_test.go(3 hunks)go.mod(7 hunks)node/broadcaster/broadcaster.go(1 hunks)provider/host/parse.go(1 hunks)provider/host/parse_test.go(1 hunks)provider/host/query.go(1 hunks)provider/host/testutil.go(1 hunks)
🔇 Additional comments (55)
cmd/opinitd/tx.go (10)
7-7: No concerns for this import statement.
27-28: Imports for opchildtypes and ophosttypes
No concerns; these imports align with the usage in the code below.
42-42: New subcommand integrated
The registration of the new "update-batch-info" subcommand looks straightforward.
62-66: Loading executor config
This change to load the executor configuration here is consistent and logically placed.
67-67: Retrieving L2 broadcaster account
No issues found. This call aligns with the new function in the codebase.
199-210: Validation function is well-defined
This function neatly checks the chain type and bech32 prefix. No issues noted.
212-225: Executor config loading
The updated logic properly loads and unmarshals the executor config from JSON. Looks good.
227-245: Query for bridge ID
The function is straightforward, fetching the child chain's bridge info. Ensure the potential "bridge not found" scenario is handled as needed upstream.
247-270: Correct retrieval of L1 proposer
This function properly fetches the L1 bridge info and addresses. No issues identified.
272-272: New L2 broadcaster account function
This function is consistent with the pattern in “l1ProposerAccount” and reuses broadcaster logic.executor/batchsubmitter/batch_info.go (1)
24-24: Verify chain type validity
When usingophosttypes.BatchInfo_ChainType_value[chain], ifchainis not a valid key, it defaults to 0. Consider adding validation to avoid silent fallback.executor/batchsubmitter/common_test.go (1)
35-36: LGTM! Clean implementation of the updated interface.The simplified implementation correctly returns the batch infos directly, aligning with the interface changes.
executor/batchsubmitter/batch_info_test.go (1)
40-40: LGTM! Consistent updates to chain type constants.The chain type constants have been consistently updated across all test cases while maintaining the original test logic.
Also applies to: 55-55, 64-64, 79-79, 94-94
e2e/da_chain.go (1)
61-64: LGTM! Chain type constants updated consistently.The switch statement correctly uses the new chain type constants while maintaining the original logic.
e2e/reconnect_node_test.go (1)
44-44: LGTM! Chain type constant updated consistently.The chain type constant in DAChainConfig has been updated to use the new format.
provider/host/testutil.go (1)
53-53: LGTM!The change to use
String()instead ofStringWithoutPrefix()aligns with the standardization of chain type constants across the codebase.executor/batchsubmitter/batch_submitter.go (2)
24-25: LGTM!The interface change simplifies the return type by directly returning the slice of batch infos, reducing pointer dereferencing and improving error handling.
106-106: LGTM!The initialization logic is correctly updated to handle the new return type from
QueryBatchInfos.e2e/update_batch_info_test.go (1)
16-162: LGTM! Comprehensive test coverage.The test thoroughly validates the batch info update functionality:
- Tests configuration setup
- Verifies error handling for premature updates
- Validates chain type and submitter address changes
- Ensures proper operation resumption
- Verifies batch data integrity
e2e/l1_chain.go (1)
146-166: LGTM! Well-implemented pagination.The implementation correctly handles pagination to fetch all batch infos:
- Properly accumulates results
- Handles pagination keys
- Good error handling
executor/host/batch_test.go (1)
145-145: LGTM! Chain type constant updated.The change to use
BatchInfo_INITIAaligns with the standardization of chain type constants across the codebase.executor/batchsubmitter/handler_test.go (1)
74-74: LGTM! Chain type constant updated.The change to use
BatchInfo_INITIAaligns with the standardization of chain type constants across the codebase.e2e/multiple_txs_test.go (1)
49-49: LGTM! Chain type constant updated.The change to use
BatchInfo_INITIAaligns with the standardization of chain type constants across the codebase.provider/host/parse.go (1)
66-66: LGTM! Chain type validation updated.The change to use
String()method for chain type validation aligns with the standardization of chain type constants across the codebase.e2e/opbot.go (2)
71-92: LGTM! Well-implemented timeout mechanism.The implementation correctly handles:
- Timeout with proper timer usage
- Context cancellation
- Resource cleanup with defer statements
- Container lifecycle checks
236-241: LGTM! Clean command construction.The command construction is straightforward and follows the established pattern.
executor/executor.go (1)
154-170: LGTM! Consistent chain type constant updates.The changes align with the standardization of chain type constants across the codebase, simplifying the naming convention while maintaining the same functionality.
node/broadcaster/broadcaster.go (1)
258-263: LGTM! Simplified message processing logic.The implementation has been streamlined by directly creating ProcessedMsgs instances instead of using an intermediate map, reducing complexity while maintaining functionality.
provider/host/parse_test.go (1)
49-49: LGTM! Updated test to use standardized chain type constant.The test has been correctly updated to use the new chain type constant, maintaining consistency with the broader changes across the codebase.
e2e/batch_reconstruction_test.go (1)
76-76: LGTM! Chain type constant updates are consistent.The changes simplify the chain type constants by removing the redundant
CHAIN_TYPE_prefix while maintaining the same functionality.Also applies to: 84-84, 104-104
e2e/docker.go (3)
337-357: Return type enhancement improves transaction tracking.The method now returns the transaction hash along with any error, which is useful for transaction tracking and verification.
359-379: New UpdateBatchInfo method follows good practices.The implementation:
- Has proper timeout handling
- Follows consistent error handling pattern
- Returns transaction hash for tracking
557-557: Interface update maintains backward compatibility.The OPBotCommander interface is correctly updated to include the new UpdateBatchInfo method.
e2e/helper.go (3)
346-346: Chain type constant update maintains consistency.The condition update aligns with the new naming convention used across the codebase.
486-493: Oracle permissions block adds important validation.The new block:
- Correctly checks both conditions (OracleEnabled and BotExecutor)
- Includes proper error handling
- Waits for block confirmation
645-650: ChangeDA method provides clean DA chain updates.The method:
- Preserves the old DA chain for potential rollback
- Updates configuration appropriately
- Returns the old chain for cleanup
executor/batchsubmitter/batch_test.go (1)
56-56: Test cases updated consistently with new chain type constants.All test cases have been updated to use the simplified chain type constants, maintaining test coverage while aligning with the new naming convention.
Also applies to: 83-83, 110-110, 116-116, 146-146, 152-152
go.mod (14)
10-10: Upgrade Dependency:cosmossdk.io/x/txto v0.13.7.
The version update on line 10 aligns with the recent dependency upgrades and should enable new tx functionalities.
13-13: Upgrade Dependency:github.com/cosmos/cosmos-sdkto v0.50.11.
This update should bring in the latest fixes and features from the Cosmos SDK. Please verify that no breaking changes affect current functionality.
18-18: Upgrade Dependency:github.com/initia-labs/OPinitto v0.6.2.
Ensure that all modules consuming OPinit (including any API interfaces) are compatible with this version update.
33-33: Upgrade Dependency:cosmossdk.io/apito v0.7.6.
The new version should support the latest API contracts. Confirm that all dependent modules and tooling are tested against this update.
35-35: Upgrade Dependency:cosmossdk.io/depinjectto v1.1.0.
This version bump should enhance dependency injection features. Please verify that integration tests pass after this upgrade.
59-59: Upgrade Dependency:github.com/cosmos/cosmos-dbto v1.1.0.
Updating to this version may provide performance and stability improvements. Ensure that data persistence operations are thoroughly verified.
62-62: Upgrade Dependency:github.com/cosmos/iavlto v1.2.2.
This change should reflect the latest improvements from the iavl library. Please check that state tree validations and related functionalities are unaffected.
75-75: Upgrade Dependency:github.com/emicklei/dotto v1.6.2.
This upgrade may introduce minor improvements or bug fixes in DOT graph generation. Verify that any tooling using this library remains compatible.
87-87: Upgrade Dependency:github.com/golang/glogto v1.2.2.
The updated version should resolve any known issues; please ensure logging output remains consistent.
92-92: Upgrade Dependency:github.com/google/btreeto v1.1.3.
This minor version bump should enhance performance or stability. Confirm that consumers of B-tree functionalities have been validated.
116-116: Upgrade Dependency:github.com/initia-labs/OPinit/apito v0.6.2.
This update complements the change on line 18 and is key for API compatibility across modules. Ensure that interfaces and endpoints are well-tested.
172-172: Upgrade Dependency:google.golang.org/genproto/googleapis/rpcto v0.0.0-20240930140551-af27646dc61f.
The new pre-release version should be compatible with the rest of the protobuf ecosystem; please verify the integration with RPC handling code.
173-173: Upgrade Dependency:google.golang.org/protobufto v1.35.1.
This update should incorporate the latest fixes and improvements in the protobuf library. Testing of serialization and deserialization routines is recommended.
28-28:⚠️ Potential issueGRPC Version Discrepancy Check.
The require block updatesgoogle.golang.org/grpcto v1.67.1, but note that in the replace block (lines 185–187) it is forced to v1.65.0. Please confirm if this intentional downgrade is needed for compatibility or if it was an oversight.e2e/go.mod (4)
7-9: Upgrade Dependencies:github.com/cosmos/ibc-go/v8,github.com/initia-labs/OPinit, andgithub.com/initia-labs/OPinit/apiUpdate.
Lines 7–9 update the IB C and OPinit modules to v8.5.0, v0.6.2, and v0.6.2 respectively. These changes should ensure consistency with the main module’s dependencies. Confirm that any tests for batch information updates or chain interactions reflect these updates.
51-51: Upgrade Dependency:cosmossdk.io/apito v0.7.6 in e2e module.
This update in the e2e context mirrors the main module change—please ensure the e2e tests that rely on API features are passing.
54-54: Upgrade Dependency:cosmossdk.io/depinjectto v1.1.0 in e2e module.
This change improves dependency injection within the e2e tests. Validate that the test infrastructure remains compatible with this upgrade.
60-60: Upgrade Dependency:cosmossdk.io/x/txto v0.13.7 in e2e module.
Ensuring that the e2e environment uses the latest tx functionality is critical for testing the new batch info update commands. Confirm that pagination and polling features work as expected.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cmd/opinitd/tx.go (2)
199-205: Consider using constants for supported chain types.To improve maintainability and reduce the risk of typos, consider defining constants for supported chain types.
+const ( + ChainTypeInitia = "INITIA" + ChainTypeCelestia = "CELESTIA" +) func validateBatchInfoArgs(chainType string) error { chainType = strings.ToUpper(chainType) - if chainType != ophosttypes.BatchInfo_INITIA.String() && chainType != ophosttypes.BatchInfo_CELESTIA.String() { - return fmt.Errorf("supported chain type: %s, %s", ophosttypes.BatchInfo_INITIA.String(), ophosttypes.BatchInfo_CELESTIA.String()) + if chainType != ChainTypeInitia && chainType != ChainTypeCelestia { + return fmt.Errorf("supported chain type: %s, %s", ChainTypeInitia, ChainTypeCelestia) } return nil }
242-285: Consider reducing code duplication in account initialization.The account initialization logic is similar in both
l1ProposerAccountandl2BroadcasterAccount. Consider extracting common initialization code into a helper function.+func initBroadcasterAccount( + ctx types.Context, + rpcAddr string, + bech32Prefix string, + broadcasterConfig *broadcastertypes.BroadcasterConfig, + keyringConfig broadcastertypes.KeyringConfig, +) (*broadcaster.BroadcasterAccount, error) { + cdc, txConfig, err := child.GetCodec(bech32Prefix) + if err != nil { + return nil, err + } + + rpcClient, err := rpcclient.NewRPCClient(cdc, rpcAddr) + if err != nil { + return nil, err + } + + return broadcaster.NewBroadcasterAccount(ctx, *broadcasterConfig, cdc, txConfig, rpcClient, keyringConfig) +}BatchUpdateGuide.md (1)
1-28: Fix markdown formatting issues.The document has several formatting issues:
- Heading levels should increment by one level at a time
- Headings should not have trailing punctuation
- Hard tabs should be replaced with spaces
# UpdateBatchInfo Follow the steps below. -### Step 1. Register the key for the new batch submitter address. +## Step 1: Register the key for the new batch submitter address `opinitd keys add mocha-4 batch --bech32=celestia` *Make sure that this new account has a sufficient balance.* -### Step 2. Run the Update batch info command. +## Step 2: Run the Update batch info command `opinitd tx update-batch-info CELESTIA celestia...` The proposer sends a transaction that includes an update batch info message with the new `ChainType` and `SubmitterAddress`. The bot will `shut down` when it encounters the `MsgUpdateBatchInfo`. -### Step 3. Update the DA node config in the executor config. +## Step 3: Update the DA node config in the executor config ```json { ... "da_node": { "chain_id": "mocha-4", "bech32_prefix": "celestia", "rpc_address": "", "gas_price": "", "gas_adjustment": 1.5, "tx_timeout": 60 } }-### Step 4. Start the OPInit bot.
+## Step 4: Start the OPInit bot<details> <summary>🧰 Tools</summary> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> 3-3: Heading levels should only increment by one level at a time Expected: h2; Actual: h3 (MD001, heading-increment) --- 3-3: Trailing punctuation in heading Punctuation: '.' (MD026, no-trailing-punctuation) --- 8-8: Hard tabs Column: 12 (MD010, no-hard-tabs) --- 8-8: Trailing punctuation in heading Punctuation: '.' (MD026, no-trailing-punctuation) --- 13-13: Trailing punctuation in heading Punctuation: '.' (MD026, no-trailing-punctuation) --- 28-28: Trailing punctuation in heading Punctuation: '.' (MD026, no-trailing-punctuation) </details> </details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between d7ef56086418c7808eb53053892b07e6b399dbbe and 96fb46c0c9cea3b586e41c36bb08f90dcf1c6959. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `BatchUpdateGuide.md` (1 hunks) * `cmd/opinitd/tx.go` (6 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> <details> <summary>BatchUpdateGuide.md</summary> 3-3: Heading levels should only increment by one level at a time Expected: h2; Actual: h3 (MD001, heading-increment) --- 3-3: Trailing punctuation in heading Punctuation: '.' (MD026, no-trailing-punctuation) --- 8-8: Hard tabs Column: 12 (MD010, no-hard-tabs) --- 8-8: Trailing punctuation in heading Punctuation: '.' (MD026, no-trailing-punctuation) --- 13-13: Trailing punctuation in heading Punctuation: '.' (MD026, no-trailing-punctuation) --- 28-28: Trailing punctuation in heading Punctuation: '.' (MD026, no-trailing-punctuation) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms (1)</summary> * GitHub Check: golangci-lint </details> <details> <summary>🔇 Additional comments (5)</summary><blockquote> <details> <summary>cmd/opinitd/tx.go (4)</summary> `7-7`: **LGTM!** The imports and command registration are properly structured. Also applies to: 27-29, 41-43 --- `62-65`: **LGTM!** The refactoring improves code reusability by extracting config loading logic into a separate function. Also applies to: 67-67 --- `120-197`: **Consider enhancing error handling for broadcast failures.** The TODO comment at line 183 indicates a need for better error handling for broadcast failures. Consider implementing retry logic with exponential backoff. Would you like me to generate an implementation for the retry logic? --- `222-240`: **LGTM!** The implementation follows best practices with proper context management and error handling. </details> <details> <summary>BatchUpdateGuide.md (1)</summary> `20-21`: **Add required values for RPC address and gas price.** The configuration example shows empty values for `rpc_address` and `gas_price`. Consider adding example values or a note about required values. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Summary by CodeRabbit
New Features
Refactor
Tests
Chores