chore(billing-platform): Convert BalanceService dataclasses in protobufs#339
chore(billing-platform): Convert BalanceService dataclasses in protobufs#339brendanhsentry wants to merge 2 commits into
Conversation
|
The latest Buf updates on your PR. Results from workflow ci / buf-checks (pull_request).
|
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| } | ||
|
|
||
| message UpsertContractBalanceResponse { | ||
| uint64 new_balance_cents = 1; |
There was a problem hiding this comment.
Bug: The response field new_balance_cents is a uint64, but the request's amount_cents is a signed int64. This can cause silent data corruption if a negative balance is returned.
Severity: HIGH
Suggested Fix
To prevent silent data corruption from negative balances, change the response field new_balance_cents from uint64 to int64. This ensures that both the request and response can correctly represent positive and negative monetary values.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location:
proto/sentry_protos/billing/v1/services/engagement/v1/endpoint_upsert_contract_balance.proto#L13
Potential issue: There is a type mismatch between the request field `amount_cents` which
is an `int64` and the response field `new_balance_cents` which is a `uint64`. If the
server-side logic calculates a negative balance, Protobuf's encoding will convert this
negative `int64` into a very large positive `uint64` via two's complement
representation. This results in silent data corruption, where the client receives an
incorrect and astronomically large balance without any error being raised. This can
happen if `amount_cents` is used for deductions that result in a negative total balance.
Also affects:
rust/src/sentry_protos.billing.v1.services.engagement.v1.rs:567~567
Did we get this right? 👍 / 👎 to inform future reviews.
volokluev
left a comment
volokluev
left a comment
There was a problem hiding this comment.
These names are not self explanatory, please add some documentation as to what these endpoints actually do
|
|
||
| package sentry_protos.billing.v1.services.engagement.v1; | ||
|
|
||
| message AssignBalanceContractRequest { |
There was a problem hiding this comment.
yeah you're right this is pretty bad. i have a PR to refactor in getsentry.
after that's merged, i'll come back and fix these protobuf messages
2 participants
Turns dataclasses into protobufs