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: Fix duplicate json key in SetWithdrawAddress #2944

Merged
merged 2 commits into from
Nov 29, 2018
Merged

R4R: Fix duplicate json key in SetWithdrawAddress #2944

merged 2 commits into from
Nov 29, 2018

Conversation

hendrikhofstadt
Copy link
Contributor

@hendrikhofstadt hendrikhofstadt commented Nov 29, 2018

Closes #2940

  • 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)

@fedekunze
Copy link
Collaborator

I like this idea. We need to double check that the msg type matches the tag, otherwise we should label this as breaking changes.

Also please make sure to update https://github.com/cosmos/cosmos-sdk/blob/develop/docs/gaia/gaiacli.md#matching-a-set-of-tags

@hendrikhofstadt
Copy link
Contributor Author

@fedekunze Yes they indeed don't match. I am currently fixing all tests and will update everything as you said 👍

@hendrikhofstadt hendrikhofstadt changed the title Fix duplicate json key in SetWithdrawAddress R4R: Fix duplicate json key in SetWithdrawAddress Nov 29, 2018
@codecov
Copy link

codecov bot commented Nov 29, 2018

Codecov Report

Merging #2944 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #2944   +/-   ##
========================================
  Coverage    56.32%   56.32%           
========================================
  Files          120      120           
  Lines         8417     8417           
========================================
  Hits          4741     4741           
  Misses        3354     3354           
  Partials       322      322

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.

LGTM. I think the integration_tests is a regression due to the latest TM update? Not sure.

@hendrikhofstadt
Copy link
Contributor Author

@alexanderbez Thanks for the review 👍 yes it is. I fixed the integration test in #2943 and we are tracking the root cause in #2945

@alexanderbez
Copy link
Contributor

Thanks for the update and staying on top of it @SLAMPER!

@alessio alessio self-requested a review November 29, 2018 15:36
Copy link
Contributor

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

Awesome!

@jackzampolin
Copy link
Member

This is now failing due to an out of date IAVL dep. Can you rebase on develop @SLAMPER?

@hendrikhofstadt
Copy link
Contributor Author

@jackzampolin uhm. I just changed a single string. Sure that I have to rebase ?

@jackzampolin
Copy link
Member

@SLAMPER The commit that tendermint/iavl was pinned to got squashed, so yes.

@hendrikhofstadt
Copy link
Contributor Author

@jackzampolin Oh just saw it. Did a rebase 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants