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

Test Only: coderrabit changes #214

Closed
wants to merge 6 commits into from
Closed

Test Only: coderrabit changes #214

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 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
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
4 changes: 4 additions & 0 deletions .coderabbit.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
reviews:
path_instructions:
- path: "**/*.go"
instructions: "Ignore making test suggestions for Go files. Review best practices, security, and performance."
auto_review:
base_branches:
- "master"
- "dev"
- "feat/.*"

chat:
auto_reply: true
64 changes: 13 additions & 51 deletions client/chain/chain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,11 +419,11 @@
}
}

// prepareFactory ensures the account defined by ctx.GetFromAddress() exists and
// PrepareFactory ensures the account defined by ctx.GetFromAddress() exists and
// 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
Copy link

Choose a reason for hiding this comment

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

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?

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 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?

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
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 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?

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.

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?

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

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 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?

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 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?

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
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
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 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?

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

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
}
Loading