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

Implement getAddress / factor out some code #631

Merged
merged 2 commits into from
Mar 5, 2020

Conversation

AndrejMitrovic
Copy link
Contributor

Extracted from #621

@AndrejMitrovic AndrejMitrovic added the type-refactoring An improvement on existing code without visible functional change label Mar 5, 2020
@Geod24
Copy link
Collaborator

Geod24 commented Mar 5, 2020

Love that last commit. As for the Vibe.d deserialization one, not too convinced it's the right solution. Could you take it out for the time being ?

@AndrejMitrovic
Copy link
Contributor Author

AndrejMitrovic commented Mar 5, 2020

Actually yes I guess that one is more in line with belonging in #621, I'll remove it.

This function was only used from one place.
@codecov
Copy link

codecov bot commented Mar 5, 2020

Codecov Report

Merging #631 into v0.x.x will not change coverage.
The diff coverage is 88.88%.

Impacted file tree graph

@@           Coverage Diff           @@
##           v0.x.x     #631   +/-   ##
=======================================
  Coverage   90.07%   90.07%           
=======================================
  Files          62       62           
  Lines        4564     4564           
=======================================
  Hits         4111     4111           
  Misses        453      453
Flag Coverage Δ
#integration 53.65% <87.5%> (+0.97%) ⬆️
#unittests 88.91% <77.77%> (-0.03%) ⬇️
Impacted Files Coverage Δ
source/agora/test/Base.d 88.54% <100%> (ø) ⬆️
source/agora/network/NetworkManager.d 74.49% <87.5%> (ø) ⬆️

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 49a7468...a43d2ad. Read the comment docs.

The NetworkManager in the unittests uses the public
key of the node as the effective address,
but then the base class also used IP-based address
banning despite of that.

By abstracting the address, we can keep the behavior
predictable.

In addition, we will need to pass the address
later when the handshake stage is changed later
in preparation to fix bosagora#606
Copy link
Collaborator

@Geod24 Geod24 left a comment

Choose a reason for hiding this comment

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

LGTM, rebased for you, merge at will

@Geod24 Geod24 merged commit 7f52a57 into bosagora:v0.x.x Mar 5, 2020
@AndrejMitrovic AndrejMitrovic deleted the various-improvements branch March 5, 2020 11:15
@AndrejMitrovic AndrejMitrovic changed the title Small code improvements Implement getAddress / factor out some code Mar 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-refactoring An improvement on existing code without visible functional change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants