Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

getBaseEncoder to support "uint" #4541

Closed
charlie-kim opened this issue Jan 13, 2024 · 31 comments
Closed

getBaseEncoder to support "uint" #4541

charlie-kim opened this issue Jan 13, 2024 · 31 comments
Assignees
Labels
bug Verified to be an issue. fixed/complete This Bug is fixed or Enhancement is complete and published. v6 Issues regarding v6

Comments

@charlie-kim
Copy link

charlie-kim commented Jan 13, 2024

Ethers Version

6.9.2

Search Terms

hash

Describe the Problem

getBaseEncoder function crashes when type is uint. match[2] is an empty string and it is not equal to null.

https://github.com/ethers-io/ethers.js/blob/main/src.ts/hash/typed-data.ts#L132C2-L132C2

Updating to this might work.
(match[2] == null || match[2] == '' || match[2] === String(width))

Code Snippet

No response

Contract ABI

No response

Errors

No response

Environment

node.js (v12 or newer)

Environment (Other)

No response

@charlie-kim charlie-kim added investigate Under investigation and may be a bug. v6 Issues regarding v6 labels Jan 13, 2024
@ricmoo
Copy link
Member

ricmoo commented Jan 13, 2024

That does look wrong… I’ll get that fixed right away. Thanks!

@ricmoo ricmoo added on-deck This Enhancement or Bug is currently being worked on. next-patch Issues scheduled for the next arch release. labels Jan 13, 2024
@charlie-kim
Copy link
Author

@ricmoo I would much appreciate it! I am trying to deploy my code with ethers V6 as soon as possible.

@ricmoo
Copy link
Member

ricmoo commented Jan 15, 2024

In the meantime, you can just use "uint256" instead (which is the same thing), but I will get it out today. :)

@ricmoo
Copy link
Member

ricmoo commented Jan 16, 2024

I've added a fix to the repo, but want to sleep on the solution over night before publishing.

Please feel free to check ou the change above though. And all dust files have been updated, so you can try it out by installing it (via npm) from the GitHub repo.

Would love some feedback.

Basically, I normalize the types in the constructor so there is no change to the getBaseEncoder, since the types are also required in other parts of the encoder to reflect what the baseEncoder would use. I also add a case allowing for a custom type of "uint" and "int", since technically according to the spec there is no type alias, so we make the type-alias only available in the absence of ambiguity.

There are test cases too, if curious what I mean. :)

@charlie-kim
Copy link
Author

Thanks. I think it should support uint[] and int[] as well.

@charlie-kim
Copy link
Author

Still getting error TypeError: invalid numeric width (argument="type", value="uint", code=INVALID_ARGUMENT, version=6.10.0) with the code below

await TypedDataEncoder.resolveNames({
      name: 'Rain Collateral',
      version: '1',
      chainId: '0x7a69',
      verifyingContract: '0xCf7Ed3AccA5a467e9e704C703E8D87F634fB0Fc9',
      salt: new Uint8Array([
        61, 148, 190,  20,  21, 86,  59, 231,
        65,  41, 246, 115, 158, 73, 184,  27,
        64, 115, 249, 177, 145, 65, 205, 148,
       181, 127,  98, 129,  44, 40, 143, 144
     ])
    }, {
      Withdraw: [
        { name: 'user', type: 'address' },
        { name: 'collateral', type: 'address' },
        { name: 'asset', type: 'address' },
        { name: 'amount', type: 'uint' },
        { name: 'recipient', type: 'address' },
        { name: 'nonce', type: 'uint' },
        { name: 'expiresAt', type: 'uint' }
      ]
    }, {
      user: '0x3C44CdDdB6a900fa2b585dd299e03d12FA4293BC',
      collateral: '0x7ab4C4804197531f7ed6A6bc0f0781f706ff7953',
      asset: '0xa513E6E4b8f2a923D98304ec87F64353C4D5C853',
      amount: 1000000n,
      recipient: '0xBB1dD05e4DaaaBC9A520D7d6863D7019Aac5710f',
      nonce: 0n,
      expiresAt: 1705430871
    },
    async (v: string) => {
      return v;
    })

@ricmoo
Copy link
Member

ricmoo commented Jan 16, 2024

Is that from GitHub? The error indicates you are using 6.10.0, but the fix is only in 6.10.1 (which is not on npm yet).

@ricmoo
Copy link
Member

ricmoo commented Jan 16, 2024

You’re right. I need to add the arrayification to that. I’m going to break that out into another function, like I should’ve in the first place.

@charlie-kim
Copy link
Author

Is that from GitHub? The error indicates you are using 6.10.0, but the fix is only in 6.10.1 (which is not on npm yet).

I installed as this.

"ethers": "https://github.com/ethers-io/ethers.js#43fb9c233696aeaa80b1c2b0e5fafce90e0ad508"

by running command

yarn add ethers@https://github.com/ethers-io/ethers.js#43fb9c233696aeaa80b1c2b0e5fafce90e0ad508

@ricmoo
Copy link
Member

ricmoo commented Jan 16, 2024

@charlie-kim I've refactored the Array parsing code. I'd love an extra set of eyes to look it over. ;)

You should be able to install the main branch. I find sometimes the hash linked to by the change doesn't work... I just tried your above example locally and it seemed happy. :)

@charlie-kim
Copy link
Author

Hey @ricmoo , I just found out I was running code in lib.commonjs and it didn't have the change you made. What is the process to have the change in the code here?

@ricmoo
Copy link
Member

ricmoo commented Jan 16, 2024

The lib.commonjs code should be updated along with the lib.esm. Ca you check the version getting imported with console.log(ethers.version)? It should be 6.10.1 if the npm install from GitHub worked properly.

@charlie-kim
Copy link
Author

The current main branch seems to have the code built for lib.commonjs. I think the current main branch is working but let me test further to make sure.

@charlie-kim
Copy link
Author

FYI, my contract has signature type defined with uint and it only works with signature created by ethers.js with uint, not with uint256. I was able to get the contract validate a signature made with uint with the change you made.

It looks good to me!

@ricmoo
Copy link
Member

ricmoo commented Jan 17, 2024

There shouldn’t be any difference between using uint and uint256. Can you include a quick example input and output?

@ricmoo
Copy link
Member

ricmoo commented Jan 17, 2024

I tried your above example, swapping out the uint and uint256 and both returned the same hash.

@charlie-kim
Copy link
Author

My test called TypedDataEncoder.getPayload at some point.

domain = {
      name: 'Rain Collateral',
      version: '1',
      chainId: '0x7a69',
      verifyingContract: '0xCf7Ed3AccA5a467e9e704C703E8D87F634fB0Fc9',
      salt: new Uint8Array([
        157, 160,  69, 225, 246, 213,  92, 128,
        165, 255, 236, 208, 122, 173, 169,  52,
         24, 169,  88, 221, 214, 253, 244,  10,
         90,  74, 219,  85, 247, 228,  40,  21
      ])
    }
    types = {
      Withdraw: [
        { name: 'user', type: 'address' },
        { name: 'collateral', type: 'address' },
        { name: 'asset', type: 'address' },
        { name: 'amount', type: 'uint' },
        { name: 'recipient', type: 'address' },
        { name: 'nonce', type: 'uint' },
        { name: 'expiresAt', type: 'uint' }
      ]
    };
    data = {
      user: '0x3C44CdDdB6a900fa2b585dd299e03d12FA4293BC',
      collateral: '0x7ab4C4804197531f7ed6A6bc0f0781f706ff7953',
      asset: '0xa513E6E4b8f2a923D98304ec87F64353C4D5C853',
      amount: 1000000n,
      recipient: '0x0b4Ded768d5B7B975234c7e52053E714cCBEB5a6',
      nonce: 0n,
      expiresAt: 1705512338
    }
    console.log(JSON.stringify(await TypedDataEncoder.getPayload(
      domain,
      types,
      data,
    ), null, 4));

returns

{
    "types": {
        "Withdraw": [
            {
                "name": "user",
                "type": "address"
            },
            {
                "name": "collateral",
                "type": "address"
            },
            {
                "name": "asset",
                "type": "address"
            },
            {
                "name": "amount",
                "type": "uint"
            },
            {
                "name": "recipient",
                "type": "address"
            },
            {
                "name": "nonce",
                "type": "uint"
            },
            {
                "name": "expiresAt",
                "type": "uint"
            }
        ],
        "EIP712Domain": [
            {
                "name": "name",
                "type": "string"
            },
            {
                "name": "version",
                "type": "string"
            },
            {
                "name": "chainId",
                "type": "uint256"
            },
            {
                "name": "verifyingContract",
                "type": "address"
            },
            {
                "name": "salt",
                "type": "bytes32"
            }
        ]
    },
    "domain": {
        "name": "Rain Collateral",
        "version": "1",
        "chainId": "0x7a69",
        "verifyingContract": "0xcf7ed3acca5a467e9e704c703e8d87f634fb0fc9",
        "salt": "0x9da045e1f6d55c80a5ffecd07aada93418a958ddd6fdf40a5a4adb55f7e42815"
    },
    "primaryType": "Withdraw",
    "message": {
        "user": "0x3c44cdddb6a900fa2b585dd299e03d12fa4293bc",
        "collateral": "0x7ab4c4804197531f7ed6a6bc0f0781f706ff7953",
        "asset": "0xa513e6e4b8f2a923d98304ec87f64353c4d5c853",
        "amount": "1000000",
        "recipient": "0x0b4ded768d5b7b975234c7e52053e714ccbeb5a6",
        "nonce": "0",
        "expiresAt": "1705512338"
    }
}

and

domain = {
     name: 'Rain Collateral',
     version: '1',
     chainId: '0x7a69',
     verifyingContract: '0xCf7Ed3AccA5a467e9e704C703E8D87F634fB0Fc9',
     salt: new Uint8Array([
       157, 160,  69, 225, 246, 213,  92, 128,
       165, 255, 236, 208, 122, 173, 169,  52,
        24, 169,  88, 221, 214, 253, 244,  10,
        90,  74, 219,  85, 247, 228,  40,  21
     ])
   }
   types = {
     Withdraw: [
       { name: 'user', type: 'address' },
       { name: 'collateral', type: 'address' },
       { name: 'asset', type: 'address' },
       { name: 'amount', type: 'uint256' },
       { name: 'recipient', type: 'address' },
       { name: 'nonce', type: 'uint256' },
       { name: 'expiresAt', type: 'uint256' }
     ]
   };
   data = {
     user: '0x3C44CdDdB6a900fa2b585dd299e03d12FA4293BC',
     collateral: '0x7ab4C4804197531f7ed6A6bc0f0781f706ff7953',
     asset: '0xa513E6E4b8f2a923D98304ec87F64353C4D5C853',
     amount: 1000000n,
     recipient: '0x0b4Ded768d5B7B975234c7e52053E714cCBEB5a6',
     nonce: 0n,
     expiresAt: 1705512338
   }
   console.log(JSON.stringify(await TypedDataEncoder.getPayload(
     domain,
     types,
     data,
   ), null, 4));

returns

{
    "types": {
        "Withdraw": [
            {
                "name": "user",
                "type": "address"
            },
            {
                "name": "collateral",
                "type": "address"
            },
            {
                "name": "asset",
                "type": "address"
            },
            {
                "name": "amount",
                "type": "uint256"
            },
            {
                "name": "recipient",
                "type": "address"
            },
            {
                "name": "nonce",
                "type": "uint256"
            },
            {
                "name": "expiresAt",
                "type": "uint256"
            }
        ],
        "EIP712Domain": [
            {
                "name": "name",
                "type": "string"
            },
            {
                "name": "version",
                "type": "string"
            },
            {
                "name": "chainId",
                "type": "uint256"
            },
            {
                "name": "verifyingContract",
                "type": "address"
            },
            {
                "name": "salt",
                "type": "bytes32"
            }
        ]
    },
    "domain": {
        "name": "Rain Collateral",
        "version": "1",
        "chainId": "0x7a69",
        "verifyingContract": "0xcf7ed3acca5a467e9e704c703e8d87f634fb0fc9",
        "salt": "0x9da045e1f6d55c80a5ffecd07aada93418a958ddd6fdf40a5a4adb55f7e42815"
    },
    "primaryType": "Withdraw",
    "message": {
        "user": "0x3c44cdddb6a900fa2b585dd299e03d12fa4293bc",
        "collateral": "0x7ab4c4804197531f7ed6a6bc0f0781f706ff7953",
        "asset": "0xa513e6e4b8f2a923d98304ec87f64353c4d5c853",
        "amount": "1000000",
        "recipient": "0x0b4ded768d5b7b975234c7e52053e714ccbeb5a6",
        "nonce": "0",
        "expiresAt": "1705512338"
    }
}

And contract can only validate the signature from the first one.

My test uses hardhat-ethers and it does this to create signature.

this.provider.send("eth_signTypedData_v4", [
      this.address.toLowerCase(),
      JSON.stringify(
        TypedDataEncoder.getPayload(populated.domain, types, populated.value),
        (_k, v) => {
          if (typeof v === "bigint") {
            return v.toString();
          }

          return v;
        }
      ),
    ])

@charlie-kim
Copy link
Author

charlie-kim commented Jan 17, 2024

I can successfully test signature by using pure ethers js. But using it with hardhat-ethers does not work with the new version.

This one works.

import { Wallet } from "ethers";
const wallet = Wallet.createRandom();
const signature = await wallet.signTypedData(domain, PaySignatureTypes, data);
const recovered = verifyTypedData(domain, PaySignatureTypes, data, signature);
expect(wallet.address).to.be.eq(recovered);

This one does not work.

import { ethers } from "hardhat";
const [signer] = await ethers.getSigners();
const signature = await signer.signTypedData(domain, PaySignatureTypes, data);
const recovered = verifyTypedData(domain, PaySignatureTypes, data, signature);
expect(signer.address).to.be.eq(recovered);

The work might be on them not you.

@ricmoo
Copy link
Member

ricmoo commented Jan 17, 2024

I think I spotted the problem earlier. I just need to prepare a test case for the before-and-after.

I think it’s a one-line fix when preparing the payload.

Just got back from a tooth extraction though, so I’ll get to it in the next couple hours. :)

@charlie-kim
Copy link
Author

Oh dang! Are you still under influence of anesthesia?? Take it easy!

@ricmoo
Copy link
Member

ricmoo commented Jan 18, 2024

Oh no. They just froze my mouth. No crazy anesthesia. :)

I've pushed a new version up to GitHub, if you could test against the main branch again? :)

@charlie-kim
Copy link
Author

charlie-kim commented Jan 18, 2024

It works fine now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified to be an issue. fixed/complete This Bug is fixed or Enhancement is complete and published. v6 Issues regarding v6
Projects
None yet
Development

No branches or pull requests

2 participants