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

ChannelStateSchema.get_balance returns distributable #4348

Closed
karlb opened this issue Jul 5, 2019 · 7 comments · Fixed by #4541
Closed

ChannelStateSchema.get_balance returns distributable #4348

karlb opened this issue Jul 5, 2019 · 7 comments · Fixed by #4541
Assignees
Labels
Component / API Issues that relate the the APIs Raiden provides. Severity / Minor
Milestone

Comments

@karlb
Copy link
Contributor

karlb commented Jul 5, 2019

According to the glossary, the capacity does not care about locks:

A channel's capacity is the sum of the total deposits minus the sum of the total withdraws of both its participants

while the term "distributable" does:

distributable = get_balance(sender, receiver) - get_amount_locked(sender)

Since these two terms mean different things, the following code is wrong or at least highly misleading:

class ChannelStateSchema(BaseSchema):

    ...

    @staticmethod
    def get_balance(channel_state):
        return channel.get_distributable(channel_state.our_state, channel_state.partner_state)
@rakanalh rakanalh added this to the Ithaca milestone Jul 15, 2019
@palango palango self-assigned this Aug 5, 2019
@palango
Copy link
Contributor

palango commented Aug 5, 2019

This function is used to calculate the value for a field balance in the API.

Gives this this is plain wrong and should be fixed. The remaining question for me is whether or not we should return the distributable in the API as well?

@LefterisJP
Copy link
Contributor

I remember seeing a PR by @hackaugusto recently touching on this subject. Perhaps he is the best person to weigh in here.

I see 3 terms here. capacity, balance, distributable.

Let's simply define them in the spec and make sure the code reflects them.

I don't think we should expose more in the API since it's already confusing enough.

So in the spec I only see capacity defined in the terminology as

Current amount of tokens available for a given participant to make transfers.  See :ref:`settlement-algorithm` for how this is computed.

Interestingly enough there is more info on the glossary of the docs of this repo ...

   transferred amount
       The transferred amount is the total amount of tokens one participant of a payment channel has sent to his counterparty.

   locked amount
       The locked amount is the total amount of tokens one participant of a payment channel has locked in pending transfers towards his counterparty

   channel capacity
       A channel's capacity is the sum of the total deposits minus the sum of the total withdraws of both its participants. It is also the sum of the channel participants :term:`balance`.

   balance
       The balance :math:`B_n` of a channel participant :math:`P` is his total deposit :math:`P_d` along with the amount of tokens he received :math:`P_r` minus the amount :math:`P_s` of token he has sent. So :math:`B_n = P_d + P_r - P_s`

   locked balance
       The locked balance :math:`B_l` of a channel participant is the sum of the locked amount for all pending transfers :math:`T_p`. So :math:`B_l = \sum_{k=0}^{N-1} T_p` where :math:`N` is the number of pending transfers.

   available balance
       The available balance :math:`B_a` of a channel participant is :math:`B_a = B_n - B_l`.

First of all let's perhaps clear up the spec/specs and keep it all in one place well defined.

Secondly what should be exposed in the API? I suppose what I care about is how much I have available to send. So that would be the balance only?

@palango The distributable you propose is what? The capacity? Aka sum of balances?

@palango
Copy link
Contributor

palango commented Aug 5, 2019

@LefterisJP The code defines it as follows:

    """Return the amount of tokens that can be used by the `sender`.

    The returned value is limited to a UINT256, since that is the representation
    used in the smart contracts and we cannot use a larger value. The limit is
    enforced on transferred_amount + locked_amount to avoid overflows. This is
    an additional security check.
    """
    ...
    distributable = get_balance(sender, receiver) - get_amount_locked(sender)

So distributable = balance - locked

@LefterisJP
Copy link
Contributor

I see. Hmmm then we should add it to the spec by the way.

I guess this is not what I would be most interested in. How much I have available to send on this channel is not something that can be consumed nicely, what you need to know is your balance first and foremost right?

I suppose the problem is that there is always the incoming + outgoing channel. If you have locked transfers due to just being a node in the network and mediating:

  • Balance will always be the same, and it's probably what you want
  • Distributable will be going up and down as you are mediating and offers no real information to someone through the API. At least I can't imagine anything. Only if you want to decide where to route a payment or something? But the API does not need to know that, right?

And now that I am mentioning that ... looking at the linked code it seems that the API calls it balance, but returns the distributable. So this should change anyway.

@palango
Copy link
Contributor

palango commented Aug 5, 2019

In the mediation scenario balance would change as well, just later than distributable:

    return Balance(
        sender.contract_balance
        - max(sender.offchain_total_withdraw, sender.onchain_total_withdraw)
        - sender_transferred_amount
        + receiver_transferred_amount
    )

And now that I am mentioning that ... looking at the linked code it seems that the API calls it balance, but returns the distributable. So this should change anyway.

That's exactly what the issue is about ;)

@LefterisJP
Copy link
Contributor

In the mediation scenario balance would change as well, just later than distributable:

Hmm you are right.

Then we should maybe see balance as that of the incoming + outgoing channel? I guess as a user what do I expect the term balance to mean? Something that changes only when I perform an action and not something that changes depending on payments being mediated through me.

That's exactly what the issue is about ;)

I am slow :)

@palango
Copy link
Contributor

palango commented Aug 5, 2019

Then we should maybe see balance as that of the incoming + outgoing channel? I guess as a user what do I expect the term balance to mean? Something that changes only when I perform an action and not something that changes depending on payments being mediated through me.

True, but that info here is the per-channel info, so I guess we cannot work around it like that. I guess it would make sense to return distributable as well, because it's a better upper bound.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component / API Issues that relate the the APIs Raiden provides. Severity / Minor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants