feat(transactions): let a refund set its own description and meta_data - #350
Open
Shivam8584 wants to merge 4 commits into
Open
feat(transactions): let a refund set its own description and meta_data#350Shivam8584 wants to merge 4 commits into
Shivam8584 wants to merge 4 commits into
Conversation
The balance-applying path mutates in place: UpdateBalances writes PreciseAmount onto the transaction, and addDebit/addCredit/computeBalance mutate the balances via big.Int's Add and Sub, which write into their receiver. That makes a plain struct copy unsafe as a snapshot — it would share the underlying integers, so applying a transaction to the copy would rewrite the original too. Clone copies every *big.Int by value, preserving nil so a clone behaves identically under InitializeBalanceFields, and duplicates the MetaData map and distribution slices. Groundwork for dry-run transaction previews (blnkfinance#330), which hold a "before" snapshot alongside the balances the projection is applied to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PreviewTransaction answers "if I post this, would it succeed and what would
the balances become" without touching ledger state.
The projection runs the same arithmetic a real post runs — processBalances,
and through it UpdateBalances and canProcessTransaction — against deep copies
of the balances. Sharing the math rather than reimplementing it is what keeps
a preview from drifting away from enforcement as the posting path changes.
Two things the real path does that a preview must not:
- QueueTransaction persists a QUEUED row and applies balances later in a
worker, so the projection drives the apply seam directly instead.
- Resolving an @Indicator creates the balance when it is missing, so the
projection resolves through read-only lookups and reports an unknown
indicator as a virtual balance projected against zero.
Locks are taken for the read so both balances are seen as of one moment, but
acquired directly rather than through executeWithLock: a preview should not
record hot-pair contention and steer hot-lane routing for real traffic.
Where the ledger is permissive, the projection reports rather than invents a
rule — a currency mismatch and an in-use reference are surfaced as notes,
because the apply path allows both and a preview that were stricter than
enforcement would be misleading.
Refs blnkfinance#330.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A dry run answers what a transaction would do and stops there, so the handlers branch before anything is queued. The response is 200 rather than 201 because nothing was created, and a projected rejection is a 200 too: the request succeeded and the answer is "no". Reserving 4xx for requests that are actually malformed means no new error codes were needed. The rejection carries the same code a real post would have returned, resolved through the classifier the other endpoints already use, so existing client-side handling for TXN_INSUFFICIENT_FUNDS and friends works against a preview without change. dry_run stays out of model.Transaction. It describes what to do with the request rather than anything about the movement itself, and keeping it at the transport boundary means a transaction marked for preview cannot reach the writer by accident. On the refund endpoint the flag rides on the existing optional body, so a bodiless POST keeps meaning "refund normally". Closes blnkfinance#330 for single transactions and refunds; bulk and inflight follow. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A reversal copied the original transaction wholesale, so a refund of a transaction tagged meta_data.type = "deposit" was itself stored as a "deposit". Anything classifying ledger rows by metadata could not tell a refund apart from the movement it reversed, and there was no way to say otherwise. The optional refund body now carries description and meta_data. Metadata is merged rather than replaced. The ledger sets its own keys on the original — inflight, atomic, allow_overdraft — and downstream processing reads them, so a blind replace would drop them; merging lets the caller reclassify the reversal while those survive. It also gives callers a way to correct an inherited inflight marker, which setTransactionMetadata only ever sets and never clears. The merge builds a new map instead of writing into the inherited one. The reversal is created with a struct copy, which shares the original's metadata map, so writing through it would have rewritten the transaction being refunded. Threading is additive, matching how skip_queue was added in 8c2022a: RefundTransaction and RefundWorkerWithOptions keep their signatures and delegate to RefundTransactionWithOptions and RefundWorkerWithRefundOptions. No existing call site changes, and an absent body or absent fields reproduces today's behaviour exactly. Refs blnkfinance#291. Co-Authored-By: Claude Opus 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.
Implements #291 — letting a refund set its own
descriptionandmeta_datainstead of blind-inheriting the original's.On the issue being closed: you reviewed this back in May and said "I'd say we go with option 1: api/transactions.go — extend refundTransactionRequest". It was then closed on 2026-08-06 in a sweep alongside #290, #225 and #345 — but unlike #345 (fixed in
c8c003c) nothing had landed for this one, andrefundTransactionRequeststill carries onlyskip_queueonmain. Reading that as housekeeping rather than a decision, so here's the implementation. Happy to close this if you'd rather not take it.Stacked on #348 — please merge that first; this branch contains its three commits, so the only new commit here is
c36a753. Both PRs touchrefundTransactionRequestandprepareRefundTransaction, so they can't be independent. Rebases tomaincleanly once #348 lands, or I can rebase this to go first if you prefer.What changes
POST /refund-transaction/:id { "description": "refund for ticket #4821", "meta_data": { "type": "refund", "reason": "goodwill" } }A refund of a transaction tagged
meta_data.type = "deposit"was itself stored as a"deposit", so anything classifying ledger rows by metadata couldn't tell a refund apart from the movement it reversed.Metadata is merged, not replaced
Worth correcting something in my own issue: I justified merging by saying a replace would drop
QUEUED_PARENT_TRANSACTION. That's wrong — that key is set inQueueTransaction(transaction_queue.go:102), which runs afterprepareRefundTransaction, so it isn't at risk.Merging is still right, for different keys.
setTransactionMetadatawritesinflight,atomicandallow_overdraftonto the original, and those are inherited by the struct copy and read downstream (transaction_inflight.go:83,transaction_coalescing.go:604,transaction_rejection.go:71). A blind replace would drop them. Merging lets a caller reclassify the reversal while they survive.It also gives callers a handle on a live wart:
setTransactionMetadataonly ever sets the inflight marker and never clears it, so a reversal of an inflight transaction inheritsinflight: trueeven thoughprepareRefundTransactionexplicitly setsInflight = false— whichtransformTransactionthen reports back (that's #293 surfacing here). Passing{"meta_data": {"inflight": false}}corrects it on that row. Not fixing #293 in this PR.One bug worth calling out
The merge builds a new map rather than writing into the inherited one.
newTransaction := *originalTxnshares the original's metadata map, so writing through it would have rewritten the transaction being refunded. There's a test for exactly this (TestPrepareRefundDoesNotMutateOriginalMetaData), and I verified against a live server that the original row still readstype: depositafter a refund overrides it totype: refund.Threading is additive
Following how
skip_queuewas added to this endpoint in8c2022a:RefundTransactionRefundTransactionWithOptionsRefundWorkerWithOptionsRefundWorkerWithRefundOptionsadded alongsideRefundWorkerprepareRefundTransactionRefundOptions(2 call sites)RefundTransactionis exported and called from 13 test sites, and isn't behind an interface, so changing its signature would be source-breaking for SDK consumers. Nothing existing changes, and an absent body or absent fields reproduces today's behaviour exactly.Tests
10 new tests: description override, metadata merge with caller keys winning and ledger keys surviving, the aliasing guard above, overriding an inherited
inflight, empty overrides being ignored rather than blanking inherited values,skip_queuebehaviour preserved, and voided-original status handling unchanged.Verified live: overriding produced
{"type": "refund", "reason": "goodwill", "channel": "card", "allow_overdraft": true}— caller keys won, uninvolved keys inherited, ledger keys survived — while the original row was untouched in Postgres. A bodilessPOSTstill returns 201 and inherits both fields.go test ./...passes apart frominternal/search, which needs Typesense and fails identically onmain.