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

x/ibc: refactor CLI #6639

Merged
merged 9 commits into from
Jul 8, 2020
Merged

x/ibc: refactor CLI #6639

merged 9 commits into from
Jul 8, 2020

Conversation

fedekunze
Copy link
Collaborator

@fedekunze fedekunze commented Jul 8, 2020

Description

ref: #6423


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • 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.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@fedekunze fedekunze added Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity. x/ibc labels Jul 8, 2020
@@ -175,17 +176,3 @@ $ %s query ibc client node-state
},
}
}

// GetCmdQueryPath defines the command to query the commitment path.
func GetCmdQueryPath(clientCtx client.Context) *cobra.Command {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this query is not required anymore

@fedekunze fedekunze added the A:automerge Automatically merge PR once all prerequisites pass. label Jul 8, 2020
@fedekunze fedekunze requested a review from colin-axner July 8, 2020 09:04
@codecov
Copy link

codecov bot commented Jul 8, 2020

Codecov Report

Merging #6639 into master will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##           master    #6639   +/-   ##
=======================================
  Coverage   57.73%   57.73%           
=======================================
  Files         497      497           
  Lines       29767    29767           
=======================================
  Hits        17186    17186           
  Misses      11343    11343           
  Partials     1238     1238           

Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

lgtm overall, we could also refactor GetQueryCmd in this PR too?

@@ -55,18 +54,16 @@ $ %s query ibc client states
// a given id as defined in https://github.com/cosmos/ics/tree/master/spec/ics-002-client-semantics#query
func GetCmdQueryClientState(clientCtx client.Context) *cobra.Command {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could remove client.Context arg from GetCmd* as part of this refactor too

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you need to pass in the clientCtx afaik. cc: @alexanderbez

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually @fedekunze we need to remove it. A client.Context should never be passed as an arg -- query or tx. I'm not sure how I missed this in x/bank. Thanks for catching this @amaurymartiny!

x/bank PR: #6645

x/ibc/09-localhost/client/cli/cli.go Outdated Show resolved Hide resolved
x/ibc/04-channel/types/channel.go Show resolved Hide resolved
Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

utACK

x/ibc/04-channel/client/cli/query.go Show resolved Hide resolved
@fedekunze fedekunze merged commit e6bb2e7 into master Jul 8, 2020
@fedekunze fedekunze deleted the ibc-refactor-cli branch July 8, 2020 09:58
@aaronc aaronc mentioned this pull request Jul 10, 2020
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants