Conversation
| // stringifyJSONMsgFields converts object- and array-valued "msg" fields to | ||
| // strings. Message fields conventionally contain opaque JSON (for example, | ||
| // CosmWasm contract messages), whose runtime shape cannot be represented by a | ||
| // stable EIP-712 type. Existing string values are left alone so ordinary | ||
| // string fields and already-stringified JSON are not changed. | ||
| // | ||
| // The conversion only affects the derived EIP-712 payload. The protobuf | ||
| // transaction still contains the original value used during execution. |
There was a problem hiding this comment.
AI description--please make it more concise.
There was a problem hiding this comment.
Not sure whether to be offended or not 😅. This is all hand written
There was a problem hiding this comment.
Sorry for offending, I should have explained more the particular thing I didn't like about the comment.
It's a bit verbose for an internal method comment that is doing regular json stringify. Also it specifically calls out CosmWasm contract messages as the thing it's solving for which is something that LLMs typically do when making small changes on behalf of a specific prompt.
On second read I think it's mostly fine, but I would still like to drop the CosmWasm callout.
|
@greptile review |
Greptile SummaryThis PR makes object- and array-valued
Confidence Score: 5/5The PR appears safe to merge, with no concrete blocking or independently actionable non-blocking issue identified. The transformation is confined to the derived EIP-712 payload, preserves opaque JSON content as a string, and is applied consistently when typed data is reconstructed for verification. Important Files Changed
|
|
Seems good to merge when you update the comment. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1248 +/- ##
==========================================
+ Coverage 67.44% 67.45% +0.01%
==========================================
Files 320 320
Lines 23456 23488 +32
==========================================
+ Hits 15819 15843 +24
- Misses 6456 6460 +4
- Partials 1181 1185 +4
🚀 New features to boost your workflow:
|
| // stringifyJSONMsgFields converts object- and array-valued "msg" fields to | ||
| // strings. Message fields conventionally contain opaque JSON (for example, | ||
| // CosmWasm contract messages), whose runtime shape cannot be represented by a | ||
| // stable EIP-712 type. Existing string values are left alone so ordinary | ||
| // string fields and already-stringified JSON are not changed. | ||
| // | ||
| // The conversion only affects the derived EIP-712 payload. The protobuf | ||
| // transaction still contains the original value used during execution. |
There was a problem hiding this comment.
Sorry for offending, I should have explained more the particular thing I didn't like about the comment.
It's a bit verbose for an internal method comment that is doing regular json stringify. Also it specifically calls out CosmWasm contract messages as the thing it's solving for which is something that LLMs typically do when making small changes on behalf of a specific prompt.
On second read I think it's mostly fine, but I would still like to drop the CosmWasm callout.
| // stringifyJSONMsgFields converts object- and array-valued "msg" fields to | ||
| // strings. Message fields conventionally contain opaque JSON (for example, | ||
| // CosmWasm contract messages), whose runtime shape cannot be represented by a | ||
| // stable EIP-712 type. Existing string values are left alone so ordinary | ||
| // string fields and already-stringified JSON are not changed. |
There was a problem hiding this comment.
| // stringifyJSONMsgFields converts object- and array-valued "msg" fields to | |
| // strings. Message fields conventionally contain opaque JSON (for example, | |
| // CosmWasm contract messages), whose runtime shape cannot be represented by a | |
| // stable EIP-712 type. Existing string values are left alone so ordinary | |
| // string fields and already-stringified JSON are not changed. | |
| // stringifyJSONMsgFields converts object- and array-valued "msg" fields to | |
| // strings. Message fields conventionally contain opaque JSON | |
| // whose runtime shape cannot be represented by a stable EIP-712 type. | |
| // Existing string values are left alone so ordinary | |
| // string fields and already-stringified JSON are not changed. |
CosmWasm msg payloads are encoded as json objects in amino representation, which breaks EIP-712's strict typic requirements. This PR stringifies untyped JSON objects to make it compatible with EIP-712 signing