Skip to content

ADR-016: Add protocol Version to NodeInfo#2654

Merged
ebuchman merged 5 commits intodevelopfrom
bucky/versions-adr-016-p2p
Oct 18, 2018
Merged

ADR-016: Add protocol Version to NodeInfo#2654
ebuchman merged 5 commits intodevelopfrom
bucky/versions-adr-016-p2p

Conversation

@ebuchman
Copy link
Contributor

@ebuchman ebuchman commented Oct 17, 2018

More from #2468

  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG_PENDING.md

@ebuchman ebuchman requested a review from zramsay as a code owner October 17, 2018 21:58
@codecov-io
Copy link

codecov-io commented Oct 17, 2018

Codecov Report

❗ No coverage uploaded for pull request base (develop@455d341). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff             @@
##             develop    #2654   +/-   ##
==========================================
  Coverage           ?   61.55%           
==========================================
  Files              ?      207           
  Lines              ?    16896           
  Branches           ?        0           
==========================================
  Hits               ?    10400           
  Misses             ?     5630           
  Partials           ?      866
Impacted Files Coverage Δ
rpc/core/status.go 0% <ø> (ø)
node/node.go 64.77% <100%> (ø)
p2p/test_util.go 66.41% <100%> (ø)
p2p/node_info.go 88.33% <100%> (ø)

Copy link
Contributor

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

LGTM!

default:
return fmt.Errorf("info.Other.TxIndex should be either 'on' or 'off', got '%v'", txIndex)
}
// XXX: Should we be more strict about address formats?
Copy link
Contributor

Choose a reason for hiding this comment

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

Are they always IP4 addresses?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No actually, in theory they could be IPv6. We don't actually do anything with this RPCAddress though, just display it over RPC, so it doesn't matter too much here ...

@ebuchman ebuchman merged commit 14c1bae into develop Oct 18, 2018
@ebuchman ebuchman deleted the bucky/versions-adr-016-p2p branch October 18, 2018 14:30
switch txIndex {
case "", "on", "off":
default:
return fmt.Errorf("info.Other.TxIndex should be either 'on' or 'off', got '%v'", txIndex)
Copy link
Contributor

Choose a reason for hiding this comment

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

'on' or 'off' or empty string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks - bb173bc

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.

4 participants