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

Refactor updateState #2865 #2929

Merged
merged 3 commits into from Nov 28, 2018

Conversation

danil-lashin
Copy link
Contributor

@danil-lashin danil-lashin commented Nov 28, 2018

Fixes #2865

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

@codecov-io
Copy link

codecov-io commented Nov 28, 2018

Codecov Report

Merging #2929 into master will increase coverage by 0.02%.
The diff coverage is 76.47%.

@@            Coverage Diff             @@
##           master    #2929      +/-   ##
==========================================
+ Coverage   62.73%   62.76%   +0.02%     
==========================================
  Files         212      212              
  Lines       17410    17409       -1     
==========================================
+ Hits        10922    10926       +4     
+ Misses       5572     5568       -4     
+ Partials      916      915       -1
Impacted Files Coverage Δ
state/execution.go 76.31% <76.47%> (-0.11%) ⬇️
privval/tcp_server.go 78.57% <0%> (-2.86%) ⬇️
consensus/reactor.go 66.51% <0%> (+0.23%) ⬆️
rpc/client/httpclient.go 69.75% <0%> (+0.97%) ⬆️
privval/ipc_server.go 70.37% <0%> (+5.55%) ⬆️

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.

🍃 🌴 🌤

Thank you

state/execution_test.go Outdated Show resolved Hide resolved
state/state_test.go Outdated Show resolved Hide resolved
state/state_test.go Outdated Show resolved Hide resolved
state/state_test.go Outdated Show resolved Hide resolved
state/state_test.go Outdated Show resolved Hide resolved
state/state_test.go Show resolved Hide resolved
melekes and others added 2 commits November 28, 2018 13:26
Co-Authored-By: danil-lashin <danil-lashin@yandex.ru>
@danil-lashin
Copy link
Contributor Author

@melekes done, thanks!

@danil-lashin danil-lashin changed the base branch from master to develop November 28, 2018 10:44
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.

Thanks for this!

I was hoping the abciResponses could be removed all together as an argument to updateState, since we could instead just pass in ConsensusParams. But not sure if it's worth it.

Anyways, this is fine for now. The pkg could probably continue to use some code structure improvements in the future.

@ebuchman ebuchman merged commit 7213869 into tendermint:develop Nov 28, 2018
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.

None yet

4 participants