Skip to content
This repository has been archived by the owner on Oct 19, 2024. It is now read-only.

derive-eip712: initial implementation of eip712 derive macro #481

Merged
merged 31 commits into from
Oct 8, 2021

Conversation

Ryanmtate
Copy link
Contributor

@Ryanmtate Ryanmtate commented Oct 1, 2021

This commit provides an initial implementation for a derive macro
to encode typed data according to EIP-712, https://eips.ethereum.org/EIPS/eip-712

Additionally, this commit introduces a new signer trait method:

    async fn sign_typed_data<T: Eip712 + Send + Sync>(
        &self,
        payload: &T,
    ) -> Result<Signature, Self::Error>;

And implements the new method for each of the signers (wallet, ledger,
aws).

At the moment, derive does not recurse the primary type to find nested
Eip712 structs. In the future, a field helper attribute #[eip712] could be
used to denote a nested field and handle the proper encoding for the field.

Motivation

#16

Solution

This implementation is based on the proposed solution in #16 .

PR Checklist

  • Added Tests
  • Added Documentation
  • Updated the changelog

derive-eip712/src/lib.rs Outdated Show resolved Hide resolved
derive-eip712/src/lib.rs Outdated Show resolved Hide resolved
derive-eip712/src/lib.rs Outdated Show resolved Hide resolved
derive-eip712/src/lib.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
derive-eip712/src/lib.rs Outdated Show resolved Hide resolved
ethers-core/ethers-derive-eip712/src/lib.rs Outdated Show resolved Hide resolved
ethers-core/src/types/transaction/eip712.rs Outdated Show resolved Hide resolved
ethers-core/src/types/transaction/eip712.rs Show resolved Hide resolved
ethers-core/src/types/transaction/eip712.rs Outdated Show resolved Hide resolved
ethers-core/src/types/transaction/eip712.rs Outdated Show resolved Hide resolved
ethers-signers/src/lib.rs Outdated Show resolved Hide resolved
ethers-signers/src/wallet/mod.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some comments/questions/recommendations

ethers-core/src/types/transaction/eip712.rs Outdated Show resolved Hide resolved
ethers-core/src/types/transaction/eip712.rs Outdated Show resolved Hide resolved
ethers-core/src/types/transaction/eip712.rs Show resolved Hide resolved
ethers-core/src/types/transaction/eip712.rs Outdated Show resolved Hide resolved
ethers-core/src/types/transaction/eip712.rs Outdated Show resolved Hide resolved
ethers-core/src/types/transaction/eip712.rs Outdated Show resolved Hide resolved
ethers-core/src/types/transaction/eip712.rs Outdated Show resolved Hide resolved
ethers-core/ethers-derive-eip712/src/lib.rs Outdated Show resolved Hide resolved
@Ryanmtate
Copy link
Contributor Author

@gakonst @mattsse Looking to finish up on the example and tests today and open for a review.

One last issue I am battling is with the example/eip712.rs final encoding.

Able to confirm domain separator, type hash and struct hash are all matching their solidity equivalents, but for some reason the final encoding does not match between the contract and the Eip712 trait derived macro implementing.

examples/DeriveEip712Test.sol and examples/derive_eip712_abi.json contain the full example, but the problem method is, encodeEip712:

function encodeEip712(FooBar memory fooBar) public pure returns (bytes32) {
        return
            keccak256(
                abi.encodePacked(
                    "\\x19\\x01",
                    domainSeparator(),
                    structHash(fooBar)
                )
            );
    }

examples/eip712.rs file contains full text, but the test seems to fail on encodeEip712 comparison:

let domain_separator = contract.domain_separator().call().await?;
    let type_hash = contract.type_hash().call().await?;
    let struct_hash = contract.struct_hash(derived_foo_bar.clone()).call().await?;
    let encoded = contract
        .encode_eip_712(derived_foo_bar.clone())
        .call()
        .await?;
    let verify = contract
        .verify_foo_bar(wallet.address(), derived_foo_bar, r, s, v)
        .call()
        .await?;

    assert_eq!(
        domain_separator,
        FooBar::domain_separator()?,
        "domain separator does not match contract domain separator!"
    );

    assert_eq!(
        type_hash,
        FooBar::type_hash()?,
        "type hash does not match contract struct type hash!"
    );

    assert_eq!(
        struct_hash,
        foo_bar.clone().struct_hash()?,
        "struct hash does not match contract struct struct hash!"
    );

    assert_eq!(
        encoded,
        foo_bar.encode_eip712()?,
        "Encoded value does not match!"
    );

@mattsse
Copy link
Collaborator

mattsse commented Oct 5, 2021

hmm,
just to make sure I understand that snippet,

  • domain seprator matches
  • struct hash matches

but encode_eip712 fails?

this would mean that

                abi.encodePacked(
                    "\\x19\\x01",
                    domainSeparator(),
                    structHash(fooBar)
                )

is not identical to

               let digest_input = [
                    &[0x19, 0x01],
                    &Self::domain_separator()?[..],
                    &self.struct_hash()?[..]
                ].concat();

and since the values (domainSeparator, structHash) are identical (ensured by the test), this would indicate encodePacked is not identical to concat?
could you check that?

@gakonst
Copy link
Owner

gakonst commented Oct 5, 2021

I think abi.encode is what's equivalent to concat. encodePacked is a non-standard encoding. It may be exposed via ethabi though I haven't investigated.

@Ryanmtate
Copy link
Contributor Author

Great news! The problem was much simpler. Let it be known that \\x19\\x01 does not encode to &[x19, x01], but \x19\x01 does.

Going to open this up for review after resolving merge conflicts.

@mattsse
Copy link
Collaborator

mattsse commented Oct 6, 2021

Great news! The problem was much simpler. Let it be known that \\x19\\x01 does not encode to &[x19, x01], but \x19\x01 does.

Going to open this up for review after resolving merge conflicts.

does &[x19, x01][..] do?

This commit provides an initial implementation for a derive macro
to encode typed data according to EIP-712, https://eips.ethereum.org/EIPS/eip-712

Additionally, this commit introduces a new signer trait method:

    async fn sign_typed_data<T: Eip712 + Send + Sync>(
        &self,
        payload: &T,
    ) -> Result<Signature, Self::Error>;

And implements the new method for each of the signers (wallet, ledger,
aws).

Additionally, these changes include using `WalletError` for the Wallet
signer error type

At the moment, derive does not recurse the primary type to find nested
Eip712 structs. This is something that is noted in the source and
currently responds with an error regarding custom types.

A subsequent PR should be opened once this issue becomes needed. For the
moment, the current implementation should satisfy non-nested, basic struct types.
@Ryanmtate Ryanmtate marked this pull request as ready for review October 6, 2021 00:54
@Ryanmtate
Copy link
Contributor Author

Finding some issues in the signer methods, so let's hold off from merging until can confirm correctness.

@Ryanmtate
Copy link
Contributor Author

Was able to manually test 909c491 is working for sign typed data and recoverable inside solidity with ecrecover for ledger signer! 🎉

I have not tested AWS signer, and would love if someone who has a setup can check it out.

@Ryanmtate
Copy link
Contributor Author

Also still open to comments for possibly relocating files to enable re-exportation of the derive macro. I agree current user experience is not great with having to add another dependency. But doing will require some re-working of the file struct so we are not hitting a circular dependency.

@Ryanmtate
Copy link
Contributor Author

Thanks to @sebastinez for helping test ledger signer!

@gakonst
Copy link
Owner

gakonst commented Oct 7, 2021

@mattsse @Ryanmtate I think the best way to resolve the cyclical dependency is for us to make an ethers-macros crate where we hold all useful derives that are low-level type related. We'd still keep abigen in ethers-contract.

I'm OK with merging as-is now, and we can resolve the deps in a follow-up PR

@Ryanmtate Ryanmtate requested a review from gakonst October 7, 2021 18:29
@mattsse
Copy link
Collaborator

mattsse commented Oct 7, 2021

@mattsse @Ryanmtate I think the best way to resolve the cyclical dependency is for us to make an ethers-macros crate where we hold all useful derives that are low-level type related. We'd still keep abigen in ethers-contract.

I'm OK with merging as-is now, and we can resolve the deps in a follow-up PR

second that,
amazing work @Ryanmtate

@gakonst
Copy link
Owner

gakonst commented Oct 7, 2021

---- eth_tests::test_derive_eip712 stdout ----
thread 'eth_tests::test_derive_eip712' panicked at 'called `Result::unwrap()` on an `Err` value: SolcError("Error: Source file requires different compiler version (current compiler is 0.6.6+commit.6c089d02.Linux.g++ - note that nightly builds are considered to be strictly less than the released version\n --> tests/solidity-contracts/DeriveEip712Test.sol:2:1:\n  |\n2 | pragma solidity ^0.8.0;\n  | ^^^^^^^^^^^^^^^^^^^^^^^\n\n")', ethers-contract/tests/common/mod.rs:35:10
error: test failed, to rerun pass '-p ethers-contract --test contract'
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Just need to fix the solc version to be the same one as the one installed in CI.

The WASM test is also seemingly failing, is this a flake?

@mattsse
Copy link
Collaborator

mattsse commented Oct 7, 2021

---- eth_tests::test_derive_eip712 stdout ----
thread 'eth_tests::test_derive_eip712' panicked at 'called `Result::unwrap()` on an `Err` value: SolcError("Error: Source file requires different compiler version (current compiler is 0.6.6+commit.6c089d02.Linux.g++ - note that nightly builds are considered to be strictly less than the released version\n --> tests/solidity-contracts/DeriveEip712Test.sol:2:1:\n  |\n2 | pragma solidity ^0.8.0;\n  | ^^^^^^^^^^^^^^^^^^^^^^^\n\n")', ethers-contract/tests/common/mod.rs:35:10
error: test failed, to rerun pass '-p ethers-contract --test contract'
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Just need to fix the solc version to be the same one as the one installed in CI.

The WASM test is also seemingly failing, is this a flake?

weird, only the http provider wasm test fails due to a 400 MiddlewareError(MiddlewareError(JsonRpcClientError(SerdeJson { err: Error("invalid type: integer400, expected struct Response", line: 1, column: 3), text: "400 Bad Request" })))
still works locally -.-

@Ryanmtate
Copy link
Contributor Author

Thanks @gakonst @mattsse for the help and pointers!

@gakonst
Copy link
Owner

gakonst commented Oct 8, 2021

@Ryanmtate need to enable abi encoder v2 i think

@gakonst gakonst merged commit d7ab229 into gakonst:master Oct 8, 2021
@Ryanmtate
Copy link
Contributor Author

🎉

@sebastinez
Copy link
Contributor

kudos @Ryanmtate! 🥳

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants