Release: develop -> main - #4576
Merged
Merged
Conversation
…4568) * feat(payout): log amount, asset and chain per escalated payout order `logFailedOrders` wrote a single collecting line for the whole batch, carrying only order id, context and correlation id. Two consequences: judging an escalation always required a DB lookup for what was actually at stake, and log-based monitoring could only ever extract one order per line - a batch of seven escalations (as happened on 2026-07-24) surfaced exactly one of them, because a regexp matches a line once. Write one line per escalated order in addition to the collecting line, carrying the payout amount, its asset and the chain the payout was going out on. The collecting line is unchanged: it is also the body of the escalation mail. The wording is a parsing contract - every field is fenced by a literal on both sides so a value containing a space or a comma cannot swallow the next one - and a new spec pins the shape, including the nullable asset relation degrading to a placeholder rather than to an unparsable line. * docs(payout): name the mail path precisely in the escalation log comment * fix(payout): quote the asset name in the escalation line and cover the fence cases Review follow-up on the parsing contract, two gaps: The asset name is the only free-form value in the line. Fenced only by ` of ` and ` on chain `, a name that happened to contain " on chain " would have ended the asset field early and handed the parser a wrong chain - with no parse error at all. A silently wrong value in a critical alert is worse than a loud failure, so the name is now quoted; the only name that can still break parsing is one containing an apostrophe, and that breaks visibly. `?? 'unknown'` also did not cover an empty name: it only catches null/undefined, so `name: ''` would have produced an unparsable line - exactly what the fallback exists to prevent. Now `||`. Two tests added for the cases that were missing: an empty asset name, and a name carrying the fence wording. * test(payout): pin the apostrophe case as the accepted limit of the quoting The service comment claims that an apostrophe in the asset name breaks the line visibly rather than mis-parsing it. That was prose only: nothing failed if a future change to the escaping turned it back into a partial match with a wrong chain. Now it is a test. * test(payout): give the batch case distinct chains and assets The multi-order test used the same asset and chain for both orders and only checked the id per line, so it could not have caught fields being mixed up between orders in a batch - which is the one place where that could happen. Now the two orders differ in amount, asset and chain, and every field is matched against the line of its own order. * fix(payout): read the asset name greedily - quoting alone can be forged Review follow-up. The claim that an apostrophe in the asset name always breaks parsing visibly was wrong. Read lazily, the field ends at the first quote INSIDE the name, so a name like `Foo' on chain Ethereum` closes its own field and then imitates the next fence: the pattern matches happily and yields a WRONG chain, with no error at all - precisely the silent failure the quoting was meant to prevent. The fix belongs on the reading side. The pinned contract now reads the name greedily, up to the LAST `' on chain ` before `, context`. That fence is always the one this service wrote, so no value inside the name can forge it. Two consequences for the tests: the apostrophe case is no longer an accepted casualty but parses correctly, and the adversarial name that imitates the fence is pinned as its own case. * fix(payout): JSON-encode the free-form fields instead of fencing them with a quote Third round on the same defect, so this time the approach goes rather than the symptom. A plain quote around a free-form value is forgeable in BOTH reading directions, and the two previous attempts each closed one and opened the other: - read up to the FIRST quote, a name like `Foo" on chain Ethereum` closes its own field and imitates the next fence -> wrong chain, no error; - read up to the LAST one, a later free-form field offers a competing fence. `correlationId` is a plain string column, so `129680" on chain FAKE, context FAKE, correlation "x` does exactly that -> wrong chain, no error, and this one is reachable through the admin manual-payout path, where the id is only `@IsString()`. Both were measured, not argued. Neither reading is safe, because the ambiguity is in the format, not in the quantifier: the closing quote is not identifiable as long as the value may contain one. The free-form values are now JSON-encoded and read as JSON strings, with `(?:[^"\\]|\\.)*` skipping escape pairs. A quote inside a value arrives escaped, so the closing quote is unambiguous whatever the value contains. Verified for: apostrophe, embedded quote plus a forged fence, a correlation id carrying a full forged tail, backslash, empty name. * style(payout): apply Prettier formatting to the escalation spec The Format check step in CI is stricter than eslint, which is what I had run locally. * test(payout): pin the backslash case and fix three comments left from the quote-fence version Two review follow-ups. The commit that introduced JSON encoding claimed the backslash case as verified, but nothing pinned it. A trailing backslash is exactly what lets a forged quote slip past a reader that does not track escape pairs, because `\"` then looks like an escaped quote when it is really an escaped backslash followed by the real closing one. Both free-form fields now carry one in the tests. Three explanatory comments still illustrated the forgery with an apostrophe. The fence is a double quote since the switch to JSON encoding, so the examples described an escape character that no longer plays any role. * test(payout): pin backslash parity, not just the presence of a backslash The single backslash case was not enough to hold the escape mechanism. An encoder that doubles only the FIRST backslash of a value - a `replace` without the global flag, an entirely ordinary mistake - passes all twelve existing tests while leaving the line forgeable: with two backslashes ahead of an embedded quote it emits an odd number of them, the quote then reads as unescaped, and the chain comes back as `Ethereum" on chain Tron` instead of `Tron`. Verified by mutation rather than by argument: with that encoder patched in, the new test is the only one of the thirteen that fails; the service file was restored afterwards and is unchanged. * test(payout): pin that a newline in a value cannot split the record The escaping has to cover control characters, not just quote and backslash - and this is where getting it wrong stops being a parsing problem. A newline inside a value splits the record into two physical lines, and because the payload can spell out a complete second escalation, the log would carry a fully invented order with a freely chosen chain and amount. That is a forged record, not a mis-read field. `JSON.stringify` already prevents it, but nothing pinned it: an encoder that escapes quote and backslash correctly and globally, yet leaves control characters alone, passed all thirteen tests. Verified by mutation - with that encoder patched in, the new test is the only one of fourteen that fails; the service file was restored afterwards and is unchanged.
github-actions
Bot
requested review from
TaprootFreak and
davidleomay
as code owners
August 1, 2026 12:03
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automatic Release PR
This PR was automatically created after changes were pushed to develop.
Commits: 1 new commit(s)
Checklist