Concurrent approvals will fail with RDN

[hackaugusto]

light-client/raiden-ts/src/channels/epics.ts

Lines 698 to 702 in 335cf61

	// if needed, send approveTx and wait/assert it before proceeding; 'deposit' could be enough,
	// but we send 'prevAllowance + deposit' in case there's a pending deposit
	return defer(() =>
	tokenContract.functions.approve(tokenNetworkContract.address, allowance.add(deposit)),
	).pipe(assertTx('approve', ErrorCodes.CNL_APPROVE_TRANSACTION_FAILED, { log }));

the RDN smart contract only allows approval to change to and from zero. 0 -> x and x -> 0 where x != 0 is okay. x -> y where both x != 0 and y != 0 will fail.

    /// @notice Allows `_spender` to transfer `_value` tokens from `msg.sender` to any address.
    /// @dev Sets approved amount of tokens for spender. Returns success.
    /// @param _spender Address of allowed account.
    /// @param _value Number of approved tokens.
    /// @return Returns success of function call.
    function approve(address _spender, uint256 _value) public returns (bool) {
        require(_spender != 0x0);

        // To change the approve amount you first have to reduce the addresses`
        // allowance to zero by calling `approve(_spender, 0)` if it is not
        // already 0 to mitigate the race condition described here:
        // https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729
        require(_value == 0 || allowed[msg.sender][_spender] == 0);

        allowed[msg.sender][_spender] = _value;
        Approval(msg.sender, _spender, _value);
        return true;
    }

This is also a problem in the Python implementation. Ideally concurrent approvals should be allowed since that can have a considerable impact on time for joining a network. (Perhaps not a big problem for the light client, if the expected number of channels is 1)

[andrevmatos]

Damn... nice catch. How it deals with spenders spending less than approved? Does it zero the allowance on first tx, or reduce the allowance by the right amount, as expected?
Isn't this a problem when there's some allowance left, but not enough for a deposit? The user would need to mine approve(0) then approve(newNeededAllowance)...
If the multiple spendings work as expected, we could/should look into moving to a pattern seen on some dApps where on first usage, it "suggests" approving MaxUint256 or some other big number, and then no further approvals are required, and the user may read the SC to ensure it only transfers/spends upon user's request anyway. What do you think?

Edit: looks like reducing the allowance by spending works as expected, so we may choose to trust the contract and ask a high-enough allowance on first try, as well as validating that on approve, we do the x->0->y if x < y = our intended deposit, to comply on every case, although more slowly on this case.

[hackaugusto]

The user would need to mine approve(0) then approve(newNeededAllowance)...

This is indeed a problem if the approved amount has to increase, if it is large enough that it is not a problem.

approving MaxUint256 or some other big number, and then no further approvals are required

We have had this discussion here:

raiden-network/raiden#4625

I'd have this option behind a flag though, some users may be uncomfortable with an arbitrarily high approval.