Broken JSON Parsing of Integers

[apbendi]

Component

Forge

Have you ensured that all of these are up to date?

• Foundry
• Foundryup

What version of Foundry are you on?

forge 0.2.0 (b28119b 2022-11-23T00:17:21.324951Z)

What command(s) is the bug in?

No response

Operating System

macOS (Apple Silicon)

Describe the bug

Parsing quoted integers from a JSON file using vm.parseJson results in incorrect values at runtime. Attempting to parse non-quoted integers causes a revert.

To see this, checkout the broken-json-inputs branch of the project. Copy the .env.template file to .env and add a private key to DEPLOYER_PRIVATE_KEY=. Run forge test to see the failing tests.

Relevant code:

• JSON file
• Parsing
• Failing Test Assertions (due to numbers being incorrect at runtime)

Unquote the strings in the json file and run forge test. You'll receive the following error:

Failing tests:
Encountered 1 failing test in test/GitcoinGovernor.t.sol:GitcoinGovernorTest
[FAIL. Reason: Setup failed: Failed to parse key $] setUp() (gas: 0)

[mattsse]

cc @odyslam

[odyslam]

Quoted integers are, by design, interpreted as strings and encoded as such. The solution is for the numbers to be type number in the JSON. Everything works as expected.

{ "a": 123 }

[mds1]

The solution is for the numbers to be type number in the JSON. Everything works as expected.

It works as expected up to values of u64::MAX, after that parsing fails 😅

I also don't love that you have to store numbers as type "number", since this causes issues with other tooling. For example ethers.js will throw if you try to parse a number type larger than JS's Number.MAX_SAFE_INTEGER (~9e15) into a BigNumber, because numbers that large are unsafe in JS. This means I can't take a JSON file and read it in both solidity and JS.

Perhaps we also need new parseJson methods that lets you specify the expected type, that way you can store the data in any format? e.g. vm.parseJsonUint would mean I can do { "a": 123 } or { "a": "0x7b" } or { "a": "123" } and all get read in properly with that cheat

[odyslam]

IIRC, I settled for type number because it's a rabbit hole to parse "things" into numbers. It had a false positive with H160 (addresses), and it seemed cleaner to have it that way. I think it's a pretty decent assumption.

The u64::MAX is probs a limitation that I should document.

For the bytes, we had that discussion due to the 'gas' field a while back, if you remember, and we settled that it's best to leave byte numbers as bytes. The way to interpret a hex number would be to load it as type 'bytes' and then abi.decode it to a number.

[mds1]

IIRC, I settled for type number because it's a rabbit hole to parse "things" into numbers. It had a false positive with H160 (addresses), and it seemed cleaner to have it that way.

This make sense if you only have a single vm.parseJson cheat that needs to infer types from JSON, but I don't think we need to impose that restriction. Currently, you cannot have a JSON file that stores large numbers as human-readable strings which can be used by both solidity and JS, which feels like a significant limitation / poor UX, because:

1. Hex numbers are harder to read than decimal
2. There's no stdJson method to help go from hex string to uint through bytes, if you decided to store it as a hex string instead of decimal string (same for int, uint arrays, etc)

It seems:

1. Adding vm.parseJson*(Array) cheats for all types, e.g. vm.parseJsonUint and vm.parseJsonUintArray, and
2. Adding readBytesAsUint and readBytesArrayAsUints, etc. for ints, should be added to StdJson.sol in forge-std

would not be too difficult to add and resolve the UX issues. @odyslam @apbendi @mattsse lmk what you all think

---

Though, all that aside, still not sure the original question from @apbendi is resolved? 😅 In particular:

1. Why does it return 32 when you run the tests initially? I get that the length of the encoded string for those numbers is 32 bytes, but I don't yet understand specifically why it returns the length of the string as the parsed value.
2. Why is there a Failed to parse key error? It's not clear exactly which key failed to parse or why.

So potentially still a bug somewhere?

[odyslam]

@mds1, that sounds good. Will prioritize some time today to address these.

Sorry for the delay folks

[0xPhaze]

Just wanted to chime in that I also encountered this and spent a good part of the day debugging. I found the error message pretty confusing. Would love to see this fixed!

Edit: do we have any good workaround for now? Make all values string and call vm.parseUint256?

[odyslam]

So, I spent quite some time exploring the best solution. The TL;DR is the following:

• We can do the coercion, but only if the cheatcode parses a "field" from the Json. It must infer the field types if it attempts to parse an object. We could coerce every "bytes" field, but it doesn't seem very useful.
• The u64::MAX limitation is solvable and will be standard U256::Max from now on.
• The best solution IMHO is to do the coercion as follows:

// parse '0xbeef'
uint256 bigNumber = vm.parseUint(vm.toString(vm.parseJson(file, key)));
// parse '12344'
uint256 bigNumber = vm.parseUint(vm.parseJson(file, key));

The rust code looks considerable dirtier with the coercion and would rather have the above one-liner in the stdJson

Would that be an acceptable solution @apbendi @mds1

[mds1]

Chatted with @odyslam a bit, here's what I think is the best path forward:

The u64::MAX limitation is solvable and will be standard U256::Max from now on.

Let's still fix this. Then:

Currently vm.parseJson tries to infer types from the formatting in the JSON, which has limitations and footguns to be aware of as discussed above. Right now there are two ways a user may try to parse a uint JSON field, where the uint is stored as a decimal string or hex string:

1. uint256 x = vm.parseUint(vm.toString(vm.parseJson(file, key)))
2. uint256 x = abi.decode(vm.parseJson(file, key), (uint256))

The first case works for numbers of any size, and can be a helper in forge-std. The second case is riskier, as this can return the wrong value due to incorrect inference depending on the field format, and therefore we don't want users to use this.

We can't force users to use the StdJson library and should not have to rely on that for JSON parsing safety. Adding specifically-typed cheats such as vm.parseJsonUint would be safer and lead to the following guidance:

• Use vm.parseJson* cheats to read individual fields. Examples:
  • uint256 balance = vm.parseJsonUint(json, ".balance")
  • uint256 balances = vm.parseJsonUintArray(json, ".balance[]")
  • address user = vm.parseJsonAddress(json, ".user")
• Use vm.parseJson to parse multiple fields at once. This still returns ABI-encoded bytes that must be decoded by defining a struct of the right shape, and following the JSON parsing rules and limitations documented in the book (e.g. alphabetical order)

[odyslam]

I am on board with the above @mds1, and it should also be pretty clear in the rust code. I will probably use the parse* backend to have consistent conversion amongst the different cheatcodes.

The next steps are:

• PR with the above changes
• PR to book to update reference documentation
• PR to book for the definitive guide on having to read/write JSON files + public announcement of the improvements (e.g what to expect, new API, etc.)