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

[R4R] Unmerklize ConsensusParams.Hash() #2609

Merged
merged 4 commits into from Oct 15, 2018
Merged

[R4R] Unmerklize ConsensusParams.Hash() #2609

merged 4 commits into from Oct 15, 2018

Conversation

mossid
Copy link
Contributor

@mossid mossid commented Oct 11, 2018 •
edited by ebuchman

  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG_PENDING.md

Closes: #2602

@mossid mossid changed the title Unmerklize ConsensusParams.Hash() [WIP] Unmerklize ConsensusParams.Hash() Oct 11, 2018
@mossid mossid changed the title [WIP] Unmerklize ConsensusParams.Hash() [R4R] Unmerklize ConsensusParams.Hash() Oct 11, 2018
@codecov-io
Copy link

codecov-io commented Oct 11, 2018 •
edited

Codecov Report

Merging #2609 into develop will increase coverage by 0.08%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #2609      +/-   ##
===========================================
+ Coverage    61.26%   61.34%   +0.08%     
===========================================
  Files          203      203              
  Lines        16797    16759      -38     
===========================================
- Hits         10290    10281       -9     
+ Misses        5633     5608      -25     
+ Partials       874      870       -4
Impacted Files Coverage Δ
libs/db/remotedb/remotedb.go 36.84% <0%> (-4.69%) ⬇️
libs/events/events.go 93.2% <0%> (+0.27%) ⬆️
consensus/reactor.go 71.74% <0%> (+0.64%) ⬆️
libs/clist/clist.go 68.18% <0%> (+1.51%) ⬆️

@mossid mossid requested a review from zramsay as a code owner October 11, 2018 16:49
ValarDragon
ValarDragon approved these changes Oct 11, 2018
View reviewed changes
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.

Looks good to me! I think the spec could use some more of the motivation written as well (same comments as in the godoc would suffice)

melekes
melekes approved these changes Oct 12, 2018
View reviewed changes
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

🍰 🌮 🍉

ebuchman
ebuchman approved these changes Oct 15, 2018
View reviewed changes
Copy link
Contributor

@ebuchman ebuchman left a comment

Choose a reason for hiding this comment

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

Needs changelog update

@ebuchman
Copy link
Contributor

I'll push the changelog update

@ebuchman
Copy link
Contributor

Oh I can't push to this branch because it's a fork :) - I'll merge and fix later. Thanks!

@ebuchman ebuchman merged commit 4ab7dcf into tendermint:develop Oct 15, 2018
@ebuchman ebuchman mentioned this pull request Oct 15, 2018
Closed
@jackzampolin jackzampolin mentioned this pull request Oct 15, 2018
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@melekes melekes melekes approved these changes

@ebuchman ebuchman ebuchman approved these changes

@ValarDragon ValarDragon ValarDragon approved these changes

@xla xla Awaiting requested review from xla

@zramsay zramsay Awaiting requested review from zramsay

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mossid @codecov-io @ebuchman @melekes @ValarDragon