Vyper: VIP: Implement delegate_call for contract interfaces

[brockelmore]

Simple Summary

Allow contract interface functions to be called as DELEGATECALL instead of just staticcall or call

Abstract

Implement an optional argument to external_contract_call that allows developers to specify whether the external function should be called as delegate_call.

Motivation

To allow for use of proxy contracts without having to raw_call and deal with the hassles around that.

Specification

Looking at https://github.com/ethereum/vyper/blob/master/vyper/parser/external_call.py, way to do this would be something like this:

def external_contract_call(node,
                           context,
                           contract_name,
                           contract_address,
                           pos,
                           value=None,
                           gas=None,
                           delegate_call=None):

Where default would be false and the call stays as non-delegate.

I think we would just need to add something comparable to what is currently used in raw_call https://github.com/ethereum/vyper/blob/f890abe1907d9c4329950c400f348f9102a5263b/vyper/functions/functions.py#L667

    if delegate_call.value == 1:
        z = LLLnode.from_list(
            [
                'seq',
                copier,
                [
                    'assert',
                    [
                        'delegatecall',
                        gas,
                        to,
                        ['add', placeholder_node, 32],
                        ['mload', placeholder_node],
                        ['add', output_node, 32],
                        outsize,
                    ],
                ],
                ['mstore', output_node, outsize],
                output_node,
            ],
            typ=ByteArrayType(outsize),
            location='memory',
            pos=getpos(expr),
        )

From what I can tell, here are the functions that would need to change and how to change them potentially. I don't have full knowledge of underlying functions so these are best guesses.

def get_external_contract_keywords(stmt_expr, context):
    from vyper.parser.expr import Expr
    value, gas = None, None
    for kw in stmt_expr.keywords:
        if kw.arg not in ('value', 'gas', 'delegate_call'):
            raise TypeMismatchException(
                'Invalid keyword argument, only "gas", :"value", and "delegate_call" supported.',
                stmt_expr,
            )
        elif kw.arg == 'gas':
            gas = Expr.parse_value_expr(kw.value, context)
        elif kw.arg == 'value':
            value = Expr.parse_value_expr(kw.value, context)
        elif kw.arg == 'delegate_call':
            delegate_call = Expr.parse_value_expr(kw.value, context)
    return value, gas, delegate_call

def external_contract_call(node,
                           context,
                           contract_name,
                           contract_address,
                           pos,
                           value=None,
                           gas=None,
                           delegate_call=None):
    from vyper.parser.expr import (
        Expr,
    )
    if value is None:
        value = 0
    if gas is None:
        gas = 'gas'
    if delegate_call is None:
        delegate_call = False
    if not contract_name:
        raise StructureException(
            f'Invalid external contract call "{node.func.attr}".',
            node
        )
    if contract_name not in context.sigs:
        raise VariableDeclarationException(
            f'Contract "{contract_name}" not declared yet',
            node
        )
    method_name = node.func.attr
    if method_name not in context.sigs[contract_name]:
        raise FunctionDeclarationException(
            (
                "Function not declared yet: %s (reminder: "
                "function must be declared in the correct contract)"
                " The available methods are: %s"
            ) % (method_name, ",".join(context.sigs[contract_name].keys())),
            node.func
        )
    sig = context.sigs[contract_name][method_name]
    inargs, inargsize, _ = pack_arguments(
        sig,
        [Expr(arg, context).lll_node for arg in node.args],
        context,
        pos=pos,
    )
    output_placeholder, output_size, returner = get_external_contract_call_output(sig, context)
    sub = [
        'seq',
        ['assert', ['extcodesize', contract_address]],
        ['assert', ['ne', 'address', contract_address]],
    ]
    # NOTE: I don't know how accurate this change is, I am just guessing based on raw_call
    if delegate_call == True:
        sub.append(
            [
                'seq',
                copier,
                [
                    'assert',
                    [
                        'delegatecall',
                        gas,
                        contract_address,
                        ['add', placeholder_node, 32],
                        ['mload', placeholder_node],
                        ['add', output_node, 32],
                        outsize,
                    ],
                ],
                ['mstore', output_node, outsize],
                output_node,
            ]
        )
    elif context.is_constant() or sig.const:
        sub.append([
            'assert',
            [
                'staticcall',
                gas, contract_address, inargs, inargsize, output_placeholder, output_size,
            ]
        ])
    else:
        sub.append([
            'assert',
            [
                'call',
                gas, contract_address, value, inargs, inargsize, output_placeholder, output_size,
            ]
        ])
    sub.extend(returner)
    o = LLLnode.from_list(sub, typ=sig.output_type, location='memory', pos=getpos(node))
    return o

And I am not sure what changes would need to be made to make_external_call(stmt_expr, context)

Backwards Compatibility

Fully compatible as far as I am aware of, but not sure

Dependencies

None that I am aware of.

Copyright

Copyright and related rights waived via CC0

[jacqueswww]

@brockelmore raw_call does have support for delegate call.

[brockelmore]

@brockelmore raw_call does have support for delegate call.

I know - this is to allow contract interfaces to be called as delegate_call so that as a developer I don't have to use raw_call and all the hassles that come with it

[jacqueswww]

Ah I see, my bad.

[fubuloubu]

I like this proposal, but I'm curious if supporting it in a different way might be clearer:

contract MyInterface:
    def static_call():    constant   # Uses STATICCALL
    def normal_call():    modifying  # Uses CALL
    def delegated_call(): delgating  # Uses DELGATECALL

What do you guys think of this instead?

[brockelmore]

I like this proposal, but I'm curious if supporting it in a different way might be clearer:

contract MyInterface:
    def static_call():    constant   # Uses STATICCALL
    def normal_call():    modifying  # Uses CALL
    def delegated_call(): delgating  # Uses DELGATECALL

What do you guys think of this instead?

Love this, thought briefly about implementing this way but not well versed enough to know how it might be done

[fubuloubu]

Yeah, not sure if it is flexible enough but I think it would be clearer than sneaking a delgatecall=True into a call. Still 50-50 on which is best. Will have to discuss in the next meeting! #1541

[jacqueswww]

Applying on the interface is definitely cleaner, but wold make the risky call less clear - at the point in the code where one calls the function. Also using delegatecall=True as kwarg would allow CALL/DELEGATECALL using the same function signature - or would specific functions always be delegated functions ?

[brockelmore]

In my mind, I can't think of when I would want one function to sometimes be delegating and sometimes not, but the use case may be out there. If it is implemented as kwarg, then it inherently can be called as both, although I don't know if that is the best and maybe should be locked down? If we can think of a use case, it may make sense to allow both call/delegatecall

[jacqueswww]

The problem with delegating a function is that you end up overwriting locally defined globals _super easily_.
For a proxy contract we would need to implement msg.data.
If you want to proxy we have create_forwarder_to that helps with the factory pattern.

[jacqueswww]

I.e. for every variable slot in the calling contract you would need an empty spacer slot in the called contact.
Or else you end up corrupting the calling contract.

[fubuloubu]

This one's definitely going to take some talking through, but we'll figure out a good, safe solution

[jacqueswww]

Not accepted, in favour of exposing msg.data https://github.com/ethereum/vyper/issues/1181 instead - allowing you to implement proxy pattern in your default function.

[fubuloubu]

A little further explanation: there is a difficultly when implementing more general delegated call patterns where target and source storage slots may conflict, and can lead to subtle security bugs we do not want to introduce to the language. #1181 should allow you to get the majority of the functionality you require, at the expense of a little more complexity in how you configure this pattern.

[brockelmore]

Not accepted, in favour of exposing msg.data #1181 instead - allowing you to implement proxy pattern in your default function.

Should I go ahead and close then?

[fubuloubu]

I think we keep VIPs open if they're being passed on for now, because they could be implemented in the future, and we want to make it easy to search for.

[jacqueswww]

@brockelmore we can keep it open until #1181 gets implemented.