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.

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

R4R: CoreContext Refactor #1741

Merged
merged 5 commits into from
Aug 6, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
31 changes: 16 additions & 15 deletions Gopkg.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 12 additions & 0 deletions PENDING.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ BREAKING CHANGES
* [x/gov] Governance parameters are now stored in globalparams store
* [lcd] \#1866 Updated lcd /slashing/signing_info endpoint to take cosmosvalpub instead of cosmosvaladdr
* [types] sdk.NewCoin now takes sdk.Int, sdk.NewInt64Coin takes int64
* [cli] #1551: Officially removed `--name` from CLI commands

FEATURES
* [lcd] Can now query governance proposals by ProposalStatus
Expand Down Expand Up @@ -68,3 +69,14 @@ BUG FIXES
* \#1828 Force user to specify amount on create-validator command by removing default
* \#1839 Fixed bug where intra-tx counter wasn't set correctly for genesis validators
* [tests] \#1675 Fix non-deterministic `test_cover`
* [client] \#1551: Refactored `CoreContext`
* Renamed `CoreContext` to `QueryContext`
Copy link
Contributor

Choose a reason for hiding this comment

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

propose: QueryContext -> CliContext. Since Query isn't the only thing we're doing there.

* Removed all tx related fields and logic (building & signing) to separate
structure `TxContext` in `x/auth/client/context`
* Cleaned up documentation and API of what used to be `CoreContext`
* Implemented `KeyType` enum for key info

BUG FIXES
* \#1666 Add intra-tx counter to the genesis validators
* [tests] \#1551: Fixed invalid LCD test JSON payload in `doIBCTransfer`
* \#1787 Fixed bug where Tally fails due to revoked/unbonding validator
113 changes: 113 additions & 0 deletions client/context/context.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
package context

import (
"io"

"github.com/cosmos/cosmos-sdk/client"
"github.com/cosmos/cosmos-sdk/wire"
"github.com/cosmos/cosmos-sdk/x/auth"
"github.com/spf13/viper"
rpcclient "github.com/tendermint/tendermint/rpc/client"
)

const ctxAccStoreName = "acc"
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm should this be a constant? Why not a QueryContext field in case we want to change it?

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact we have QueryContext.AccountStore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, indeed it is already a field in the QueryContext, however:

  1. There is no path (from what I can tell) where this is actually configurable or desirable (i.e. everywhere in the code uses acc)
  2. Since there is no CLI flag for it, a user can just do queryCtx.WithAccountStore(...).

I'm happy to revert it to being hard-coded.

Copy link
Contributor

Choose a reason for hiding this comment

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

looks fine for now.


// QueryContext implements a typical context created in SDK modules for
// queries.
type QueryContext struct {
Codec *wire.Codec
AccDecoder auth.AccountDecoder
Client rpcclient.Client
Logger io.Writer
Height int64
NodeURI string
FromAddressName string
AccountStore string
TrustNode bool
UseLedger bool
Async bool
JSON bool
PrintResponse bool
}

// NewQueryContextFromCLI returns a new initialized QueryContext with
// parameters from the command line using Viper.
func NewQueryContextFromCLI() QueryContext {
var rpc rpcclient.Client

nodeURI := viper.GetString(client.FlagNode)
if nodeURI != "" {
rpc = rpcclient.NewHTTP(nodeURI, "/websocket")
}

return QueryContext{
Client: rpc,
NodeURI: nodeURI,
AccountStore: ctxAccStoreName,
FromAddressName: viper.GetString(client.FlagFrom),
Height: viper.GetInt64(client.FlagHeight),
TrustNode: viper.GetBool(client.FlagTrustNode),
UseLedger: viper.GetBool(client.FlagUseLedger),
Async: viper.GetBool(client.FlagAsync),
JSON: viper.GetBool(client.FlagJson),
PrintResponse: viper.GetBool(client.FlagPrintResponse),
}
}

// WithCodec returns a copy of the context with an updated codec.
func (ctx QueryContext) WithCodec(cdc *wire.Codec) QueryContext {
ctx.Codec = cdc
return ctx
}

// WithAccountDecoder returns a copy of the context with an updated account
// decoder.
func (ctx QueryContext) WithAccountDecoder(decoder auth.AccountDecoder) QueryContext {
ctx.AccDecoder = decoder
return ctx
}

// WithLogger returns a copy of the context with an updated logger.
func (ctx QueryContext) WithLogger(w io.Writer) QueryContext {
ctx.Logger = w
return ctx
}

// WithAccountStore returns a copy of the context with an updated AccountStore.
func (ctx QueryContext) WithAccountStore(accountStore string) QueryContext {
ctx.AccountStore = accountStore
return ctx
}

// WithFromAddressName returns a copy of the context with an updated from
// address.
func (ctx QueryContext) WithFromAddressName(addrName string) QueryContext {
ctx.FromAddressName = addrName
return ctx
}

// WithTrustNode returns a copy of the context with an updated TrustNode flag.
func (ctx QueryContext) WithTrustNode(trustNode bool) QueryContext {
ctx.TrustNode = trustNode
return ctx
}

// WithNodeURI returns a copy of the context with an updated node URI.
func (ctx QueryContext) WithNodeURI(nodeURI string) QueryContext {
ctx.NodeURI = nodeURI
ctx.Client = rpcclient.NewHTTP(nodeURI, "/websocket")
return ctx
}

// WithClient returns a copy of the context with an updated RPC client
// instance.
func (ctx QueryContext) WithClient(client rpcclient.Client) QueryContext {
ctx.Client = client
return ctx
}

// WithUseLedger returns a copy of the context with an updated UseLedger flag.
func (ctx QueryContext) WithUseLedger(useLedger bool) QueryContext {
ctx.UseLedger = useLedger
return ctx
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There are some fields in QueryContext without WithXYZ functions, although I guess if you didn't need to use them it doesn't matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll leave as is for now 👍

13 changes: 13 additions & 0 deletions client/context/errors.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package context

import (
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/pkg/errors"
)

// ErrInvalidAccount returns a standardized error reflecting that a given
// account address does not exist.
func ErrInvalidAccount(addr sdk.AccAddress) error {
return errors.Errorf(`No account with address %s was found in the state.
Are you sure there has been a transaction involving it?`, addr)
}
Loading