R4R: Allow --from to be a name or an address #2073

mslipper · 2018-08-17T09:32:20Z

Closes #1735.

Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Updated all relevant documentation (docs/)
Updated all relevant code comments
Wrote tests
Added entries in PENDING.md that include links to the relevant issue or PR that most accurately describes the change.
Updated cmd/gaia and examples/

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)

alexanderbez

@mslipper great start, but it seems some logic may be incorrect? Also left some minors remarks. Also, please make sure CI passes too 👍

alexanderbez · 2018-08-17T15:11:30Z

PENDING.md

@@ -61,6 +61,7 @@ FEATURES
 * [core] added BaseApp.Seal - ability to seal baseapp parameters once they've been set
 * [scripts] added log output monitoring to DataDog using Ansible scripts
 * [gov] added TallyResult type that gets added stored in Proposal after tallying is finished
+* [cli] --from can now be either an address or a key name


Can you reference the PR here please @mslipper ?

alexanderbez · 2018-08-17T15:36:55Z

client/context/context.go

@@ -84,7 +87,7 @@ func (ctx CLIContext) WithAccountStore(accountStore string) CLIContext {
 // WithFromAddressName returns a copy of the context with an updated from
 // address.
 func (ctx CLIContext) WithFromAddressName(addrName string) CLIContext {


WithFrom*

alexanderbez · 2018-08-17T15:46:31Z

client/context/query.go

+	return ctx.fromAddress, nil
+}
+
+func (ctx CLIContext) GetFromName() (string, error) {


Missing godoc

client/context/query.go

alexanderbez · 2018-08-17T16:13:15Z

client/context/query.go

+	return ctx.fromName, nil
+}
+
+func (ctx CLIContext) populateFromFields() error {


Are you sure this logic is correct? I have not been able to get this to work (gaiacli send --from <addr1> --to <addr2> ...)

Yep - note that you'll have to create a new key for the correct data to appear in the database.

To be clear: the way I've written it, an additional key (as in key-value store) is written to LevelDB to look up keys (as in private keys) by address. If that KV-store key doesn't exist, you won't be able to pass a public key in --from.

@mslipper I created a fresh local testnet, added all of my node's keys to a single node's key DB and still could not get this to work. I've tried with both recovered keys and with newly created keys.

Were you able to get it working @alexanderbez ? If you could post the command you're using and the error you get I can debug.

jaekwon · 2018-08-21T22:41:40Z

client/context/context.go

@@ -84,7 +87,7 @@ func (ctx CLIContext) WithAccountStore(accountStore string) CLIContext {
 // WithFromAddressName returns a copy of the context with an updated from
 // address.
 func (ctx CLIContext) WithFromAddressName(addrName string) CLIContext {


I think we'd want to search for "FromAddressName" and replace them w/ "From" as well, like this method name.

jaekwon · 2018-08-21T22:47:15Z

client/context/query.go

+
+	var info cskeys.Info
+
+	if strings.HasPrefix(ctx.From, sdk.Bech32PrefixAccAddr) {


There's a conflict if the user enters a name that happens to be a Bech32 address, or one that starts with the Bech32PrefixAccAddr. A few thoughts:

Checking for the Bech32PrefixAccAddr prefix is not enough, we should also check for the "1" that follows, as per the bech32 spec.

Maybe we should just try parsing the address via sdk.AccAddressFromBech32 and assume it's an address if there's no error. (there's a checksum in bech32 so this should be enough).

And, maybe we should instead check on the add/new side and ensure that you can't enter a valid Bech32 address as a name.

jackzampolin · 2018-08-21T23:54:45Z

Looks like a couple of changes here @mslipper

mslipper · 2018-08-22T16:42:34Z

Yep I'm checking them out @jackzampolin.

alexanderbez · 2018-08-23T15:02:57Z

@mslipper mind rebasing using the latest develop to fix the broken test_sim_gaia_fast? Also, the test_cli is failing and I still cannot get this functionality to work for me locally?

melekes · 2018-08-29T10:36:00Z

client/context/query.go

@@ -14,6 +14,7 @@ import (
 	cmn "github.com/tendermint/tendermint/libs/common"
 	rpcclient "github.com/tendermint/tendermint/rpc/client"
 	ctypes "github.com/tendermint/tendermint/rpc/core/types"
+		cskeys "github.com/cosmos/cosmos-sdk/crypto/keys"


formatting is off (use gofmt)

Formatting is still off here @mslipper

melekes · 2018-08-29T10:36:21Z

client/context/context.go

-func (ctx CLIContext) WithFromAddressName(addrName string) CLIContext {
-	ctx.FromAddressName = addrName
+// WithFrom returns a copy of the context with an updated from address.
+func (ctx CLIContext) WithFrom(addrName string) CLIContext {


addrName -> from

melekes · 2018-08-29T10:36:48Z

client/context/context.go

-// address.
-func (ctx CLIContext) WithFromAddressName(addrName string) CLIContext {
-	ctx.FromAddressName = addrName
+// WithFrom returns a copy of the context with an updated from address.


address or name

melekes · 2018-08-29T10:43:18Z

crypto/keys/keybase.go

@@ -179,6 +180,12 @@ func (kb dbKeybase) List() ([]Info, error) {
 	iter := kb.db.Iterator(nil, nil)
 	defer iter.Close()
 	for ; iter.Valid(); iter.Next() {
+		key := string(iter.Key())
+
+		if strings.HasSuffix(key, ".address") {


this needs a comment IMHO

I'd say if one needs to write code like ^^^, then something is wrong. Just a earlier warning to core devs.

Closes cosmos#1735.

codecov · 2018-08-30T08:42:48Z

Codecov Report

Merging #2073 into develop will decrease coverage by 0.05%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #2073      +/-   ##
===========================================
- Coverage    64.97%   64.92%   -0.06%     
===========================================
  Files          135      135              
  Lines         8418     8425       +7     
===========================================
  Hits          5470     5470              
- Misses        2587     2594       +7     
  Partials       361      361

alexanderbez

@mslipper I think we're almost there! Left a bit more feedback :-)

Also, integration_tests is failing. Wanna take a look at that?

alexanderbez · 2018-08-30T14:09:49Z

client/context/query.go

@@ -14,6 +14,7 @@ import (
 	cmn "github.com/tendermint/tendermint/libs/common"
 	rpcclient "github.com/tendermint/tendermint/rpc/client"
 	ctypes "github.com/tendermint/tendermint/rpc/core/types"
+		cskeys "github.com/cosmos/cosmos-sdk/crypto/keys"


Formatting is still off here @mslipper

alexanderbez · 2018-08-30T14:10:41Z

client/context/query.go

+	return ctx.fromAddress, nil
+}
+
+// GetFromname returns the key name for the current context.


GetFromName*

alexanderbez · 2018-08-30T14:14:39Z

client/context/query.go

 	}

-	return sdk.AccAddress(info.GetPubKey().Address()), nil
+	ctx.fromAddress = (sdk.AccAddress)(info.GetPubKey().Address())


Is this equivalent to: sdk.AccAddress(info.GetPubKey().Address().Bytes())?

Correct. Both Address() and Bytes() return effectively a byte slice.

alexanderbez · 2018-08-30T14:19:14Z

client/utils/utils.go

+	return cliCtx.EnsureBroadcastTx(txBytes)
+}
+
+func lookupTxCtx(txCtx authctx.TxContext, cliCtx context.CLIContext, from []byte) (*authctx.TxContext, error) {


Where did this come from? I don't think we should be returning pointers to contexts. Is this from the original simulation PR @alessio ?

Nvm, I don't see this on develop. I don't think lookupTxCtx is appropriate here. See prepareTxContext here. I think it should be analogous to that.

Was trying to reduce the cyclomatic complexity of the function above - I can revert.

ohhh no, we should definitely reduce cyclomatic complexity! We just should:

Not return a pointer

Avoid a merge conflict cause @alessio's PR has something similar

That's all :-)

alexanderbez · 2018-08-30T14:24:36Z

crypto/keys/keybase.go

+func (kb dbKeybase) GetByAddress(address types.AccAddress) (Info, error) {
+	ik := kb.db.Get(addrKey((tmcrypto.Address)(address)))
+	if len(ik) == 0 {
+		return nil, fmt.Errorf("Key with address %s not found", address.String())


Lets lowercase Key 👍

alexanderbez · 2018-08-30T14:31:35Z

crypto/keys/keybase.go

@@ -179,6 +180,13 @@ func (kb dbKeybase) List() ([]Info, error) {
 	iter := kb.db.Iterator(nil, nil)
 	defer iter.Close()
 	for ; iter.Valid(); iter.Next() {
+		key := string(iter.Key())
+
+		// need to exclude any keys in storage that have an address suffix


I think a more defensive mechanism would be to: if strings.HasSuffix(key, infoSuffix) { // read and parse...}

alexanderbez · 2018-08-30T14:32:02Z

crypto/keys/keybase.go

+}
+
+func addrKey(address tmcrypto.Address) []byte {
+	return []byte(fmt.Sprintf("%s.address", address.String()))


Since we have more than one key type now, I think we should have these two suffixes as constants.

mslipper · 2018-09-07T19:44:39Z

Hmm.. I'm on the int. test failures.

mslipper · 2018-09-09T13:15:37Z

@alexanderbez finally figured this one out... the populateFromFields method needed to be a pointer receiver since it mutated state.

alexanderbez

@mslipper tested the changes and they work (I only tested sends)!

However, I left a bit of final feedback. Also, I think this needs a rebase/merge with develop. Thanks!

PENDING.md

alexanderbez · 2018-09-10T21:54:29Z

crypto/keys/keybase.go

+func (kb dbKeybase) GetByAddress(address types.AccAddress) (Info, error) {
+	ik := kb.db.Get(addrKey(address))
+	if len(ik) == 0 {
+		return nil, fmt.Errorf("key with address %s not found", address.String())


No need to actually use String() here as Go will automatically do this for you (if the type implements Stringer) 👍

alexanderbez · 2018-09-10T21:56:43Z

client/context/query.go

+}
+
+// NOTE: mutates state so must be pointer receiver
+func (ctx *CLIContext) populateFromFields() error {


We'll need to think of another way implementing this functionality. The goal and point of contexts is that they are never modified, hence the ctx = ctx.With* logic. In our case here, maybe we'll need to to ctx = populateFromFields(ctx) somewhere upstream?

Ok, I'll figure out a solution without the pointer receiver.

crypto/keys/keybase.go

alexanderbez

Just a few minor comments @mslipper and I think we're ready for merge 👍

alexanderbez · 2018-09-20T13:41:12Z

client/context/context.go

+
+	keybase, err := keys.GetKeyBase()
+	if err != nil {
+		fmt.Println("No keybase found.")


no keybase found*

alexanderbez · 2018-09-20T13:41:27Z

client/context/context.go

@@ -106,6 +116,36 @@ func createCertifier() tmlite.Certifier {
 	return certifier
 }

+// NOTE: mutates state so must be pointer receiver


I think we can remove this note 👍

alexanderbez · 2018-09-20T13:42:50Z

client/context/context.go

@@ -106,6 +116,36 @@ func createCertifier() tmlite.Certifier {
 	return certifier
 }

+// NOTE: mutates state so must be pointer receiver
+func fromFields(from string) (types.AccAddress, string) {


func getFromFields(name string) (fromAddr types.AccAddress, fromName string) { *?

rigelrozanski · 2018-09-22T00:39:54Z

Cool - let's merge this after bez comments addressed

alexanderbez · 2018-09-25T18:34:21Z

@mslipper by any chance would you have the time to address the minor remarks left before we merge? Would be great if we could get this in before the next release 🎉

alexanderbez

utACK -- can be merged @rigelrozanski

mslipper requested review from cwgoes, ebuchman and rigelrozanski as code owners August 17, 2018 09:32

mslipper force-pushed the issue-1735 branch from 963da09 to a47efc5 Compare August 17, 2018 09:33

alexanderbez added C:CLI T: UX labels Aug 17, 2018

alexanderbez suggested changes Aug 17, 2018

View reviewed changes

alexanderbez changed the title ~~Allow --from to be a name or an address~~ WIP: Allow --from to be a name or an address Aug 17, 2018

alexanderbez added the wip label Aug 17, 2018

jackzampolin added ready-for-review and removed wip labels Aug 21, 2018

jackzampolin changed the title ~~WIP: Allow --from to be a name or an address~~ R4R: Allow --from to be a name or an address Aug 21, 2018

jaekwon reviewed Aug 21, 2018

View reviewed changes

mslipper force-pushed the issue-1735 branch 2 times, most recently from 246ab1a to 564d259 Compare August 23, 2018 08:29

melekes reviewed Aug 29, 2018

View reviewed changes

mslipper added 4 commits August 30, 2018 01:19

Allow --from to be a name or an address

ae91972

Closes cosmos#1735.

Post-rebase fixes

cd1cb15

Updates from code review

b904fd5

Updates from code review

ac4bc2a

mslipper force-pushed the issue-1735 branch from 564d259 to ac4bc2a Compare August 30, 2018 08:42

alexanderbez suggested changes Aug 30, 2018

View reviewed changes

mslipper added 2 commits September 7, 2018 09:25

Updates from code review

81df6f4

Merge remote-tracking branch 'upstream/develop' into issue-1735

9611469

Fix merge artifacts

1ee14d5

mslipper added 3 commits September 9, 2018 05:11

Merge remote-tracking branch 'upstream/develop' into issue-1735

a24beae

Fix merge conflicts

e026164

Fix integration tests

77f958f

mslipper added 2 commits September 9, 2018 06:37

Add back GetFromName() check broken during merge

3a6ac88

Merge remote-tracking branch 'upstream/develop' into issue-1735

d89664b

alexanderbez suggested changes Sep 10, 2018

View reviewed changes

fedekunze reviewed Sep 12, 2018

View reviewed changes

crypto/keys/keybase.go Show resolved Hide resolved

mslipper added 3 commits September 18, 2018 21:42

Merge remote-tracking branch 'upstream/develop' into issue-1735

d4ad0ff

Code review updates

ed7d43c

Fix failing test

ac3325c

mslipper force-pushed the issue-1735 branch from 5a7d10f to ac3325c Compare September 19, 2018 06:37

alexanderbez suggested changes Sep 20, 2018

View reviewed changes

Updates from code review

0799aa8

alexanderbez approved these changes Sep 25, 2018

View reviewed changes

rigelrozanski merged commit 24413a3 into cosmos:develop Sep 25, 2018


		var info cskeys.Info

		if strings.HasPrefix(ctx.From, sdk.Bech32PrefixAccAddr) {

R4R: Allow --from to be a name or an address #2073

R4R: Allow --from to be a name or an address #2073

Conversation

mslipper commented Aug 17, 2018 • edited by alexanderbez Loading

alexanderbez 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

jackzampolin commented Aug 21, 2018

mslipper commented Aug 22, 2018

alexanderbez commented Aug 23, 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

codecov bot commented Aug 30, 2018 • edited Loading

Codecov Report

alexanderbez 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

Choose a reason for hiding this comment

mslipper commented Sep 7, 2018

mslipper commented Sep 9, 2018

alexanderbez left a 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 left a 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

rigelrozanski commented Sep 22, 2018

alexanderbez commented Sep 25, 2018

alexanderbez left a comment

Choose a reason for hiding this comment

mslipper commented Aug 17, 2018 •

edited by alexanderbez

Loading

alexanderbez left a comment •

edited

Loading

codecov bot commented Aug 30, 2018 •

edited

Loading

alexanderbez left a comment •

edited

Loading