Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Option to disable attempts to fix sequence mismatch during broadcasting of txs #213

Closed
wants to merge 7 commits into from
+42 −56
Closed

Option to disable attempts to fix sequence mismatch during broadcasting of txs #213

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f036da9
feat: add "local" network type support
kakysha Mar 18, 2024
0fd3579
feat: option to fix seq mismatch error
kakysha Mar 18, 2024
2a7da1e
Merge branch 'master' into feat-configurable-params
kakysha Mar 18, 2024
b4bd99b
chore: refactor repetitive code by extracting common functionality
kakysha Mar 19, 2024
a9ea38c
refactorL make PrepareFactory client method public
kakysha Mar 19, 2024
8e00488
try reducing testing suggestions
Kishan-Dhakan Mar 25, 2024
ca39171
revert: remove path instructions
Kishan-Dhakan Mar 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions client/chain/chain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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") {

Check warning on line 608 in client/chain/chain.go

View check run for this annotation

Codecov / codecov/patch

client/chain/chain.go#L608 

Added line #L608 was not covered by tests
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

c.syncNonce()
sequence := c.getAccSeq()
c.txFactory = c.txFactory.WithSequence(sequence)
Expand Down Expand Up @@ -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") {

Check warning on line 671 in client/chain/chain.go

View check run for this annotation

Codecov / codecov/patch

client/chain/chain.go#L671 

Added line #L671 was not covered by tests
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

c.syncNonce()
sequence := c.getAccSeq()
c.txFactory = c.txFactory.WithSequence(sequence)
Expand All @@ -682,7 +682,6 @@
return nil, err
}
}

return res, nil
}

Expand Down Expand Up @@ -925,7 +924,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") {

Check warning on line 927 in client/chain/chain.go

View check run for this annotation

Codecov / codecov/patch

client/chain/chain.go#L927 

Added line #L927 was not covered by tests
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

c.syncNonce()
sequence := c.getAccSeq()
c.txFactory = c.txFactory.WithSequence(sequence)
Expand Down
16 changes: 15 additions & 1 deletion client/common/network.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,21 @@

func LoadNetwork(name string, node string) Network {
switch name {

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{},
}

Check warning on line 193 in client/common/network.go

View check run for this annotation

Codecov / codecov/patch

client/common/network.go#L179-L193 

Added lines #L179 - L193 were not covered by tests
Comment on lines +179 to +193
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

case "devnet-1":
return Network{
LcdEndpoint: "https://devnet-1.lcd.injective.dev",
Expand Down
18 changes: 14 additions & 4 deletions client/common/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,18 @@
}

type ClientOptions struct {
GasPrices string
TLSCert credentials.TransportCredentials
TxFactory *tx.Factory
GasPrices string
TLSCert credentials.TransportCredentials
TxFactory *tx.Factory
FixSeqMismatch bool
}

type ClientOption func(opts *ClientOptions) error

func DefaultClientOptions() *ClientOptions {
return &ClientOptions{}
return &ClientOptions{
FixSeqMismatch: true,
}

Check warning on line 33 in client/common/options.go

View check run for this annotation

Codecov / codecov/patch

client/common/options.go#L31-L33 

Added lines #L31 - L33 were not covered by tests
Comment on lines +31 to +33
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 OptionGasPrices(gasPrices string) ClientOption {
Expand Down Expand Up @@ -61,3 +64,10 @@
return nil
}
}

func OptionFixSeqMismatch(fixSeqMismatch bool) ClientOption {
return func(opts *ClientOptions) error {
opts.FixSeqMismatch = fixSeqMismatch
return nil
}

Check warning on line 72 in client/common/options.go

View check run for this annotation

Codecov / codecov/patch

client/common/options.go#L68-L72 

Added lines #L68 - L72 were not covered by tests
Comment on lines +68 to +72
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
Loading