fix(cheatcodes): Make serializeString always serialize values into a string, add serializeJson cheatcode

[mattsse]

Motivation

Closes #4601
don't rely on from_str when using serializeString

this fails a test test_serializeNotSimpleJson that uses a serialized string a serializes it again.

not sure what to make of this. wdyt @odyslam ?

I think serialize string was rather serialize object, so unsure if this change breaks some assumptions.

Solution

[Evalir]

gm, adding this for convo and alignment (as i'm working on parseJson stuff i can take over on this)

I think the assumption being broken here is that serializeString has two uses:

• If the value passed in is a valid object, then it will be serialized as an object.
• If the value passed in is a string, then it will be serialized as a string.

The note on the book supports this assumption: https://book.getfoundry.sh/cheatcodes/serialize-json?highlight=serializestring#example

IMHO, this is a huge footgun actually. I believe serializeString should explicitly, always serialize objects to string, and have some other cheatcode like serializeJson that always expects serializable objects for composability. wdyt @mds1 ?

[mds1]

IMHO, this is a huge footgun actually. I believe serializeString should explicitly, always serialize objects to string, and have some other cheatcode like serializeJson that always expects serializable objects for composability.

This seems like a good solution to me, would be curious to confirm with @sakulstra also

[Evalir]

@mds1 & @mattsse updated this PR with the new scope:

• serializeString will now always serialize values as a string. It CANNOT be used to serialize json values, as they will be parsed as a string. This might be a breaking change for some I understand.
• A new cheatcode, serializeJson, now performs the other function serializeString had, which is, it takes a valid JSON string and serializes it to a json object. It will revert if it fails to do so.

[mds1]

sgtm!

This might be a breaking change for some I understand.

Yea, given this we probably should merge this into the v1 branch and hold off on releasing it, since it might break user scripts

[mattsse]

agree, lgtm

[sakulstra]

lgtm, agree with the gist of the discussion here.

---

Regarding v1 or main i don't have a strong opinion.
It wouldn't have been the first breaking change on foundry <1, but it's also not so urgent to change the behavior 🤷
Imo both paths are fine.

[Evalir]

cool! i'd say let's keep this for v1, as it's a breaking change

[DaniPopes]

Closing as superseded by #5755 (from what I can tell).
Anyway the diff is not that big and would have to be re-implemented entirely due to #6131.