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

Allow null-ish properties in TypedData domains to imply unspecified #3623

Open
frangio opened this issue Jan 8, 2023 · 3 comments
Open

Allow null-ish properties in TypedData domains to imply unspecified #3623

frangio opened this issue Jan 8, 2023 · 3 comments
Assignees
Labels
enhancement New feature or improvement. minor-bump Planned for the next minor version bump. v5 Issues regarding legacy-v5

Comments

@frangio
Copy link

frangio commented Jan 8, 2023

Ethers Version

5.7.2

Search Terms

hashDomain

Describe the Problem

hashDomain throws an error when the argument is a domain object with name: undefined. Note that it correctly processes a domain object with no name at all, because it's an optional field. A name field with an undefined value should be treated the same way, if only because TypeScript can't distinguish a missing field from an undefined-valued field.

This probably applies to all the other optional domain fields as well, though I haven't tested.

Code Snippet

utils._TypedDataEncoder.hashDomain({ name: undefined })

Contract ABI

No response

Errors

Uncaught TypeError: Cannot read properties of undefined (reading 'length')
    at toUtf8Bytes (node_modules/@ethersproject/strings/lib/utf8.js:179:29)
    at id (node_modules/@ethersproject/hash/lib/id.js:7:65)
    at node_modules/@ethersproject/hash/lib/typed-data.js:161:32
    at node_modules/@ethersproject/hash/lib/typed-data.js:293:56

Environment

No response

Environment (Other)

No response

@frangio frangio added the investigate Under investigation and may be a bug. label Jan 8, 2023
@ricmoo
Copy link
Member

ricmoo commented Jan 22, 2023

I tend to agree that undefined should be treated as not specified. I would also expand this to null too.

But one quick note is that Typescript can distinguish a missing field (i.e. !("foo" in bar)) and an undefined value (i.e. "foo" in bar && bar.foo === undefined).

Moving this to an enhancement, which will probably be included in v6. :)

@ricmoo ricmoo added enhancement New feature or improvement. on-deck This Enhancement or Bug is currently being worked on. and removed investigate Under investigation and may be a bug. labels Jan 22, 2023
@ricmoo ricmoo changed the title Bug Report Title Allow null-ish properties in TypedData domains to imply unspecified Jan 22, 2023
@frangio
Copy link
Author

frangio commented Jan 23, 2023

But one quick note is that Typescript can distinguish a missing field (i.e. !("foo" in bar)) and an undefined value (i.e. "foo" in bar && bar.foo === undefined).

I meant at the type level. A function that accepts an argument of type { foo?: string } will accept both { } and { foo: undefined }. Not so with null, which is why I wasn't including it.

The concrete example here is that the expression hashDomain({ name: undefined }) is well typed, yet it results in a runtime error because undefined is not properly handled. I consider this a bug rather than an enhancement.

@ricmoo ricmoo added v5 Issues regarding legacy-v5 minor-bump Planned for the next minor version bump. v6 Issues regarding v6 labels Feb 18, 2023
@ricmoo
Copy link
Member

ricmoo commented Mar 7, 2023

Added in v6. I'll leave open to remind me for the next minor bump of v5.

@ricmoo ricmoo removed on-deck This Enhancement or Bug is currently being worked on. v6 Issues regarding v6 labels Mar 7, 2023
Woodpile37 pushed a commit to Woodpile37/ethers.js that referenced this issue Jan 14, 2024
Woodpile37 pushed a commit to Woodpile37/ethers.js that referenced this issue Jan 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement. minor-bump Planned for the next minor version bump. v5 Issues regarding legacy-v5
Projects
None yet
Development

No branches or pull requests

2 participants