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

lite2: indicate success/failure of Update #4536

Merged
merged 6 commits into from
Mar 6, 2020
Merged

Conversation

melekes
Copy link
Contributor

@melekes melekes commented Mar 5, 2020

Description

error itself is not enough since it only signals if there were any
errors. Either (types.SignedHeader) or (success bool) is needed to
indicate the status of the operation. Returning a header is optimal
since most of the clients will want to get a newly verified header
anyway.


For contributor use:

  • Wrote tests
  • Updated CHANGELOG_PENDING.md
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments
  • Re-reviewed Files changed in the Github PR explorer

`error` itself is not enough since it only signals if there were any
errors. Either (types.SignedHeader) or (success bool) is needed to
indicate the status of the operation. Returning a header is optimal
since most of the clients will want to get a newly verified header
anyway.
@melekes melekes added the C:light Component: Light label Mar 5, 2020
@melekes melekes requested a review from cmwaters March 5, 2020 18:57
@melekes melekes self-assigned this Mar 5, 2020
Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

👍 -> the only qualm I have is that if you wanted to grab the latest header on the chain, you would have to first run Update() and then check if the header was nil, and if so then run TrustedHeader(0). I guess this is a matter of: is signalling that the state of the light client has updated more important than retrieving the latest header, regardless of whether it has been verified before.

@codecov-io
Copy link

Codecov Report

Merging #4536 into master will increase coverage by 0.07%.
The diff coverage is 28.57%.

@@            Coverage Diff             @@
##           master    #4536      +/-   ##
==========================================
+ Coverage   65.26%   65.33%   +0.07%     
==========================================
  Files         229      229              
  Lines       20272    20273       +1     
==========================================
+ Hits        13230    13245      +15     
+ Misses       5988     5979       -9     
+ Partials     1054     1049       -5
Impacted Files Coverage Δ
lite2/client.go 72.18% <28.57%> (-0.16%) ⬇️
crypto/sr25519/pubkey.go 32.14% <0%> (-7.15%) ⬇️
blockchain/v2/reactor.go 35.31% <0%> (-3.41%) ⬇️
consensus/state.go 77.58% <0%> (+0.51%) ⬆️
p2p/pex/pex_reactor.go 83.05% <0%> (+0.56%) ⬆️
consensus/reactor.go 79.58% <0%> (+0.57%) ⬆️
blockchain/v0/reactor.go 78.77% <0%> (+0.94%) ⬆️
privval/signer_listener_endpoint.go 89.13% <0%> (+2.17%) ⬆️
libs/events/events.go 98.05% <0%> (+4.85%) ⬆️
privval/signer_endpoint.go 81.08% <0%> (+5.4%) ⬆️

@melekes melekes merged commit d3f965b into master Mar 6, 2020
@melekes melekes deleted the anton/lite2-update branch March 6, 2020 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:light Component: Light
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants