go-perun icon indicating copy to clipboard operation
go-perun copied to clipboard

Redesign channel updates API

Open manoranjith opened this issue 4 years ago • 7 comments

Currently, the go-perun client exposes two APIs for updating a channel: Update and `UpdateBy'.

func (c *Channel) Update(ctx context.Context, next *channel.State) (err error) {...}
func (c *Channel) UpdateBy(ctx context.Context, update func(*channel.State) error) (err error) {...}

In both cases, the user can modify any property of the state:

  1. Version
  2. Any field in Allocation (Assets, Balances or Locked funds that contain sub-allocations).
  3. Data
  4. IsFinal flag

In many scenarios, arbitrarily modifying these fields would lead to invalid updates that will fail. For instance,

  1. Version must be incremented by 1 for each update, but this API allows the user to arbitrarily modify the version.
  2. Assets field in Allocation must not be changed. It should only read by the user.
  3. Locked funds should not be modified by the user. It is meant only for those updates that fund/withdraw sub/virtual channels. These updates will be done only by the SDK and not by the user.
  4. IsFinal flag can be set only when certain conditions are met. Eg: For ledger channels, there should be no locked funds.

It will a lot of overhead on the user to ensure these conditions when making an update. Hence, I propose to redesign the update API to make it more easy to use. It can replace both the existing APIs:

Update(updater func(Balances, Data) error) (err error) {...}

In this API,

  1. User is able to modify only the balances and data.
    1. API will validate if sum of balances remain constant and if data is valid for the given app.
  2. If user needs info on Assets or App, they could read it from the channel. These are set during channel opening and never change during the lifetime of the channel. ~3. Finalize flag can be used to finalize a channel. The API will apply the finalize flag only after ensuring if necessary conditions are met.~

Update 1: Removed the finalize flag in the API. If we do not want the user to initiate a final update. The final update can be initiated by the framework when settling the channel.

manoranjith avatar Aug 13 '21 12:08 manoranjith

@matthiasgeihs

manoranjith avatar Aug 13 '21 12:08 manoranjith

@manoranjith

I agree with much of what was written. Some thoughts and questions below.

  • You propose Update(updater func(Balances, Data) error) (err error) {...}, but how to return the modified values?
  • How to access the state when the channel is locked during the update?
  • At first we may want to include a way to modify set the final flag manually. Otherwise the scope of this change will become much larger.

matthiasgeihs avatar Dec 09 '21 16:12 matthiasgeihs

@matthiasgeihs

  • The modified values need not be returned. Because, what the SDK does it:

    1. Read the latest state.
    2. Modify it using the updater.
    3. Modify some more parameters (inc version, mark as final etc.,)
    4. Do sanity checks
    5. Sign it and propose the update to peer and apply if accepted.

    So, the best way for the user to retrieve the modified state will be to use ch.State() after the Update / ForceUpdate function returns.

  • On accessing the state of the app (for reading other parameters like App, Assets etc., My (unstated) assumption was that, user could retain a copy of it when the channel was opened. It is safe to keep a copy, because these are invariants. On the otherhand, the alternate approach, which you have taken in your WIP also looks good (where you provide an object from which all parameters of a state can be read, but only some of them can be modified).

  • Yes, I agree that, we should provide a final flag. Client could validate the necessary per-requisites before marking the channel as final and return an error if they are not met.

    So, the updated signature could be

Update(updater func(Balances, Data) error), finalize bool) (err error) {...}

What are your thoughts one the two different approaches ?

manoranjith avatar Dec 10 '21 09:12 manoranjith

I have made a suggestion here: https://github.com/hyperledger-labs/go-perun/compare/main...perun-network:c8b747a1eba9503372e8d16b073d6e7e79f57e83

At the core it proposes the following:

  • A new type UpdateState where only certain state properties are mutable.
  • The function signature Update(ctx context.Context, update func(UpdateState) error) error, which passes UpdateState to the update function and thereby controls which state properties can be modified by the user.

matthiasgeihs avatar Dec 10 '21 10:12 matthiasgeihs

Yes, I was referring to that,

One change in the suggestion would be to take finalize as a boolean flag, rather than making it mutable. Because a state should be marked final only when it meets certain pre-conditions (such as no locked funds), which the client should ensure before marking it as final.

On the other hand changing SetFinal(b bool) to SetFinal(b bool) error could also work.

manoranjith avatar Dec 10 '21 10:12 manoranjith

But we need to be able to set the final flag somewhere. If we cannot set it during state update, we need to be able to set it through calling a different function. For example, we might modify channel.Settle so that it first attempts to update the channel to final off chain. However, this would be a larger change with consequences on how a user interacts with the framework overall, and therefore I would rather like to do that separately. The main point with my suggestion is that I would like to prevent the user from being able to update the version when this is clearly not intended.

matthiasgeihs avatar Dec 10 '21 11:12 matthiasgeihs

Yes, I agree that changing to a different paradigm of automatically trying to finalize before settle would be a larger change that should be done separately. So, we should indeed allow the user to mark a state as final.

But, there can be an error when SetFinal is called. For eg: when there are locked funds. But, the current API SetFinal(b bool) assumes there will be no error and that the call will always succeed.

The solution, as I described in the previous comment is either modify the signature of SetFinal or to take finalize as a boolean argument and when the the pre-conditions are not met Update should return an error.

manoranjith avatar Dec 10 '21 11:12 manoranjith