feat/tx_broadcast_refactoring #272
Conversation
…allow using the Sync broadcast without passing though the SDK mechanism to ensure the TX is included in one of the next blocks (CHAIN-277)
|
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.62.2)level=warning msg="[lintersdb] The name "gas" is deprecated. The linter has been renamed to: gosec." WalkthroughThe changes primarily focus on refactoring the transaction broadcasting mechanism in the chain client across multiple files. The core modification introduces a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant ChainClient
participant Blockchain
Client->>ChainClient: BroadcastMsg(mode, msgs)
ChainClient->>ChainClient: Prepare transaction
ChainClient->>Blockchain: Submit transaction
alt Sync Mode
Blockchain-->>ChainClient: Wait for confirmation
ChainClient-->>Client: Return response
else Async Mode
Blockchain-->>ChainClient: Immediate response
ChainClient-->>Client: Return immediate response
end
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
examples/chain/permissions/1_CreateNamespace/example.go (1)
Line range hint
28-28: Security: Remove hardcoded private key from examples.The example includes a hardcoded private key. Even in example code, it's recommended to:
- Use environment variables or configuration files for sensitive data
- Add a comment explaining that this is for demonstration only
- Use a clearly fake/invalid key in examples
- "f9db9bf330e23cb7839039e944adef6e9df447b90b503d5b4464c90bea9022f3", // keyring will be used if pk not provided + os.Getenv("EXAMPLE_PRIVATE_KEY"), // Example only: Use environment variables for private keys in real applicationsexamples/chain/permissions/5_RevokeNamespaceRoles/example.go (1)
Line range hint
27-33: Remove hardcoded credentials from example code.The example contains hardcoded sensitive information including private key and password. This poses a security risk if someone copies this example directly.
Replace with placeholder values and add comments indicating these need to be replaced:
senderAddress, cosmosKeyring, err := chainclient.InitCosmosKeyring( os.Getenv("HOME")+"/.injectived", "injectived", "file", - "inj-user", - "12345678", - "f9db9bf330e23cb7839039e944adef6e9df447b90b503d5b4464c90bea9022f3", + "your-key-name", + "your-password", + "your-private-key", // Replace with your private key or use keyring false, )
🧹 Nitpick comments (12)
client/chain/chain.go (1)
2543-2583: Ensure consistent error handling inSyncBroadcastMsgIn the
SyncBroadcastMsgmethod, consider improving error handling to account for possible edge cases, such as whenresisnilor when decoding theTxHashfails.Apply this diff to enhance error handling:
func (c *chainClient) SyncBroadcastMsg(msgs ...sdk.Msg) (*txtypes.BroadcastTxResponse, error) { req, res, err := c.BroadcastMsg(txtypes.BroadcastMode_BROADCAST_MODE_SYNC, msgs...) if err != nil || res == nil || res.TxResponse.Code != 0 { return res, err } awaitCtx, cancelFn := context.WithTimeout(context.Background(), defaultBroadcastTimeout) defer cancelFn() - txHash, _ := hex.DecodeString(res.TxResponse.TxHash) + txHash, decodeErr := hex.DecodeString(res.TxResponse.TxHash) + if decodeErr != nil { + return nil, errors.Wrap(decodeErr, "failed to decode TxHash") + } t := time.NewTimer(defaultBroadcastStatusPoll) for { select { case <-awaitCtx.Done(): err := errors.Wrapf(ErrTimedOut, "%s", res.TxResponse.TxHash) t.Stop() return nil, err case <-t.C: resultTx, err := c.ctx.Client.Tx(awaitCtx, txHash, false) if err != nil { if errRes := client.CheckCometError(err, req.TxBytes); errRes != nil { return &txtypes.BroadcastTxResponse{TxResponse: errRes}, err } t.Reset(defaultBroadcastStatusPoll) continue } else if resultTx.Height > 0 { resResultTx := sdk.NewResponseResultTx(resultTx, res.TxResponse.Tx, res.TxResponse.Timestamp) res = &txtypes.BroadcastTxResponse{TxResponse: resResultTx} t.Stop() return res, nil } t.Reset(defaultBroadcastStatusPoll) } } }examples/chain/permissions/6_ClaimVoucher/example.go (1)
6-6: Fix import ordering as per Go conventionsThe imports need to be organized correctly to comply with Go's import formatting conventions. This improves code readability and consistency.
Apply this diff to fix the import order:
package main import ( "encoding/json" "fmt" + "os" + permissionstypes "github.com/InjectiveLabs/sdk-go/chain/permissions/types" + "github.com/InjectiveLabs/sdk-go/client" + chainclient "github.com/InjectiveLabs/sdk-go/client/chain" + "github.com/InjectiveLabs/sdk-go/client/common" + rpchttp "github.com/cometbft/cometbft/rpc/client/http" txtypes "github.com/cosmos/cosmos-sdk/types/tx" - "os" - permissionstypes "github.com/InjectiveLabs/sdk-go/chain/permissions/types" - "github.com/InjectiveLabs/sdk-go/client" - chainclient "github.com/InjectiveLabs/sdk-go/client/chain" - "github.com/InjectiveLabs/sdk-go/client/common" - rpchttp "github.com/cometbft/cometbft/rpc/client/http" )🧰 Tools
🪛 GitHub Actions: pre-commit
[warning] Import ordering needs to be fixed. The 'go imports' check failed and modified the file.
examples/chain/permissions/2_DeleteNamespace/example.go (1)
8-10: Organize imports according to Go standardsThe import statements are not organized properly. Go tools like
goimportscan automatically format and group imports into standard sections: standard library packages, third-party packages, and local packages.Apply this diff to organize the imports:
import ( "encoding/json" "fmt" "os" + permissionstypes "github.com/InjectiveLabs/sdk-go/chain/permissions/types" + "github.com/InjectiveLabs/sdk-go/client" + chainclient "github.com/InjectiveLabs/sdk-go/client/chain" + "github.com/InjectiveLabs/sdk-go/client/common" + rpchttp "github.com/cometbft/cometbft/rpc/client/http" + txtypes "github.com/cosmos/cosmos-sdk/types/tx" - rpchttp "github.com/cometbft/cometbft/rpc/client/http" - txtypes "github.com/cosmos/cosmos-sdk/types/tx" - permissionstypes "github.com/InjectiveLabs/sdk-go/chain/permissions/types" - "github.com/InjectiveLabs/sdk-go/client" - chainclient "github.com/InjectiveLabs/sdk-go/client/chain" - "github.com/InjectiveLabs/sdk-go/client/common" )examples/chain/permissions/1_CreateNamespace/example.go (3)
8-10: Remove redundant import.The
rpchttppackage is already imported and used fortmClient. This duplicate import can be removed.- rpchttp "github.com/cometbft/cometbft/rpc/client/http" txtypes "github.com/cosmos/cosmos-sdk/types/tx"
87-87: Document ignored return value and consider encapsulating broadcast mode.
- The first return value is being ignored without documentation of what it represents.
- Since all examples use SYNC mode, consider encapsulating this in the chainClient to reduce duplication.
- _, response, err := chainClient.BroadcastMsg(txtypes.BroadcastMode_BROADCAST_MODE_SYNC, msg) + // txHash is ignored as we only need the response + txHash, response, err := chainClient.BroadcastMsg(txtypes.BroadcastMode_BROADCAST_MODE_SYNC, msg)Consider adding a convenience method to chainClient:
// SyncBroadcastMsg broadcasts a message in SYNC mode func (cc *chainClient) SyncBroadcastMsg(msg sdk.Msg) (string, *sdk.TxResponse, error) { return cc.BroadcastMsg(txtypes.BroadcastMode_BROADCAST_MODE_SYNC, msg) }
Line range hint
89-91: Improve error handling in example code.The current error handling using
panicwithout context is not ideal for example code. Consider adding error context and using proper error handling to demonstrate best practices.if err != nil { - panic(err) + fmt.Printf("Failed to broadcast create namespace message: %v\n", err) + os.Exit(1) }client/chain/chain_test.go (1)
22-56: Consider adding error handling for file operations.While the function correctly sets up the test environment, it could benefit from proper cleanup in case of errors.
Consider wrapping the file operations in a defer function to ensure cleanup:
func setUpChainTest(t *testing.T) { + cleanup := func() { + if err := os.RemoveAll(chain.OfacListPath); err != nil { + t.Logf("Failed to cleanup temporary directory: %v", err) + } + } + + // Ensure cleanup on test failure + t.Cleanup(cleanup) + // Store the original OfacListPath originalOfacListPath = chain.OfacListPath // Update OfacListPath to point to the temporary directory chain.OfacListPath = t.TempDir() // Rest of the function... }client/chain/chain_test_support.go (1)
76-78: Consider adding validation for input parameters.The mock implementation should validate input parameters to better mirror the real implementation's behavior.
Consider adding basic validation:
func (c *MockChainClient) BroadcastMsg(broadcastMode txtypes.BroadcastMode, msgs ...sdk.Msg) (*txtypes.BroadcastTxRequest, *txtypes.BroadcastTxResponse, error) { + if len(msgs) == 0 { + return nil, nil, fmt.Errorf("no messages to broadcast") + } + if broadcastMode != txtypes.BroadcastMode_BROADCAST_MODE_SYNC && + broadcastMode != txtypes.BroadcastMode_BROADCAST_MODE_ASYNC && + broadcastMode != txtypes.BroadcastMode_BROADCAST_MODE_BLOCK { + return nil, nil, fmt.Errorf("invalid broadcast mode: %v", broadcastMode) + } return &txtypes.BroadcastTxRequest{}, &txtypes.BroadcastTxResponse{}, nil }examples/chain/oracle/1_MsgRelayPriceFeedPrice/example.go (1)
72-72: Consider handling the request object.The broadcast request object is being ignored but could be useful for debugging or logging purposes.
Consider logging the request object:
- _, result, err := chainClient.BroadcastMsg(txtypes.BroadcastMode_BROADCAST_MODE_SYNC, msg) + req, result, err := chainClient.BroadcastMsg(txtypes.BroadcastMode_BROADCAST_MODE_SYNC, msg) + fmt.Printf("Broadcast request: %+v\n", req)examples/chain/permissions/5_RevokeNamespaceRoles/example.go (1)
Line range hint
75-81: Consider enhancing error handling.The current error handling simply panics, which may not be ideal for production code. Consider adding more context to errors and proper error propagation.
_, response, err := chainClient.BroadcastMsg(txtypes.BroadcastMode_BROADCAST_MODE_SYNC, msg) if err != nil { - panic(err) + fmt.Printf("Failed to broadcast message: %v\n", err) + os.Exit(1) }examples/chain/permissions/3_UpdateNamespace/example.go (1)
Line range hint
63-70: Add documentation for namespace operations.The example demonstrates critical namespace operations but lacks documentation explaining the impact of these changes.
Add comments explaining each operation:
msg := &permissionstypes.MsgUpdateNamespace{ Sender: senderAddress.String(), NamespaceDenom: namespaceDenom, + // Configure webhook for namespace operations WasmHook: &permissionstypes.MsgUpdateNamespace_MsgSetWasmHook{NewValue: "inj19ld6swyldyujcn72j7ugnu9twafhs9wxlyye5m"}, + // Pause all token operations for security SendsPaused: &permissionstypes.MsgUpdateNamespace_MsgSetSendsPaused{NewValue: true}, MintsPaused: &permissionstypes.MsgUpdateNamespace_MsgSetMintsPaused{NewValue: true}, BurnsPaused: &permissionstypes.MsgUpdateNamespace_MsgSetBurnsPaused{NewValue: true}, }examples/chain/permissions/4_UpdateNamespaceRoles/example.go (1)
Line range hint
66-82: Add documentation for role permissions.The example demonstrates role-based permissions but could benefit from documentation explaining the permission model.
Add comments explaining the permission structure:
msg := &permissionstypes.MsgUpdateNamespaceRoles{ Sender: senderAddress.String(), NamespaceDenom: namespaceDenom, RolePermissions: []*permissionstypes.Role{ + // Grant receive permission to all addresses { Role: permissionstypes.EVERYONE, Permissions: uint32(permissionstypes.Action_RECEIVE), }, + // Blacklisted role has no permissions { Role: "blacklisted", Permissions: 0, }, }, + // Assign roles to specific addresses AddressRoles: []*permissionstypes.AddressRoles{ { Address: blockedAddress, Roles: []string{"blacklisted"}, }, }, }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
client/chain/chain.go(4 hunks)client/chain/chain_test.go(4 hunks)client/chain/chain_test_support.go(1 hunks)client/chain/ofac.go(1 hunks)examples/chain/13_BroadcastMsgWithoutSimulation/example.go(2 hunks)examples/chain/oracle/1_MsgRelayPriceFeedPrice/example.go(2 hunks)examples/chain/permissions/1_CreateNamespace/example.go(2 hunks)examples/chain/permissions/2_DeleteNamespace/example.go(2 hunks)examples/chain/permissions/3_UpdateNamespace/example.go(2 hunks)examples/chain/permissions/4_UpdateNamespaceRoles/example.go(2 hunks)examples/chain/permissions/5_RevokeNamespaceRoles/example.go(2 hunks)examples/chain/permissions/6_ClaimVoucher/example.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- client/chain/ofac.go
🧰 Additional context used
🪛 GitHub Actions: pre-commit
examples/chain/permissions/6_ClaimVoucher/example.go
[warning] Import ordering needs to be fixed. The 'go imports' check failed and modified the file.
client/chain/chain.go
[error] 881-881: Ineffectual assignment to err variable. The error value is assigned but never used.
🔇 Additional comments (8)
examples/chain/permissions/1_CreateNamespace/example.go (1)
87-87: Changes align well with PR objectives.The refactoring of the TX broadcasting logic is well implemented:
- Consistent usage across all examples
- Explicit broadcast mode specification
- Improved flexibility in message broadcasting
client/chain/chain_test.go (3)
18-20: LGTM: Variable declaration for storing original OFAC list path.The variable is appropriately scoped at package level to maintain state between setup and teardown.
58-61: LGTM: Clean teardown function.The function correctly restores the original OFAC list path.
135-158: LGTM: Well-structured client creation function.The function properly handles error cases and sets up the client with appropriate configuration.
examples/chain/permissions/5_RevokeNamespaceRoles/example.go (1)
8-10: LGTM! Import changes align with new broadcast API.The added imports support the new broadcast mode functionality.
examples/chain/permissions/3_UpdateNamespace/example.go (1)
Line range hint
27-33: Remove hardcoded credentials from example code.examples/chain/permissions/4_UpdateNamespaceRoles/example.go (2)
Line range hint
66-82: LGTM! Good use of type-safe constants for permissions.The code correctly uses the
permissionstypes.Action_RECEIVEconstant for type-safe permission assignment.
Line range hint
27-33: Remove hardcoded credentials from example code.
client/chain/chain.go
Outdated
| if c.opts.ShouldFixSequenceMismatch && strings.Contains(err.Error(), "account sequence mismatch") { | ||
| c.syncNonce() | ||
| sequence := c.getAccSeq() | ||
| c.txFactory = c.txFactory.WithSequence(sequence) | ||
| c.txFactory = c.txFactory.WithAccountNumber(c.accNum) | ||
| log.Debugln("retrying broadcastTx with nonce", sequence) | ||
| req, res, err = c.broadcastTx(c.ctx, c.txFactory, broadcastMode, msgs...) | ||
| } | ||
| if err != nil { | ||
| resJSON, _ := json.MarshalIndent(res, "", "\t") | ||
| c.logger.WithField("size", len(msgs)).WithError(err).Errorln("failed to asynchronously broadcast messagess:", string(resJSON)) | ||
| return nil, nil, err | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Avoid potential panic due to nil res in error handling
In the BroadcastMsg method, when logging the error, there's a possibility that res might be nil if the broadcastTx function returns an error before obtaining a response. Accessing res fields when res is nil can cause a panic.
Apply this diff to prevent nil pointer dereference:
if err != nil {
if c.opts.ShouldFixSequenceMismatch && strings.Contains(err.Error(), "account sequence mismatch") {
// retry logic
}
if err != nil {
- resJSON, _ := json.MarshalIndent(res, "", "\t")
- c.logger.WithField("size", len(msgs)).WithError(err).Errorln("failed to asynchronously broadcast messagess:", string(resJSON))
+ var resJSON []byte
+ if res != nil {
+ resJSON, _ = json.MarshalIndent(res, "", "\t")
+ }
+ c.logger.WithField("size", len(msgs)).WithError(err).Errorln("failed to broadcast messages:", string(resJSON))
return nil, nil, err
}
}📝 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 c.opts.ShouldFixSequenceMismatch && strings.Contains(err.Error(), "account sequence mismatch") { | |
| c.syncNonce() | |
| sequence := c.getAccSeq() | |
| c.txFactory = c.txFactory.WithSequence(sequence) | |
| c.txFactory = c.txFactory.WithAccountNumber(c.accNum) | |
| log.Debugln("retrying broadcastTx with nonce", sequence) | |
| req, res, err = c.broadcastTx(c.ctx, c.txFactory, broadcastMode, msgs...) | |
| } | |
| if err != nil { | |
| resJSON, _ := json.MarshalIndent(res, "", "\t") | |
| c.logger.WithField("size", len(msgs)).WithError(err).Errorln("failed to asynchronously broadcast messagess:", string(resJSON)) | |
| return nil, nil, err | |
| } | |
| } | |
| if c.opts.ShouldFixSequenceMismatch && strings.Contains(err.Error(), "account sequence mismatch") { | |
| c.syncNonce() | |
| sequence := c.getAccSeq() | |
| c.txFactory = c.txFactory.WithSequence(sequence) | |
| c.txFactory = c.txFactory.WithAccountNumber(c.accNum) | |
| log.Debugln("retrying broadcastTx with nonce", sequence) | |
| req, res, err = c.broadcastTx(c.ctx, c.txFactory, broadcastMode, msgs...) | |
| } | |
| if err != nil { | |
| var resJSON []byte | |
| if res != nil { | |
| resJSON, _ = json.MarshalIndent(res, "", "\t") | |
| } | |
| c.logger.WithField("size", len(msgs)).WithError(err).Errorln("failed to broadcast messages:", string(resJSON)) | |
| return nil, nil, err | |
| } | |
| } |
| return | ||
| } | ||
| } | ||
| res, err := c.SyncBroadcastMsg(toSubmit...) |
There was a problem hiding this comment.
Handle the error returned by SyncBroadcastMsg
At line 881, the error returned by c.SyncBroadcastMsg(toSubmit...) is assigned to err but not used. Ignoring errors can lead to silent failures and unexpected behaviors. It's important to check and handle errors appropriately.
Apply this diff to handle the error:
func (c *chainClient) runBatchBroadcast() {
// ... existing code ...
submitBatch := func(toSubmit []sdk.Msg) {
- res, err := c.SyncBroadcastMsg(toSubmit...)
+ res, err := c.SyncBroadcastMsg(toSubmit...)
+ if err != nil {
+ c.logger.WithError(err).Errorln("failed to broadcast messages batch")
+ return
+ }
if res.TxResponse.Code != 0 {
err = errors.Errorf("error %d (%s): %s", res.TxResponse.Code, res.TxResponse.Codespace, res.TxResponse.RawLog)
log.WithField("txHash", res.TxResponse.TxHash).WithError(err).Errorln("failed to broadcast messages batch")Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: pre-commit
[error] 881-881: Ineffectual assignment to err variable. The error value is assigned but never used.
| func accountForTests() (cosmtypes.AccAddress, keyring.Keyring, error) { | ||
| senderAddress, cosmosKeyring, err := chain.InitCosmosKeyring( | ||
| os.Getenv("HOME")+"/.injectived", | ||
| "injectived", | ||
| "file", | ||
| "inj-user", | ||
| "12345678", | ||
| "5d386fbdbf11f1141010f81a46b40f94887367562bd33b452bbaa6ce1cd1381e", // keyring will be used if pk not provided | ||
| false, | ||
| ) | ||
|
|
||
| return senderAddress, cosmosKeyring, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using environment variables for sensitive data.
The function contains hardcoded credentials which is not a security best practice.
Consider using environment variables for sensitive data:
func accountForTests() (cosmtypes.AccAddress, keyring.Keyring, error) {
senderAddress, cosmosKeyring, err := chain.InitCosmosKeyring(
os.Getenv("HOME")+"/.injectived",
"injectived",
"file",
- "inj-user",
- "12345678",
- "5d386fbdbf11f1141010f81a46b40f94887367562bd33b452bbaa6ce1cd1381e",
+ os.Getenv("INJ_USER"),
+ os.Getenv("INJ_PASS"),
+ os.Getenv("INJ_PRIVATE_KEY"),
false,
)
return senderAddress, cosmosKeyring, err
}📝 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 accountForTests() (cosmtypes.AccAddress, keyring.Keyring, error) { | |
| senderAddress, cosmosKeyring, err := chain.InitCosmosKeyring( | |
| os.Getenv("HOME")+"/.injectived", | |
| "injectived", | |
| "file", | |
| "inj-user", | |
| "12345678", | |
| "5d386fbdbf11f1141010f81a46b40f94887367562bd33b452bbaa6ce1cd1381e", // keyring will be used if pk not provided | |
| false, | |
| ) | |
| return senderAddress, cosmosKeyring, err | |
| } | |
| func accountForTests() (cosmtypes.AccAddress, keyring.Keyring, error) { | |
| senderAddress, cosmosKeyring, err := chain.InitCosmosKeyring( | |
| os.Getenv("HOME")+"/.injectived", | |
| "injectived", | |
| "file", | |
| os.Getenv("INJ_USER"), | |
| os.Getenv("INJ_PASS"), | |
| os.Getenv("INJ_PRIVATE_KEY"), | |
| false, | |
| ) | |
| return senderAddress, cosmosKeyring, err | |
| } |
| str, _ := json.MarshalIndent(response, "", " ") | ||
| fmt.Print(string(str)) |
There was a problem hiding this comment.
Handle JSON marshaling errors.
The JSON marshaling error is being ignored which could hide potential issues.
Apply this fix:
- str, _ := json.MarshalIndent(response, "", " ")
- fmt.Print(string(str))
+ str, err := json.MarshalIndent(response, "", " ")
+ if err != nil {
+ panic(err)
+ }
+ fmt.Println(string(str))📝 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.
| str, _ := json.MarshalIndent(response, "", " ") | |
| fmt.Print(string(str)) | |
| str, err := json.MarshalIndent(response, "", " ") | |
| if err != nil { | |
| panic(err) | |
| } | |
| fmt.Println(string(str)) |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
client/chain/chain.go (1)
883-884:⚠️ Potential issueEnsure the error is properly logged
In the
runBatchBroadcastmethod, within the error handling block,c.logger.WithError(err)is called without a terminating logging function like.Errorln(). This results in the error not being logged.Apply this diff to correctly log the error:
if err != nil { - c.logger.WithError(err) + c.logger.WithError(err).Errorln("failed to broadcast messages batch") return }
🧹 Nitpick comments (1)
client/chain/chain.go (1)
2607-2614: Handle sequence mismatch errors more robustlyWhen retrying due to an "account sequence mismatch" error, it would be prudent to also re-fetch the account number, as it may have changed. This ensures that both the account number and sequence are synchronized before retrying.
Consider modifying the retry logic:
if c.opts.ShouldFixSequenceMismatch && strings.Contains(err.Error(), "account sequence mismatch") { c.syncNonce() + c.accNum, c.accSeq = c.GetAccNonce() sequence := c.getAccSeq() c.txFactory = c.txFactory.WithSequence(sequence) c.txFactory = c.txFactory.WithAccountNumber(c.accNum) c.logger.Debugln("retrying broadcastTx with nonce", sequence) req, res, err = c.broadcastTx(c.ctx, c.txFactory, broadcastMode, msgs...) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/chain/chain.go(4 hunks)examples/chain/permissions/6_ClaimVoucher/example.go(2 hunks)
🔇 Additional comments (2)
client/chain/chain.go (1)
2615-2619: Avoid potential panic due to nilresin error handlingIn the
BroadcastMsgmethod, there is a possibility thatresmight benilif thebroadcastTxfunction returns an error before obtaining a response. Accessingresfields whenresisnilcan cause a panic.Apply this diff to prevent nil pointer dereference:
if err != nil { + var resJSON []byte + if res != nil { resJSON, _ = json.MarshalIndent(res, "", "\t") + } c.logger.WithField("size", len(msgs)).WithError(err).Errorln("failed to asynchronously broadcast messages:", string(resJSON)) return nil, nil, err }examples/chain/permissions/6_ClaimVoucher/example.go (1)
68-68: Update to useBroadcastMsgwith specified broadcast modeThe method call has been correctly updated to
BroadcastMsgwith the broadcast mode parameter. This aligns with the refactored transaction broadcasting logic and demonstrates proper usage.
| res, err := c.SyncBroadcastMsg(toSubmit...) | ||
|
|
||
| if res.TxResponse.Code != 0 { | ||
| err = errors.Errorf("error %d (%s): %s", res.TxResponse.Code, res.TxResponse.Codespace, res.TxResponse.RawLog) | ||
| log.WithField("txHash", res.TxResponse.TxHash).WithError(err).Errorln("failed to broadcast messages batch") | ||
| if err != nil { | ||
| c.logger.WithError(err) | ||
| } else { | ||
| log.WithField("txHash", res.TxResponse.TxHash).Debugln("msg batch broadcasted successfully at height", res.TxResponse.Height) | ||
| if res.TxResponse.Code != 0 { | ||
| err = errors.Errorf("error %d (%s): %s", res.TxResponse.Code, res.TxResponse.Codespace, res.TxResponse.RawLog) | ||
| c.logger.WithField("txHash", res.TxResponse.TxHash).WithError(err).Errorln("failed to broadcast messages batch") | ||
| } else { | ||
| c.logger.WithField("txHash", res.TxResponse.TxHash).Debugln("msg batch broadcasted successfully at height", res.TxResponse.Height) | ||
| } |
There was a problem hiding this comment.
Check for nil response before accessing fields
In the submitBatch function within runBatchBroadcast, there is a risk of a nil pointer dereference if res is nil when res.TxResponse.Code is accessed. Ensure that res and res.TxResponse are not nil before accessing their fields.
Apply this diff to prevent potential panic:
if err != nil {
c.logger.WithError(err).Errorln("failed to broadcast messages batch")
return
} else {
- if res.TxResponse.Code != 0 {
+ if res == nil || res.TxResponse == nil || res.TxResponse.Code != 0 {
err = errors.Errorf("error %d (%s): %s", res.TxResponse.Code, res.TxResponse.Codespace, res.TxResponse.RawLog)
c.logger.WithField("txHash", res.TxResponse.TxHash).WithError(err).Errorln("failed to broadcast messages batch")
} else {
c.logger.WithField("txHash", res.TxResponse.TxHash).Debugln("msg batch broadcasted successfully at height", res.TxResponse.Height)
}
}Committable suggestion skipped: line range outside the PR's diff.
Solves CHAIN-277
Summary by CodeRabbit
Release Notes
New Features
BroadcastMsgmethod that supports different broadcast modes.Improvements
Testing
Chores