refactor: improve transaction processing flow with observability, structured logging, and test coverage#2042
Conversation
…itAccountAliases 🔨
…eConcatAliases/MutateSplitAliases 🔨
…ransactionInput 🐛
…py for persistence and cache consistency 🐛
…eBehindTransaction 🐛
…teBehindTransaction 🔨
… postgres adapter 🐛
…larify flat value constraint 📚
…sing empty string 🐛
…anitized error to client 🐛
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
components/ledger/internal/adapters/http/in/transaction_state_handlers.go (2)
486-527:⚠️ Potential issue | 🟡 MinorRemove dead code: the first
fromToinitialization withMutateConcatAliases(lines 413–414) is never used.The code builds a concatenated-alias
fromToat lines 413–414, appends to it conditionally at line 414, then completely overwrites it with split aliases at lines 506–507. The concat version is never passed to any function. Delete lines 413–414 and the conditional append at line 414 (or lines 412–418 for the full var/append block) since only the split aliases version at lines 506–507 is consumed byBuildOperations.If this concat building was intended for error recovery or logging, clarify the purpose with a comment; otherwise, remove it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/internal/adapters/http/in/transaction_state_handlers.go` around lines 486 - 527, Remove the dead concat-alias construction: delete the unused variable/append that calls mtransaction.MutateConcatAliases and any conditional appends that populate the same fromTo prior to it being overwritten; keep only the split-alias assignments that call mtransaction.MutateSplitAliases (the ones used before calling handler.BuildOperations). If the concat variant was intended for a different purpose (e.g., logging/error recovery), either add a clear comment near the MutateConcatAliases usage explaining that purpose and use a distinct variable name, or remove the MutateConcatAliases block entirely so fromTo only comes from mtransaction.MutateSplitAliases before BuildOperations is invoked.
411-418:⚠️ Potential issue | 🟡 MinorRemove lines 413-418: first fromTo block is dead code and serves no purpose.
Lines 413-418 build
fromTousingMutateConcatAliases, which transforms aliases to the concatenated format"index#alias#balanceKey". However, this block is never used before being completely replaced at line 506, wherefromTois rebuilt withMutateSplitAliases(restoring clean aliases). Since database-sourced balances have clean aliases (not concatenated), only the split format at lines 506-511 is correct for matching againstblc.AliasinBuildOperationsat line 519. Delete lines 413-418 to eliminate the unused code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/internal/adapters/http/in/transaction_state_handlers.go` around lines 411 - 418, Remove the dead initialization that builds fromTo with concatenated aliases: delete the block that sets fromTo := append(fromTo, mtransaction.MutateConcatAliases(transactionInput.Send.Source.From)...) and the variable to := mtransaction.MutateConcatAliases(transactionInput.Send.Distribute.To) plus the conditional append guarded by transactionStatus != constant.CANCELED; the code is unused and later fromTo is correctly rebuilt with mtransaction.MutateSplitAliases for matching against blc.Alias in BuildOperations, so remove only the concatenation-based fromTo construction to avoid confusion and incorrect alias formats.components/ledger/api/swagger.json (1)
9128-9185:⚠️ Potential issue | 🟠 MajorConfirm:
pendingfield is missing fromCreateTransactionInflowInputwhile present in other variants.The inflow request model (
CreateTransactionInflowInput) does not include apendingfield, whereas bothCreateTransactionInputandCreateTransactionOutflowInputexpose it. The Go struct definitions confirm this—BuildInflowEntry()never assigns aPendingvalue to the returnedTransaction, leaving it as the zero value (false), whileBuildOutflowEntry()andBuildTransaction()both explicitly setPending: c.Pending/Pending: cti.Pending.If the inflow handler is intended to support pending transactions in the future, this API contract inconsistency will silently drop the field from generated SDKs, creating a mismatch between what clients can send and what the backend may accept.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/api/swagger.json` around lines 9128 - 9185, CreateTransactionInflowInput is missing the pending field which causes SDKs to drop it and BuildInflowEntry() never sets Transaction.Pending; add a boolean "pending" property to the CreateTransactionInflowInput schema (matching CreateTransactionInput/CreateTransactionOutflowInput) and update BuildInflowEntry() to assign the incoming pending value (e.g., Pending: c.Pending) to the returned Transaction so the API contract and Go construction stay consistent with BuildOutflowEntry() and BuildTransaction().
♻️ Duplicate comments (3)
components/ledger/api/openapi.yaml (3)
8027-8033:⚠️ Potential issue | 🟠 MajorThe metadata schema still contradicts the flat-metadata contract.
These fields use
additionalProperties: { type: object }, which only permits nested objects, while the descriptions/examples say metadata values are flat primitives. That will produce the wrong SDK types and reject valid client payloads.Based on learnings, metadata must be flat key-value pairs only, and this generated OpenAPI should be fixed via source annotations or a deterministic post-generation patch.
Also applies to: 8110-8115, 8199-8205, 8367-8370
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/api/openapi.yaml` around lines 8027 - 8033, The metadata schema currently uses additionalProperties: { type: object } which permits nested objects; change each metadata schema (the "metadata" object blocks in this OpenAPI spec) to restrict values to flat primitives by replacing additionalProperties: { type: object } with additionalProperties: { oneOf: [{ type: "string" }, { type: "number" }, { type: "boolean" }] } (or another explicit union of primitive types), keep the description/example as-is, and apply the same deterministic fix to all other occurrences mentioned (the other metadata blocks at the same pattern).
6926-6947:⚠️ Potential issue | 🟠 MajorEncode the
credit/debitmatrix in the schema, not just the description.
AccountingEntrystill leaves both fields optional at the schema level, so generated SDKs and validators cannot tell which payloads the server will reject. Please emit theoperationType/scenario rules asoneOf/conditional branches from the Swaggo source (or a deterministic post-gen transform) instead of relying on prose alone.Based on learnings, in this repo’s Go/Swaggo-generated OpenAPI specs, don’t hand-edit the generated Swagger/OpenAPI YAML; fix it in the generator source or a deterministic post-generation patch step.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/api/openapi.yaml` around lines 6926 - 6947, AccountingEntry currently leaves credit and debit optional in the generated schema; update the generator (Swaggo source) or add a deterministic post-generation transform to emit an OpenAPI conditional schema for AccountingEntry that encodes the operationType/scenario matrix (use oneOf or if/then/else predicates on operationType and scenario) so that the credit and debit properties become required/forbidden per rule (e.g., source+direct/commit => debit required, credit optional; source+hold/cancel => both required; destination+direct/commit => credit required, debit optional; destination+hold/cancel/revert and source+revert => forbid; bidirectional => both required). Ensure the transform targets the AccountingEntry schema and the credit and debit property definitions rather than hand-editing the generated YAML so SDKs/validators pick up the constraints.
8034-8041:⚠️ Potential issue | 🟡 MinorMark deprecated
routerequest fields as deprecated in the schema.The prose says
routeis deprecated, but withoutdeprecated: truedocs and generated clients will continue to present it as a normal field. Add the OpenAPI deprecation flag for each request-siderouteproperty in the generator source/post-gen transform.Based on learnings, in this repo’s Go/Swaggo-generated OpenAPI specs, don’t hand-edit the generated Swagger/OpenAPI YAML; fix it in the generator source or a deterministic post-generation patch step.
Also applies to: 8124-8131, 8215-8221, 8376-8382
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/api/openapi.yaml` around lines 8034 - 8041, The schema property named "route" is described as deprecated but lacks the OpenAPI flag; update the OpenAPI generation pipeline (the generator template or the deterministic post-gen transform that emits/patches request schemas) to add deprecated: true for every request-side "route" property so generated YAML includes deprecated: true for that property (apply the same change across the other occurrences referenced in the comment). Locate the code that constructs request property metadata for "route" and emit deprecated: true, or add a post-generation patch step that finds property name "route" in request schemas and inserts deprecated: true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Around line 602-626: Add a prominent note above the "Log Level Guidelines"
table stating that "Never log sensitive data (balances, financial values, PII)
applies to all log levels," and remove the redundant "Never log sensitive data
(balances, financial values, PII)" phrase from the Debug row; update the Debug
row example text accordingly so the restriction is not duplicated. Ensure you
modify the "Log Level Guidelines" header and the table rows (especially the
Debug row) to reflect the global sensitive-data rule.
In `@components/ledger/api/docs.go`:
- Around line 10012-10015: The Rate.externalId schema is declared as a plain
string but should enforce UUID format like AssetRate.externalId; update the Rate
schema's "externalId" property (symbol: Rate.externalId) to include "format":
"uuid" (keeping or adjusting the example as needed) so clients validate it as a
UUID consistently with the rest of the spec.
- Around line 9195-9200: The Swaggo directives are embedded in field comment
text (causing them to appear in generated descriptions); for the structs
CreateTransactionInflowInput, CreateTransactionInput,
CreateTransactionOutflowInput and their fields Pending and Send in
pkg/mtransaction/input.go, move any `swagger:type ...` (or `swagger: type ...`)
annotations out of the descriptive comment and place them as separate
single-line comment directives immediately above the field/struct declaration
(so they are parsed as metadata, not description), remove the directive text
from the inline description, and then regenerate the docs to verify the
directives no longer leak into docs.go.
In `@components/ledger/api/swagger.json`:
- Around line 9359-9363: The schema for the "to" and "from" array properties
currently only marks them as present but allows empty arrays; update the JSON
Schema for both "to" and "from" (the properties that use "items":
{"$ref":"#/definitions/FromTo"}) to require at least one element by adding
"minItems": 1 (and optionally "uniqueItems": true if duplicate legs should be
prevented), ensuring the contract rejects [] as invalid.
- Around line 9153-9156: Replace the permissive additionalProperties: {} on the
JSON Schema "metadata" property with a schema that only allows flat scalar
values (string, number, boolean), e.g. additionalProperties: { "type":
["string","number","boolean"] } (or equivalent oneOf) so nested objects/arrays
are rejected; apply this change to every "metadata" property instance in the
swagger JSON (the same metadata blocks flagged in the review).
- Around line 9988-9991: The schema for Rate.externalId currently allows any
string but should enforce UUID format; update the Rate model's externalId
property (Rate.externalId) to include UUID typing by adding the UUID format
(e.g., keep "type": "string" and add "format": "uuid" or an equivalent UUID
pattern) so validation requires a UUID-shaped value and the example remains
valid.
In `@components/ledger/internal/adapters/http/in/transaction_create.go`:
- Around line 898-902: The deletion error from TransactionRedisRepo.Del in
deleteIdempotencyKey is being ignored; change the function to capture the
returned error and, if non-nil, log it at Debug level for observability.
Specifically, call err := handler.Command.TransactionRedisRepo.Del(ctx,
*internalKey) and then use the handler's logger (e.g., handler.Logger.Debug or
handler.Logger.Debugf) to emit a brief message including the internalKey and err
so cleanup failures are visible without raising noise.
- Around line 845-851: When BuildOperations fails, the idempotency key and the
Redis backup queue entry created earlier must be removed like the other error
paths; update the error branch that handles the BuildOperations error in the
handler (the block that currently calls libOpentelemetry.HandleSpanError and
logger.Log and returns http.WithError) to also call deleteIdempotencyKey(ctx,
idempotencyKey) and RemoveTransactionFromRedisQueue(ctx, redisQueueKeyOrID)
before returning; ensure you use the same identifiers used earlier for the
idempotency key and the queued backup entry so cleanup mirrors the other error
paths (see functions deleteIdempotencyKey and RemoveTransactionFromRedisQueue
and the BuildOperations error handling).
- Around line 804-805: The span is being marked as a business error via
libOpentelemetry.HandleSpanBusinessErrorEvent(span, ...) but the log uses
logger.Log(..., libLog.LevelError), causing inconsistency; pick one of the two
consistent fixes: either (A) treat failures from ProcessBalanceOperations as
business/validation issues—change the log level to libLog.LevelWarn in the
logger.Log call to match HandleSpanBusinessErrorEvent, or (B) treat them as
infrastructure errors—replace
libOpentelemetry.HandleSpanBusinessErrorEvent(span, ...) with
libOpentelemetry.HandleSpanError(span, ...) so the span classification matches
the existing libLog.LevelError; update only the calls around
ProcessBalanceOperations, keeping the descriptive message and including err.
In `@components/ledger/internal/adapters/http/in/transaction_helper.go`:
- Around line 125-135: The code currently logs unmatched aliases at
libLog.LevelDebug in the From/To iteration (see mtransaction.SplitAliasWithKey
and balanceByAliasKey) and then continues, which can hide upstream
alias-resolution bugs; change the logger.Log call to libLog.LevelWarn (or
LevelError) and stop processing by returning a clear error (e.g.,
ErrAliasNotFound or a contextual error describing the missing alias) from the
surrounding function so the transaction is rejected instead of proceeding with
incomplete operations; apply the same change to both the From block and the
corresponding To block.
In `@components/ledger/internal/adapters/http/in/transaction.go`:
- Around line 316-329: Start the get_operations child span (tracer.Start
creating ctxGetTransaction and spanGetTransaction) only after
http.ValidateParameters succeeds; move the tracer.Start(...
"handler.get_transaction.get_operations") call (and its defer
spanGetTransaction.End()) to immediately before the call to
handler.Query.GetOperationsByTransaction so the span only wraps the I/O call.
Keep using the same ctx when starting the child span, ensure
headerParams.Metadata = &bson.M{} is set before calling
GetOperationsByTransaction, and pass ctxGetTransaction into
handler.Query.GetOperationsByTransaction as currently done.
- Around line 295-302: The current handler collapses all
GetWriteBehindTransaction errors into a debug "cache miss"; change the logic in
the call to handler.Query.GetWriteBehindTransaction so you explicitly
distinguish a cache "not found" result from an operational/backend error: if the
call returns a not-found sentinel (e.g., ErrNotFound or via an IsNotFound
helper) treat it as a cache miss (set X-Cache-Hit="false" or leave current
behavior) and fall back to DB, but if it returns any other error treat it as an
infrastructure failure — log it at warn or error level via logger.Log (use
libLog.LevelWarn or LevelError and include libLog.Err(wbErr) and the
transaction_id) and return an appropriate error response instead of silently
falling back; keep successful path (c.Set and return http.OK with wbTran)
unchanged.
- Around line 7-21: The import block is misordered: move external packages
(libCommons, libLog, libOpentelemetry, github.com/gofiber/fiber/v2,
go.mongodb.org/mongo-driver/bson) into a single external group and place all
internal midaz imports (github.com/LerianStudio/midaz/v3/... like pkg, constant,
goldTransaction, mtransaction, net/http, and services/command and
services/query) in a separate internal group, keeping a blank line between
groups; update the imports in the transaction.go import block accordingly so the
groups follow stdlib → external → internal ordering.
In `@pkg/mtransaction/input.go`:
- Around line 98-141: CreateTransactionInflowInput lacks the Pending flag and
BuildInflowEntry does not propagate it, causing /transactions/inflow to never
create pending transactions; add a Pending field (e.g., Pending *bool
`json:"pending,omitempty" validate:"omitempty"`) to CreateTransactionInflowInput
and to the corresponding JSON/schema metadata, then update BuildInflowEntry to
copy input.Pending into the domain transaction's Pending/status field (the same
field other endpoints use) so the handler that derives initial status will honor
pending=true; ensure validation tags match other transaction inputs.
- Around line 47-50: Change the RouteID field (and any other DTO fields
currently declared as *string but documented/validated as UUIDs) to use
*uuid.UUID instead of *string; update the struct definitions (e.g., the RouteID
field) to use uuid.UUID pointer, add the appropriate uuid package import
(github.com/google/uuid or your project's chosen uuid package), and update any
related validation/parsing code and tests that construct or read these DTOs to
accept/produce *uuid.UUID (adjust struct tags/validation if needed to work with
the uuid type).
- Around line 75-85: The Transaction builder is assigning Metadata by reference
(Metadata: cti.Metadata), which allows downstream mutation of the caller's map;
instead create a new map, copy each key/value from cti.Metadata into it (shallow
copy) and assign that new map to Transaction.Metadata to avoid shared-memory
side effects and ensure metadata stays flat; apply the same map-clone change to
the other builders that currently set Transaction.Metadata in the same file (the
two other occurrences that mirror this pattern) so all Metadata assignments use
a copied map rather than the original.
In `@pkg/mtransaction/transaction.go`:
- Around line 195-227: MutateConcatAliases and MutateSplitAliases currently
modify entries in-place; instead make and operate on a local copy of each entry
before changing AccountAlias so the input slice isn't mutated—inside each loop,
do something like copy := entries[i] (use the local variable name 'copy' or
'eCopy'), check isConcatedAlias(copy.AccountAlias) and call copy.ConcatAlias(i)
or copy.SplitAlias() to set copy.AccountAlias, then append the copy to result;
keep all other logic and helpers (isConcatedAlias, ConcatAlias, SplitAlias)
unchanged so the functions truly "return copies of all entries".
---
Outside diff comments:
In `@components/ledger/api/swagger.json`:
- Around line 9128-9185: CreateTransactionInflowInput is missing the pending
field which causes SDKs to drop it and BuildInflowEntry() never sets
Transaction.Pending; add a boolean "pending" property to the
CreateTransactionInflowInput schema (matching
CreateTransactionInput/CreateTransactionOutflowInput) and update
BuildInflowEntry() to assign the incoming pending value (e.g., Pending:
c.Pending) to the returned Transaction so the API contract and Go construction
stay consistent with BuildOutflowEntry() and BuildTransaction().
In `@components/ledger/internal/adapters/http/in/transaction_state_handlers.go`:
- Around line 486-527: Remove the dead concat-alias construction: delete the
unused variable/append that calls mtransaction.MutateConcatAliases and any
conditional appends that populate the same fromTo prior to it being overwritten;
keep only the split-alias assignments that call mtransaction.MutateSplitAliases
(the ones used before calling handler.BuildOperations). If the concat variant
was intended for a different purpose (e.g., logging/error recovery), either add
a clear comment near the MutateConcatAliases usage explaining that purpose and
use a distinct variable name, or remove the MutateConcatAliases block entirely
so fromTo only comes from mtransaction.MutateSplitAliases before BuildOperations
is invoked.
- Around line 411-418: Remove the dead initialization that builds fromTo with
concatenated aliases: delete the block that sets fromTo := append(fromTo,
mtransaction.MutateConcatAliases(transactionInput.Send.Source.From)...) and the
variable to :=
mtransaction.MutateConcatAliases(transactionInput.Send.Distribute.To) plus the
conditional append guarded by transactionStatus != constant.CANCELED; the code
is unused and later fromTo is correctly rebuilt with
mtransaction.MutateSplitAliases for matching against blc.Alias in
BuildOperations, so remove only the concatenation-based fromTo construction to
avoid confusion and incorrect alias formats.
---
Duplicate comments:
In `@components/ledger/api/openapi.yaml`:
- Around line 8027-8033: The metadata schema currently uses
additionalProperties: { type: object } which permits nested objects; change each
metadata schema (the "metadata" object blocks in this OpenAPI spec) to restrict
values to flat primitives by replacing additionalProperties: { type: object }
with additionalProperties: { oneOf: [{ type: "string" }, { type: "number" }, {
type: "boolean" }] } (or another explicit union of primitive types), keep the
description/example as-is, and apply the same deterministic fix to all other
occurrences mentioned (the other metadata blocks at the same pattern).
- Around line 6926-6947: AccountingEntry currently leaves credit and debit
optional in the generated schema; update the generator (Swaggo source) or add a
deterministic post-generation transform to emit an OpenAPI conditional schema
for AccountingEntry that encodes the operationType/scenario matrix (use oneOf or
if/then/else predicates on operationType and scenario) so that the credit and
debit properties become required/forbidden per rule (e.g., source+direct/commit
=> debit required, credit optional; source+hold/cancel => both required;
destination+direct/commit => credit required, debit optional;
destination+hold/cancel/revert and source+revert => forbid; bidirectional =>
both required). Ensure the transform targets the AccountingEntry schema and the
credit and debit property definitions rather than hand-editing the generated
YAML so SDKs/validators pick up the constraints.
- Around line 8034-8041: The schema property named "route" is described as
deprecated but lacks the OpenAPI flag; update the OpenAPI generation pipeline
(the generator template or the deterministic post-gen transform that
emits/patches request schemas) to add deprecated: true for every request-side
"route" property so generated YAML includes deprecated: true for that property
(apply the same change across the other occurrences referenced in the comment).
Locate the code that constructs request property metadata for "route" and emit
deprecated: true, or add a post-generation patch step that finds property name
"route" in request schemas and inserts deprecated: true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 34d25e1c-087d-4ad8-bfe0-9dac2cdc6841
📒 Files selected for processing (16)
CLAUDE.mdcomponents/ledger/api/docs.gocomponents/ledger/api/openapi.yamlcomponents/ledger/api/swagger.jsoncomponents/ledger/internal/adapters/http/in/transaction.gocomponents/ledger/internal/adapters/http/in/transaction_create.gocomponents/ledger/internal/adapters/http/in/transaction_helper.gocomponents/ledger/internal/adapters/http/in/transaction_state_handlers.gocomponents/ledger/internal/adapters/postgres/balance/balance.postgresql.gocomponents/ledger/internal/services/query/get_balances.gocomponents/ledger/internal/services/query/validate_accounting_routes.godocs/PROJECT_RULES.mdpkg/mmodel/operation-route.gopkg/mtransaction/input.gopkg/mtransaction/input_test.gopkg/mtransaction/transaction.go
…aking into generated docs 🐛
… intentional in-place mutation 📚
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (12)
components/ledger/internal/adapters/http/in/transaction_create.go (1)
844-850:⚠️ Potential issue | 🟠 MajorClean up idempotency and backup state when
BuildOperationsfails.This branch still returns without calling
deleteIdempotencyKeyorRemoveTransactionFromRedisQueue, even though the transaction has already claimed an idempotency slot and been queued for recovery.🛠️ Suggested fix
operations, _, err := handler.BuildOperations(ctx, balancesBefore, fromTo, transactionInput, *tran, validate, transactionDate, transactionStatus == constant.NOTED, ledgerSettings.Accounting.ValidateRoutes, routeCache, action) if err != nil { libOpentelemetry.HandleSpanError(span, "Failed to validate balances", err) logger.Log(ctx, libLog.LevelError, "Failed to validate balance", libLog.Err(err)) + + handler.deleteIdempotencyKey(ctx, idempotencyResult.InternalKey) + handler.Command.RemoveTransactionFromRedisQueue(ctx, logger, params.OrganizationID, params.LedgerID, transactionID.String()) return http.WithError(c, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/internal/adapters/http/in/transaction_create.go` around lines 844 - 850, The error path after handler.BuildOperations (in the transaction creation flow) returns without cleaning up idempotency and backup queue state; before returning on error from BuildOperations, call the existing cleanup helpers (deleteIdempotencyKey(...) and RemoveTransactionFromRedisQueue(...)) using the same idempotency key / transaction identifier used earlier, handle/log any cleanup errors (but preserve and return the original BuildOperations error), and then return http.WithError(c, err); update the error branch in the BuildOperations failure block to perform these cleanup calls (and keep libOpentelemetry.HandleSpanError and logger.Log as-is).pkg/mtransaction/input.go (1)
100-138:⚠️ Potential issue | 🟠 Major
/transactions/inflowcan no longer create pending transactions.
CreateTransactionInflowInputdroppedPending, butCreateTransactionInflowstill derives the initial status fromtransactionInput.InitialStatus(). That makes every inflow request start asCREATED, even when clients needPENDING.🛠️ Suggested fix
type CreateTransactionInflowInput struct { // Chart of accounts group name for accounting purposes // example: FUNDING // maxLength: 256 ChartOfAccountsGroupName string `json:"chartOfAccountsGroupName,omitempty" validate:"max=256" maxLength:"256" example:"FUNDING"` // Human-readable description of the transaction // example: New Transaction // maxLength: 256 Description string `json:"description,omitempty" validate:"max=256" example:"New Transaction" maxLength:"256"` // Transaction code for reference // example: TR12345 // maxLength: 100 Code string `json:"code,omitempty" validate:"max=100" example:"TR12345" maxLength:"100"` + + // Whether the transaction should be created in pending state + // example: true + Pending bool `json:"pending" example:"true" default:"false"` // Additional custom key-value attributes. Values must be flat (string, number, boolean) — no nested objects. // example: {"reference": "TRANSACTION-001", "source": "api"} Metadata map[string]any `json:"metadata" validate:"dive,keys,keymax=100,endkeys,omitempty,nonested,valuemax=2000"` @@ return &Transaction{ ChartOfAccountsGroupName: c.ChartOfAccountsGroupName, Description: c.Description, Code: c.Code, + Pending: c.Pending, Metadata: c.Metadata, TransactionDate: c.TransactionDate,Also applies to: 151-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/mtransaction/input.go` around lines 100 - 138, CreateTransactionInflowInput no longer exposes a Pending flag so CreateTransactionInflow still calling transactionInput.InitialStatus() always yields CREATED; restore the ability for clients to request PENDING by either adding a Pending (or Status) field back to CreateTransactionInflowInput (e.g. Pending *bool `json:"pending,omitempty"`) or modify transactionInput.InitialStatus() to derive the initial status from an existing SendInflow property (e.g. Send.Pending) and update CreateTransactionInflow to use that new field; ensure the symbol CreateTransactionInflowInput is updated and CreateTransactionInflow continues to call transactionInput.InitialStatus() (or the new field) so inflows can start as PENDING when requested.components/ledger/internal/adapters/http/in/transaction.go (1)
294-301:⚠️ Potential issue | 🟠 MajorDon't collapse cache backend failures into a debug cache miss.
GetWriteBehindTransactionerrors are all treated as ordinary misses here. That hides real Redis/write-behind outages and silently degrades the request path instead of surfacing an infrastructure problem.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/internal/adapters/http/in/transaction.go` around lines 294 - 301, The code currently treats any error from handler.Query.GetWriteBehindTransaction as a cache miss; instead, call GetWriteBehindTransaction and then: if wbErr == nil set "X-Cache-Hit" and return http.OK(wbTran); else if wbErr represents a genuine cache miss (e.g., compare against the expected sentinel like redis.Nil or your store's NotFound error) then log the debug "Write-behind cache miss..." via logger.Log and fall back to DB; otherwise treat wbErr as a backend failure—log it at error level with libLog.Err(wbErr) and return an error response (e.g., http.InternalServerError) rather than silently continuing. Ensure you reference handler.Query.GetWriteBehindTransaction, logger.Log, http.OK and http.InternalServerError/X-Cache-Hit in the change.components/ledger/api/docs.go (2)
10012-10015:⚠️ Potential issue | 🟡 MinorAdd UUID format to
Rate.externalId.This helper schema still exposes
externalIdas a free-form string even though the example and sibling rate schemas treat it as a UUID. Please fix the source annotation/tag and regenerate the docs so clients validate it consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/api/docs.go` around lines 10012 - 10015, Rate.externalId is declared as a plain string in the schema; change its source struct/tag to mark it as a UUID (e.g., use `format:"uuid"` or the appropriate OpenAPI struct tag used in this repo, or add `swagger:"format=uuid"`/`jsonschema:"format=uuid"` as consistent with sibling Rate schemas) on the Rate.externalId field so the generated docs set type string and format uuid, then regenerate the OpenAPI docs so clients see the UUID format. Ensure you update the Rate struct's annotation (the field name Rate.externalId) to match other rate schemas before running the docs generator.
9177-9180:⚠️ Potential issue | 🟠 MajorFlat metadata is still published as unrestricted JSON.
The descriptions now say nested metadata is invalid, but
additionalProperties: {}still allows nested objects and arrays in all four schemas. That keeps generated clients out of sync with the documented contract. Please tighten the source model/annotations for these metadata maps and regeneratedocs.go.Also applies to: 9235-9238, 9299-9302, 9455-9458
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/api/docs.go` around lines 9177 - 9180, The JSON schema for the "metadata" property is currently declared with additionalProperties: {} which permits nested objects/arrays; update the source model/annotations that define each metadata map (the "metadata" property used in the four schemas shown in docs.go) to restrict additionalProperties to primitive types only (e.g. additionalProperties with type array of "string","number","boolean" or equivalent annotation in your model generator), then regenerate docs.go so the OpenAPI schema and generated clients reflect a flat key->primitive-value contract; repeat for the other occurrences noted (around the other "metadata" properties).components/ledger/api/openapi.yaml (3)
6926-6948:⚠️ Potential issue | 🟠 MajorMake the
credit/debitrules machine-readable, not just descriptive.The schema now leaves both fields optional and moves the actual requirement matrix into prose. That means generated clients and validators still cannot tell which combinations are valid until the server rejects them. Please emit this as
oneOf/conditional schema logic from the Swaggo source (or a deterministic post-gen transform), not only in the description.Based on learnings, in this repo’s Go/Swaggo-generated OpenAPI specs, don’t hand-edit the generated Swagger/OpenAPI YAML; fix the generator source or apply a deterministic post-generation patch step.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/api/openapi.yaml` around lines 6926 - 6948, The AccountingEntry schema currently leaves credit and debit optional and buries the validity matrix in prose; change the generator (Swaggo source) or add a deterministic post-gen transformer to emit machine-readable OpenAPI conditionals so clients can validate combinations: update the AccountingEntry schema to use oneOf/anyOf/if-then-else rules (referencing AccountingRubric for the credit and debit properties) that enforce the matrix based on operationType and scenario (and cover the bidirectional, source/destination direct/commit/hold/cancel/revert cases and the "neither is rejected" rule); implement these conditional constructs in the Swaggo struct tags or generator code that produces the AccountingEntry, credit, and debit schema instead of hand-editing the generated YAML.
8122-8129:⚠️ Potential issue | 🟡 MinorMark legacy
routefields as deprecated in the schema.The description tells clients to migrate to
routeId, but these changed request fields still omit OpenAPI’sdeprecated: true. Without that keyword, docs and generated SDKs will keep presentingrouteas a normal input.Based on learnings, in this repo’s Go/Swaggo-generated OpenAPI specs, don’t hand-edit the generated Swagger/OpenAPI YAML; fix the generator source or apply a deterministic post-generation patch step.
Also applies to: 8371-8377
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/api/openapi.yaml` around lines 8122 - 8129, The OpenAPI schema for the legacy property named "route" is missing the OpenAPI/deprecation flag, so update the OpenAPI generator source (or add a deterministic post-generation patch) to emit deprecated: true for the "route" property where the schema describes it (the property named route that references routeId as the replacement) and for the other occurrence noted (the similar block at 8371-8377); specifically modify the code that emits property metadata for the "route" field (or the Swaggo struct tag/comment that drives generation) so the generated YAML includes deprecated: true alongside type/description/example so docs and SDKs treat it as deprecated.
8027-8033:⚠️ Potential issue | 🟠 Major
metadatastill contradicts its own contract.These blocks say metadata values are flat primitives and even show string examples, but
additionalProperties: { type: object }only permits object values. SDKs will still generate the wrong type and client-side validation will reject the documented payloads.Based on learnings, in this repo’s Go/Swaggo-generated OpenAPI specs, don’t hand-edit the generated Swagger/OpenAPI YAML; fix the generator source or apply a deterministic post-generation patch step.
Also applies to: 8108-8114, 8196-8202
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/api/openapi.yaml` around lines 8027 - 8033, The OpenAPI spec's metadata property declares values must be flat primitives but uses additionalProperties: { type: object }, causing generated SDKs to expect objects; update the generator or add a deterministic post-generation patch to replace the incorrect additionalProperties for the metadata property (the "metadata" schema/field in components/schemas) with an additionalProperties schema that allows primitive types (e.g., a oneOf/anyOf of string, number, boolean) so client validation matches the documented examples; apply the same fix for the other occurrences mentioned (the duplicated metadata blocks).components/ledger/api/swagger.json (4)
9359-9363:⚠️ Potential issue | 🟠 MajorRequire at least one source/destination leg.
toandfromare required to exist, but[]is still valid. That leaves the public contract accepting transactions with no actual distribution/source entries.Suggested contract fix
"to": { "type": "array", + "minItems": 1, "items": { "$ref": "#/definitions/FromTo" } } ... "from": { "type": "array", + "minItems": 1, "items": { "$ref": "#/definitions/FromTo" } }Also applies to: 10159-10163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/api/swagger.json` around lines 9359 - 9363, The schema allows empty arrays for the "to" and "from" properties; update the OpenAPI/Swagger definitions so both array schemas require at least one element by adding "minItems": 1 to the "to" and "from" array definitions (the array items already reference "#/definitions/FromTo"), and do the same for the duplicate occurrence around lines 10159-10163; keep the existing "required" flags but enforce non-empty arrays via minItems on the "to" and "from" properties.
9153-9156:⚠️ Potential issue | 🟠 MajorAlign
metadatavalidation with the flat-value contract.These schemas still use
additionalProperties: {}, so nested objects and arrays remain valid even though the descriptions now say values must be flat scalars. Generated clients will accept payloads the API documents as invalid.Also applies to: 9211-9214, 9275-9278
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/api/swagger.json` around lines 9153 - 9156, The metadata schemas currently allow arbitrary nested values via additionalProperties: {}; change each "metadata" schema's additionalProperties to a schema that restricts values to flat scalars (e.g. additionalProperties: { "oneOf": [ { "type": "string" }, { "type": "number" }, { "type": "boolean" } ] } or equivalent) so objects/arrays are rejected; update every "metadata" definition in the document (the entries named "metadata" near the repeated resource schemas) to use this scalar-only additionalProperties schema.
9988-9991:⚠️ Potential issue | 🟡 MinorAdd UUID typing to
Rate.externalId.This nested rate field is UUID-shaped in the example, but the schema still accepts any string. That weakens validation compared with the top-level
AssetRate.externalIdcontract.Suggested schema fix
"externalId": { "type": "string", + "format": "uuid", "example": "00000000-0000-0000-0000-000000000000" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/api/swagger.json` around lines 9988 - 9991, The nested Rate.externalId schema currently only types as "string" but should enforce UUID like the top-level AssetRate.externalId; update the Rate.externalId definition (the "externalId" property inside the Rate object) to include UUID typing—for example keep "type": "string" and add "format": "uuid" (or a UUID pattern) so it validates as a UUID consistent with AssetRate.externalId.
8293-8312:⚠️ Potential issue | 🟠 MajorReject empty
AccountingEntrypayloads in the schema.
{}is still valid here even though the description says entries with neitherdebitnorcreditare rejected. That weakens generated validation and pushes a documented contract failure to runtime.Suggested contract fix
"AccountingEntry": { "description": "AccountingEntry object containing debit and credit rubrics for a specific action. Field requirements depend on the parent operationType and scenario: - source + direct or commit: debit is REQUIRED, credit is optional. - source + hold or cancel: both debit AND credit are REQUIRED. - destination + direct or commit: credit is REQUIRED, debit is optional. - destination + hold or cancel: NOT ALLOWED (rejected at creation). - destination + revert: NOT ALLOWED (rejected at creation). - source + revert: NOT ALLOWED (rejected at creation). - bidirectional (all scenarios including revert): both debit AND credit are REQUIRED. An entry with neither debit nor credit is always rejected.", "type": "object", + "additionalProperties": false, + "minProperties": 1, "properties": {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/api/swagger.json` around lines 8293 - 8312, The AccountingEntry schema currently allows an empty object; update the AccountingEntry definition to require at least one of the two fields by adding an anyOf (or oneOf) constraint such as anyOf: [{"required":["debit"]},{"required":["credit"]}] so that either debit or credit must be present; modify the schema alongside the existing properties (credit, debit) in the AccountingEntry definition to enforce this rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Line 611: Replace the sentence fragment in the table row that currently reads
"Should be sparse enough to read in production without filtering." with a
complete sentence such as "Info logs should be sparse enough to read in
production without filtering." so the guideline for "Info" becomes a full,
grammatically correct sentence; locate the row containing the "Info" cell and
update the right-hand cell text accordingly.
In `@components/ledger/api/openapi.yaml`:
- Around line 8288-8293: The from and to array properties currently allow empty
arrays; update the OpenAPI schema that defines the transaction legs (the
properties named "from" and "to" which reference components/schemas/FromTo) to
require at least one element by adding "minItems: 1" to each array definition
(so "from" and "to" are still required but cannot be empty). Apply the same
change to the other occurrence noted (the second schema around lines 9169-9178)
so both array usages enforce at least one source/destination leg.
In `@components/ledger/internal/adapters/http/in/transaction_create.go`:
- Around line 758-767: The handler currently returns after
SendTransactionToRedisQueue fails without releasing the idempotency reservation,
causing permanent ErrIdempotencyKey on retries; update the error branch in
transaction_create.go (around handler.Command.SendTransactionToRedisQueue) to
always call handler.deleteIdempotencyKey(ctx, idempotencyResult.InternalKey)
before returning for any failure (not only when errors.Is(err,
constant.ErrTransactionBackupCacheMarshalFailed)), ensuring the idempotency slot
created by CreateOrCheckTransactionIdempotency is released on all queueing
failures; keep the existing logging and span error handling intact.
In `@Makefile`:
- Around line 583-590: The current check_or_install helper swallows failures by
echoing on a failed go install and continuing, which lets make dev-setup succeed
despite missing tools; update the check_or_install implementation so that when
go install "$$pkg" fails it returns a non-zero exit (or calls exit 1) instead of
only echoing, and ensure callers (the check_or_install invocations for gitleaks,
gofumpt, goimports, gosec, golangci-lint) will therefore fail the make target on
install error; locate the check_or_install function in the Makefile and change
the error path to propagate the failure (use set -e, return/exit, or conditional
&&/|| to fail) so missing tool installs cause make to fail.
In `@pkg/mtransaction/input.go`:
- Around line 70-72: BuildTransaction and BuildInflowEntry are returning the
caller's destination slice (send.distribute.to) which later gets mutated by
executeCreateTransaction (calls ApplyDefaultBalanceKeys, MutateConcatAliases,
MutateSplitAliases) and thus leaks aliases/balance keys back to the request; fix
by making a defensive copy of send.distribute.to before returning the built
transaction(s): allocate a new slice, copy the slice header and then deep-copy
each destination element (copy fields or clone pointer targets as appropriate)
so subsequent in-place mutations in executeCreateTransaction do not affect the
original caller slice; apply the same cloning logic where send.distribute.to
(and analogous destination slices) are returned (also in the other affected
return sites referenced in the comment).
---
Duplicate comments:
In `@components/ledger/api/docs.go`:
- Around line 10012-10015: Rate.externalId is declared as a plain string in the
schema; change its source struct/tag to mark it as a UUID (e.g., use
`format:"uuid"` or the appropriate OpenAPI struct tag used in this repo, or add
`swagger:"format=uuid"`/`jsonschema:"format=uuid"` as consistent with sibling
Rate schemas) on the Rate.externalId field so the generated docs set type string
and format uuid, then regenerate the OpenAPI docs so clients see the UUID
format. Ensure you update the Rate struct's annotation (the field name
Rate.externalId) to match other rate schemas before running the docs generator.
- Around line 9177-9180: The JSON schema for the "metadata" property is
currently declared with additionalProperties: {} which permits nested
objects/arrays; update the source model/annotations that define each metadata
map (the "metadata" property used in the four schemas shown in docs.go) to
restrict additionalProperties to primitive types only (e.g. additionalProperties
with type array of "string","number","boolean" or equivalent annotation in your
model generator), then regenerate docs.go so the OpenAPI schema and generated
clients reflect a flat key->primitive-value contract; repeat for the other
occurrences noted (around the other "metadata" properties).
In `@components/ledger/api/openapi.yaml`:
- Around line 6926-6948: The AccountingEntry schema currently leaves credit and
debit optional and buries the validity matrix in prose; change the generator
(Swaggo source) or add a deterministic post-gen transformer to emit
machine-readable OpenAPI conditionals so clients can validate combinations:
update the AccountingEntry schema to use oneOf/anyOf/if-then-else rules
(referencing AccountingRubric for the credit and debit properties) that enforce
the matrix based on operationType and scenario (and cover the bidirectional,
source/destination direct/commit/hold/cancel/revert cases and the "neither is
rejected" rule); implement these conditional constructs in the Swaggo struct
tags or generator code that produces the AccountingEntry, credit, and debit
schema instead of hand-editing the generated YAML.
- Around line 8122-8129: The OpenAPI schema for the legacy property named
"route" is missing the OpenAPI/deprecation flag, so update the OpenAPI generator
source (or add a deterministic post-generation patch) to emit deprecated: true
for the "route" property where the schema describes it (the property named route
that references routeId as the replacement) and for the other occurrence noted
(the similar block at 8371-8377); specifically modify the code that emits
property metadata for the "route" field (or the Swaggo struct tag/comment that
drives generation) so the generated YAML includes deprecated: true alongside
type/description/example so docs and SDKs treat it as deprecated.
- Around line 8027-8033: The OpenAPI spec's metadata property declares values
must be flat primitives but uses additionalProperties: { type: object }, causing
generated SDKs to expect objects; update the generator or add a deterministic
post-generation patch to replace the incorrect additionalProperties for the
metadata property (the "metadata" schema/field in components/schemas) with an
additionalProperties schema that allows primitive types (e.g., a oneOf/anyOf of
string, number, boolean) so client validation matches the documented examples;
apply the same fix for the other occurrences mentioned (the duplicated metadata
blocks).
In `@components/ledger/api/swagger.json`:
- Around line 9359-9363: The schema allows empty arrays for the "to" and "from"
properties; update the OpenAPI/Swagger definitions so both array schemas require
at least one element by adding "minItems": 1 to the "to" and "from" array
definitions (the array items already reference "#/definitions/FromTo"), and do
the same for the duplicate occurrence around lines 10159-10163; keep the
existing "required" flags but enforce non-empty arrays via minItems on the "to"
and "from" properties.
- Around line 9153-9156: The metadata schemas currently allow arbitrary nested
values via additionalProperties: {}; change each "metadata" schema's
additionalProperties to a schema that restricts values to flat scalars (e.g.
additionalProperties: { "oneOf": [ { "type": "string" }, { "type": "number" }, {
"type": "boolean" } ] } or equivalent) so objects/arrays are rejected; update
every "metadata" definition in the document (the entries named "metadata" near
the repeated resource schemas) to use this scalar-only additionalProperties
schema.
- Around line 9988-9991: The nested Rate.externalId schema currently only types
as "string" but should enforce UUID like the top-level AssetRate.externalId;
update the Rate.externalId definition (the "externalId" property inside the Rate
object) to include UUID typing—for example keep "type": "string" and add
"format": "uuid" (or a UUID pattern) so it validates as a UUID consistent with
AssetRate.externalId.
- Around line 8293-8312: The AccountingEntry schema currently allows an empty
object; update the AccountingEntry definition to require at least one of the two
fields by adding an anyOf (or oneOf) constraint such as anyOf:
[{"required":["debit"]},{"required":["credit"]}] so that either debit or credit
must be present; modify the schema alongside the existing properties (credit,
debit) in the AccountingEntry definition to enforce this rule.
In `@components/ledger/internal/adapters/http/in/transaction_create.go`:
- Around line 844-850: The error path after handler.BuildOperations (in the
transaction creation flow) returns without cleaning up idempotency and backup
queue state; before returning on error from BuildOperations, call the existing
cleanup helpers (deleteIdempotencyKey(...) and
RemoveTransactionFromRedisQueue(...)) using the same idempotency key /
transaction identifier used earlier, handle/log any cleanup errors (but preserve
and return the original BuildOperations error), and then return
http.WithError(c, err); update the error branch in the BuildOperations failure
block to perform these cleanup calls (and keep libOpentelemetry.HandleSpanError
and logger.Log as-is).
In `@components/ledger/internal/adapters/http/in/transaction.go`:
- Around line 294-301: The code currently treats any error from
handler.Query.GetWriteBehindTransaction as a cache miss; instead, call
GetWriteBehindTransaction and then: if wbErr == nil set "X-Cache-Hit" and return
http.OK(wbTran); else if wbErr represents a genuine cache miss (e.g., compare
against the expected sentinel like redis.Nil or your store's NotFound error)
then log the debug "Write-behind cache miss..." via logger.Log and fall back to
DB; otherwise treat wbErr as a backend failure—log it at error level with
libLog.Err(wbErr) and return an error response (e.g., http.InternalServerError)
rather than silently continuing. Ensure you reference
handler.Query.GetWriteBehindTransaction, logger.Log, http.OK and
http.InternalServerError/X-Cache-Hit in the change.
In `@pkg/mtransaction/input.go`:
- Around line 100-138: CreateTransactionInflowInput no longer exposes a Pending
flag so CreateTransactionInflow still calling transactionInput.InitialStatus()
always yields CREATED; restore the ability for clients to request PENDING by
either adding a Pending (or Status) field back to CreateTransactionInflowInput
(e.g. Pending *bool `json:"pending,omitempty"`) or modify
transactionInput.InitialStatus() to derive the initial status from an existing
SendInflow property (e.g. Send.Pending) and update CreateTransactionInflow to
use that new field; ensure the symbol CreateTransactionInflowInput is updated
and CreateTransactionInflow continues to call transactionInput.InitialStatus()
(or the new field) so inflows can start as PENDING when requested.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 149567ef-8eb7-496f-90b0-09d3be03ad6a
📒 Files selected for processing (9)
CLAUDE.mdMakefilecomponents/ledger/api/docs.gocomponents/ledger/api/openapi.yamlcomponents/ledger/api/swagger.jsoncomponents/ledger/internal/adapters/http/in/transaction.gocomponents/ledger/internal/adapters/http/in/transaction_create.gopkg/mtransaction/input.gopkg/mtransaction/transaction.go
gandalf-at-lerian
left a comment
gandalf-at-lerian
left a comment
There was a problem hiding this comment.
Code Review
Overview
Large refactor that decomposes the monolithic transaction creation flow into focused, well-named units. The direction is clearly right — the old transaction_create_handlers.go / create_transaction.go were doing too much, and this extraction into process_balance_operations.go, create_transaction_idempotency.go, transaction_helper.go, and the pkg/mtransaction/input.go DTOs brings real structural clarity.
CI passes. Test coverage for the new code is solid (703 lines for process_balance_operations, 453 for input_test.go). The cache-aside patterns in get_balances.go and get_ledger_settings.go are clean and consistent.
That said, there are issues that need to be addressed before merge.
Blocking Issues
1. Missing cleanup on BuildOperations failure (transaction_create.go ~L845)
When BuildOperations returns an error, the handler returns http.WithError(c, err) but does not clean up:
- The idempotency key (via
deleteIdempotencyKey) - The Redis backup queue entry (via
RemoveTransactionFromRedisQueue)
Every other error path in executeCreateTransaction correctly cleans up both. This one is the exception — and it is the most dangerous one, because by this point the atomic balance operation has already succeeded. A retry (same idempotency key) will hit the existing slot and either get a stale empty value or a conflict, while the balances have already been mutated.
Fix: add the same cleanup pattern used in the surrounding error paths.
2. Fire-and-forget goroutines using context.Background() without timeout (~L886-890)
bgCtx := tmcore.ContextWithTenantID(context.Background(), tmcore.GetTenantIDContext(ctx))
go handler.Command.SetTransactionIdempotencyValue(bgCtx, ...)
go handler.Command.SendLogTransactionAuditQueue(bgCtx, ...)These goroutines have no timeout and no error handling. If Redis or RabbitMQ hangs, the goroutine leaks indefinitely. Use context.WithTimeout(context.Background(), 30*time.Second) at minimum, and log errors. This is a pre-existing pattern, but since you are touching this code, it is the right time to fix it.
3. CreateTransactionInflowInput has no Pending field
The original CreateTransactionInput has Pending bool, but CreateTransactionInflowInput does not. InitialStatus() on an inflow input will always return CREATED, meaning inflow transactions can never be created in PENDING state. If this is intentional, document it. If not, add the field.
Should Fix (non-blocking but high value)
4. MutateConcatAliases / MutateSplitAliases mutate input slices
These functions modify the AccountAlias field of FromTo items in place. In executeCreateTransaction, MutateConcatAliases is called first (~L694), then MutateSplitAliases is called later (~L813) on the same slice (transactionInput.Send.Source.From). The second call operates on already-mutated data. This works by accident because concat prepends an index and split strips it, but it is fragile and will break silently if either function changes behavior.
Consider operating on copies, or at least add a comment explaining the coupling.
5. //nolint:gocognit on BuildOperations
The comment says "Will be refactored into smaller functions." This PR is the refactoring PR. If it is not being broken up now, when? At minimum, the double-entry logic (tryBuildDoubleEntryOps, buildDoubleEntryPendingOps, buildDoubleEntryCanceledOps) is already extracted — the nolint may no longer be needed. Check the actual complexity score.
6. OpenAPI schemas for CreateTransaction*Input are empty objects
The swagger regeneration produced empty {} schemas for CreateTransactionInput, CreateTransactionInflowInput, and CreateTransactionOutflowInput. This means SDK generators and Swagger UI show no request body fields. This is a real usability regression for API consumers. The swaggo annotations likely need updating to reference the new pkg/mtransaction types.
Nice-to-haves
- Import ordering: Several files have stdlib/external/internal groups mixed. Cosmetic, but easy to fix in a follow-up.
fmt.Sprintfin logger calls: ~10 instances acrosstransaction_create.goandtransaction_state_handlers.gothat should use structured fields (libLog.String,libLog.Err). Not blocking, but inconsistent with the rest of this PR which correctly uses structured logging.isConcatedAliasis too permissive:strings.Contains(alias, "#")matches things like"12#treasury". A stricter check (e.g., verifying the prefix is a valid integer) would prevent false positives.- Duplicated
c.Set(libConstants.IdempotencyReplayed, "false")at L671 — set twice, second one is dead code.
What is good
ProcessBalanceOperationsInputparameter object — clean pattern, good docs, backward-compatible for future extensions.buildBalanceOperationsis pure — no I/O, testable in isolation, sorted by internal key to prevent Lua deadlocks. Well done.- Cache-aside in
GetLedgerSettingsandGetBalances— consistent pattern, non-blocking cache errors, proper span management. - Idempotency extraction —
CreateOrCheckTransactionIdempotencyis cleaner than the inline version, with proper error wrapping. - Test coverage — the new
process_balance_operations_test.goandinput_test.goare thorough. - Defensive clone in
BuildTransaction(copying theFromslice before mutation) shows awareness of the mutation problem.
Verdict
Fix #1 (missing cleanup on BuildOperations failure) — that is a correctness bug. #2 and #3 should also be addressed before merge. The rest can be tracked as follow-ups.
Not approving yet, but the architecture is solid. Happy to re-review after fixes.
…r BuildOperations failure 📚
gandalf-at-lerian
left a comment
gandalf-at-lerian
left a comment
There was a problem hiding this comment.
Both fixes look good:
-
BuildOperations error path comment: clearly explains the intentional no-cleanup — Kiwi consumer recovers from the backup queue, idempotency key preserved to prevent double balance mutation. The log message was also corrected from "Failed to validate balances" to "Failed to build operations" which is more accurate.
-
Inflow Pending documentation: godoc + swagger description both explain why Pending is omitted, with reference to error 0098 (ErrOnHoldExternalAccount).
Approving. Nice refactor overall 👊
3 participants
Refactors the transaction creation and balance processing pipeline (from HTTP handler through the command layer to the Redis adapter) focusing on clarity, observability, and correctness.
Key changes:
Test additions: