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: CLIContext Refactor #3321

Closed
wants to merge 14 commits into from
Closed

R4R: CLIContext Refactor #3321

wants to merge 14 commits into from

Conversation

jackzampolin
Copy link
Member

@jackzampolin jackzampolin commented Jan 18, 2019

Fixes: #3249 #3255 #3178 #2953 #2856

This PR is the follow up to #3320 and contains:

  • A cleanup of unused methods and code in CLIContext
  • Merges the client/utils package into client/context package
  • Adds the auth.TxBuilder to the CLIContext object and ensures it is properly initialized at the times it is called.

I've also tried to add some comments explaining the code. Hopefully this helps reviewers.

  • 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
Copy link

codecov bot commented Jan 18, 2019

Codecov Report

Merging #3321 into develop will decrease coverage by 2.41%.
The diff coverage is 5.01%.

@@             Coverage Diff             @@
##           develop    #3321      +/-   ##
===========================================
- Coverage    58.88%   56.47%   -2.42%     
===========================================
  Files          131      137       +6     
  Lines         9836    10247     +411     
===========================================
- Hits          5792     5787       -5     
- Misses        3669     4084     +415     
- Partials       375      376       +1

client/context/tx.go Outdated Show resolved Hide resolved
client/context/context.go Outdated Show resolved Hide resolved
client/context/context.go Outdated Show resolved Hide resolved
@jackzampolin jackzampolin changed the base branch from jack/cli-outputs to develop January 23, 2019 23:00
@jackzampolin jackzampolin changed the title WIP: CLIContext Refactor R4R: CLIContext Refactor Jan 24, 2019
@jackzampolin
Copy link
Member Author

jackzampolin commented Jan 24, 2019

CLIContext is once again treated as an immutable object. This PR is ready for review again.

@@ -34,6 +28,7 @@ type CLIContext struct {
Codec *codec.Codec
AccDecoder auth.AccountDecoder
Client rpcclient.Client
TxBldr authtxb.TxBuilder
Copy link
Contributor

@alessio alessio Jan 24, 2019

Choose a reason for hiding this comment

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

IMO it should be TxBuilder to encapsulate a CLIContext - rationale:

  1. CLIContext never calls TxBuilder methods. The opposite is true, thus TxBuilder should depend on CLIContext, not the contrary.
  2. This introduces a dependency on github.com/cosmos/cosmos-sdk/x/auth/client/txbuilder. Again, should be TxBuilder dependending on CLIContext

Copy link
Member Author

Choose a reason for hiding this comment

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

Would be interested to hear other thoughts from the team @cosmos/cosmossdk

Copy link
Contributor

Choose a reason for hiding this comment

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

@alessio's points make sense to me - are there counterarguments?

Copy link
Contributor

Choose a reason for hiding this comment

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

Totally agree here. I also think the fromName and fromAddress fields (and corresponding logic to get these values) should be moved into the TxBuilder.

Copy link
Contributor

Choose a reason for hiding this comment

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

In all honesti, I think we'd better keep CLIContext and TxBuilder separate to avoid establishing any unnecessary dependency link between the two. For sake of caller's convenience though, I'm OK with TxBuilder to encapsulate .CLIContext.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeahhh, I really don't see a big reason for this PR then imho. Sure, the CLI context can be cleaned up a bit (some of which I'll be doing in #3396)

client/context/context.go Outdated Show resolved Hide resolved
client/context/context.go Outdated Show resolved Hide resolved
client/context/context.go Outdated Show resolved Hide resolved
client/context/context.go Outdated Show resolved Hide resolved
client/context/context.go Outdated Show resolved Hide resolved
client/context/errors.go Outdated Show resolved Hide resolved
client/context/tx.go Outdated Show resolved Hide resolved
client/context/tx.go Outdated Show resolved Hide resolved
client/context/tx.go Outdated Show resolved Hide resolved
client/context/tx.go Show resolved Hide resolved
@alexanderbez
Copy link
Contributor

Please allow me to review this prior to merging @jackzampolin (will do shortly).

@jackzampolin
Copy link
Member Author

Well @alexanderbez sounds like theres no alignment here so I wouldn't merge anyway.

@jackzampolin
Copy link
Member Author

Going to go ahead and close this in favor of other work on these same issues.

@jackzampolin jackzampolin deleted the jack/clictx-refactor branch January 31, 2019 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants