search

raiden-network / raiden-contracts

Raiden Network Smart Contracts
MIT License
53 stars 45 forks source link

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

Open loredanacirstea opened 6 years ago

loredanacirstea commented 6 years ago

(In work issue)

tl;dr Code splitting with libraries and/or proxies could make the code cleaner. Feature requested for a while. Latest discussion: https://github.com/raiden-network/raiden-contracts/pull/213#issuecomment-410789290

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 https://github.com/raiden-network/raiden/pull/1286 refactoring:

Current issues

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

Known existing solutions:

TODO: properly document known existing solutions.

loredanacirstea commented 6 years ago

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:

CONs:

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:

CONs:

3. Libraries

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

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 https://github.com/raiden-network/raiden-contracts/blob/052faf488da140e84bb854272f96a5cdec773b38/raiden_contracts/contracts/TokenNetwork.sol#L1322-L1325

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 commented 5 years ago

Team retreat November - we scoped out upgradability and decided to go with the approach suggested in https://github.com/raiden-network/raiden-contracts/pull/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 commented 5 years ago

Opened https://github.com/raiden-network/raiden-contracts/issues/401 for the current Ithaca solution mentioned in https://github.com/raiden-network/raiden-contracts/issues/215#issuecomment-442031445. This parent issue can be labeled with backlog