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

app/peerinfo: implement protocol skeleton #1225

Merged
merged 2 commits into from
Oct 5, 2022
Merged

Conversation

corverroos
Copy link
Contributor

@corverroos corverroos commented Oct 5, 2022

This adds the skeleton for the new peerinfo protocol. It is aimed at sharing peer information/metadata.

The supported use-cases are:

  • Track peer clock offset
  • Track peer lock hash
  • Track peer app version

Tests will be added in a separate PR.

category: feature
ticket: #1224

@codecov
Copy link

codecov bot commented Oct 5, 2022

Codecov Report

Base: 53.20% // Head: 53.13% // Decreases project coverage by -0.06% ⚠️

Coverage data is based on head (6fa918e) compared to base (305a37e).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1225      +/-   ##
==========================================
- Coverage   53.20%   53.13%   -0.07%     
==========================================
  Files         136      136              
  Lines       15958    15958              
==========================================
- Hits         8490     8479      -11     
- Misses       6227     6234       +7     
- Partials     1241     1245       +4     
Impacted Files Coverage Δ
app/app.go 57.83% <0.00%> (-2.77%) ⬇️
core/qbft/qbft.go 81.54% <0.00%> (-0.43%) ⬇️
app/vmock.go 71.89% <0.00%> (+1.96%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Comment on lines +42 to +45
type (
tickerProvider func() (<-chan time.Time, func())
nowFunc func() time.Time
)
Copy link
Contributor

Choose a reason for hiding this comment

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

why these types?
for tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup

tickerProvider, nowFunc)
}

func newInternal(tcpNode host.Host, peers []peer.ID, version string, lockHash []byte,
Copy link
Contributor

Choose a reason for hiding this comment

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

new internal what?
this function name seems vague

Copy link
Contributor

Choose a reason for hiding this comment

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

also add godoc

Copy link
Contributor Author

@corverroos corverroos Oct 5, 2022

Choose a reason for hiding this comment

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

New and NewForT both call newInternal since it is the new function used internally

@corverroos corverroos added the merge when ready Indicates bulldozer bot may merge when all checks pass label Oct 5, 2022
@obol-bulldozer obol-bulldozer bot merged commit 6d2236f into main Oct 5, 2022
@obol-bulldozer obol-bulldozer bot deleted the corver/peerinfo branch October 5, 2022 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge when ready Indicates bulldozer bot may merge when all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants