Rename to `eth_signTypedData_v4` method for accounts provider. #1346

nlordell · 2021-03-23T17:52:26Z

This PR changes the EIP-712 handler in the accounts provider to use the method name eth_signTypedData_v4. This was originally proposed in #1200 and I believe was incorrectly closed.

The main motivation for this change is when using hardhat and hardhat-ethers on the hardhat-network, the following works like a charm (support was added in #1189):

const [signer] = await ethers.getSigners();
await signer._signTypedData(...);

However, when using other networks, such as connected to Infura with a private key or mnemonic (for example when implementing a task to interact with a deployed SC on mainnet that requires an EIP-712 signature), the following error occurs:

ProviderError: The method eth_signTypedData_v4 does not exist/is not available

Since the configuration includes a private key for signing, there is no reason not to be able to generate an ECDSA signature, noting that eth_sign works as expected, so it is not an issue with account configuration,

I believe this stems from the fact that the hardhat-network provider expects eth_signTypedData_v4 (again, added in #1189):
https://github.com/nomiclabs/hardhat/blob/f604e23d32ba3a21c3cd4490f67ed7a7fec25fd8/packages/hardhat-core/src/internal/hardhat-network/provider/modules/eth.ts#L251-L264

While, the accounts provider expects eth_signTypedData. The proposed change makes these two providers consistent with respect to which method name they are expecting (prefering eth_signTypedData_v4).

Some unit tests were thrown in for fun 🎉.

nlordell · 2021-03-23T18:43:35Z

Hmm, CI appears to be failing when installing packages:

error An unexpected error occurred: "https://registry.yarnpkg.com/string.prototype.trim/-/string.prototype.trim-1.2.3.tgz: Request failed \"502 Bad Gateway\"".

I don't think this is related to the changes.

alcuadrado · 2021-03-30T15:36:36Z

Hey @nlordell, thanks a lot for sending this PR. Unfortunately we don't have enough time to review this right now, but we'll get back to this once Berlin is implemented in the next couple of weeks.

bstchow · 2021-04-21T06:21:15Z

Good catch! I'm also seeing this blow up my EIP712 signing/hashing when switching providers. Specifically, Optimistic Ethereum tests are currently constrained to using a local testnet. Switching from L1 testing to L2 testing surfaced this mismatch since it changed provider interfaces (HTTP provider instead of local accounts)

fvictorio

LGTM, and thanks for including tests!

fvictorio

Oh wait, there's a failing test that is not a false negative, I'll look into it.

fvictorio · 2021-04-22T14:44:03Z

I made two changes:

Removed the address !== undefined check because validateParams already handles that (I guess the address is not optional).
Forwarded the method when the address is not local. I'm not sure if this is the behavior we want, but since we've had users complain about not being able to use the (unlocked, remote) accounts when local accounts are available, I think this makes sense.

@alcuadrado what do you think?

fvictorio · 2021-04-22T14:44:57Z

Btw, I think this:

      const [address, data] = validateParams(params, rpcAddress, t.any);

could be stricter. I don't think the data should be allowed to be undefined. I'm not sure how well defined its format is though (I haven't read the EIP).

nlordell · 2021-04-22T16:05:02Z

I made two changes:

I imagine these should also be applied to right above to eth_sign then right? Since it follows the same:

if (address !== undefined) {
  // ...
}

pattern and doesn't check if a private key is found for the address.

Btw, I think this:

🙈 I didn't look much when I rebased and didn't run the tests locally either. Sorry!

I see the failing test is:

        hardfork
          When forking
            Remote hardfork validation
              Shouldn't work with block numbers from before spurious dragon:

Because of AssertionError: expected promise to be rejected with 'InternalError' but it was rejected with 'TypeError: Only absolute URLs are supported'. Maybe there are some secrets that aren't there when running CI from a fork?

nlordell · 2021-04-22T16:07:50Z

I don't think the data should be allowed to be undefined

No, it actually needs to be an object with the following schema:

{
  type: 'object',
  properties: {
    types: {
      type: 'object',
      properties: {
        EIP712Domain: {type: 'array'},
      },
      additionalProperties: {
        type: 'array',
        items: {
          type: 'object',
          properties: {
            name: {type: 'string'},
            type: {type: 'string'}
          },
          required: ['name', 'type']
        }
      },
      required: ['EIP712Domain']
    },
    primaryType: {type: 'string'},
    domain: {type: 'object'},
    message: {type: 'object'}
  },
  required: ['types', 'primaryType', 'domain', 'message']
}

I just imagined that the ethSigUtil.signTypedData_v4 would throw and bubble up an error on bad data.

fvictorio · 2021-04-22T19:12:25Z

I imagine these should also be applied to right above to eth_sign then right? Since it follows the same:

Oh, maybe? We recently made validateParams smarter, so there might be some checks that are not needed anymore.

Because of AssertionError: expected promise to be rejected with 'InternalError' but it was rejected with 'TypeError: Only absolute URLs are supported'. Maybe there are some secrets that aren't there when running CI from a fork?

Yes, that's unrelated to this PR, don't worry. The test that was failing before was one of the new ones, but it's fixed now.

I just imagined that the ethSigUtil.signTypedData_v4 would throw and bubble up an error on bad data.

Yep, that's fair. Maybe we can just check that something exists and that it's an object.

bstchow · 2021-04-24T06:03:17Z

I believe this also needs similar JSON.parse logic to the hardhat-network provider.
https://github.com/nomiclabs/hardhat/blob/f604e23d32ba3a21c3cd4490f67ed7a7fec25fd8/packages/hardhat-core/src/internal/hardhat-network/provider/modules/eth.ts#L909-L921

When working with an up to date version of ethers, the params property input to the LocalAccountsProvider.request function is stringified JSON, as called out in the linked comment.

I think we should check if the data parameter in the modified code is a string.

      // if we don't manage the address, the method is forwarded
      const privateKey = this._getPrivateKeyForAddressOrNull(address);
      if (privateKey !== null) {
        let typedMessage = data;
        if (typeof data === "string") {
          try {
            typedMessage = JSON.parse(data);
          } catch (error) {
            throw new InvalidInputError(
              `The message parameter is an invalid JSON. Either pass a valid JSON or a plain object conforming to EIP712 TypedData schema.`
            );
          }
        }
        return ethSigUtil.signTypedData_v4(privateKey, {
          data: typedMessage,
        });
      }

~~It also seems like we would also want another unit test which checks that stringified JSON is accepted as valid input. I would submit a commit but I don't know that I have the permissions~~ Working on a PR

EDIT: Here's a PR:
https://github.com/nlordell/hardhat/pull/1

alcuadrado · 2021-05-25T19:45:49Z

Hey @nlordell, sorry we've been slow with this PR. We are trying to be extra cautious with this RPC method, as its EIP is not finalized and we already made a few mistakes. Do you have any opinion on @bstchow suggestion? Also, @fvictorio?

nlordell · 2021-06-01T09:06:30Z

sorry we've been slow with this PR.

No worries! I also completely missed https://github.com/nlordell/hardhat/pull/1, sorry @bstchow 🙈.

Add string support for eth_signTypedData_v4

bstchow · 2021-06-01T14:11:22Z

@nlordell no worries! Thanks for the merge and for finding the issue in the first place. You saved me a decent amount of time.

fvictorio · 2021-06-15T15:24:55Z

Thanks a lot for this @nlordell and @bstchow, and sorry for taking so long to review it!

nlordell · 2021-06-18T07:11:45Z

No worries! And thank you for the awesomeness that is hardhat. Seriously, its a great tool that substantially improved our workflow,

This PR bumps Ethers.js to the latest version. Note that it required changing the signing scheme to EIP-712. This is because it appears that Ethers + Hardhat `eth_sign` signatures are currently broken (NomicFoundation/hardhat#1981). This is because they recently changed to always use `personal_sign` for `signMessage` calls (ethers-io/ethers.js@8947fd4), which isn't supported by Hardhat: ``` MethodNotFoundError: Method personal_sign not found ``` Note that we had previously used `eth_sign` signatures because EIP-712 signatures were broken with hardhat, but this should no longer be the case (NomicFoundation/hardhat#1346). ### Test Plan CI still passes.

Nicholas Rodrigues Lordello added 2 commits April 21, 2021 09:03

Rename sign typed data method for accounts provider

72ab0b1

fix lint

6351b07

fvictorio approved these changes Apr 22, 2021

View reviewed changes

fvictorio requested changes Apr 22, 2021

View reviewed changes

Forward sign method if address is not local

6ffc54a

Interpret empty strings as undefined in env vars

40b2e32

Add string support for eth_signTypedData_v4

b141c08

fvictorio added the package:hardhat-core label May 14, 2021

Merge pull request #1 from zapstash/account-sign-typed-data

2d0e52a

Add string support for eth_signTypedData_v4

fvictorio added 3 commits June 15, 2021 11:44

Merge branch 'master' into account-sign-typed-data

ea6de2b

Fix typo in error message

787c450

Use === instead of ==

4dd9c71

fvictorio merged commit 567b70a into NomicFoundation:master Jun 15, 2021

nlordell mentioned this pull request Oct 29, 2021

Bump to latest Ethers and switch to EIP-712 signatures gnosis/gp-v2-contracts#875

Merged

github-actions bot locked as resolved and limited conversation to collaborators Nov 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename to `eth_signTypedData_v4` method for accounts provider. #1346

Rename to `eth_signTypedData_v4` method for accounts provider. #1346

nlordell commented Mar 23, 2021 •

edited

Loading

nlordell commented Mar 23, 2021

alcuadrado commented Mar 30, 2021

bstchow commented Apr 21, 2021 •

edited

Loading

fvictorio left a comment

fvictorio left a comment

fvictorio commented Apr 22, 2021

fvictorio commented Apr 22, 2021 •

edited

Loading

nlordell commented Apr 22, 2021 •

edited

Loading

nlordell commented Apr 22, 2021 •

edited

Loading

fvictorio commented Apr 22, 2021

bstchow commented Apr 24, 2021 •

edited

Loading

alcuadrado commented May 25, 2021

nlordell commented Jun 1, 2021

bstchow commented Jun 1, 2021

fvictorio commented Jun 15, 2021

nlordell commented Jun 18, 2021

Rename to eth_signTypedData_v4 method for accounts provider. #1346

Rename to eth_signTypedData_v4 method for accounts provider. #1346

Conversation

nlordell commented Mar 23, 2021 • edited Loading

nlordell commented Mar 23, 2021

alcuadrado commented Mar 30, 2021

bstchow commented Apr 21, 2021 • edited Loading

fvictorio left a comment

Choose a reason for hiding this comment

fvictorio left a comment

Choose a reason for hiding this comment

fvictorio commented Apr 22, 2021

fvictorio commented Apr 22, 2021 • edited Loading

nlordell commented Apr 22, 2021 • edited Loading

nlordell commented Apr 22, 2021 • edited Loading

fvictorio commented Apr 22, 2021

bstchow commented Apr 24, 2021 • edited Loading

alcuadrado commented May 25, 2021

nlordell commented Jun 1, 2021

bstchow commented Jun 1, 2021

fvictorio commented Jun 15, 2021

nlordell commented Jun 18, 2021

Rename to `eth_signTypedData_v4` method for accounts provider. #1346

Rename to `eth_signTypedData_v4` method for accounts provider. #1346

nlordell commented Mar 23, 2021 •

edited

Loading

bstchow commented Apr 21, 2021 •

edited

Loading

fvictorio commented Apr 22, 2021 •

edited

Loading

nlordell commented Apr 22, 2021 •

edited

Loading

nlordell commented Apr 22, 2021 •

edited

Loading

bstchow commented Apr 24, 2021 •

edited

Loading