R4R: Allow overriding of CLI transaction response handling #3276

aaronc · 2019-01-10T20:16:43Z

Currently printing of tx results in the cli is pretty basic and encodes things like Tags as byte arrays whereas in most cases they are actually strings. Also, in my case I would sometimes like to use Data to return useful information to the client.

This is just one of many approaches that could be used, but is pretty flexible in that it lets cli commands customize the output.

Open for discussion.

Targeted PR against correct branch (see CONTRIBUTING.md)
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)

cwgoes · 2019-01-11T12:06:53Z

Ref #3277; cc @sunnya97

jackzampolin · 2019-01-17T22:32:14Z

@aaronc Thank you for sharing this. I'm currently refactoring that code.

aaronc · 2019-01-18T01:22:48Z

So sounds like what you're working on will supercede this PR @jackzampolin ?

jackzampolin · 2019-01-18T01:29:21Z

@aaronc Yup. Was going to follow roughly this approach. When working on this I also found a bunch of code in the context package that needs some refactor. Working on that now. You can check out the query responses PR here: #3320

aaronc · 2019-01-29T16:08:55Z

@jackzampolin I'm not seeing this addressed in the PR that you referenced or any PR that has been merged. I do see you're working on #3321, but I also don't see it addressed there.

What I want is first and foremost the ability to override the default response printing if I need something different and secondly, I'd like the default response printing to be better. For me, that means printing Tags and usually Data as ASCII strings as opposed to byte arrays or base64. But I think the ability to actually set a custom response handler should be there regardless because different CLI authors may have different needs. Is there somewhere these concerns are being addressed?

alexanderbez · 2019-01-29T16:38:07Z

@aaronc you are correct and in fact I do recall seeing a PR that did address this but maybe it was closed as I cannot find it. Essentially it simply decoded any raw bytes from the response. I believe it was mainly the tags.

jackzampolin · 2019-01-29T17:00:06Z

@aaronc We have prioritized this work to be prelaunch. We are planning to complete it during this sprint (sometime in the next 2 weeks). Since this PR wasn't complete, and you weren't pushing more commits I've gone ahead and closed it. If this is in error please reopen.

aaronc · 2019-01-29T18:59:57Z

Okay, well seems like this is still relevant and I'm making some tweaks. Can you reopen it please @jackzampolin? I don't seem to be able.

codecov · 2019-01-29T19:23:32Z

Codecov Report

Merging #3276 into develop will increase coverage by <.01%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #3276      +/-   ##
===========================================
+ Coverage    55.33%   55.34%   +<.01%     
===========================================
  Files          135      135              
  Lines         9596     9598       +2     
===========================================
+ Hits          5310     5312       +2     
  Misses        3954     3954              
  Partials       332      332

codecov · 2019-01-29T19:23:33Z

Codecov Report

Merging #3276 into develop will decrease coverage by 0.02%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #3276      +/-   ##
===========================================
- Coverage    59.41%   59.39%   -0.03%     
===========================================
  Files          135      135              
  Lines         9975     9975              
===========================================
- Hits          5927     5925       -2     
- Misses        3676     3678       +2     
  Partials       372      372

…neric

on top) [finishes #163538725]

aaronc · 2019-01-29T22:12:58Z

I've updated this to be a little more generic - overriding all of the default response handling behavior in broadcastTxCommit if the user wants.

I haven't updated docs or written tests as the checklist requests because as far as I know there are really no docs for the CLI and I'm not sure what the testing procedure would be for something like this if it's even needed. So I'm going to change this to R4R and if any of that stuff is needed in some form, please let me know.

alessio

Rationale of this is a tad unclear to me. Please elaborate.

client/context/broadcast.go

…e of broadcastTxCommit

aaronc · 2019-01-30T00:53:46Z

@alessio I put more context on what I'm looking for in related issue #2953. For now, I think this PR could be considered the "escape hatch" solution which from my position as an SDK user feels really helpful to have even if the default formatting is improved which I hope happens too.

Maybe there should be a better name than ResponseHandler it's just what occurred to me at the time.

alexanderbez

Thanks @aaronc -- this looks clean! I left a few remarks. Please lmk your thoughts 👍

alexanderbez · 2019-01-30T16:04:19Z

client/context/broadcast.go

@@ -131,6 +131,11 @@ func (ctx CLIContext) broadcastTxCommit(txBytes []byte) (*ctypes.ResultBroadcast
 		return res, err
 	}

+	if ctx.ResponseHandler != nil {


I suppose this is a flexible way of doing this. May I suggest a few things?

Rename ResponseHandler to TxResponseHandler to provide better context

Take the two if statements below this and stuff them into a DefaultTxResponseHandler. This will DRY and cleanup broadcastTxCommit.

Perhaps implement another TxResponseHandler that pretty prints the response (this may require merging @sunnya97's PR).

Rename ResponseHandler to TxResponseHandler to provide better context

Take the two if statements below this and stuff them into a DefaultTxResponseHandler. This will DRY and cleanup broadcastTxCommit.

Okay, pushed these two changes. I added error as the return to TxResponseHandler because MarshalJSON could return an error in DefaultTxResponseHandler. Looks like the error is handled higher up. I agree this is nice for DRY because it would allow someone to just wrap the default functionality.

Perhaps implement another TxResponseHandler that pretty prints the response (this may require merging @sunnya97's PR).

Happy to help with that. Is it okay if it's in a separate PR connected with the discussion in #2953 ?

…seHandler from body of broadcastTxCommit.

aaronc · 2019-01-30T19:42:36Z

Looks like linting failed in CI because of some unrelated error. Can someone familiar with that setup take a look?

jackzampolin · 2019-02-08T01:51:11Z

This has been fixed via: #3451 going to go ahead and close this.

on top) [finishes #163538725]

aaronc requested review from cwgoes, ebuchman and rigelrozanski as code owners January 10, 2019 20:16

aaronc changed the title ~~Regen network/cli handle result~~ More flexible printing of tx responses in the cli Jan 10, 2019

aaronc changed the title ~~More flexible printing of tx responses in the cli~~ WIP: More flexible printing of tx responses in the cli Jan 11, 2019

alessio self-requested a review January 15, 2019 20:36

jackzampolin closed this Jan 29, 2019

aaronc changed the title ~~WIP: More flexible printing of tx responses in the cli~~ WIP: Allow overriding of CLI transaction response handling Jan 29, 2019

jackzampolin reopened this Jan 29, 2019

aaronc added 3 commits January 29, 2019 16:07

Added ResponseHandler field to CLIContext

64554b6

Change ResponseHandler -> ResponsePrinter, set to resStr

465c660

Change ResponsePrinter -> ResponseHandler making the override more ge…

707f364

…neric

aaronc force-pushed the regen-network/cli-handle-result branch from f1a9f06 to 707f364 Compare January 29, 2019 21:09

Update docstring, cleanup indentation

6e1f27e

aaronc added a commit to regen-network/regen-ledger that referenced this pull request Jan 29, 2019

Upgrade to latest cosmos-sdk (plus our pending PR cosmos/cosmos-sdk#3276

a57b3c1

on top) [finishes #163538725]

aaronc changed the title ~~WIP: Allow overriding of CLI transaction response handling~~ R4R: Allow overriding of CLI transaction response handling Jan 29, 2019

alessio suggested changes Jan 29, 2019

View reviewed changes

client/context/broadcast.go Outdated Show resolved Hide resolved

Simplified ResponseHandler - no need for it to handle the return valu…

b2410f6

…e of broadcastTxCommit

aaronc mentioned this pull request Jan 30, 2019

Intelligently format transaction responses & errors in CLI #2953

Closed

alexanderbez suggested changes Jan 30, 2019

View reviewed changes

Renamed ResponseHandler -> TxResponseHandler. Created DefaultTxRespon…

d0006b5

…seHandler from body of broadcastTxCommit.

jackzampolin closed this Feb 8, 2019

aaronc added a commit to regen-network/testnets that referenced this pull request May 31, 2019

Upgrade to latest cosmos-sdk (plus our pending PR cosmos/cosmos-sdk#3276

40fe6dd

on top) [finishes #163538725]

ryanchristo deleted the regen-network/cli-handle-result branch December 12, 2022 18:14

Beardev118 pushed a commit to RegenNetwork/regen-ledger that referenced this pull request Oct 3, 2023

Upgrade to latest cosmos-sdk (plus our pending PR cosmos/cosmos-sdk#3276

db9c3ef

on top) [finishes #163538725]

Beardev118 added a commit to RegenNetwork/testnets that referenced this pull request Oct 3, 2023

Upgrade to latest cosmos-sdk (plus our pending PR cosmos/cosmos-sdk#3276

7782b0b

on top) [finishes #163538725]

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Allow overriding of CLI transaction response handling #3276

R4R: Allow overriding of CLI transaction response handling #3276

aaronc commented Jan 10, 2019

cwgoes commented Jan 11, 2019 •

edited

Loading

jackzampolin commented Jan 17, 2019

aaronc commented Jan 18, 2019

jackzampolin commented Jan 18, 2019

aaronc commented Jan 29, 2019

alexanderbez commented Jan 29, 2019

jackzampolin commented Jan 29, 2019

aaronc commented Jan 29, 2019

codecov bot commented Jan 29, 2019

codecov bot commented Jan 29, 2019 •

edited

Loading

aaronc commented Jan 29, 2019

alessio left a comment

aaronc commented Jan 30, 2019

alexanderbez left a comment

alexanderbez Jan 30, 2019

aaronc Jan 30, 2019

aaronc commented Jan 30, 2019

jackzampolin commented Feb 8, 2019

R4R: Allow overriding of CLI transaction response handling #3276

R4R: Allow overriding of CLI transaction response handling #3276

Conversation

aaronc commented Jan 10, 2019

cwgoes commented Jan 11, 2019 • edited Loading

jackzampolin commented Jan 17, 2019

aaronc commented Jan 18, 2019

jackzampolin commented Jan 18, 2019

aaronc commented Jan 29, 2019

alexanderbez commented Jan 29, 2019

jackzampolin commented Jan 29, 2019

aaronc commented Jan 29, 2019

codecov bot commented Jan 29, 2019

Codecov Report

codecov bot commented Jan 29, 2019 • edited Loading

Codecov Report

aaronc commented Jan 29, 2019

alessio left a comment

Choose a reason for hiding this comment

aaronc commented Jan 30, 2019

alexanderbez left a comment

Choose a reason for hiding this comment

alexanderbez Jan 30, 2019

Choose a reason for hiding this comment

aaronc Jan 30, 2019

Choose a reason for hiding this comment

aaronc commented Jan 30, 2019

jackzampolin commented Feb 8, 2019

cwgoes commented Jan 11, 2019 •

edited

Loading

codecov bot commented Jan 29, 2019 •

edited

Loading