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

p2p: peerstore backed phonebook #5710

Merged
merged 16 commits into from
Aug 31, 2023
Merged

p2p: peerstore backed phonebook #5710

merged 16 commits into from
Aug 31, 2023

Conversation

shiqizng
Copy link
Contributor

peerstore backed phonebook prototype.

@shiqizng shiqizng requested review from Eric-Warehime, a team, winder, tzaffi, algochoi, cce and iansuvak and removed request for a team August 24, 2023 20:21
@shiqizng shiqizng self-assigned this Aug 24, 2023
@shiqizng shiqizng added Team Lamprey p2p Work related to the p2p project Enhancement labels Aug 24, 2023
@codecov
Copy link

codecov bot commented Aug 24, 2023 •
edited
Loading

Codecov Report

Merging #5710 (f77fa12) into master (0f2ab92) will increase coverage by 0.04%.
Report is 29 commits behind head on master.
The diff coverage is 73.29%.

@@            Coverage Diff             @@
##           master    #5710      +/-   ##
==========================================
+ Coverage   55.17%   55.22%   +0.04%     
==========================================
  Files         465      465              
  Lines       65032    65207     +175     
==========================================
+ Hits        35883    36012     +129     
- Misses      26764    26793      +29     
- Partials     2385     2402      +17
Files Changed Coverage Δ
network/phonebook.go 91.33% <ø> (ø)
network/p2p/peerstore/utils.go 77.77% <55.55%> (-11.12%) ⬇️
network/p2p/peerstore/peerstore.go 74.28% <74.25%> (-3.50%) ⬇️

... and 10 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

tzaffi
tzaffi reviewed Aug 24, 2023
View reviewed changes
Copy link
Contributor

@tzaffi tzaffi left a comment

Choose a reason for hiding this comment

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

Some questions/suggestions

network/p2p/peerstore/peerstore.go Outdated Show resolved Hide resolved
network/p2p/peerstore/peerstore.go Outdated Show resolved Hide resolved
network/p2p/peerstore/peerstore.go Outdated Show resolved Hide resolved
network/p2p/peerstore/peerstore.go Outdated Show resolved Hide resolved
network/p2p/peerstore/utils.go Show resolved Hide resolved
@shiqizng shiqizng marked this pull request as ready for review August 30, 2023 16:11
tzaffi
tzaffi previously approved these changes Aug 31, 2023
View reviewed changes
Copy link
Contributor

@tzaffi tzaffi left a comment

Choose a reason for hiding this comment

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

LGTM

iansuvak
iansuvak previously approved these changes Aug 31, 2023
View reviewed changes
Copy link
Contributor

@iansuvak iansuvak left a comment

Choose a reason for hiding this comment

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

LGTM as well!

cce
cce requested changes Aug 31, 2023
View reviewed changes
Copy link
Contributor

@cce cce left a comment

Choose a reason for hiding this comment

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

Could you remove the duplicate Phonebook interface definition? Otherwise LGTM

@shiqizng shiqizng dismissed stale reviews from iansuvak and tzaffi via 47bd821 August 31, 2023 19:15
cce
cce previously approved these changes Aug 31, 2023
View reviewed changes
@cce cce merged commit 0e67a0b into algorand:master Aug 31, 2023
10 checks passed
@cce cce mentioned this pull request Aug 31, 2023
Merged
This was referenced Oct 13, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbroder-algo bbroder-algo bbroder-algo left review comments

@Eric-Warehime Eric-Warehime Eric-Warehime approved these changes

@cce cce cce approved these changes

@iansuvak iansuvak iansuvak left review comments

@tzaffi tzaffi tzaffi left review comments

@winder winder Awaiting requested review from winder winder was automatically assigned from algorand/lamprey

@algochoi algochoi Awaiting requested review from algochoi algochoi was automatically assigned from algorand/lamprey

Assignees

@shiqizng shiqizng

Labels
Enhancement p2p Work related to the p2p project Team Lamprey
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@shiqizng @cce @tzaffi @iansuvak @Eric-Warehime @bbroder-algo