refactor(minor-voting-verifier): integrate the voting-verifier with the into-event macro#743
refactor(minor-voting-verifier): integrate the voting-verifier with the into-event macro#743fish-sammy wants to merge 7 commits intomainfrom
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #743 +/- ##
========================================
Coverage 93.36% 93.36%
========================================
Files 225 225
Lines 36359 36253 -106
========================================
- Hits 33947 33849 -98
+ Misses 2412 2404 -8 ☔ View full report in Codecov by Sentry. |
…he into-event macro
ee375c1 to
b1a9069
Compare
cgorenflo
left a comment
There was a problem hiding this comment.
Discussion: Should this be a major change? We are breaking compatibility with older ampd versions
| .non_generic() | ||
| .into(); | ||
|
|
||
| goldie::assert_json!(json!({ |
There was a problem hiding this comment.
can you make sure that we're exhaustively testing all events? So the test should break if we add a new event variant without adding it here
There was a problem hiding this comment.
Not sure how you can do that, but it's not necessary since this test is there to make sure the differences in the JSON before and after using the macro are OK. With new events, do we need them to be added here?
There was a problem hiding this comment.
Bug: JSON Serialization Inconsistency
Event attribute string values are now double-encoded as JSON strings (e.g., serviceName becomes \"serviceName\", 10 becomes \"10\"), while other values remain unquoted. This inconsistent and unintended serialization change breaks external consumers expecting plain string values, leading to parsing failures.
contracts/voting-verifier/src/testdata/events_should_not_change.golden#L4-L155
Was this report helpful? Give feedback by reacting with 👍 or 👎
|
taken over by #952 |
Description
Todos
Steps to Test
Expected Behaviour
Other Notes