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

go/common/node/address: use custom type instead of internal net.TCPAddr #4744

Merged
merged 1 commit into from
May 17, 2022

Conversation

ptrus
Copy link
Member

@ptrus ptrus commented May 9, 2022

Since Address is used in consensus state, it makes sense using an explicitly defined struct. The struct serialization is defined in a backwards compatible way.

@Yawning
Copy link
Contributor

Yawning commented May 9, 2022

Crap, because we did this late, guess we can't even omitempty the Zone field huh.

@codecov
Copy link

codecov bot commented May 9, 2022

Codecov Report

Merging #4744 (534d913) into master (513420e) will increase coverage by 0.09%.
The diff coverage is 86.95%.

@@            Coverage Diff             @@
##           master    #4744      +/-   ##
==========================================
+ Coverage   66.77%   66.87%   +0.09%     
==========================================
  Files         441      441              
  Lines       49348    49357       +9     
==========================================
+ Hits        32952    33007      +55     
+ Misses      12317    12285      -32     
+ Partials     4079     4065      -14     
Impacted Files Coverage Δ
go/worker/common/p2p/p2p.go 67.27% <80.00%> (+0.45%) ⬆️
go/common/node/address.go 58.53% <84.61%> (+2.98%) ⬆️
go/registry/tests/tester.go 92.49% <100.00%> (-0.03%) ⬇️
go/worker/common/p2p/peermgmt.go 79.51% <100.00%> (ø)
go/oasis-node/cmd/common/metrics/disk.go 65.51% <0.00%> (-20.69%) ⬇️
go/oasis-node/cmd/common/metrics/resource.go 84.00% <0.00%> (-8.00%) ⬇️
go/worker/common/p2p/dispatch.go 71.52% <0.00%> (-5.56%) ⬇️
go/common/grpc/grpc.go 79.88% <0.00%> (-2.37%) ⬇️
go/oasis-node/cmd/stake/delegations.go 78.99% <0.00%> (-1.69%) ⬇️
go/consensus/tendermint/roothash/roothash.go 67.97% <0.00%> (-1.24%) ⬇️
... and 25 more

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 92f25b0...534d913. Read the comment docs.

@ptrus
Copy link
Member Author

ptrus commented May 9, 2022

Yeah not at the moment, but we can open a follow issue for the next breaking release to change these.

@Yawning
Copy link
Contributor

Yawning commented May 9, 2022

If anything, for the next breaking release we should start using net/netip/AddrPort (1.18-ism)

@ptrus ptrus force-pushed the ptrus/feature/registry-tcpaddress-custom branch from a33b9b6 to cb63aa5 Compare May 17, 2022 08:28
@ptrus ptrus marked this pull request as ready for review May 17, 2022 08:28
@ptrus ptrus force-pushed the ptrus/feature/registry-tcpaddress-custom branch from cb63aa5 to 3e9c0db Compare May 17, 2022 08:41
@ptrus ptrus force-pushed the ptrus/feature/registry-tcpaddress-custom branch from 3e9c0db to 534d913 Compare May 17, 2022 09:31
@ptrus ptrus merged commit 95cfb7d into master May 17, 2022
@ptrus ptrus deleted the ptrus/feature/registry-tcpaddress-custom branch May 17, 2022 10:41
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