[contracts] add FreeBalanceApp, extending ETHBucket to n-party

[alxiong]

extending the ETHBucket contract to n-party, so that right now all contracts are n-party compatible. (for channelized coinshuffle and others)

To avoid breaking existing code base especially dependencies in machine and node, I name it FreeBalanceApp,

minor update on the types as well.

[snario]

I would name is WeiFreeBalanceApp or ETHFreeBalanceApp or perhaps even ETHPaymentChannelApp? I like to be explicit about the asset this thing supports.

[alxiong]

The FreeBucket or even the current ETHBucket can actually support any assetType:

[alxiong]

Which brings up anther interesting question: can a channel supports a mix of different tokens (e.g. Alice deposit 5 ETH, 4 ZRX, Bob deposits 8 ETH, 2 DAI etc.)

I don't think current contract allows for that. At least we would modify

function resolve(bytes memory encodedState, Transfer.Terms memory terms) 
  public 
  pure 
  returns (Transfer.Transaction memory)

into

function resolve(bytes memory encodedState, Transfer.Terms[] memory terms) 
  public 
  pure 
  returns (Transfer.Transaction[] memory)

@IIIIllllIIIIllllIIIIllllIIIIllllIIIIll

[snario]

Yeah, I have been asking @IIIIllllIIIIllllIIIIllllIIIIllllIIIIll and @emansipater to explore these structs to see if there are better and more versatile alternatives we could implement.

[ldct]

The current Transaction struct doesn't support different tokens to different beneficiaries. That should be fixed separately from this PR.

[ldct]

Why doesn't this PR modify ETHBucketAppState instead of creating a new contract?

[snario]

@IIIIllllIIIIllllIIIIllllIIIIllllIIIIll

The current Transaction struct doesn't support different tokens to different beneficiaries. That should be fixed separately from this PR.

This is not true precisely. to, value, and data can be encoded to multiple token addresses inside the to array. It is the execute(Transfer.Transaction) method on the Transfer library that uses the address payable's send method that causes the problem because it limits the amount of gas used to be like 2300 or something.

[alxiong]

Why doesn't this PR modify ETHBucketAppState instead of creating a new contract?

Because I don't want to break the other code that relies on this, it would be too major of a change (not sure the dependency level?) since all playground and node is using this ETHBucket's 2 player mode.

I thought adding support for n-party could be pipelined, parallel work that doesn't break current MVP/demo, and slowly adding respective support in the Node, then by the time both are ready, we can merge to make n-party the default.

[alxiong]

@snario

that uses the address payable's send method that causes the problem because it limits the amount of gas used to be like 2300 or something.

why would this be a problem?

to, value, and data can be encoded to multiple token addresses inside the to array

agree,
so if the same person/depositor wants to deposit multiple/mixed set of assets (e.g. ETH and DAI), technically it's still possible with the current Multisig, but the multisig would have several owners with the same address, but one assetType per owner. it's a bit strange but workable.
Did I miss anything?

[snario]

why would this be a problem?

Because the to address might be a contract and data might be a function call on that contract (thus it would probably need more than 2300 gas)

agree,
so if the same person/depositor wants to deposit multiple/mixed set of assets (e.g. ETH and DAI), technically it's still possible with the current Multisig, but the multisig would have several owners with the same address, but one assetType per owner. it's a bit strange but workable.
Did I miss anything?

I don't think there is a relationship between the owners of the multisig and the perceived owners of the assets. For example, if you want to deposit DAI into the multisig then you would assign the multisig as the owner of the DAI in the DAI token contract. There would be a DaiRefundApp.sol open in the channel that declares one of the participants of the channel as the designated owner of all DAI that the multisig owns in the same way that the ETHBalanceRefundApp.sol works.

[alxiong]

Because the to address might be a contract

yes indeed, but in our case, it has to be an address payable, and in case of ERC20 or others, it would be contract function call instead of a eth transfer which forwards more than just 2300 gas, Sorry, I still don't see a problem

I don't think there is a relationship between the owners of the multisig and the perceived owners of the assets.

Ah.. you are right, which means we also need to extend RefundApp to other token and multi-party right?
Do you prefer

1. change the current ETHBalanceRefundApp to BalanceRefundApp and add an assetType in the AppState
2. have another ERC20BalanceRefundApp ?

[snario]

yes indeed, but in our case, it has to be an address payable, and in case of ERC20 or others, it would be contract function call instead of a eth transfer which forwards more than just 2300 gas, Sorry, I still don't see a problem

Currently the way the code works, we use the assetType flag as a kind of helper bit to figure out which function to call with the to, value, data tuple, right? Imagine a situation where we did not have that helper bit. Then in that case we would just blindly call address(to).call.value(val)(data). However it is strictly for safety that we use the assetType bit in order to dispatch this to address payable(to).send(value).

The point I am making is that in the unsafe world, we could ignore assetType and just do address(to).call.value(val)(data). In the case of a Dai payment to would be DaiTokenContract.address, val would be 0, and data would be the encoded version of transfer(to, val).

Does it make sense?

[snario]

There is an important task I am implying here with @IIIIllllIIIIllllIIIIllllIIIIllllIIIIll and @emansipater that is we need a better more abstract data structure to get the benefits of both safe function calls and clean Solidity executors (e.g., address payable(to).send(val)) and maximum flexibility.

We don't want to have to hard code new asset types into Transfer.sol for things like Chess, etc.

[snario]

Also for now probably an ERC20RefundApp is simplest.

[snario]

Closing as work in #1263 will supercede this.