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

Increase poll::Validate visibility to pub #956

Merged
merged 2 commits into from
Jun 18, 2021

Conversation

jkczyz
Copy link
Contributor

@jkczyz jkczyz commented Jun 17, 2021

Widening the visibility to pub relaxes this requirement permits either copying or moving lightning-block-sync's test utilities to a separate crate since they use poll::Validate.

@codecov
Copy link

codecov bot commented Jun 17, 2021

Codecov Report

Merging #956 (7302c8c) into main (2940099) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #956      +/-   ##
==========================================
- Coverage   90.59%   90.57%   -0.02%     
==========================================
  Files          60       60              
  Lines       30423    30423              
==========================================
- Hits        27561    27555       -6     
- Misses       2862     2868       +6     
Impacted Files Coverage Δ
lightning-block-sync/src/poll.rs 91.66% <ø> (ø)
lightning/src/ln/functional_tests.rs 97.16% <0.00%> (-0.09%) ⬇️
lightning/src/ln/channelmanager.rs 83.85% <0.00%> (-0.05%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2940099...7302c8c. Read the comment docs.

@TheBlueMatt
Copy link
Collaborator

Hmm, do we actually need to expose Validate fully? It seems if we just want to expose it to callers, but not implementors, we could seal it instead?

@jkczyz
Copy link
Contributor Author

jkczyz commented Jun 17, 2021

Hmm, do we actually need to expose Validate fully? It seems if we just want to expose it to callers, but not implementors, we could seal it instead?

Yeah, just needed for callers.

@jkczyz jkczyz force-pushed the 2021-06-validate-pub branch from 2f7065e to 14b79bc Compare June 17, 2021 22:53
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

LGTM aside from the one comment

jkczyz added 2 commits June 18, 2021 00:26
Previously, poll::Validate was pub(crate) to force external implementors
of Poll to define their implementation in terms of ChainPoller. This was
because ChainPoller::look_up_previous_header checks for consistency
between headers and any errors would be checked at that level rather
than by the caller. Otherwise, if performed by the caller, a Poll
implementation would not be aware if the underlying BlockSource was
providing bad data and therefore couldn't react accordingly.

Widening the visibility to pub relaxes this requirement, although it's
still encourage to use ChainPoller to implement Poll. This permits
either copying or moving lightning-block-sync's test utilities to a
separate crate since they use poll::Validate.
@jkczyz jkczyz force-pushed the 2021-06-validate-pub branch from 14b79bc to 7302c8c Compare June 18, 2021 05:27
@TheBlueMatt TheBlueMatt merged commit 4d1c1a3 into lightningdevkit:main Jun 18, 2021
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.

3 participants