-
Notifications
You must be signed in to change notification settings - Fork 267
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
Add API call to get general network statistics #1100
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1100 +/- ##
==========================================
+ Coverage 82.56% 82.57% +0.01%
==========================================
Files 101 101
Lines 7697 7708 +11
Branches 302 306 +4
==========================================
+ Hits 6355 6365 +10
- Misses 1342 1343 +1
|
(updateCount: Long,avgCltvExpiryDelta: Long,avgHtlcMinimumMsat: Long,avgFeeBaseMsat: Long,avgFeeProportionalMillionths: Long,avgHtlcMaximumMsat: Long, mf: FlagCounter, cf:FlagCounter ) <- allUpdates(None).map{ | ||
updates => updates.foldLeft(Tuple8(0L, 0L, 0L, 0L, 0L, 0L,FlagCounter(), FlagCounter())) { | ||
(t, c) => | ||
Tuple8(t._1 + 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this hard-to-read Tuple8
why don't you directly use a GetNetworkInfoResponse
here?
I think it would be cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this is possible. As GetNetworkInfoResponse include results of the earlier futures too.
Could fill with dummy zeros but think that would make more confusing.
Especially if additional calls/futures were added to get more data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would do something like (I slightly renamed the fields, you'll have to adapt):
override def getNetworkInfoResponse()(implicit timeout: Timeout): Future[GetNetworkInfoResponse] = {
for {
nodeCount <- allNodes().map(_.size)
channelCount <- allChannels().map(_.size)
channelUpdates <- allUpdates(None)
val networkInfo = channelUpdates.foldLeft(GetNetworkInfoResponse(channelCount, nodeCount, channelUpdates.size, 0, MilliSatoshi(0), MilliSatoshi(0), 0, MilliSatoshi(0))) {
(current, next) => current.copy(
avgCltvExpiryDelta = current.avgCltvExpiryDelta + next.cltvExpiryDelta,
avgFeeBaseMsat = current.avgFeeBaseMsat + next.feeBaseMsat
// etc
)
}
} yield networkInfo.copy(
avgFeeBaseMsat = networkInfo.avgFeeBaseMsat / networkInfo.updates
// etc
)
}
And I would refactor to make dividing all avg
fields by the updateCount
be done in a method in the case class or something like that.
eclair-core/src/test/scala/fr/acinq/eclair/EclairImplSpec.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution, I think this is a useful addition to the API and we can enrich this response later (I'm thinking that average channel capacity would be useful to get).
I have a couple of code clean-up comments but concept ack.
eclair-core/src/test/scala/fr/acinq/eclair/EclairImplSpec.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/EclairImplSpec.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/EclairImplSpec.scala
Outdated
Show resolved
Hide resolved
f.router.expectMsg('channels) | ||
val channelId_ab = ShortChannelId(420000, 1, 0) | ||
val channelId_bc = ShortChannelId(420000, 2, 0) | ||
val chan_ab = fr.acinq.eclair.router.BaseRouterSpec.channelAnnouncement(channelId_ab, randomKey, randomKey, randomKey, randomKey) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove fr.acinq.eclair.router
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am in EclairImpSpec so do need it. Wanted to make clear was coming from another Spec.
allchannels is now so slow it timesout.
This API call returns general stats on the network that is useful to know.