Skip to content

Test Only: coderrabit changes #214

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

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 @@ func (c *chainClient) syncTimeoutHeight() {
}
}

// 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) {
Copy link

@coderabbitai coderabbitai bot Mar 25, 2024

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 @@ func (c *chainClient) SyncBroadcastMsg(msgs ...sdk.Msg) (*txtypes.BroadcastTxRes
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") {
Copy link

@coderabbitai coderabbitai bot Mar 25, 2024

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) GetFeeDiscountInfo(ctx context.Context, account string) (*
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)
Copy link

@coderabbitai coderabbitai bot Mar 25, 2024

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 @@ func (c *chainClient) AsyncBroadcastMsg(msgs ...sdk.Msg) (*txtypes.BroadcastTxRe
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") {
Copy link

@coderabbitai coderabbitai bot Mar 25, 2024

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 @@ func (c *chainClient) AsyncBroadcastMsg(msgs ...sdk.Msg) (*txtypes.BroadcastTxRe
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...)
Copy link

@coderabbitai coderabbitai bot Mar 25, 2024

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

@coderabbitai coderabbitai bot Mar 25, 2024

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 @@ func (c *chainClient) BuildSignedTx(clientCtx client.Context, accNum, accSeq, in
c.gasWanted = adjustedGas
}

txf, err := c.prepareFactory(clientCtx, txf)
txf, err := PrepareFactory(clientCtx, txf)
if err != nil {
return nil, errors.Wrap(err, "failed to prepareFactory")
}
Expand Down Expand Up @@ -800,48 +801,9 @@ func (c *chainClient) broadcastTx(
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...)
Copy link

@coderabbitai coderabbitai bot Mar 25, 2024

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")
return nil, err
}

Expand All @@ -850,7 +812,7 @@ func (c *chainClient) broadcastTx(
Mode: txtypes.BroadcastMode_BROADCAST_MODE_SYNC,
}
// use our own client to broadcast tx
ctx = c.getCookie(ctx)
ctx := c.getCookie(context.Background())
res, err := c.txClient.BroadcastTx(ctx, &req)
if !await || err != nil {
return res, err
Expand Down Expand Up @@ -925,7 +887,7 @@ func (c *chainClient) runBatchBroadcast() {
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") {
Copy link

@coderabbitai coderabbitai bot Mar 25, 2024

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 (network *Network) ExplorerMetadata(provider MetadataProvider) (string, err

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{},
}
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 @@ func init() {
}

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

func OptionGasPrices(gasPrices string) ClientOption {
Expand Down Expand Up @@ -61,3 +64,10 @@ func OptionTxFactory(txFactory *tx.Factory) ClientOption {
return nil
}
}

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