Assess code splitting for core contracts (libraries, proxies etc.)

[loredanacirstea]

(In work issue)

tl;dr Code splitting with libraries and/or proxies could make the code cleaner.
Feature requested for a while. Latest discussion: #213 (comment)

History

Initial contracts from the raiden repo https://github.com/raiden-network/raiden/tree/9e12805fccf777cda8446b33842ad7ca3215d6c8/raiden/smart_contracts had a structure based on libraries.
This does not exist in the current contracts.
My main reasons for not keeping it in the raiden-network/raiden#1286 refactoring:

• It added a lot of duplicate code, making it more difficult to implement the protocol changes that followed.
• Other (possibly better?) patterns surfaced for code splitting/upgradability. I thought that code splitting might go hand in hand with the upgradability issue that was and still is open: Decide on smart contracts and channels upgradability pattern. spec#22.
  Therefore, I thought it might be better for the contracts development to handle this issue after the protocol is stable and we have time to research other patterns.

Current issues

we are facing that might be solved with better code splitting:

• Current TokenNetwork contract has the old NettingChannel and ChannelManager combined in a single contract. Therefore, this can happen: Add revert error strings for TokenNetwork #213 (comment)
• Current TokenNetworkRegistry contract imports the TokenNetwork contract and deployment is > 5mil gas, with an expensive token network creation >3mil gas
• There are some functions that receive a lot of variables as input, therefore filling up the stack faster. E.g. updateNonClosingBalanceProof, settleChannel, cooperativeSettle. This currently prevents us to have clean state modifiers Ideally, we should use modifiers for function input checks #197

Known existing solutions:

TODO: properly document known existing solutions.

[loredanacirstea]

As mentioned above, there are two main issues:
A. TokenNetworkRegistry bytecode size -> either running into Contract code size exceeds EIP170 limit of 24577 or contract being too hard to deploy when the block gas limit is lower.
B. stack too deep error in settleChannel, updateNonClosingBalanceProof, cooperativeSettle when trying to use modifiers / clean up the code.

Possible solutions for A:

1. Delegate Proxy with separate TokenChannel

Moving the channels code into a separate contract, therefore we could have something like: (note that the proxy setup can be improved, this is just a proof of concept)

contract TokenNetworkRegistry {
    address public erc20_token_channel_implementation;
    mapping(address => address) public token_to_token_networks;
    event TokenNetworkCreated(address indexed token_address, address indexed token_network_address);
   
    constructor(address _erc20_token_channel_implementation) {
        erc20_token_channel_implementation = _erc20_token_channel_implementation;
    }

    // Note: if we are going to support multiple token standards (and we should), then we could have:
    // mapping(uint256 => address) public token_standard_to_implementation;
    // function setImplementation(uint256 standard, address implementation) {
    //     require(token_standard_to_implementation[standard] == 0);
    //     token_standard_to_implementation[standard]
    // }

    function createERC20TokenNetwork(address _token_address)
        external
        returns (address token_network_address)
    {
        require(token_to_token_networks[_token_address] == address(0x0));

        TokenNetwork token_network;
        token_network = new TokenNetwork(_token_address, erc20_token_channel_implementation);
        token_network_address = address(token_network);

        token_to_token_networks[_token_address] = token_network_address;
        emit TokenNetworkCreated(_token_address, token_network_address);

        return token_network_address;
    }
}

contract TokenNetwork {
    Token public token;
    TokenChannel public token_channel_implementation;
    // mapping (address => Channel) public channels;
    
    event ChannelOpened(
        address indexed channel_identifier,
        address indexed participant1,
        address indexed participant2,
        uint256 settle_timeout
    );
    event ChannelRemoved(uint256 indexed channel_identifier);
    
    constructor(address _token_address, address _token_channel_implementation) {
        token = Token(_token_address);
        token_channel_implementation = TokenChannel(_token_channel_implementation);
    }
    
    function openChannel(address participant1, address participant2, uint256 settle_timeout) {
        TokenChannel channel = TokenChannel(new TokenChannelProxy(
            token_channel_implementation,
            address(this),
            participant1,
            participant2,
            settle_timeout
        ));
        emit ChannelOpened(address(channel), participant1, participant2, settle_timeout);
    }
}

contract TokenChannelData is ProxyData {
    address token_network;
    address participant1;
    address participant2;
    uint256 settle_block_number;
    
    mapping(address => Participant) public participants;
    
    struct Participant {
        uint256 deposit;
    }
    
    event ChannelNewDeposit(
        uint256 indexed channel_identifier,
        address indexed participant,
        uint256 total_deposit
    );
}

contract TokenChannel is TokenChannelData {
    function setTotalDeposit(address participant, uint256 total_deposit)
    {
        Participant storage participant_state = participants[participant];
        participant_state.deposit = total_deposit;
    }
}

contract TokenChannelProxy is Proxy, TokenChannelData {
    constructor(
        address proxied,
        address _token_network,
        address _participant1,
        address _participant2,
        uint256 _settle_block_number
    ) Proxy(proxied) {
        token_network = _token_network;
        participant1 = _participant1;
        participant2 = _participant2;
        settle_block_number = _settle_block_number;
    }
}

PROs:

• solves the bytecode size issue
• creating a new TokenNetwork will cost way less gas (I assume less than 800k, instead of 3.5mil as it is now)
• doesn't duplicate as much code as the libraries approach (see https://github.com/raiden-network/raiden/tree/6ada80cc0a5aa012b1cf8ab3704b0b63a6dc7fb5/raiden/smart_contracts) and we don't have to deal with linked libraries issues (e.g. compiling using linked libraries & remappings needs some tweaks due to the ~36 characters limit on the library name)
• if we want upgradable contracts in case of bugs (big topic, will be discussed in Smart contract upgradability #258), this implementation would allow us to offer users the chance to upgrade their implementation address, migrate their data and destroy the old TokenChannel contract (research needs to be done to see how much would this cost).

CONs:

• storage management; note the additional TokenChannelData contract that needs to be inherited by both TokenChannelProxy and TokenChannel, in order to keep the storage aligned between the implementation contract and the proxy that we create for each channel. This is very important. We might want to make a TokenChannelDataInternal for TokenChannelProxy, to not have the getters in the ABI (needs more thought, this means duplicating storage definitions).
• a bit more costly than our current implementation: the above is 210535 gas -> ~25k + 32k CREATE + 20k * 5 SSTORE + 375 LOG4. So, the main difference comes from also storing the _token_network address and the proxied address. But it is way less costly than using the old libraries approach (see https://github.com/raiden-network/raiden/tree/6ada80cc0a5aa012b1cf8ab3704b0b63a6dc7fb5/raiden/smart_contracts)
• this implementation would require changes in the Raiden client (to be looked into)
• an additional call to TokenNetwork will be needed when the TokenChannel contract will be destroyed, in order to keep the network state clean for the Raiden client

Note for DelegateProxy, if we use it, we should follow https://github.com/ethereum/EIPs/blob/master/EIPS/eip-897.md:

interface ERCProxy {
    // Forwarding proxy id = 1
    // Upgradeable proxy id = 2
    function proxyType() public pure returns (uint256 proxyTypeId);
    function implementation() public view returns (address codeAddr);
}

2. Delegate Proxy for the TokenNetwork.

The pattern is the same as in 1., but we do not have a separate channel contract.

PROs:

• deploying a TokenNetwork will cost less gas (I assume <500k instead of the 3.5 mil we have now)
• again, it would keep the upgradability option open with minimal source code changes

CONs:

• if used alone, it won't help us that much with bytecode size (especially if we want revert messages Add revert error strings for TokenNetwork #213); we can pair this with a TokenNetworkUtils contract/library that will contain pure functions for various calculations, as shown in Move constant functions to TokenNetworkUtils library #264

3. Libraries

Using libraries, as done in the old implementation https://github.com/raiden-network/raiden/tree/6ada80cc0a5aa012b1cf8ab3704b0b63a6dc7fb5/raiden/smart_contracts

• solves bytecode size only if we also have a separate TokenChannel contract (equivalent to the old NettingChannel contract)
• we decided against this for a reason: mainly the big gas cost needed for opening a channel; I would not recommend this approach now, that DelegateProxy can do the same thing but way cheaper.

Possible solutions for B:

1. structs for external/public calls

With the new ABI encoder from solc 0.5.0, we can also use structs for public or external functions. This can potentially mitigate the issue with the stack filling up (not sure how much, needs to be tested) and can potentially lead to cleaner code.

We are currently using structs for internal function calls - see

raiden-contracts/raiden_contracts/contracts/TokenNetwork.sol

Lines 1322 to 1325 in 052faf4

	function getMaxPossibleReceivableAmount(
	SettlementData participant1_settlement,
	SettlementData participant2_settlement
	)

The stack too deep issue when adding modifiers appeared with settleChannel, updateNonClosingBalanceProof, cooperativeSettle . So, these would look like:

// NOW:
function settleChannel(
        uint256 channel_identifier,
        address participant1,
        uint256 participant1_transferred_amount,
        uint256 participant1_locked_amount,
        bytes32 participant1_locksroot,
        address participant2,
        uint256 participant2_transferred_amount,
        uint256 participant2_locked_amount,
        bytes32 participant2_locksroot
    )

// AFTER:

struct SettlementData {
        address participant;
        uint256 transferred;
        uint256 locked;
        bytes32 locksroot;
    }

function settleChannel(
        uint256 channel_identifier,
        SettlementData participant1,
        SettlementData participant2
    )

// NOW:
function updateNonClosingBalanceProof(
        uint256 channel_identifier,
        address closing_participant,
        address non_closing_participant,
        bytes32 balance_hash,
        uint256 nonce,
        bytes32 additional_hash,
        bytes closing_signature,
        bytes non_closing_signature
)

// AFTER
struct BalanceData {
        bytes32 balance_hash;
        uint256 nonce;
        bytes32 additional_hash;
        bytes closing_signature;
}
function updateNonClosingBalanceProof(
        uint256 channel_identifier,
        address closing_participant,
        address non_closing_participant,
        BalanceData balance_data,
        bytes non_closing_signature
)

// NOW:
function cooperativeSettle(
        uint256 channel_identifier,
        address participant1_address,
        uint256 participant1_balance,
        address participant2_address,
        uint256 participant2_balance,
        bytes participant1_signature,
        bytes participant2_signature
)

// AFTER:
struct CooperativeSettlementData {
        address participant1_address;
        uint256 participant1_balance;
        address participant2_address;
        uint256 participant2_balance;
}

function cooperativeSettle(
        uint256 channel_identifier,
        CooperativeSettlementData coop_settlement_data,
        bytes participant1_signature,
        bytes participant2_signature
    )

2. Functions instead of modifiers

Instead of using modifiers, which keep the stack occupied until the function call is finished, we could use normal functions, that we always call first (code style to enforce readability and security). These would not fill up the main stack.

[loredanacirstea]

Team retreat November - we scoped out upgradability and decided to go with the approach suggested in #264 (splitting pure functions in a separate library).
Using the DelegateProxy pattern on the TokenNetwork contract in order to lower gas costs is not a priority, so it can be scoped out from Ithaca and reconsidered in case the economic model would require it.

[loredanacirstea]

Opened #401 for the current Ithaca solution mentioned in #215 (comment).
This parent issue can be labeled with backlog