vyperlang / vyper

Pythonic Smart Contract Language for the EVM
https://vyperlang.org
Other
4.87k stars 801 forks source link

VIP: Signed Structs (EIP 712 Support) #1020

Open fubuloubu opened 6 years ago

fubuloubu commented 6 years ago

Simple Summary

Allow defining a struct that contains a signature field such that there is a standard algorithm for performing ecrecover on it. Would be most useful with #1019

Abstract

In constructions like Plasma, it is often useful to define a transaction data structure to manage the complexity of a transaction type and how it is used to validate logic in the contract. Having a struct type that can define an extra signature field which can be processed in a standard algorithm to recover the signer of the transaction would be very helpful to streamline the design of these algorithms without too much complexity in the contract.

Motivation

We want to make working with different constructions like Plasma as simple and expressive as possible. Primatives like working with signed transactions are very commonplace in this constructions, and we should support primitives like this as first class citizens to make it easier and safer to write these kinds of contracts.

Specification

We might define a structure as follows that contains a VRS signature with which recovery of the signing address can be performed via exclusion of the signature, hashing of the rest of the struct as an RLP encoded object, and performing ecrecover(hash, r, s, v) of the result. The field that would contain this signature in this special struct would be denoted by __sig__ . get_signer(struct) would be the function that performed this function to return the signer's address.

struct Transaction:
    a: address
    b: uint256
    __sig__: VRS_SECP256K1  # Special type representing a VRS signature for a Secp256k1 signature
...

# Assert that the transaction's signer is the caller here.
assert get_signer(transaction) == msg.sender
...

There might potentially be defined a custom type to represent a VRS signature, but should be extensible with other types for other signature methods that may be included later.

Backwards Compatibility

This would be fully backwards compatible as it is a new featureset that would be added.

Copyright

Copyright and related rights waived via CC0

jacqueswww commented 6 years ago

Approved, dependency on #1019 .

fubuloubu commented 6 years ago

Note: dependency is only recommended to make it easier to work with. This proposal stands alone.

charles-cooper commented 5 years ago

Linking to https://github.com/ethereum/EIPs/blob/c6a476b64d9dff50cd18f12d03bd552751956537/EIPS/eip-712.md for reference

jacqueswww commented 5 years ago

We will evaluate this one at implementation time, to probably use EIP712 :)

charles-cooper commented 5 years ago

After absorbing EIP712 a little more, it seems that this VIP and EIP712 are fairly closely aligned and we can follow the spec in EIP712 for hashing/signing structs. I that __sig__ doesn't need to be part of the struct, but rather could be carried around as associated data. So for instance, following the analogy between eth_sign and eth_signTypedData, ecrecover for structs could look like ecrecover_packed(digest_typed_data(MyDomain, struct), sig) as in https://github.com/ethereum/EIPs/blob/f2d42c39e9318d4a706e1e06bacf09c9383a981e/assets/eip-712/Example.sol#L74.

The only thing in EIP712 which hasn't been considered yet for the scope of this VIP is the domain separator struct. I propose a new keyword to help define this struct, EIP712Domain, and if/when EIP712 is broadly implemented we can change the keyword simply to domain. This keyword can be implemented similarly to contract and struct, and have the following syntax:

EIP712Domain MyDomain:
    name: [string literal]
    chainId: [uint256 literal]
    verifyingContract: [address literal]
    salt: [bytes32 literal]

As in the EIP712 spec, the EIP712Domain definition must have one or more of those fields, but may omit unused fields. The EIP712Domain may have a name, which permits defining multiple domains in a single .vy file. This is allows the dapp developer to sign structs for multiple other contracts, or maintain multiple domains in a single contract. Additionally, I considered that a struct definition could include a domain so that digest_typed_data does not need to take a domain as a parameter, but to avoid complications around type-checking and usability, I think they should be defined and passed around completely separately.

fubuloubu commented 5 years ago

Mostly agree with that assessment, with a few caveats.


"[The signature can] be carried around as associated data."

How would this work in practice?

My idea with having the signature defined as part of the struct would be to allow signed transactions to be submitted as struct data inputs to functions that may use that transaction to verify or make an assertion about some larger action, like the Plasma exit use case (reference my code).

I also wanted to make it easier to do core operations on these data structures, like recover the signing account, without having to directly handle the signature or anything. Something like assert msg.sender == recover_account(signed_transaction)

fubuloubu commented 5 years ago

Linking to https://github.com/ethereum/EIPs/pull/712 for posterity

charles-cooper commented 5 years ago

A couple conclusions from gitter: EIP712Domain should also include version (of course!) verifyingContract should allow self as an option chainId should allow the chainId opcode as an option (dependent on EIP 1344)

Also, the contract should expose the domain separator values as public variables

fubuloubu commented 5 years ago

Should we allow chainId before EIP 1344 is implemented?

jacqueswww commented 5 years ago

Yes, rather safe than sorry.

charles-cooper commented 5 years ago

One idea I have been toying with is to just let the user set any domains in the constructor. However, then we need a way to determine which fields are set (since according to the spec fields can be nullable). Not just for calculating the hash in contract, but also if we expose those fields to a user agent. We could perhaps use 0 or the null string as sentinel values but I am sure some user will want to use those values. Actually I think this issue needs to be solved whether the domain fields are set in storage or the runtime code.

On Thu, Apr 4, 2019, 2:49 PM Jacques Wagener notifications@github.com wrote:

Yes, rather safe than sorry.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ethereum/vyper/issues/1020#issuecomment-480076260, or mute the thread https://github.com/notifications/unsubscribe-auth/ADsDbV29LIxbM67nJsiOF-wAxI1URAuuks5vdnNKgaJpZM4Woht2 .

fubuloubu commented 5 years ago

Note: Cannot implement this manually without #1406

charles-cooper commented 5 years ago

Note: Cannot implement this manually without #1406

Really? I figure you only need abi.encode in order to compute the EIP712 hash.

fubuloubu commented 5 years ago

Yeah, that was my point haha. Without abi.encode I can't implement EIP712 in Vyper manually.

This VIP would add an additional syntax to shortcut the manual process. It could also get away with not leveraging abi.encode internally, which is why this proposal does not depend on #1406

fubuloubu commented 5 years ago

What do we think of the following as a specification for this type of message signing and signature recovery, using EIP712:

from vyper.messages import EIP712Domain, recover_message
from vyper.signatures import VRS_SECP256K1  # (v, r, s) tuple type, 65 bytes

# Can be any valid struct
# (gets signed offline using EIP712 API with above domain)
struct Transaction:
    a: address
    b: uint256

@public
def foo(txn: Transaction, sig: VRS_SECP256K1)
    # Assert that the transaction's signer is the caller here.
    myDomain: EIP712Domain = EIP712Domain({
        name: [string literal]                        # optional unless no others
        version: [string literal]                     # optional unless no others
        chainId: [uint256 literal or variable]        # optional unless no others
        verifyingContract: [address literal OR self]  # optional unless no others
        salt: [bytes32 literal]                       # optional unless no others
    })
    assert recover_message(txn, sig, type=myDomain) == msg.sender
    ...  # Stuff, now that we authenticated the signer
fubuloubu commented 5 years ago

With the upcoming Istanbul hardfork, we should include support for EIP-1344 in this feature.

fubuloubu commented 5 years ago

Had another idea for syntax using decorators which I think would look much cleaner:

from vyper.message import EIP712Domain, EIP712Message, eip712_recover

EIP712Domain TransactionDomain:
    name: "My Protocol"      # must be a string
    version: "1.0"           # must be a string
    chainId: tx.chain_id     # this or uint256 literal
    verifyingContract: self  # this or address literal

# By decorating with this datatype, this informs
# the compiler to use the EIP712 Hashing strategy
@EIP712Message(TransactionDomain)
struct Transaction:
    a: address
    b: uint256

@public
def do_something(
    # Hash via EIP712 hashing strategy because of EIP712Message decorator
    _msg: Transaction,
    # bytes[65] under the hood, note: web3py uses RSV convention... but precompile uses VRS
    _sig: RSV_SECP256K1,
) -> address:
    # 1. Hash TransactionDomain and _msg together (3 hashes)
    # 2. Extract _sig to r, s, v components
    # 3. Run ecrecover precompile
    return eip712_recover(_msg, _sig)