Repository navigation
fix(purchase): always send tax in ConfirmPayment, including 0 - #79
Merged
Merged
Conversation
ConfirmPaymentRequest lists tax as required and documents that it can be 0, but it was a plain proto3 uint32 with implicit presence. protojson omits an implicit-presence zero, so a purchase with no tax reached the game backend with no tax key at all, and a backend that validates the published schema rejected the confirm as missing tax. Make tax `optional uint32` so presence is explicit and the sender always serializes the literal 0, the same change made for decimal_places in #23. Field number, type and JSON name are unchanged, so the wire and JSON stay compatible. buf breaking reports the implicit-to-explicit presence change on this field; that is expected and acknowledged in the PR rather than silenced in buf.yaml. part of DIS-199 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
Why
A game backend that validates the published ConfirmPayment schema declined test purchases that had no tax, answering HTTP 400 "tax missing or invalid": the schema lists
taxas required and says it can be 0, but Stash dropped the key whenever the value was 0. This PR makes the contract able to carry an explicit 0, so the sender can keep the promise the docs already make. Ticket: DIS-199 ("Purchase confirmation drops a zero tax, so the game backend declines the purchase"), https://linear.app/stashgg/issue/DIS-199Intent
taxonConfirmPaymentRequestbecomesoptional uint32, so it has explicit presence and protojson emits it whenever it is set, including the literal 0. This is the same change #23 made fordecimal_placeson Register, Confirm and Cancel.flowchart LR P["ConfirmPaymentRequest<br/>tax = 5<br/>new: optional uint32"]:::this D["swagger.v1.json<br/>tax description<br/>new: always sent"]:::this G["Go stubs<br/>Tax becomes *uint32"]:::this S["pie-mono clients<br/>set Tax explicitly"]:::open J{{"protojson<br/>emits any set field,<br/>including 0"}}:::existing B(["Game backend<br/>confirm endpoint"]):::existing P --> D P --> G G --> S S --> J J -- "tax: 0 present" --> B classDef done fill:#d3f9d8,stroke:#2f9e44,stroke-width:1px,color:#111 classDef this fill:#ffd8a8,stroke:#e8590c,stroke-width:4px,color:#111 classDef open fill:#e5dbff,stroke:#7048e8,stroke-width:1px,color:#111 classDef todo fill:#f1f3f5,stroke:#868e96,stroke-width:1px,stroke-dasharray:5 5,color:#333 classDef existing fill:#dbe4ff,stroke:#4263eb,stroke-width:1px,color:#111Legend: orange = changed here, purple = the open consumer PR that sets the field, blue = existing behavior; hexagon = rule, rounded = outside service.
Big Picture
flowchart LR subgraph S1["Contract"] direction TB A1["public-api 79<br/>tax optional"]:::this end subgraph S2["Wire fix"] direction TB A2["pie-mono 19541<br/>bump + set tax"]:::open end subgraph S3["Verify"] direction TB A3["Deploy pie-mono<br/>test shop 0-tax<br/>confirm shows tax 0"]:::todo end subgraph S4["Precedent"] direction TB A0["public-api 074a2fe<br/>decimal_places optional"]:::done end S4 -- "same pattern" --> S1 S1 -- "regenerated stubs" --> S2 S2 -- "deployed" --> S3 subgraph Legend direction TB L1["merged"]:::done L2["this PR"]:::this L3["other open PR"]:::open L4["not done yet"]:::todo end classDef done fill:#d3f9d8,stroke:#2f9e44,stroke-width:1px,color:#111 classDef this fill:#ffd8a8,stroke:#e8590c,stroke-width:4px,color:#111 classDef open fill:#e5dbff,stroke:#7048e8,stroke-width:1px,color:#111 classDef todo fill:#f1f3f5,stroke:#868e96,stroke-width:1px,stroke-dasharray:5 5,color:#333 classDef existing fill:#dbe4ff,stroke:#4263eb,stroke-width:1px,color:#111proto/public, settaxon both confirm builders, tests)"tax":0This PR goes first; the consumer PR points its submodule at this PR's merge commit on
mainand is what changes the bytes partners receive. Nothing is removed or superseded later. The JSON name, field number and type are unchanged, and the OpenAPIrequiredlist is unchanged; only thetaxdescription gains "Always sent, including the literal 0 when the purchase has no tax."Details
Breaking-change check (expected).
buf breakingreports field 5taxonConfirmPaymentRequestchanging from implicit to explicit presence (FIELD_SAME_CARDINALITY), so theCheckjob fails by design. Per this repo's rule, there is nobuf.yamlexception. Why it is safe for game backends: on the wire the field keeps its number, type and JSON name, so every existing reader decodes the same values; the only observable change is that a 0 tax now arrives as"tax": 0instead of no key, which the schema already required. In generated Go code the field becomes*uint32(GetTax()still returnsuint32); the only Go consumer is updated in the companion PR.Changed files.
server/egress/shop/purchase/v1/service.proto(field and description) and the regenerateddocs/gen/swagger.v1.json. A structural diff of the regenerated swagger shows exactly two leaves changed, bothtax.description.Audit of the other request messages. Every request Stash sends under
server/egress/**was checked for the same pattern (required or documented always-sent, non-optional scalar or enum, legitimately zero or empty).taxis the only one. The other required scalars either forbid zero or empty by validation (total,time_millis, line-itemquantityandcents, bonusquantity, ids and currency) or are alreadyoptional(decimal_places).part of DIS-199
🤖 Generated with Claude Code
Caution
💥 Blast Radius — Critical Risk
Intent: Enable valid zero-tax purchase confirmations to satisfy the published schema and avoid rejection by game backends.
The acknowledged protobuf presence change breaks regenerated Go callers that assign scalar
Taxvalues, requiring coordinated consumer adoption and explicit owner sign-off.Overview
server/egress/shop/purchase/v1/service.protochangesConfirmPaymentRequest.taxtooptional uint32and documents explicit zero emission. One withheld generated file is accounted for without inspection.Shop reach: all consumers of the shared
ConfirmPaymentcontract; no shop gate exists on this field, and this checkout provides no deployed-shop inventory.Data/proto: Field number
5, wire type, JSON name, required-schema membership, and minor-unit meaning remain unchanged. Generated GoTaxchanges fromuint32to*uint32; the PR explicitly acknowledges the expectedFIELD_SAME_CARDINALITYfailure without changing the breaking-check configuration.Payments path: With an updated sender, confirmation builder →
ConfirmPaymentRequest.tax→protojson→/api/v1/purchase/confirm→ game backend carries"tax": 0where zero previously disappeared. Nonzero numeric values retain their meaning; an unset optional field remains omitted.Rollout: No sender implementation exists in this checkout. The companion consumer update must populate
Taxexplicitly; this schema change alone does not establish that deployed confirmations emit zero.Validation: Source and consumer searches confirm the scoped contract change.
bufandprotocare unavailable locally, so their checks were not run.Scope is the shared confirmation contract and one generated artifact; registration, cancellation, confirmation error statuses, and
decimal_placesremain unchanged.Not reviewed: 1 generated file(s) — marked
linguist-generatedin.gitattributes:docs/gen/swagger.v1.json.