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.

Sign up for GitHub

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

Jump to bottom

Move client/{tx,rest,utils} into x/auth/client #4555

Merged
merged 8 commits into from
Jun 15, 2019
Merged

Move client/{tx,rest,utils} into x/auth/client #4555

merged 8 commits into from
Jun 15, 2019

Conversation

alessio
Copy link
Contributor

@alessio alessio commented Jun 14, 2019 •
edited by alexanderbez
Loading

Ref: #4488

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added a relevant changelog entry: clog add [section] [stanza] [message]
  • 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)

@alessio alessio requested review from sabau and fedekunze June 14, 2019 13:34
@codecov
Copy link

codecov bot commented Jun 14, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@941effc). Click here to learn what that means.
The diff coverage is 29.62%.

@@            Coverage Diff            @@
##             master    #4555   +/-   ##
=========================================
  Coverage          ?   52.65%           
=========================================
  Files             ?      261           
  Lines             ?    16410           
  Branches          ?        0           
=========================================
  Hits              ?     8640           
  Misses            ?     7124           
  Partials          ?      646

alexanderbez
alexanderbez approved these changes Jun 15, 2019
View reviewed changes
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

ACK

@alexanderbez alexanderbez added the T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). label Jun 15, 2019
fedekunze
fedekunze approved these changes Jun 15, 2019
View reviewed changes
Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

ACK. 1 minor nit but not blocking

x/auth/client/rest/rest.go

// TODO: Change path or mount under x/bank if possible
r.HandleFunc(
"/bank/balances/{address}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we delete this endpoint? it's a dup of the one above

Copy link
Contributor

@alexanderbez alexanderbez Jun 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Hmmm we should't remove it but rather move or rename it. One endpoint returns the entire account and the other returns the balance.

I'll handle this in a subsequent PR ⚡️

@alexanderbez alexanderbez merged commit 73700df into master Jun 15, 2019
@alexanderbez alexanderbez deleted the alessio/move-client-pkgs-under-auth branch June 15, 2019 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexanderbez alexanderbez alexanderbez approved these changes

@fedekunze fedekunze fedekunze approved these changes

@sabau sabau Awaiting requested review from sabau

@jackzampolin jackzampolin Awaiting requested review from jackzampolin

@rigelrozanski rigelrozanski Awaiting requested review from rigelrozanski

Assignees
No one assigned
Labels
T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine).
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alessio @alexanderbez @fedekunze