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

SignTypedData incorrectly handling bigints when BigInt.toJSON is defined #2395

Closed
1 task done
LufyCZ opened this issue Jun 12, 2024 · 5 comments
Closed
1 task done

Comments

@LufyCZ
Copy link

LufyCZ commented Jun 12, 2024

Check existing issues

Viem Version

2.10.11

Current Behavior

To support bigints across our repo, including Next.js server <-> client component communication, we've decided to override JSON.parse and define BigInt.prototype.toJSON.

This has worked quite well so far, but we've found that our permits had stopped working, due to viem using the toJSON function we defined instead of just toString():

image

This is how BigInt.prototype.toJSON is defined:

if (!BigInt.prototype.toJSON) {
  Object.defineProperty(BigInt.prototype, 'toJSON', {
    get() {
      return () => ({
        __type: 'bigint',
        value: this.toString(),
      })
    },
  })
}

Expected Behavior

Viem should serialize bigints using toString instead of (I'm guessing?) first trying toJSON and then if that fails toString.

Steps To Reproduce

No response

Link to Minimal Reproducible Example

https://stackblitz.com/edit/viem-getting-started-hevaba?file=index.js

Anything else?

The linked repro sandbox is broken, had to change the index file over to .js

@jxom
Copy link
Member

jxom commented Jun 12, 2024

I don't think patching the toJSON built-in on global BigInt is a good idea as it confuses downstream functions like JSON.stringify's reviver. Viem doesn't know what the shape of the patched type will be at this point, so we can't really do much.

I would suggest to create a custom stringify function that utilizes the reviver, or use Viem's stringify export.

const replacer = (key, value) =>
  typeof value === "bigint" ? { $bigint: value.toString() } : value;

const data = {
  number: 1,
  big: 18014398509481982n,
};
const stringified = JSON.stringify(data, replacer);

console.log(stringified);
// {"number":1,"big":{"$bigint":"18014398509481982"}}

@jxom jxom closed this as not planned Won't fix, can't repro, duplicate, stale Jun 12, 2024
@LufyCZ
Copy link
Author

LufyCZ commented Jun 13, 2024

I don't think patching the toJSON built-in on global BigInt is a good idea as it confuses downstream functions like JSON.stringify's reviver. Viem doesn't know what the shape of the patched type will be at this point, so we can't really do much.

I would suggest to create a custom stringify function that utilizes the reviver, or use Viem's stringify export.

const replacer = (key, value) =>
  typeof value === "bigint" ? { $bigint: value.toString() } : value;

const data = {
  number: 1,
  big: 18014398509481982n,
};
const stringified = JSON.stringify(data, replacer);

console.log(stringified);
// {"number":1,"big":{"$bigint":"18014398509481982"}}

Affecting things like JSON.stringify is the point. I was following MDN's and The bigint police's recommendations regarding stringifying bigints (though I chose the format from your stringify function).

The alternative is to patch JSON.stringify directly, as we did with JSON.parse, though I found adding toJSON to the bigint prototype to be a cleaner solution.

What I am confused about is why is viem able to stringify bigints correctly (using toString) if toJSON is not defined, but fails to do so if it is? Is toString a fallback of toJSON?

@jxom
Copy link
Member

jxom commented Jun 13, 2024

Patching the .toJSON built-in confuses the reviver in JSON.stringify functions – the passed value is the one mutated via the .toJSON patch. Hence why I think the recommendation on the MDN docs is pretty bad. Reviver functions are built for this purpose, so global built-ins should not intervene.

What I am confused about is why is viem able to stringify bigints correctly (using toString) if toJSON is not defined, but fails to do so if it is? Is toString a fallback of toJSON?

420n.toString() === '420' regardless if .toJSON has been patched or not. The issue lies when it comes to the reviver function as noted above.

Strongly recommend the second approach on the MDN docs:

CleanShot 2024-06-14 at 07 30 19@2x

@LufyCZ
Copy link
Author

LufyCZ commented Jun 14, 2024

Patching the .toJSON built-in confuses the reviver in JSON.stringify functions – the passed value is the one mutated via the .toJSON patch. Hence why I think the recommendation on the MDN docs is pretty bad. Reviver functions are built for this purpose, so global built-ins should not intervene.

What I am confused about is why is viem able to stringify bigints correctly (using toString) if toJSON is not defined, but fails to do so if it is? Is toString a fallback of toJSON?

420n.toString() === '420' regardless if .toJSON has been patched or not. The issue lies when it comes to the reviver function as noted above.

Strongly recommend the second approach on the MDN docs:

CleanShot 2024-06-14 at 07 30 19@2x

We can not just override the reviver function everywhere, as then it'd break Next.js. I cannot tell Next.js to use a specific reviver for example, meaning that passing bigints between server and client components would not be possible without first stringifying them and then (probably using a slow zod schema) parsing them again.

Which would also move typechecks to runtime instead of compile time, which makes no sense.

We have had 0 issues with our approach until we came across this viem one. Doing 420n.toString() still yields 420 even when .toJSON is patched.

However, from what I can see from testing, toJSON runs before you even get to the reviver, so it's just straight up not possible for you to fix this. Should've checked that before creating the issue. Think patching JSON.stringify instead of BigInt.prototype.toJSON will get us the result we want then.

Soo dw about it 😅

Copy link
Contributor

This issue has been locked since it has been closed for more than 14 days.

If you found a concrete bug or regression related to it, please open a new bug report with a reproduction against the latest Viem version. If you have any questions or comments you can create a new discussion thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants