Test Only: coderrabit changes #214
Conversation
needed to sdk clients that use own tx factory and want to initialize it
WalkthroughThese updates enhance the codebase by focusing on testing practices, security, performance, and user customization. Public access is granted to previously private functions in the Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #214 +/- ##
==========================================
+ Coverage 25.01% 25.10% +0.09%
==========================================
Files 17 17
Lines 3022 3011 -11
==========================================
Hits 756 756
+ Misses 2234 2223 -11
Partials 32 32 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 8
Configuration used: .coderabbit.yaml
Files selected for processing (4)
- .coderabbit.yaml (1 hunks)
- client/chain/chain.go (9 hunks)
- client/common/network.go (1 hunks)
- client/common/options.go (2 hunks)
Additional comments: 6
.coderabbit.yaml (2)
- 2-4: The instructions for reviewing Go files are clear and align with the PR objectives. This should help ensure that reviews focus on best practices, security, and performance.
- 10-10: Enabling the auto-reply feature in chat configurations is a straightforward change that should improve interaction efficiency.
client/common/options.go (3)
- 22-25: The addition of the
FixSeqMismatchfield to theClientOptionsstruct is clear and follows Go conventions.- 31-33: Initializing the
FixSeqMismatchfield totrueby default inDefaultClientOptionsis a clear and sensible default setting.- 68-72: The
OptionFixSeqMismatchfunction provides a clear and idiomatic way to set theFixSeqMismatchfield via aClientOption.client/common/network.go (1)
- 179-193: The addition of the "local" network configuration case in the
LoadNetworkfunction is well-implemented, with sensible defaults and appropriate use of theDisabledCookieAssistantfor a local environment.
| // if the account number and/or the account sequence number are zero (not set), | ||
| // they will be queried for and set on the provided Factory. A new Factory with | ||
| // the updated fields will be returned. | ||
| func (c *chainClient) prepareFactory(clientCtx client.Context, txf tx.Factory) (tx.Factory, error) { | ||
| func PrepareFactory(clientCtx client.Context, txf tx.Factory) (tx.Factory, error) { |
There was a problem hiding this comment.
The change to make PrepareFactory public is noted and aligns with the PR objectives. However, it's important to ensure that this function is covered by tests, especially now that it's part of the public API.
Would you like assistance in creating test cases for this function?
| @@ -605,7 +605,7 @@ | |||
| res, err := c.broadcastTx(c.ctx, c.txFactory, true, msgs...) | |||
|
|
|||
| if err != nil { | |||
| if strings.Contains(err.Error(), "account sequence mismatch") { | |||
| if c.opts.FixSeqMismatch && strings.Contains(err.Error(), "account sequence mismatch") { | |||
There was a problem hiding this comment.
The addition of handling for "account sequence mismatch" in SyncBroadcastMsg is a practical improvement. Ensure this error handling path is covered by tests to verify its effectiveness.
Would you like assistance in creating test cases for this error handling scenario?
| @@ -633,7 +633,7 @@ | |||
| func (c *chainClient) SimulateMsg(clientCtx client.Context, msgs ...sdk.Msg) (*txtypes.SimulateResponse, error) { | |||
| c.txFactory = c.txFactory.WithSequence(c.accSeq) | |||
| c.txFactory = c.txFactory.WithAccountNumber(c.accNum) | |||
| txf, err := c.prepareFactory(clientCtx, c.txFactory) | |||
| txf, err := PrepareFactory(clientCtx, c.txFactory) | |||
There was a problem hiding this comment.
The addition of PrepareFactory within SimulateMsg is a logical improvement. It's important to ensure this behavior is covered by tests to verify its correctness.
Would you like assistance in creating test cases for this behavior?
| @@ -668,7 +668,7 @@ | |||
| c.txFactory = c.txFactory.WithAccountNumber(c.accNum) | |||
| res, err := c.broadcastTx(c.ctx, c.txFactory, false, msgs...) | |||
| if err != nil { | |||
| if strings.Contains(err.Error(), "account sequence mismatch") { | |||
| if c.opts.FixSeqMismatch && strings.Contains(err.Error(), "account sequence mismatch") { | |||
There was a problem hiding this comment.
The addition of handling for "account sequence mismatch" in AsyncBroadcastMsg is consistent with the changes in SyncBroadcastMsg and improves resilience. Ensure this error handling path is covered by tests.
Would you like assistance in creating test cases for this error handling scenario?
| func (c *chainClient) BuildSignedTx(clientCtx client.Context, accNum, accSeq, initialGas uint64, msgs ...sdk.Msg) ([]byte, error) { | ||
| txf := NewTxFactory(clientCtx).WithSequence(accSeq).WithAccountNumber(accNum).WithGas(initialGas) | ||
| return c.buildSignedTx(clientCtx, txf, msgs...) |
There was a problem hiding this comment.
The delegation of transaction building to a new buildSignedTx function in BuildSignedTx improves modularity. It's important to ensure both functions are covered by tests to verify their correctness.
Would you like assistance in creating test cases for these functions?
|
|
||
| func (c *chainClient) buildSignedTx(clientCtx client.Context, txf tx.Factory, msgs ...sdk.Msg) ([]byte, error) { |
There was a problem hiding this comment.
The addition of the buildSignedTx function enhances modularity by encapsulating the logic for building signed transactions. Ensure comprehensive tests cover the simulation logic and error handling within this function.
Would you like assistance in creating test cases for this function?
| } | ||
|
|
||
| txBytes, err := clientCtx.TxConfig.TxEncoder()(txn.GetTx()) | ||
| txBytes, err := c.buildSignedTx(clientCtx, txf, msgs...) |
There was a problem hiding this comment.
The integration of buildSignedTx within broadcastTx is a positive change that improves modularity. Ensure that the interaction between these functions and the error handling logic is covered by comprehensive tests.
Would you like assistance in creating test cases for this integration and error handling?
| @@ -925,7 +887,7 @@ | |||
| log.Debugln("broadcastTx with nonce", sequence) | |||
| res, err := c.broadcastTx(c.ctx, c.txFactory, true, toSubmit...) | |||
| if err != nil { | |||
| if strings.Contains(err.Error(), "account sequence mismatch") { | |||
| if c.opts.FixSeqMismatch && strings.Contains(err.Error(), "account sequence mismatch") { | |||
There was a problem hiding this comment.
The addition of handling for "account sequence mismatch" in runBatchBroadcast is consistent with similar improvements in other functions. Ensure this error handling path is covered by tests to verify its effectiveness.
Would you like assistance in creating test cases for this error handling scenario?
Summary by CodeRabbit
New Features
Refactor
Chores