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

Add `balances` API endpoint #962

Merged
merged 9 commits into from Jun 26, 2019

Conversation

Projects
None yet
6 participants
@btcontract
Copy link
Contributor

commented Apr 24, 2019

Currently balances can be obtained from channels call but this requires a lot of work on caller side and also some specific knowledge (reserves, commit tx fee, in-flight payments), so this new balances endpoint only returns a correct balance info for each channel.

Data is taken from Relayer actor which already has exactly what we need: a map of OutgoingChannel objects which now contain correctly calculated can send / can receive balances for each NORMAL channel. Additionally, obtaining balances this way has a minimal performance hit when compared to querying each channel used in channels.

@@ -73,10 +73,12 @@ case class Commitments(localParams: LocalParams, remoteParams: RemoteParams,

def announceChannel: Boolean = (channelFlags & 0x01) != 0

def availableBalanceForSendMsat: Long = {
def availableBalances: (Long, Long) = {

This comment has been minimized.

Copy link
@dpad85

dpad85 Apr 24, 2019

Member

It's quite hard to understand what this tuple of Long is. I think that availableBalances should be typed, with something like:

case class AvailableBalance(local: MilliSatoshi, remote: MilliSatoshi)
@pm47

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

I am not against adding this feature, but this should be done independently of the Relayer, which should only have one job.

@btcontract

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

Even if that would mean precisely copying what Relayer does already? Reading off Relayer state seems innocent in a sense that it does not make Relayer change anything.

@btcontract

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

Now there's a liveBalanceTracker actor which is only created if API is enabled, will this work?

@codecov-io

This comment has been minimized.

Copy link

commented Jun 2, 2019

Codecov Report

Merging #962 into master will increase coverage by 1.74%.
The diff coverage is 64.28%.

@@            Coverage Diff             @@
##           master     #962      +/-   ##
==========================================
+ Coverage   80.27%   82.02%   +1.74%     
==========================================
  Files          98       97       -1     
  Lines        7611     7489     -122     
  Branches      296      301       +5     
==========================================
+ Hits         6110     6143      +33     
+ Misses       1501     1346     -155
Impacted Files Coverage Δ
...r-core/src/main/scala/fr/acinq/eclair/Eclair.scala 52.85% <0%> (-0.77%) ⬇️
...c/main/scala/fr/acinq/eclair/payment/Relayer.scala 88.05% <100%> (+0.18%) ⬆️
...in/scala/fr/acinq/eclair/channel/Commitments.scala 86.92% <25%> (-1.02%) ⬇️
...e/src/main/scala/fr/acinq/eclair/api/Service.scala 68.94% <66.66%> (-0.05%) ⬇️
...main/scala/fr/acinq/eclair/crypto/Generators.scala 78.57% <0%> (-21.43%) ⬇️
...-core/src/main/scala/fr/acinq/eclair/io/Peer.scala 74.76% <0%> (-1.43%) ⬇️
...rc/main/scala/fr/acinq/eclair/io/Switchboard.scala 73.68% <0%> (-0.17%) ⬇️
...ala/fr/acinq/eclair/payment/PaymentLifecycle.scala 88.49% <0%> (-0.11%) ⬇️
...in/scala/fr/acinq/eclair/api/JsonSerializers.scala 96.87% <0%> (-0.1%) ⬇️
...ain/scala/fr/acinq/eclair/wire/ChannelCodecs.scala 100% <0%> (ø) ⬆️
... and 17 more
@pm47
Copy link
Member

left a comment

There are several layering violations and inconsistencies in this PR.

But more importantly this does not return all balances: it will ignore channel being created, or in the process of closing. The logic from Relayer was custom tailored for relaying payments and doesn't take care of all those cases.

From that p.o.v. it would make more sense to start from Eclair.channelsInfo. It may be slower but it will be self contained and correct. WDYT?

@btcontract

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2019

But more importantly this does not return all balances: it will ignore channel being created, or in the process of closing. The logic from Relayer was custom tailored for relaying payments and doesn't take care of all those cases.

It also ignores offline channels and this is by design, the purpose of this method as I see it is to return exactly what can be sent and received right now (including a possibility to ignore private channels as they are useless for routing).

Maybe "balances" is a wrong word for it.

Use case for this is to show a service user how much can be received/withdrawn from a service at this very moment.

@pm47

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

Use case for this is to show a service user how much can be received/withdrawn from a service at this very moment.

Oh, I see! So it is basically about availableForSend/availableForReceive at one point in time. With that in mind I have far less concern with putting this in the Relayer.

Maybe "balances" is a wrong word for it.

Yup, the name is clearly misleading. How about availablebalance or usablebalance? Not very satisfactory.

@pm47 pm47 assigned pm47 and sstone and unassigned pm47 and sstone Jun 11, 2019

anton

@btcontract btcontract force-pushed the btcontract:api-balances branch from 528f3c1 to e0ab3f7 Jun 11, 2019

@btcontract

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

Is some kind of test needed for this?

@pm47

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

Hey @btcontract, are you still on this?

@btcontract

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

Yes!

@btcontract

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

Actually just started, I don't yet understand why UsableBalance with isFunder boolean, remoteNodeId and channel_update is needed in Relayer if there is already defined a

case class OutgoingChannel(nextNodeId: PublicKey, channelUpdate: ChannelUpdate ...

Perhaps it should be something like this in Relayer:

case class UsableBalances(canSend: Long, canReceive: Long, isPublic: Boolean)
case class OutgoingChannel(nextNodeId: PublicKey, channelUpdate: ChannelUpdate, usableBalances: UsableBalances)

Basically almost like it is now but UsableBalances moved to Relayer?

@pm47

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

I wrote isFunder but I meant isPublic, does that make more sense?

The goal is to separate concerns and keep internal classes clean. UsableBalance would be an "API" class and you can put things as distinct as availableBalanceForSendMsat and isPublic in it.

@btcontract

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

OK, but where should UsableBalance be located, still inside OutgoingChannel or separate map in Relayer (or something else)?

anton
@pm47

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

Nice, now it is just missing a simple test in RelayerSpec.

anton added some commits Jun 24, 2019

anton
anton

@btcontract btcontract changed the title [WIP] Add `balances` API endpoint Add `balances` API endpoint Jun 24, 2019

anton added some commits Jun 24, 2019

anton

def availableBalanceForSendMsat: Long = {
val reduced = CommitmentSpec.reduce(remoteCommit.spec, remoteChanges.acked, localChanges.proposed)
val feesMsat = if (localParams.isFunder) Transactions.commitTxFee(Satoshi(remoteParams.dustLimitSatoshis), reduced).amount * 1000 else 0
reduced.toRemoteMsat - remoteParams.channelReserveSatoshis * 1000 - feesMsat
}

def availableBalanceForReceiveMsat: Long = {

This comment has been minimized.

Copy link
@pm47

pm47 Jun 25, 2019

Member

Can you make this a lazy val? That's my last comment.

(same for availableBalanceForSendMsat)

anton

@pm47 pm47 dismissed their stale review Jun 25, 2019

Tests do not compile

@btcontract

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

Yes, I have forgot to override val, not def in tests. What do you think about this though: btcontract@1f18c59

An idea is to move subtle computations in a same place (commitments object) instead of repeating them, tests pass locally with this change.

@araspitzu

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

TODO: once merged, update the docs

@pm47

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

@btcontract sounds like premature optimization, let's keep it for later

anton
@btcontract

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

@pm47 An idea was not so much to save on computation but rather to remove duplicate logic as I've made a couple of mistakes while trying to copy it. Anyway, tests are fixed now.

@pm47

pm47 approved these changes Jun 26, 2019

@pm47 pm47 merged commit a35d50d into ACINQ:master Jun 26, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.