Option to disable attempts to fix sequence mismatch during broadcasting of txs #213
Conversation
WalkthroughThe recent updates introduce enhancements to address "account sequence mismatch" errors in the blockchain client by offering an option to automatically resolve these discrepancies. Furthermore, support for configuring a local network has been added, enhancing testing and development flexibility. These changes encompass improvements in error handling, network setup, and client options. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 #213 +/- ##
==========================================
+ 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: 5
Configuration used: CodeRabbit UI
Files selected for processing (3)
- client/chain/chain.go (4 hunks)
- client/common/network.go (1 hunks)
- client/common/options.go (2 hunks)
Additional comments: 1
client/common/options.go (1)
- 25-25: The addition of the
FixSeqMismatchboolean field to theClientOptionsstruct is clear and well-named, effectively allowing for easy configuration of the sequence mismatch handling feature.
| return &ClientOptions{ | ||
| FixSeqMismatch: true, | ||
| } |
There was a problem hiding this comment.
The default setting of FixSeqMismatch to true in DefaultClientOptions is a good choice for maintaining backward compatibility. However, it's recommended to add tests to cover this new default behavior.
Would you like assistance in creating tests for this new default setting?
| func OptionFixSeqMismatch(fixSeqMismatch bool) ClientOption { | ||
| return func(opts *ClientOptions) error { | ||
| opts.FixSeqMismatch = fixSeqMismatch | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The OptionFixSeqMismatch function provides a flexible way to configure the sequence mismatch handling feature. It's recommended to add tests to ensure this function behaves as expected when configuring the client.
Would you like assistance in creating tests for this configuration option?
| case "local": | ||
| return Network{ | ||
| LcdEndpoint: "", | ||
| TmEndpoint: "tcp://localhost:26657", | ||
| ChainGrpcEndpoint: "tcp://localhost:9900", | ||
| ChainStreamGrpcEndpoint: "", | ||
| ExchangeGrpcEndpoint: "", | ||
| ExplorerGrpcEndpoint: "", | ||
| ChainId: "injective-1", | ||
| Fee_denom: "inj", | ||
| Name: "local-1", | ||
| chainCookieAssistant: &DisabledCookieAssistant{}, | ||
| exchangeCookieAssistant: &DisabledCookieAssistant{}, | ||
| explorerCookieAssistant: &DisabledCookieAssistant{}, | ||
| } |
There was a problem hiding this comment.
The addition of the "local" case in the LoadNetwork function provides a clear and concise setup for local network configurations. It's recommended to add tests to ensure this new configuration behaves as expected, especially considering the specific endpoints and disabled cookie assistants.
Would you like assistance in creating tests for this new network configuration?
needed to sdk clients that use own tx factory and want to initialize it
| @@ -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 logic to handle sequence mismatches during synchronous transaction broadcasting has been updated to check the FixSeqMismatch option. However, this change lacks test coverage.
Consider adding unit tests to cover the new logic introduced for handling sequence mismatches. This will ensure the feature works as expected and will help catch any potential issues or edge cases.
| @@ -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.
Similar to the synchronous broadcast method, the asynchronous transaction broadcasting method now includes logic to handle sequence mismatches. This change is also not covered by tests.
It's important to extend the testing suite to include scenarios that exercise the new asynchronous sequence mismatch handling logic. Ensuring comprehensive test coverage will help maintain the robustness of the transaction broadcasting feature.
| @@ -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 logic for handling sequence mismatches during the batch broadcast of transactions is a valuable addition. However, like the other related changes, this update lacks test coverage.
Adding tests for the batch broadcast functionality, especially with the new sequence mismatch handling logic, is crucial for ensuring the reliability and correctness of this feature.
| // 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 PrepareFactory function has been made public, but there's no test coverage for this change.
Adding tests for the PrepareFactory function would ensure its behavior is as expected, especially since it now plays a crucial role in preparing the transaction factory with the correct account sequence and number.
| @@ -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 SimulateMsg method has been updated to use the PrepareFactory function, but this change is not covered by tests.
Consider adding tests for the SimulateMsg method to verify that it correctly simulates transactions with the updated transaction factory. This is important for ensuring the accuracy of gas estimation and other simulation outcomes.
|
|
||
| func (c *chainClient) buildSignedTx(clientCtx client.Context, txf tx.Factory, msgs ...sdk.Msg) ([]byte, error) { |
There was a problem hiding this comment.
The buildSignedTx helper method has been added but is not covered by tests.
Adding unit tests for the buildSignedTx method would help ensure that it correctly builds signed transactions, including simulating transactions when necessary. This is important for the overall reliability of transaction creation and broadcasting.
| @@ -709,7 +710,7 @@ | |||
| c.gasWanted = adjustedGas | |||
| } | |||
|
|
|||
| txf, err := c.prepareFactory(clientCtx, txf) | |||
| txf, err := PrepareFactory(clientCtx, txf) | |||
There was a problem hiding this comment.
The buildSignedTx method's use of PrepareFactory is not covered by tests.
Ensure that the integration of PrepareFactory within buildSignedTx is tested, particularly to verify that the transaction factory is correctly prepared with the updated account sequence and number before building signed transactions.
| } | ||
|
|
||
| txBytes, err := clientCtx.TxConfig.TxEncoder()(txn.GetTx()) | ||
| txBytes, err := c.buildSignedTx(clientCtx, txf, msgs...) |
There was a problem hiding this comment.
The broadcastTx method's introduction is a significant change that lacks test coverage.
Adding tests for the broadcastTx method is essential to ensure that it correctly broadcasts transactions, handles errors, and optionally awaits transaction inclusion in a block. This is crucial for the robustness of the transaction broadcasting process.
| if err != nil { | ||
| err = errors.Wrap(err, "failed TxEncoder to encode Tx") | ||
| err = errors.Wrap(err, "failed to build signed Tx") |
There was a problem hiding this comment.
The error handling logic in the broadcastTx method is not covered by tests.
Consider adding tests to cover the error handling logic in the broadcastTx method, especially to ensure that errors are correctly identified, logged, and handled. This will help in maintaining the reliability of the transaction broadcasting feature.
| @@ -850,7 +812,7 @@ | |||
| Mode: txtypes.BroadcastMode_BROADCAST_MODE_SYNC, | |||
| } | |||
| // use our own client to broadcast tx | |||
| ctx = c.getCookie(ctx) | |||
| ctx := c.getCookie(context.Background()) | |||
There was a problem hiding this comment.
The use of getCookie in the broadcastTx method for setting metadata in the context is not covered by tests.
Adding tests to verify the correct setting of metadata in the context by the getCookie method within broadcastTx would ensure that the necessary metadata is included in the gRPC calls for transaction broadcasting.
|
I am closing this PR because it was created pointing to |
I want my Async to be truly async, so can't wait for nonce to be synced in case of error
Summary by CodeRabbit