R4R: CoreContext Refactor #1741

alexanderbez · 2018-07-18T14:03:08Z

ChangeLog

Separated logic/concerns from CoreContext. QueryContext now facilitates querying and sending txs and TxContext facilitates building and signing txs.
Cleaned up a bunch of the actual query context code
- Separated into smaller composable methods with clear godocs
Implemented GetKeyInfo and GetPassphrase as a general helper as I saw the same logic being used a lot
Added enums for key.Info opposed to checking just strings

I've updated the democoin cool cmd to show how the new contexts would be used. One thing I came across and am not sure if it's the best approach is the utils.SendTx. That is what EnsureSignBuildBroadcast used to handle, but now that the logic and concerns have been split, not sure where to place that. Maybe utils is fine?

closes: #1551

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

ValarDragon · 2018-07-19T16:12:29Z

client/utils/utils.go

+// signed transaction to a node.
+func SendTx(txCtx authctx.TxContext, queryCtx context.QueryContext, from []byte, name string, msgs []sdk.Msg) error {
+	if txCtx.AccountNumber == 0 {
+		accNum, err := queryCtx.GetAccountNumber(from)


When rebasing, can you be sure to add the "EnsureAccountExists" function?

@ValarDragon why is that? SendTx does what EnsureAccountExists virtually did. Do you want me to rip that out and call it EnsureAccountExists which SendTx would call?

Ideally I'd like the more user-friendly error message which EnsureAccountExists provides before checking the account number / sequence. If the account doesn't exist, then we shouldn't be printing messages for updating account num / sequence, and we should never get to the point where it signs. (Then we're asking for password, and only get returned with an unfriendly ABCI error)

I see what you mean. Yeah I think I can leverage something similar 👍

I'll add a method to QueryContext#EnsureAccountExists. This will get called before the any signing even happens.

Edit: nvm, I already have GetAccount. I'll just use this.

ValarDragon · 2018-07-19T19:28:11Z

crypto/keys/types.go

+type KeyType uint
+
+// Info KeyTypes
+const (


I think key types is a great idea!

In the future, hopefully we can offload more code onto the key type, rather than have switch statements everyhere!

maybe KeyType should be a string?

alexanderbez · 2018-07-19T19:33:23Z

@ValarDragon @cwgoes would love some initial/high-level feedback if this is a good way to approach things.

If so, then all that's left is updating all the internal CLI run functions.

ValarDragon · 2018-07-19T19:34:33Z

Anything else you're looking for feedback on, other than what I commented on?

alexanderbez · 2018-07-19T19:42:20Z

@ValarDragon, nope! I updated the cool example. It now checks that the account exists first.

ValarDragon · 2018-07-19T20:41:23Z

Maybe i'm misunderstanding. I thought SendTx is intended to replace the EnsureSignBuildBroadcast method. If so, then we want this check to go inside of SendTx similar to whats done here: https://github.com/cosmos/cosmos-sdk/blob/develop/client/context/viper.go#L76. I think we want this in the send functionality, not inside of everything trying to call send. (Similar to how we get account number and sequence number in send)

codecov · 2018-07-24T16:30:37Z

Codecov Report

Merging #1741 into develop will increase coverage by 1.13%.
The diff coverage is 60%.

@@             Coverage Diff             @@
##           develop    #1741      +/-   ##
===========================================
+ Coverage    63.55%   64.68%   +1.13%     
===========================================
  Files          119      114       -5     
  Lines         7062     6782     -280     
===========================================
- Hits          4488     4387     -101     
+ Misses        2314     2115     -199     
- Partials       260      280      +20

cwgoes

First pass (apologies for the delay in review) - definitely progress, this is still rife with conditionals but I guess that can't really be avoided.

Also I know @sunnya97 was working on integrating the keepers into the context somehow (so that gaiacli could just call keeper functions to query, which would be super convenient) - not sure exactly what the implementation plan is there but worth touching base.

This also needs a rebase/merge with develop.

cwgoes · 2018-08-01T01:14:39Z

client/context/context.go

+	rpcclient "github.com/tendermint/tendermint/rpc/client"
+)
+
+const ctxAccStoreName = "acc"


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

In fact we have QueryContext.AccountStore

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

There is no path (from what I can tell) where this is actually configurable or desirable (i.e. everywhere in the code uses acc)

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.

looks fine for now.

cwgoes · 2018-08-01T01:15:41Z

client/context/context.go

+func (ctx QueryContext) WithUseLedger(useLedger bool) QueryContext {
+	ctx.UseLedger = useLedger
+	return ctx
+}


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

Ok, I'll leave as is for now 👍

cwgoes · 2018-08-01T01:16:44Z

client/context/query.go

+		return res, err
+	}
+
+	if res.CheckTx.Code != uint32(0) {


Can we reference some constant here - sdk.ABCICodeOK in types/ IIRC?

cwgoes · 2018-08-01T01:16:55Z

client/context/query.go

+			res.CheckTx.Log)
+	}
+
+	if res.DeliverTx.Code != uint32(0) {


Ditto on using a constant.

cwgoes · 2018-08-01T01:18:32Z

client/context/query.go

+	}
+
+	resp := result.Response
+	if resp.Code != uint32(0) {


sdk.ABCICodeOK

cwgoes · 2018-08-01T01:20:30Z

client/utils/utils.go

+		return err
+	}
+
+	if txCtx.AccountNumber == 0 {


This really isn't the right check - what if the account number was intentionally tried as 0? I think we want to check whether or not the user passed the --account-number flag at all.

Hmmm, it seems to be identical to the logic that currently exists (unless I'm missing something?):

https://github.com/cosmos/cosmos-sdk/blob/develop/client/context/viper.go#L105-L122

If the client does indeed intentionally pass 0, then it'll look it up automatically -- kind of quirky, I understand.

If, as you're suggesting, we want to check if the client passed 0 and actually go with that, then'll be need to introduce the dependency of viper here. Thoughts?

Can't we just change the default value to like -1 to mean automatic?

Yeah, but then the UX will be a little weird cause the CLI description will have the default in there of -1.

Let me think on this a bit more -- I can probably come up with a cleaner way outside the scope of utils.

Sounds good! I'd actually recommend just putting a "TODO: " above the relevant lines / moving this to a new issue (since this isn't a performance regression). We can do these UI nice to haves until later! (I doubt practically anyone will be doing --sequence=0 for the next month)

Actually, fair point! I'll do this :-)

Hmmm, it seems to be identical to the logic that currently exists (unless I'm missing something?):

It is; the current logic is wrong. Unfortunately IIRC it's wrong due to a bug in Viper which we couldn't fix without forking the upstream.

I guess it's fine to leave as-is (with a comment) for now.

cwgoes · 2018-08-01T01:20:56Z

client/utils/utils.go

+		txCtx = txCtx.WithAccountNumber(accNum)
+	}
+
+	if txCtx.Sequence == 0 {


Same as above, with --sequence instead of --account-number.

cwgoes · 2018-08-01T01:21:47Z

crypto/keys/types.go

+
+// String implements the stringer interface for KeyType.
+func (kt KeyType) String() string {
+	keyTypes := map[KeyType]string{


This can be a constant? No need to recreate a map every time .String() is called.

cwgoes · 2018-08-01T01:23:45Z

client/context/query.go

+			}
+
+			ctx.Logger.Write(bz)
+			io.WriteString(ctx.Logger, "\n")


Would it be better to use the logger here instead of just io.WriteString and fmt.Printf in various places? Although I guess we want machine-readable output and the logger adds extra info - maybe we could have a json logger and a human-readable logger though.

Hmmm not sure I follow? The io.WriteString is just a convenience method for writing a newline. I can easily just append a newline byte to bz. The actual logger here is os.Stdout. I also don't see any fmt.Printfs.

Sorry, you're right, I misread - never mind.

ValarDragon · 2018-08-01T16:06:51Z

@alexanderbez while we're still reviewing, could you hold off on squashing? It makes it harder to to check if a fix was resolved as intended. I think we should actually update the PR template to not encourage squashing, and only have the merger squash when merging the PR. (Unless specific commit history is requested, or if review is complete)

alexanderbez · 2018-08-01T16:10:23Z

@ValarDragon sorry, I squashed already (to resolve rebasing). I won't squash from here on out until approved 👍

alexanderbez · 2018-08-01T16:48:04Z

@cwgoes, @ValarDragon I believe I addressed majority of your concerns and comments.

Regarding some aspects being rife with conditionals, if you could point them out, I'd be happy to improve! But generally I found there is little way around it just due to the nature of the way the context is used.

Regarding @sunnya97's proposed integration of keepers -- great idea, but let's address that in a separate PR that uses this as the basis 👍

jaekwon · 2018-08-02T23:48:58Z

client/context/query.go

+// queryStore performs a query from a Tendermint node with the provided a store
+// name and path.
+func (ctx QueryContext) queryStore(key cmn.HexBytes, storeName, endPath string) ([]byte, error) {
+	path := fmt.Sprintf("/store/%s/%s", storeName, endPath)


maybe "store" here should likewise be parameterized

That prefix (/store for store queries) is hardcoded in baseapp

jaekwon · 2018-08-02T23:49:37Z

client/keys/utils.go

+
+	// we only need a passphrase for locally stored keys
+	if keyInfo.GetType() == keys.TypeLocal {
+		passphrase, err = ReadPassphraseFromStdin(name)


TODO: Make Issue to address this before launch

(we shouldn't be passing around passphrases even in memory)

There already is an issue for this. #864

How would we avoid storing the passphrase (transiently) in memory? We need to decrypt the encrypted keyfile.

@cwgoes brings up a good point. Warrants further discussion. I'll add a reference to #864 in the code tho

jaekwon · 2018-08-02T23:51:41Z

client/tx/broadcast.go

@@ -13,7 +13,7 @@ type BroadcastTxBody struct {
 }

 // BroadcastTx REST Handler
-func BroadcastTxRequestHandlerFn(ctx context.CoreContext) http.HandlerFunc {
+func BroadcastTxRequestHandlerFn(queryTx context.QueryContext) http.HandlerFunc {


errr typo. gracias.

jaekwon · 2018-08-02T23:52:12Z

client/tx/search.go

@@ -53,19 +57,22 @@ func SearchTxCmd(cdc *wire.Codec) *cobra.Command {
 	return cmd
 }

-func searchTxs(ctx context.CoreContext, cdc *wire.Codec, tags []string) ([]txInfo, error) {
+func searchTxs(queryTx context.QueryContext, cdc *wire.Codec, tags []string) ([]txInfo, error) {


jaekwon · 2018-08-02T23:53:41Z

x/auth/client/context/context.go

+)
+
+// TxContext implements a transaction context created in SDK modules.
+type TxContext struct {


jaekwon · 2018-08-02T23:54:35Z

x/auth/client/context/utils.go

+
+// defaultChainID returns the chain ID from the genesis file if present. An
+// error is returned if the file cannot be read or parsed.
+func defaultChainID() (string, error) {


seems kinda weird to define this here... should it be defined in tendermint/commands?

if so maybe a NOTE is enough.

I'll add a note because I do not have enough context here. Honestly, I think we should remove it all together and require that the user always provide the correct chainID.

jaekwon · 2018-08-02T23:55:41Z

PENDING.md

@@ -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`


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

jaekwon · 2018-08-02T23:57:39Z

Overall great refactor for splitting the piece into two. Have a few items commented, like renaming QueryContext to CliContext.

cwgoes

utACK

Agree with Jae on the renaming but not strongly opinionated; my previous comments have all been addressed.

- Rename QueryContext - Add a few TODOs

alexanderbez · 2018-08-03T17:43:23Z

Updated to reflect the latest PR review(s):

Cleaned up faulty variable names
Add TODOs with issue references
Renamed QueryContext => CLIContext

/cc @jaekwon @cwgoes

alexanderbez · 2018-08-06T15:11:49Z

@cwgoes @jaekwon can this be merged?

cwgoes · 2018-08-06T18:11:41Z

@cwgoes @jaekwon can this be merged?

Verily!

alexanderbez added lcd C:CLI labels Jul 18, 2018

alexanderbez requested review from sunnya97, ebuchman, rigelrozanski and cwgoes July 18, 2018 14:03

alexanderbez mentioned this pull request Jul 19, 2018

lcd/client: Abstract method for determining when to get passphrase from stdin #1446

Closed

ValarDragon reviewed Jul 19, 2018

View reviewed changes

alexanderbez force-pushed the bez/1551-core-ctx-refactor branch 2 times, most recently from c8d8e17 to c746e31 Compare July 23, 2018 16:00

alexanderbez force-pushed the bez/1551-core-ctx-refactor branch from c746e31 to 9fce3cf Compare July 23, 2018 17:10

alexanderbez force-pushed the bez/1551-core-ctx-refactor branch from 9fce3cf to 05832e5 Compare July 23, 2018 19:25

alexanderbez force-pushed the bez/1551-core-ctx-refactor branch from 6e9f11a to afdec83 Compare July 24, 2018 13:38

alexanderbez force-pushed the bez/1551-core-ctx-refactor branch from afdec83 to ff5439a Compare July 24, 2018 13:41

alexanderbez force-pushed the bez/1551-core-ctx-refactor branch from ff5439a to cd8e0df Compare July 24, 2018 14:06

alexanderbez force-pushed the bez/1551-core-ctx-refactor branch from 4828e0c to 7312475 Compare July 24, 2018 16:13

alexanderbez pushed a commit that referenced this pull request Jul 24, 2018

Merge pull request #1741: CoreContext Refactor

7312475

cwgoes suggested changes Aug 1, 2018

View reviewed changes

alexanderbez force-pushed the bez/1551-core-ctx-refactor branch from 356574c to aa7a016 Compare August 1, 2018 14:17

Merge pull request #1741: CoreContext Refactor

debbf0d

alexanderbez force-pushed the bez/1551-core-ctx-refactor branch from aa7a016 to debbf0d Compare August 1, 2018 14:39

Update keyTypes to be a global var

87becc0

alexanderbez mentioned this pull request Aug 1, 2018

Allow Usage of Client Provided Account Number/Sequence in Context Query #1903

Closed

4 tasks

Add TODOs to update SendTx

0355028

jaekwon reviewed Aug 2, 2018

View reviewed changes

Merge branch 'develop' into bez/1551-core-ctx-refactor

8abd1e7

cwgoes approved these changes Aug 3, 2018

View reviewed changes

Address PR comments and concerns

8e034fb

- Rename QueryContext - Add a few TODOs

cwgoes merged commit 12c2c23 into develop Aug 6, 2018

cwgoes deleted the bez/1551-core-ctx-refactor branch August 6, 2018 18:11

mslipper mentioned this pull request Aug 28, 2018

Edit Validator command fails silently #1957

Closed

R4R: CoreContext Refactor #1741

R4R: CoreContext Refactor #1741

Conversation

alexanderbez commented Jul 18, 2018 • edited Loading

ChangeLog

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez commented Jul 19, 2018 • edited Loading

ValarDragon commented Jul 19, 2018

alexanderbez commented Jul 19, 2018

ValarDragon commented Jul 19, 2018

codecov bot commented Jul 24, 2018 • edited Loading

Codecov Report

cwgoes left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez Aug 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez Aug 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon commented Aug 1, 2018 • edited Loading

alexanderbez commented Aug 1, 2018

alexanderbez commented Aug 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaekwon commented Aug 2, 2018

cwgoes left a comment

Choose a reason for hiding this comment

alexanderbez commented Aug 3, 2018

alexanderbez commented Aug 6, 2018

cwgoes commented Aug 6, 2018

alexanderbez commented Jul 18, 2018 •

edited

Loading

alexanderbez Jul 19, 2018 •

edited

Loading

alexanderbez commented Jul 19, 2018 •

edited

Loading

codecov bot commented Jul 24, 2018 •

edited

Loading

cwgoes left a comment •

edited

Loading

alexanderbez Aug 1, 2018 •

edited

Loading

alexanderbez Aug 1, 2018 •

edited

Loading

ValarDragon commented Aug 1, 2018 •

edited

Loading