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

Add correct processing for non-existent json-keys #5511

Merged
merged 3 commits into from
Aug 1, 2023

Conversation

klkvr
Copy link
Member

@klkvr klkvr commented Jul 31, 2023

Motivation

Right now following code will return uint256(32): abi.decode(vm.parseJson('{"a": 1}', '.b'), (uint256)) while expected behavior would be for it to revert because vm.parseJson('{"a": 1}', '.b') should result in bytes("")

But, in fact, it results in 0x00000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000000 == abi.encode(bytes(""))

Solution

Add processing of edge case when there are no json tokens corresponding to given path

@klkvr klkvr changed the title Add correct processing for non-existent keys Add correct processing for non-existent json-keys Jul 31, 2023
Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this lgtm—just unsure if this conforms to the earlier spec we had — @mds1 would love an eye on this!

@mds1
Copy link
Collaborator

mds1 commented Aug 1, 2023

Sanity checking my understanding: previously calling parseJson with a non-existent key returned bytes of abi.encode(bytes("")), and now it returns empty bytes, ""? If so, agreed with this change

Also @klkvr you might know this, but there's a keyExists cheat you can use to check if a key exists

@klkvr
Copy link
Member Author

klkvr commented Aug 1, 2023

Sanity checking my understanding: previously calling parseJson with a non-existent key returned bytes of abi.encode(bytes("")), and now it returns empty bytes, ""? If so, agreed with this change

Yes, that's right

Also @klkvr you might know this, but there's a keyExists cheat you can use to check if a key exists

I know about it, but for me it's easier when code just reverts, because inserting sanity-checks everywhere makes code less readable IMO

@Evalir
Copy link
Member

Evalir commented Aug 1, 2023

This is technically a breaking change @mds1 — should we merge or hold off?

@mds1
Copy link
Collaborator

mds1 commented Aug 1, 2023

I'd say merge (and include in the changelog) because the existing behavior feels like a footgun. We don't want to return data for a field that doesn't exist, since there really is no data.

Potentially related to #3754 (comment) where 32 used to be erroneously returned

@klkvr
Copy link
Member Author

klkvr commented Aug 1, 2023

@Evalir parseJson does not revert if passed key does not exist, it just returns bytes which are not decodable to anything instead of abi.encode(bytes(""))

@Evalir
Copy link
Member

Evalir commented Aug 1, 2023

whoops my bad, mixed another unrelated change here

@Evalir Evalir force-pushed the klkvr/correct-parseJson branch from 626c420 to 0574666 Compare August 1, 2023 01:35
@Evalir Evalir merged commit 9a4bb7f into foundry-rs:master Aug 1, 2023
aathan pushed a commit to aathan/foundry that referenced this pull request Aug 2, 2023
* Add correct processing for non-existent keys

* Fix clippy error

* chore: include changes in changelog

---------

Co-authored-by: Enrique Ortiz <hi@enriqueortiz.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants