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

Jump to bottom

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

Closed
wants to merge 7 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 4 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
62 changes: 12 additions & 50 deletions client/chain/chain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,7 @@
// 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) {

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

View check run for this annotation

Codecov / codecov/patch

client/chain/chain.go#L426 

Added line #L426 was not covered by tests
from := clientCtx.GetFromAddress()

if err := txf.AccountRetriever().EnsureExists(clientCtx, from); err != nil {
Expand Down 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 @@ -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)

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

View check run for this annotation

Codecov / codecov/patch

client/chain/chain.go#L636 

Added line #L636 was not covered by tests
if err != nil {
err = errors.Wrap(err, "failed to prepareFactory")
return nil, err
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,13 +682,14 @@
return nil, err
}
}

return res, nil
}

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...)

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

View check run for this annotation

Codecov / codecov/patch

client/chain/chain.go#L689 

Added line #L689 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 BuildSignedTx method's introduction is a significant change that lacks test coverage.

It's crucial to add tests for the BuildSignedTx method to ensure that it correctly builds signed transactions with the provided account number, sequence, and initial gas. This will help in validating the correctness of offline signing functionality.

}

func (c *chainClient) buildSignedTx(clientCtx client.Context, txf tx.Factory, msgs ...sdk.Msg) ([]byte, error) {

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

View check run for this annotation

Codecov / codecov/patch

client/chain/chain.go#L692 

Added line #L692 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 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.

if clientCtx.Simulate {
simTxBytes, err := txf.BuildSimTx(msgs...)
if err != nil {
Expand All @@ -709,7 +710,7 @@
c.gasWanted = adjustedGas
}

txf, err := c.prepareFactory(clientCtx, txf)
txf, err := prepareFactory(clientCtx, txf)

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

View check run for this annotation

Codecov / codecov/patch

client/chain/chain.go#L713 

Added line #L713 was not covered by tests
if err != nil {
return nil, errors.Wrap(err, "failed to prepareFactory")
}
Expand Down Expand Up @@ -800,48 +801,9 @@
await bool,
msgs ...sdk.Msg,
) (*txtypes.BroadcastTxResponse, error) {
txf, err := c.prepareFactory(clientCtx, txf)
if err != nil {
err = errors.Wrap(err, "failed to prepareFactory")
return nil, err
}
ctx := context.Background()
if clientCtx.Simulate {
simTxBytes, err := txf.BuildSimTx(msgs...)
if err != nil {
err = errors.Wrap(err, "failed to build sim tx bytes")
return nil, err
}
ctx := c.getCookie(ctx)
simRes, err := c.txClient.Simulate(ctx, &txtypes.SimulateRequest{TxBytes: simTxBytes})
if err != nil {
err = errors.Wrap(err, "failed to CalculateGas")
return nil, err
}

adjustedGas := uint64(txf.GasAdjustment() * float64(simRes.GasInfo.GasUsed))
txf = txf.WithGas(adjustedGas)

c.gasWanted = adjustedGas
}

txn, err := txf.BuildUnsignedTx(msgs...)

if err != nil {
err = errors.Wrap(err, "failed to BuildUnsignedTx")
return nil, err
}

txn.SetFeeGranter(clientCtx.GetFeeGranterAddress())
err = tx.Sign(txf, clientCtx.GetFromName(), txn, true)
if err != nil {
err = errors.Wrap(err, "failed to Sign Tx")
return nil, err
}

txBytes, err := clientCtx.TxConfig.TxEncoder()(txn.GetTx())
txBytes, err := c.buildSignedTx(clientCtx, txf, msgs...)

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

View check run for this annotation

Codecov / codecov/patch

client/chain/chain.go#L804 

Added line #L804 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 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")

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

View check run for this annotation

Codecov / codecov/patch

client/chain/chain.go#L806 

Added line #L806 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 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.

return nil, err
}

Expand All @@ -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())

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

View check run for this annotation

Codecov / codecov/patch

client/chain/chain.go#L815 

Added line #L815 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 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.

res, err := c.txClient.BroadcastTx(ctx, &req)
if !await || err != nil {
return res, err
Expand Down Expand Up @@ -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") {

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

View check run for this annotation

Codecov / codecov/patch

client/chain/chain.go#L890 

Added line #L890 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