R4R: Implement command/REST endpoint for offline signing #1953 #2216

alessio · 2018-09-03T00:32:03Z

Add sign command to sign transactions generated with the --generate-only flag.

Closes: #1953

Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added entries in PENDING.md with issue #
rereviewed Files changed in the github PR explorer

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)

codecov · 2018-09-03T00:41:42Z

Codecov Report

Merging #2216 into develop will decrease coverage by 0.22%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #2216      +/-   ##
===========================================
- Coverage    63.95%   63.73%   -0.23%     
===========================================
  Files          140      140              
  Lines         8611     8641      +30     
===========================================
  Hits          5507     5507              
- Misses        2725     2755      +30     
  Partials       379      379

jackzampolin

Nothing from me really. Looks good!

client/lcd/lcd_test.go

x/auth/client/cli/sign.go

x/auth/client/rest/sign.go

alessio · 2018-09-05T01:09:41Z

CLI tests are fixed now.

client/input.go

x/auth/client/cli/sign.go

docs/sdk/clients.md

fedekunze

LGTM. Left a few minor comments regarding docs, but logic looks good. Thanks @alessio !

docs/sdk/clients.md

x/auth/client/rest/query.go

x/auth/client/cli/sign.go

alexanderbez

@alessio left some surface level feedback. I'll want to play around with this a bit later today before an approval 👍 otherwise, great job!

x/auth/client/cli/sign.go

client/lcd/lcd_test.go

alexanderbez · 2018-09-05T16:15:57Z

x/auth/client/context/context.go

+
+// SignStdTx attach a signature to a StdTx and returns a copy of a it. If overwriteSigs is true,
+// it replaces the signatures already attached if there's any with the given signature.
+func SignStdTx(stdTx auth.StdTx, stdSignature auth.StdSignature, overwriteSigs bool) auth.StdTx {


Should we take an array/slice of signatures here instead to be more flexible?

It could work. I'll make it take a slice and either append or replace the sigs

x/auth/client/rest/sign.go

alexanderbez

@alessio just a few more minor bits of feedback.

Also, I've also tested it and seems to be functional! But I have question. I can generate a tx with a --from and that would be the designated signer/account. Now, I can sign this generated tx JSON with a completely different signer. I know this allows for offline signing, but I think generating a tx with an intended signer and signing it with a completely different one would lead to a weird "UX". We should check the generated txs signer address against the signee address and at the very least print a warning if they don't match. Thoughts @jackzampolin @cwgoes?

Thank you!

x/auth/client/cli/sign.go

x/auth/client/rest/sign.go

alexanderbez

One small remark @alessio, otherwise LGTM 👍

alexanderbez · 2018-09-06T19:12:57Z

client/utils/utils.go

+
+	// Check whether the address is a signer
+	if !isTxSigner(sdk.AccAddress(addr), stdTx.GetSigners()) {
+		fmt.Fprintf(os.Stderr, "warning: the transaction is not supposed to be signed by the key '%v'\n", name)


Thanks @alessio! I think something along the lines of the following might read better. Thoughts?

WARNING: The generated transaction's intended signer does not match the given signer: '%v'

alessio · 2018-09-06T19:16:00Z

ACKd and incorporated. Thanks!

…

On Thu, Sep 6, 2018 at 8:14 PM, Alexander Bezobchuk < ***@***.***> wrote: ***@***.**** requested changes on this pull request. One small remark @alessio <https://github.com/alessio>, otherwise LGTM 👍 ------------------------------ In client/utils/utils.go <#2216 (comment)>: > +func SignStdTx(txCtx authctx.TxContext, cliCtx context.CLIContext, name string, stdTx auth.StdTx, appendSig bool) (auth.StdTx, error) { + var signedStdTx auth.StdTx + + keybase, err := keys.GetKeyBase() + if err != nil { + return signedStdTx, err + } + info, err := keybase.Get(name) + if err != nil { + return signedStdTx, err + } + addr := info.GetPubKey().Address() + + // Check whether the address is a signer + if !isTxSigner(sdk.AccAddress(addr), stdTx.GetSigners()) { + fmt.Fprintf(os.Stderr, "warning: the transaction is not supposed to be signed by the key '%v'\n", name) Thanks @alessio <https://github.com/alessio>! I think something along the lines of the following might read better. Thoughts? WARNING: The generated transaction's intended signer does not match the given signer: '%v' — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2216 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAN_7GrGx-gTIkZuD-nALU68OsrfGhUrks5uYXQJgaJpZM4WW0yM> .

-- Alessio Treglia | alessio@tendermint.com 0416 0004 A827 6E40 BB98 90FB E8A4 8AE5 311D 765A

alexanderbez

Thanks @alessio -- tested ACK.

jackzampolin · 2018-09-06T20:22:11Z

This looks great. Happy with how this has come together. LGTM!

* Add sign CLI command to sign transactions generated with the --generate-only flag. * Add /sign REST endpoint for Voyager support. Redirect password prompt to STDERR to avoid messing up cli commands output. As a rule of thumb, program's output should always go to STDOUT, whilst errors&diagnostics go to STDERR as per POSIX's philosophy and specs.

cwgoes

Mostly LGTM, a few tiny comments.

cwgoes · 2018-09-07T13:48:26Z

Makefile

@@ -179,7 +179,7 @@ test_cover:

 test_lint:
 	gometalinter.v2 --config=tools/gometalinter.json ./...
-	!(gometalinter.v2 --disable-all --enable='errcheck' --vendor ./... | grep -v "client/")
+	!(gometalinter.v2 --disable-all --enable='errcheck' --vendor ./... | grep -v -e "client/" -e "fmt\.Fprintf")


This has since been fixed, and this is the wrong way to fix it anyways (the linter is right) - #2257

Well, by definition linter is correct, but I wonder how helpful it would be to check errors reported by fprintf() :). I'll revert my changes nonetheless, thanks

client/lcd/lcd_test.go

client/utils/utils.go

cmd/gaia/cli_test/cli_test.go

x/auth/client/rest/sign.go

cwgoes

utACK 🎉

alessio added C:CLI prelaunch labels Sep 3, 2018

alessio requested review from cwgoes, ebuchman, rigelrozanski and zramsay as code owners September 3, 2018 00:32

alessio force-pushed the alessio/1953-generate-signature branch 2 times, most recently from a278464 to 4105de8 Compare September 3, 2018 00:34

alessio force-pushed the alessio/1953-generate-signature branch 7 times, most recently from 5b12981 to c59dac6 Compare September 5, 2018 00:36

alessio changed the title ~~WIP: Alessio/1953 generate signature~~ Implement command/REST endpoint for offline signing #1953 Sep 5, 2018

jackzampolin reviewed Sep 5, 2018

View reviewed changes

client/lcd/lcd_test.go Outdated Show resolved Hide resolved

x/auth/client/cli/sign.go Show resolved Hide resolved

x/auth/client/rest/sign.go Outdated Show resolved Hide resolved

alessio added T: UX lcd and removed wip labels Sep 5, 2018

alessio requested review from jaekwon, alexanderbez, fedekunze and ValarDragon September 5, 2018 01:27

alessio force-pushed the alessio/1953-generate-signature branch from e53cbe4 to 0ada320 Compare September 5, 2018 01:29

ValarDragon reviewed Sep 5, 2018

View reviewed changes

client/input.go Show resolved Hide resolved

ValarDragon reviewed Sep 5, 2018

View reviewed changes

x/auth/client/cli/sign.go Show resolved Hide resolved

alessio changed the title ~~Implement command/REST endpoint for offline signing #1953~~ R4R: Implement command/REST endpoint for offline signing #1953 Sep 5, 2018

alessio added the ready-for-review label Sep 5, 2018

fedekunze reviewed Sep 5, 2018

View reviewed changes

docs/sdk/clients.md Outdated Show resolved Hide resolved

alessio force-pushed the alessio/1953-generate-signature branch from 0ada320 to 052bb1f Compare September 5, 2018 15:53

fedekunze approved these changes Sep 5, 2018

View reviewed changes

docs/sdk/clients.md Show resolved Hide resolved

x/auth/client/rest/query.go Show resolved Hide resolved

x/auth/client/cli/sign.go Show resolved Hide resolved

alexanderbez suggested changes Sep 5, 2018

View reviewed changes

alessio force-pushed the alessio/1953-generate-signature branch 2 times, most recently from b95da66 to 20a22ea Compare September 6, 2018 13:55

alexanderbez suggested changes Sep 6, 2018

View reviewed changes

x/auth/client/cli/sign.go Outdated Show resolved Hide resolved

x/auth/client/rest/sign.go Outdated Show resolved Hide resolved

alexanderbez suggested changes Sep 6, 2018

View reviewed changes

alexanderbez approved these changes Sep 6, 2018

View reviewed changes

alessio force-pushed the alessio/1953-generate-signature branch from 13f76e4 to 21b00a8 Compare September 6, 2018 19:22

alessio force-pushed the alessio/1953-generate-signature branch from 21b00a8 to 8e8208b Compare September 7, 2018 10:59

alessio force-pushed the alessio/1953-generate-signature branch from 8e8208b to e6a8a4d Compare September 7, 2018 11:58

cwgoes suggested changes Sep 7, 2018

View reviewed changes

ACK and incorporate @cwgoes comments

f5a7f25

cwgoes approved these changes Sep 7, 2018

View reviewed changes

cwgoes merged commit 896d259 into develop Sep 7, 2018

cwgoes deleted the alessio/1953-generate-signature branch September 7, 2018 15:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Implement command/REST endpoint for offline signing #1953 #2216

R4R: Implement command/REST endpoint for offline signing #1953 #2216

alessio commented Sep 3, 2018 •

edited

Loading

codecov bot commented Sep 3, 2018 •

edited

Loading

jackzampolin left a comment

alessio commented Sep 5, 2018

fedekunze left a comment

alexanderbez left a comment

alexanderbez Sep 5, 2018

alessio Sep 5, 2018

alexanderbez left a comment

alexanderbez left a comment

alexanderbez Sep 6, 2018

alessio commented Sep 6, 2018 via email

alexanderbez left a comment

jackzampolin commented Sep 6, 2018

cwgoes left a comment

cwgoes Sep 7, 2018

alessio Sep 7, 2018

cwgoes left a comment

R4R: Implement command/REST endpoint for offline signing #1953 #2216

R4R: Implement command/REST endpoint for offline signing #1953 #2216

Conversation

alessio commented Sep 3, 2018 • edited Loading

codecov bot commented Sep 3, 2018 • edited Loading

Codecov Report

jackzampolin left a comment

Choose a reason for hiding this comment

alessio commented Sep 5, 2018

fedekunze left a comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

alexanderbez Sep 5, 2018

Choose a reason for hiding this comment

alessio Sep 5, 2018

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

alexanderbez Sep 6, 2018

Choose a reason for hiding this comment

alessio commented Sep 6, 2018 via email

alexanderbez left a comment

Choose a reason for hiding this comment

jackzampolin commented Sep 6, 2018

cwgoes left a comment

Choose a reason for hiding this comment

cwgoes Sep 7, 2018

Choose a reason for hiding this comment

alessio Sep 7, 2018

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

alessio commented Sep 3, 2018 •

edited

Loading

codecov bot commented Sep 3, 2018 •

edited

Loading