[CP-517] improve broadcast functions #323
Conversation
…entV2 to allow cancelation and timeouts. Added new broadcast functions in ChainClient (v1) to include the context as a parameter, but without changing the existing functions (keeping backwards compatibility)
WalkthroughThis update introduces context-aware operations throughout the chain client, examples, and test support code. All chain client transaction simulation, broadcasting, and gas price retrieval methods now accept a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ExampleApp
participant ChainClient
participant Blockchain
User->>ExampleApp: Run example
ExampleApp->>ExampleApp: Create context with timeout
ExampleApp->>ChainClient: CurrentChainGasPrice(ctx)
ChainClient->>Blockchain: Query gas price (with ctx)
Blockchain-->>ChainClient: Return gas price
ChainClient-->>ExampleApp: Return gas price
ExampleApp->>ChainClient: BroadcastMsg(ctx, msg, mode)
ChainClient->>Blockchain: Submit transaction (with ctx)
Blockchain-->>ChainClient: Broadcast result
ChainClient-->>ExampleApp: Return broadcast response
ExampleApp->>User: Print response
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code Graph Analysis (1)client/chain/chain.go (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (17)
✨ Finishing Touches
🪧 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
Documentation and Community
|
There was a problem hiding this comment.
Pull Request Overview
This PR improves broadcast functions in the SDK by adding context parameter support to allow for proper cancellation and timeout handling. The changes ensure that blockchain operations can be cancelled or timed out gracefully, providing better control over long-running operations.
- Added context parameter to all broadcast functions in ChainClientV2 to enable cancellation and timeout support
- Added new broadcast functions in ChainClient (v1) with context support while maintaining backward compatibility
- Updated all example files to use the new context-aware broadcast functions with proper timeout handling
Reviewed Changes
Copilot reviewed 66 out of 66 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| client/chain/chain_v2.go | Added context parameter to all broadcast functions and enhanced SyncBroadcast methods with retry logic |
| client/chain/chain.go | Added new context-aware broadcast functions for v1 client maintaining backward compatibility |
| client/chain/chain_test_support.go | Updated mock methods to match new signatures for v2 client |
| client/chain/chain_test_support_v1.go | Added new mock methods for context-aware functions in v1 client |
| examples/chain/*/example.go | Updated all example files to use new context-aware broadcast functions |
| typeddata/typed_data.go | Fixed function name comment from "SignTextWithValidator" to "SignTextValidator" |
| chain/exchange/types/key.go | Fixed function name comment from "orderIndexByMarketDirectionSubaccountPrefix" to "ordersByMarketDirectionPricePrefix" |
| auth_vote/authz_vote.go | Updated to use context-aware broadcast functions |
| close(c.msgC) | ||
| } | ||
|
|
||
| if c.cancelFn != nil { |
There was a problem hiding this comment.
[nitpick] Consider adding a nil check for c.cancelFn before calling it. The current check only verifies it's not nil but doesn't handle potential race conditions.
| t.Reset(defaultBroadcastStatusPoll) | ||
| continue | ||
| // If we've reached max retries, return error | ||
| if totalAttempts >= maxRetries { |
There was a problem hiding this comment.
The retry logic should check if maxRetries is 0 or negative to avoid infinite loops. Consider adding validation for maxRetries parameter.
| } | ||
|
|
||
| // If we've reached max retries, return error | ||
| if totalAttempts >= maxRetries { |
There was a problem hiding this comment.
Same issue as in chain_v2.go - the retry logic should validate maxRetries parameter to prevent infinite loops when maxRetries is 0 or negative.
| //AsyncBroadcastMsg, SyncBroadcastMsg, QueueBroadcastMsg | ||
| response, err := chainClient.SyncBroadcastMsg(msg) | ||
| pollInterval := 100 * time.Millisecond | ||
| response, err := chainClient.SyncBroadcastMsg(ctx, &pollInterval, 5, msg) |
There was a problem hiding this comment.
The magic number '5' for maxRetries should be defined as a constant to improve maintainability and consistency across examples.
| response, err := chainClient.SyncBroadcastMsg(ctx, &pollInterval, 5, msg) | |
| response, err := chainClient.SyncBroadcastMsg(ctx, &pollInterval, MaxRetries, msg) |
| //AsyncBroadcastMsg, SyncBroadcastMsg, QueueBroadcastMsg | ||
| response, err := chainClient.SyncBroadcastMsg(msg) | ||
| pollInterval := 100 * time.Millisecond | ||
| response, err := chainClient.SyncBroadcastMsg(ctx, &pollInterval, 5, msg) |
There was a problem hiding this comment.
The magic number '5' for maxRetries should be defined as a constant to improve maintainability and consistency across examples.
| response, err := chainClient.SyncBroadcastMsg(ctx, &pollInterval, 5, msg) | |
| response, err := chainClient.SyncBroadcastMsg(ctx, &pollInterval, MaxRetries, msg) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
client/chain/chain_v2.go (2)
691-715: Use the passed context in PrepareFactory call.The method accepts a
ctxparameter but usesc.ctxwhen callingPrepareFactory. This prevents proper context propagation for cancellation and timeout handling.Apply this fix:
- txf, err := PrepareFactory(c.ctx, c.txFactory) + txf, err := PrepareFactory(ctx, c.txFactory)
727-768: Multiple context usage issues in buildSignedTx.The method has several places where it uses
c.ctxinstead of the passedctxparameter:
- Line 728: Uses
c.ctx.Simulate- Line 749: Uses
c.ctxin PrepareFactory- Line 760: Uses
c.ctx.GetFeeGranterAddress()- Line 761: Uses
c.ctx.GetFromName()- Line 767: Uses
c.ctx.TxConfigThis prevents proper context isolation and cancellation propagation.
Consider refactoring to use a context-aware approach or pass these values as parameters instead of relying on the stored context.
🧹 Nitpick comments (1)
examples/chain/exchange/9_MsgBatchUpdateOrders/example.go (1)
47-50: Inconsistent error handling pattern.While the broadcasting error handling was updated to use
panic(err), the client initialization errors still usefmt.Println(err)andreturn. For consistency with other examples, these should also usepanic(err).Apply this diff to make error handling consistent:
if err != nil { - fmt.Println(err) - return + panic(err) }And for the second occurrence:
if err != nil { - fmt.Println(err) - return + panic(err) }Also applies to: 60-62
| func (c *chainClientV2) BuildSignedTx(ctx context.Context, accNum, accSeq, initialGas uint64, gasPrice uint64, msgs ...sdk.Msg) ([]byte, error) { | ||
| txf := NewTxFactory(c.ctx).WithSequence(accSeq).WithAccountNumber(accNum) | ||
| txf = txf.WithGas(initialGas) | ||
|
|
||
| gasPriceWithDenom := fmt.Sprintf("%d%s", gasPrice, client.InjDenom) | ||
| txf = txf.WithGasPrices(gasPriceWithDenom) | ||
|
|
||
| return c.buildSignedTx(clientCtx, txf, msgs...) | ||
| return c.buildSignedTx(ctx, txf, msgs...) | ||
| } |
There was a problem hiding this comment.
Use the passed context in NewTxFactory call.
The method uses c.ctx when calling NewTxFactory instead of the passed ctx parameter.
Apply this fix:
- txf := NewTxFactory(c.ctx).WithSequence(accSeq).WithAccountNumber(accNum)
+ txf := NewTxFactory(ctx).WithSequence(accSeq).WithAccountNumber(accNum)📝 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 (c *chainClientV2) BuildSignedTx(ctx context.Context, accNum, accSeq, initialGas uint64, gasPrice uint64, msgs ...sdk.Msg) ([]byte, error) { | |
| txf := NewTxFactory(c.ctx).WithSequence(accSeq).WithAccountNumber(accNum) | |
| txf = txf.WithGas(initialGas) | |
| gasPriceWithDenom := fmt.Sprintf("%d%s", gasPrice, client.InjDenom) | |
| txf = txf.WithGasPrices(gasPriceWithDenom) | |
| return c.buildSignedTx(clientCtx, txf, msgs...) | |
| return c.buildSignedTx(ctx, txf, msgs...) | |
| } | |
| func (c *chainClientV2) BuildSignedTx(ctx context.Context, accNum, accSeq, initialGas uint64, gasPrice uint64, msgs ...sdk.Msg) ([]byte, error) { | |
| txf := NewTxFactory(ctx).WithSequence(accSeq).WithAccountNumber(accNum) | |
| txf = txf.WithGas(initialGas) | |
| gasPriceWithDenom := fmt.Sprintf("%d%s", gasPrice, client.InjDenom) | |
| txf = txf.WithGasPrices(gasPriceWithDenom) | |
| return c.buildSignedTx(ctx, txf, msgs...) | |
| } |
🤖 Prompt for AI Agents
In client/chain/chain_v2.go around lines 717 to 725, the BuildSignedTx method
incorrectly uses c.ctx instead of the passed ctx parameter when calling
NewTxFactory. Replace c.ctx with ctx in the NewTxFactory call to ensure the
passed context is used properly.
Fixes CP-517
Summary by CodeRabbit
New Features
Refactor
Style
Documentation