feat(raiden): channel balance by currency #1066
Conversation
|
Needs rebase |
|
Rebased and tested locally, the new logic works for me. |
|
Restarted the failed tests - passed |
| @@ -41,6 +41,12 @@ class RaidenClient extends SwapClient { | |||
| private host: string; | |||
| private disable: boolean; | |||
|
|
|||
| // TODO: Populate the mapping from the database (Currency.decimalPlaces). | |||
There was a problem hiding this comment.
Maybe open an issue for this TODO so that you don't forget about it
There was a problem hiding this comment.
I think #1054 is tracking this.
lib/raidenclient/RaidenClient.ts
Outdated
| const balance = channels.filter(channel => channel.state === 'opened') | ||
| .map(channel => channel.balance) | ||
| .reduce((acc, sum) => sum + acc, 0); | ||
| .reduce((acc, sum) => sum + (acc / RaidenClient.UNITS_PER_CURRENCY[currency]), 0); |
There was a problem hiding this comment.
Think we want to switch places with sum and acc here (and rename them to sum and channelBalance.
arr.reduce(callback(accumulator, currentValue[, index[, array]]), [, initialValue])
Maybe do the conversion after reduce so that it's only going to be calculated once?
I think this will cause confusion on the CLI level if we show satoshis per coin as it's meant to be our internal unit only. What do you think about converting Raiden channel balance values to satsToCoinsStr and then doing additional conversion to smallest unit of the currency on CLI level?
There was a problem hiding this comment.
I'm a bit confused by what you mean in the first paragraph, could you clarify? I went ahead and made the conversion happen after reduce though.
We can have the cli print whole units for the "formatted" output, that'd be consistent with how we're displaying amounts most other places. That should be a separate PR though I think. It would tricky to convert back down to the smallest units, since I figure that would require a DB lookup or an additional field in the rpc response, and I don't see it as being particularly useful or readable to display values smaller than 10^-8.
There was a problem hiding this comment.
What I was trying to say was that the first argument for the callback of reduce is sum and the second acc.
With the previous logic:
.reduce((acc, sum) => {
console.log('sum is', sum, 'and converted', (acc / RaidenClient.UNITS_PER_CURRENCY[currency]));
return sum + (acc / RaidenClient.UNITS_PER_CURRENCY[currency]);
}, 0);
Printed:
[1] sum is 75000000000000000 and converted 0
As a result converting back to satoshis per unit didn't have any effect:
➜ xud git:(channel-mgmt-plus-raiden-balance) ✗ ./bin/xucli channelbalance
{
"balancesMap": [
...
[
"WETH",
{
"balance": 75000000000000000,
"pendingOpenBalance": 0
}
]
]
}
With the new logic this isn't an issue, but I'd still rename the arguments.
As for converting back to smallest units on the CLI level - let's open an issue for that to discuss. I think it will cause confusion for the end user:
"WETH",
{
"balance": 7500000,
"pendingOpenBalance": 0
}
This modifies the logic around querying raiden for channel balances. Rather than summing up all balances across all tokens, it allows for specifying a currency and retrieving only the balance for that currency. Calling `ChannelBalance` without specifying a currency will return each currency separately. It also scales the balance returned by raiden to satoshis (10^-8). This is because the default units of 10^-18 for currencies such as WETH can exceed the maximum value of a `uint64` used by the gRPC layer. Closes #1051.
| const balance = channels.filter(channel => channel.state === 'opened') | ||
| .map(channel => channel.balance) | ||
| .reduce((acc, sum) => sum + acc, 0); | ||
| .reduce((sum, acc) => sum + acc, 0) | ||
| / (RaidenClient.UNITS_PER_CURRENCY[currency] || 1); |
There was a problem hiding this comment.
One last nit - when commenting out this line the tests are still passing. We might want to consider covering the conversion in channelBalance calculates the total balance of open channels for a currency test as well. Up to you :).
There was a problem hiding this comment.
Makes sense, I'll add this test case and merge.
There was a problem hiding this comment.
Actually, I remember now that I tried to do this originally but in its current state I'd have to mock a static value and I'm not really sure how to do that. Maybe revisit this when we get the units per currency from the database?
ghost
merged commit 
Closes #1051.
Note: This is based off of #1064.
This modifies the logic around querying raiden for channel balances. Rather than summing up all balances across all tokens, it allows for specifying a currency and retrieving only the balance for that currency. Calling
ChannelBalancewithout specifying a currency will return each currency separately.It also scales the balance returned by raiden to satoshis (10^-8). This is because the default units of 10^-18 for currencies such as WETH can exceed the maximum value of a
uint64used by the gRPC layer.