feat(payout): log amount, asset and chain per escalated payout order - #4568
Merged
Conversation
`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.
…e 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.
…oting 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.
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.
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.
… 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.
The Format check step in CI is stricter than eslint, which is what I had run locally.
… 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.
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.
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.
Collaborator
Author
|
Ten review passes to zero findings (two parallel reviews per pass: conformance and logic). What the passes actually changed, worst first:
Two reported findings were rejected with evidence instead of being applied. The more interesting one: Verification: 14 tests in |
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
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.
When
processFailedOrdersparks orders inPayoutUncertain,logFailedOrderswrote a single collecting line for the whole batch:Two problems with that as the only record:
Change
One line per escalated order, in addition to the unchanged collecting line:
The collecting line is deliberately left alone —
processFailedOrderspasses its return value tocreateMailRequest, which puts it into the escalation mail.Field choice:
amountandchainare non-nullable columns on the entity and always populated;assetis a nullable eager relation and degrades tounknown.Parsing contract
The wording is an interface, not prose.
amountis numeric andchain/contextare enums, so a literal fence holds for them by construction.The free-form values — the asset name and the correlation id — are JSON-encoded, and that is a deliberate choice over fencing them with a plain quote. A plain quote is forgeable in both reading directions, and each direction closes one hole while opening the other:
Foo" on chain Ethereumcloses its own field and imitates the next fence;A newline is worse still: it splits the record in two, and the second line can spell out a complete, invented escalation with a freely chosen chain and amount. Both quote cases produce a wrong chain with no parse error at all — a silently wrong value in a critical alert, which is worse than a loud failure. Both were measured rather than argued. The ambiguity is in the format, not in the quantifier: a closing quote is not identifiable while the value may contain one. JSON encoding removes the class instead of moving it — a quote inside a value arrives escaped, and the reader skips escape pairs with
(?:[^"\\]|\\.)*.The fallback uses
||, not??: an empty name would otherwise encode to""and read back as an empty asset rather than as a name. (CONTRIBUTING.mdprefers??; this is the documented exception, and the|| 'fallback'form has precedent in the codebase, e.g.blockchain-transaction.service.ts.)Verification
payout-log.service.spec.ts— 14 tests, including the two adversarial cases above (a name carrying a quote plus a forged fence, and a correlation id carrying a full forged tail), a backslash in both free-form fields and an even run of backslashes ahead of an embedded quote, and a newline (the escape mechanism, its parity, and control characters — each verified by mutation: the matching faulty encoder fails that one test and no other), plus a missing asset relation, an empty name, a name with a space, a name with an apostrophe, and a batch with differing chains/assetstsc --noEmitand eslint cleanDeploy note
A monitoring rule in the infrastructure config is being switched onto this line. It must go live after this release, not before — otherwise it watches for a line nobody writes yet.