feat(reconciliation): add assertion modes and min buffer#47
Open
feat(reconciliation): add assertion modes and min buffer#47
Conversation
WalkthroughAdds assertion-mode support (COVERAGE, EQUALITY, MIN_BUFFER): extends Policy model and API responses with mode and assertionConfig, validates and parses MIN_BUFFER configs during policy creation, updates reconciliation to enforce mode-specific drift rules, adds DB migration, and adjusts tests and mocks. Changes
Sequence DiagramsequenceDiagram
participant Client
participant PolicyAPI as Policy API
participant Service as Policy Service
participant Reconciler as Reconciliation Service
participant Storage as Storage Layer
Client->>PolicyAPI: POST /policies (mode, assertionConfig)
PolicyAPI->>Service: CreatePolicy(req)
Service->>Service: Normalize & validate mode
alt mode == MIN_BUFFER
Service->>Service: Parse & validate min-buffer config
end
Service->>Storage: Store Policy (mode, assertionConfig)
Storage-->>Service: Saved
Service-->>PolicyAPI: Return Policy (mode, assertionConfig)
PolicyAPI-->>Client: 201 Created
Client->>Reconciler: Start reconciliation(policyID)
Reconciler->>Storage: GetPolicy(policyID)
Storage-->>Reconciler: Policy (mode, config)
Reconciler->>Reconciler: Normalize mode
Reconciler->>Reconciler: evaluateDrift(mode, minBuffer)
alt COVERAGE
Reconciler->>Reconciler: allow extra ledger assets
else EQUALITY
Reconciler->>Reconciler: require exact asset equality
else MIN_BUFFER
Reconciler->>Reconciler: compute requiredBufferForAsset and compare drift
end
Reconciler->>Storage: Store reconciliation result
Storage-->>Reconciler: Saved
Reconciler-->>Client: 200 OK (result)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ssertion-modes # Conflicts: # internal/api/service/reconciliation.go
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/api/service/reconciliation.go (2)
130-143:⚠️ Potential issue | 🟡 MinorUnreachable branch:
ledgerBalance == nil && paymentBalance != nil(line 134).Since the loop on line 98 iterates over
ledgerBalances,ledgerBalancewill always be non-nil (barring an explicitnilvalue stored in the map). ThepaymentBalanceis looked up separately and can be nil. So the case at line 134 is effectively dead code. If payment-only assets are handled (per the comment above), this branch becomes reachable again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/service/reconciliation.go` around lines 130 - 143, The switch branch checking "ledgerBalance == nil && paymentBalance != nil" is unreachable because the surrounding loop iterates only over ledgerBalances; to fix, either change the iteration to walk the union of keys from ledgerBalances and paymentBalances (so ledgerBalance can be nil) or keep the current loop and remove that unreachable case and instead add a separate loop over paymentBalances to handle payment-only assets; update references to ledgerBalance, paymentBalance and res.DriftBalances accordingly and preserve the existing behavior/error messages for missing assets.
98-113:⚠️ Potential issue | 🔴 CriticalPayment-only assets are silently ignored — incorrect for EQUALITY and MIN_BUFFER modes.
The loop only iterates over
ledgerBalances. If an asset exists inpaymentsBalancesbut has no corresponding ledger entry, it is completely skipped. ForEQUALITYmode, this is a correctness bug: a non-zero payment balance with no ledger counterpart should fail the equality check. The same applies toMIN_BUFFER.The existing
harmonizeBalanceshelper (lines 217–237) was presumably used before this refactor to handle exactly this case, but it's no longer called — making it dead code.Consider either:
- Re-introducing
harmonizeBalancesbefore the loop, or- Adding a second pass over
paymentsBalancesfor assets not inledgerBalances.Option 2: Add a second pass for payment-only assets
for asset, ledgerBalance := range ledgerBalances { paymentBalance, ok := paymentsBalances[asset] if !ok { paymentBalance = nil } err := s.computeDrift(res, asset, ledgerBalance, paymentBalance, assertionMode, minBuffer) if err != nil { res.Status = models.ReconciliationNotOK if res.Error == "" { res.Error = err.Error() } else { res.Error = res.Error + "; " + err.Error() } } } + + for asset, paymentBalance := range paymentsBalances { + if _, ok := ledgerBalances[asset]; ok { + continue // already handled above + } + err := s.computeDrift(res, asset, nil, paymentBalance, assertionMode, minBuffer) + if err != nil { + res.Status = models.ReconciliationNotOK + if res.Error == "" { + res.Error = err.Error() + } else { + res.Error = res.Error + "; " + err.Error() + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/service/reconciliation.go` around lines 98 - 113, The loop only iterates ledgerBalances and thus skips assets present only in paymentsBalances, causing incorrect results for EQUALITY and MIN_BUFFER; fix by ensuring payment-only assets are processed: either call the existing harmonizeBalances helper (restore its invocation) before the loop so ledgerBalances and paymentsBalances are aligned, or add a second pass iterating paymentsBalances and for any asset missing in ledgerBalances call s.computeDrift(res, asset, nil, paymentBalance, assertionMode, minBuffer); keep the same error-aggregation logic used in the current loop so Status/Error are updated consistently.
🧹 Nitpick comments (9)
internal/api/service/policy_validation_test.go (1)
10-82: Good validation coverage; consider a couple of additional cases.The existing tests cover the key paths well. Two gaps worth noting:
- No test for
EQUALITYmode validation (should pass with empty/nil config).- No test for negative
bpsvalues (the validation rejects them pervalidateMinBufferRule).These would round out coverage of the validation branches.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/service/policy_validation_test.go` around lines 10 - 82, Add two unit test cases to TestCreatePolicyRequestValidateAssertionMode: one verifying that setting req.AssertionMode = models.AssertionModeEquality (or "EQUALITY") with no AssertionConfig passes validation (call req.Validate() and require.NoError), and another that sets AssertionMode = models.AssertionModeMinBuffer and provides an AssertionConfig with a negative "bps" value (e.g., "assets": {"USD/2": {"bps": -10}}) and asserts req.Validate() returns an error; these target CreatePolicyRequest.Validate and the validateMinBufferRule branches to cover the missing paths.internal/api/service/policy.go (3)
97-129: Parsed config is discarded; consider caching or documenting the intent.
parseMinBufferConfig(Line 46) validates the config during creation but discards the result. During reconciliation evaluation, the rawmap[string]interface{}will need to be re-parsed. This is fine if intentional (keeping the storage format as raw JSON), but worth a brief comment to clarify.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/service/policy.go` around lines 97 - 129, parseMinBufferConfig currently parses and validates the raw map into a minBufferConfig but returns it without being cached and the review notes that the parsed config is discarded; either persist the parsed structure or document the intent. Update the caller or the policy struct to store the returned *minBufferConfig (so reconciliation can reuse the validated config) or, if keeping raw JSON is intentional, add a concise comment in parseMinBufferConfig explaining that the function only validates and that callers must re-parse and why; reference parseMinBufferConfig, minBufferConfig, and validateMinBufferRule when making the change.
88-95:Absoluteasint64may be insufficient for large token amounts.The
Policy.AssertionConfigstores absolute buffer values asint64(max ~9.2×10¹⁸). In crypto/DeFi contexts, token amounts can exceed this (e.g., ERC-20 tokens with 18 decimals). If this service handles such assets, consider usingjson.Numberor a string representation that maps to*big.Int.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/service/policy.go` around lines 88 - 95, The Absolute field in minBufferAssetRule uses int64 which can overflow for large token amounts; change Absolute to a big-integer-backed type (e.g., *big.Int) or a string-wrapped big-int type and implement JSON (un)marshalling (TextMarshaler/TextUnmarshaler) so large values are encoded/decoded as decimal strings; update minBufferAssetRule and any references such as minBufferConfig and Policy.AssertionConfig to use that new type and ensure all serialization, validation, and any arithmetic code uses the big-int API instead of int64.
27-57:assertionConfigis accepted without validation for COVERAGE/EQUALITY modes.Lines 45–49 only validate
assertionConfigwhen mode isMIN_BUFFER. For other modes, arbitrary JSON payloads will be silently stored in the database. Consider either rejecting non-emptyassertionConfigfor modes that don't use it, or documenting that it's ignored.💡 Suggested validation
if mode == models.AssertionModeMinBuffer { if _, err := parseMinBufferConfig(r.AssertionConfig); err != nil { return fmt.Errorf("%w: %s", ErrValidation, err.Error()) } + } else if len(r.AssertionConfig) > 0 { + return fmt.Errorf("%w: assertionConfig is not supported for mode %s", ErrValidation, mode) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/service/policy.go` around lines 27 - 57, CreatePolicyRequest.Validate currently only parses/validates AssertionConfig for models.AssertionModeMinBuffer, allowing arbitrary JSON for other modes; update Validate (in function CreatePolicyRequest.Validate) to handle AssertionConfig for other AssertionMode values returned by models.NormalizeAssertionMode/IsValid: either (a) reject a non-empty r.AssertionConfig for modes that don’t use it (e.g., models.AssertionModeCoverage, models.AssertionModeEquality) by returning fmt.Errorf("%w: unexpected assertionConfig for mode", ErrValidation) or (b) explicitly clear r.AssertionConfig = map[string]interface{}{} for those modes if the intended behavior is to ignore it; keep the existing parseMinBufferConfig check for models.AssertionModeMinBuffer and assign r.AssertionMode = mode at the end.internal/api/service/service_test.go (1)
126-130: Shallow copy shares map fields with the original.
policy := *s.policyis a shallow copy —LedgerQueryandAssertionConfigmaps will alias the original. This is safe as long as tests don't mutate the returned policy's maps concurrently with readings.policy. Current tests appear safe, but worth noting for future test authors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/service/service_test.go` around lines 126 - 130, The current mockStore.GetPolicy uses a shallow copy (policy := *s.policy) which aliases map fields like LedgerQuery and AssertionConfig to the original; change GetPolicy to perform a deep copy of those map fields so the returned *models.Policy has independent maps: create a new models.Policy value copying primitive fields from *s.policy, allocate new maps for LedgerQuery and AssertionConfig and copy each key/value into them, set policy.ID = id, and return &policy; this ensures mockStore.GetPolicy does not share mutable map state with s.policy.internal/api/service/reconciliation_test.go (1)
248-251: Policy test fixtures only set mode/config fields; other fields are zero-valued.The custom
*models.Policyinstances (e.g., Lines 248–251) omitID,Name,LedgerName,PaymentsPoolID, etc. SincenewMockStoreuses the provided policy as-is when non-nil,GetPolicyreturns a policy with zero-valuedLedgerNameandPaymentsPoolID. This works today because the mock SDK ignores those values, but it's fragile — any future change that validates or routes on those fields will silently break these tests.Consider building on the default policy and only overriding mode/config:
💡 Suggested approach
// In the test case setup, build a full policy: func testPolicy(mode models.AssertionMode, config map[string]interface{}) *models.Policy { return &models.Policy{ ID: uuid.New(), CreatedAt: time.Date(2021, 1, 1, 0, 0, 0, 0, time.UTC), Name: "test", LedgerName: "default", LedgerQuery: map[string]interface{}{}, PaymentsPoolID: uuid.New(), AssertionMode: mode, AssertionConfig: config, } }Also applies to: 276-285, 310-319, 344-353, 368-377
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/service/reconciliation_test.go` around lines 248 - 251, The tests create partial *models.Policy instances (e.g., in the table entries that feed newMockStore) leaving ID, Name, LedgerName, PaymentsPoolID, etc. zero-valued which makes GetPolicy return incomplete policies; instead add a small helper (e.g., testPolicy or buildTestPolicy) that returns a fully-populated Policy with sensible defaults (ID, CreatedAt, Name, LedgerName, LedgerQuery, PaymentsPoolID) and set only AssertionMode/AssertionConfig for each case, then use that helper when constructing the policy passed into newMockStore and all referenced test cases (lines around the examples shown) so tests get a complete, stable policy object.internal/models/policy.go (1)
31-36:NormalizeAssertionModesilently passes through invalid non-empty values.This is fine today because the caller in
reconciliation.gofollows up withIsValid(), but the function name "Normalize" suggests it would produce a valid output. Consider documenting this expectation or adding validation here to make the function self-contained.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/models/policy.go` around lines 31 - 36, NormalizeAssertionMode currently returns non-empty invalid AssertionMode values unchanged; update it to ensure it always returns a valid mode by checking the value with the AssertionMode.IsValid() method and returning AssertionModeCoverage when the input is either empty or not valid. Modify the NormalizeAssertionMode function to call a.IsValid() (in addition to the existing empty check) and return AssertionModeCoverage for any invalid input, and add a brief doc comment to the function stating it guarantees a valid AssertionMode so callers (e.g., reconciliation.go) no longer need to re-validate.internal/api/service/reconciliation.go (2)
217-237: Remove theharmonizeBalancesfunction—it is not called anywhere in the codebase.This is dead code that should be removed. If the logic is needed for handling payment-only assets, consider extracting and reusing it; otherwise, delete it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/service/reconciliation.go` around lines 217 - 237, Remove the unused harmonizeBalances function from reconciliation.go: locate the function named harmonizeBalances(ledgerBalances, paymentsBalances map[string]*big.Int) and delete its entire declaration and body since it is not referenced anywhere; if you expect this logic to be required later, extract the logic into a well-named utility (e.g., EnsureZeroForMissingAssets) in a shared package and call it where appropriate instead of leaving dead code.
188-215: Integer truncation in BPS calculation can silently result in zero buffer requirements for small exposures.The calculation at line 210 uses integer division:
required = exposure * bps / 10000. For small ledger balances, this truncates toward zero. For example, with exposure=50 and bps=100, the result is (50 * 100) / 10000 = 0, making the min buffer check pass trivially. Consider documenting this behavior or using ceiling division (e.g.,(exposure * bps + 9999) / 10000) if a conservative rounding approach is preferred.The
minBufferConfigtype andparseMinBufferConfigfunction are defined ininternal/api/service/policy.go(not shown in this diff).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/service/reconciliation.go` around lines 188 - 215, The BPS calculation in requiredBufferForAsset currently uses floor integer division and can yield zero for small exposures; change the math in requiredBufferForAsset when rule.BPS is set to perform ceiling division so tiny exposures still produce a non-zero buffer (e.g., compute (exposure * bps + 10000 - 1) / 10000 using big.Int ops) and return that result; ensure you use exposure.Abs(ledgerBalance) as before, multiply by big.NewInt(*rule.BPS), add big.NewInt(9999) before dividing by big.NewInt(10000), and return the computed big.Int to preserve behavior for other branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/api/policy.go`:
- Line 51: listPoliciesHandler is rendering raw models.Policy via
api.RenderCursor and therefore can return an empty AssertionMode string; update
listPoliciesHandler to normalize each policy's AssertionMode before rendering
(use models.NormalizeAssertionMode(policy.AssertionMode).String()) and return
the same policyResponse shape as getPolicyHandler/createPolicyHandler so list
output matches normalization. Locate listPoliciesHandler and replace or map the
rendered models.Policy items to policyResponse objects (or set
policy.AssertionMode =
models.NormalizeAssertionMode(policy.AssertionMode).String()) prior to calling
api.RenderCursor.
In `@internal/models/policy.go`:
- Around line 10-16: The public API and persisted names use the misspelled
"Assertation" variant; rename all occurrences to the correct "Assertion" form:
rename the type (e.g., AssertationMode → AssertionMode), its constants (e.g.,
AssertationModeCoverage → AssertionModeCoverage, AssertationModeEquality →
AssertionModeEquality, AssertationModeMinBuffer → AssertionModeMinBuffer),
related struct/type names (e.g., AssertationConfig → AssertionConfig), and
functions (e.g., NormalizeAssertationMode → NormalizeAssertionMode), update JSON
tags and DB column names/defaults and any migration or seed that persists the
old string values, and adjust all callers/tests to use the new identifiers so
the API, JSON, and DB values consistently use "Assertion".
---
Outside diff comments:
In `@internal/api/service/reconciliation.go`:
- Around line 130-143: The switch branch checking "ledgerBalance == nil &&
paymentBalance != nil" is unreachable because the surrounding loop iterates only
over ledgerBalances; to fix, either change the iteration to walk the union of
keys from ledgerBalances and paymentBalances (so ledgerBalance can be nil) or
keep the current loop and remove that unreachable case and instead add a
separate loop over paymentBalances to handle payment-only assets; update
references to ledgerBalance, paymentBalance and res.DriftBalances accordingly
and preserve the existing behavior/error messages for missing assets.
- Around line 98-113: The loop only iterates ledgerBalances and thus skips
assets present only in paymentsBalances, causing incorrect results for EQUALITY
and MIN_BUFFER; fix by ensuring payment-only assets are processed: either call
the existing harmonizeBalances helper (restore its invocation) before the loop
so ledgerBalances and paymentsBalances are aligned, or add a second pass
iterating paymentsBalances and for any asset missing in ledgerBalances call
s.computeDrift(res, asset, nil, paymentBalance, assertionMode, minBuffer); keep
the same error-aggregation logic used in the current loop so Status/Error are
updated consistently.
---
Nitpick comments:
In `@internal/api/service/policy_validation_test.go`:
- Around line 10-82: Add two unit test cases to
TestCreatePolicyRequestValidateAssertionMode: one verifying that setting
req.AssertionMode = models.AssertionModeEquality (or "EQUALITY") with no
AssertionConfig passes validation (call req.Validate() and require.NoError), and
another that sets AssertionMode = models.AssertionModeMinBuffer and provides an
AssertionConfig with a negative "bps" value (e.g., "assets": {"USD/2": {"bps":
-10}}) and asserts req.Validate() returns an error; these target
CreatePolicyRequest.Validate and the validateMinBufferRule branches to cover the
missing paths.
In `@internal/api/service/policy.go`:
- Around line 97-129: parseMinBufferConfig currently parses and validates the
raw map into a minBufferConfig but returns it without being cached and the
review notes that the parsed config is discarded; either persist the parsed
structure or document the intent. Update the caller or the policy struct to
store the returned *minBufferConfig (so reconciliation can reuse the validated
config) or, if keeping raw JSON is intentional, add a concise comment in
parseMinBufferConfig explaining that the function only validates and that
callers must re-parse and why; reference parseMinBufferConfig, minBufferConfig,
and validateMinBufferRule when making the change.
- Around line 88-95: The Absolute field in minBufferAssetRule uses int64 which
can overflow for large token amounts; change Absolute to a big-integer-backed
type (e.g., *big.Int) or a string-wrapped big-int type and implement JSON
(un)marshalling (TextMarshaler/TextUnmarshaler) so large values are
encoded/decoded as decimal strings; update minBufferAssetRule and any references
such as minBufferConfig and Policy.AssertionConfig to use that new type and
ensure all serialization, validation, and any arithmetic code uses the big-int
API instead of int64.
- Around line 27-57: CreatePolicyRequest.Validate currently only
parses/validates AssertionConfig for models.AssertionModeMinBuffer, allowing
arbitrary JSON for other modes; update Validate (in function
CreatePolicyRequest.Validate) to handle AssertionConfig for other AssertionMode
values returned by models.NormalizeAssertionMode/IsValid: either (a) reject a
non-empty r.AssertionConfig for modes that don’t use it (e.g.,
models.AssertionModeCoverage, models.AssertionModeEquality) by returning
fmt.Errorf("%w: unexpected assertionConfig for mode", ErrValidation) or (b)
explicitly clear r.AssertionConfig = map[string]interface{}{} for those modes if
the intended behavior is to ignore it; keep the existing parseMinBufferConfig
check for models.AssertionModeMinBuffer and assign r.AssertionMode = mode at the
end.
In `@internal/api/service/reconciliation_test.go`:
- Around line 248-251: The tests create partial *models.Policy instances (e.g.,
in the table entries that feed newMockStore) leaving ID, Name, LedgerName,
PaymentsPoolID, etc. zero-valued which makes GetPolicy return incomplete
policies; instead add a small helper (e.g., testPolicy or buildTestPolicy) that
returns a fully-populated Policy with sensible defaults (ID, CreatedAt, Name,
LedgerName, LedgerQuery, PaymentsPoolID) and set only
AssertionMode/AssertionConfig for each case, then use that helper when
constructing the policy passed into newMockStore and all referenced test cases
(lines around the examples shown) so tests get a complete, stable policy object.
In `@internal/api/service/reconciliation.go`:
- Around line 217-237: Remove the unused harmonizeBalances function from
reconciliation.go: locate the function named harmonizeBalances(ledgerBalances,
paymentsBalances map[string]*big.Int) and delete its entire declaration and body
since it is not referenced anywhere; if you expect this logic to be required
later, extract the logic into a well-named utility (e.g.,
EnsureZeroForMissingAssets) in a shared package and call it where appropriate
instead of leaving dead code.
- Around line 188-215: The BPS calculation in requiredBufferForAsset currently
uses floor integer division and can yield zero for small exposures; change the
math in requiredBufferForAsset when rule.BPS is set to perform ceiling division
so tiny exposures still produce a non-zero buffer (e.g., compute (exposure * bps
+ 10000 - 1) / 10000 using big.Int ops) and return that result; ensure you use
exposure.Abs(ledgerBalance) as before, multiply by big.NewInt(*rule.BPS), add
big.NewInt(9999) before dividing by big.NewInt(10000), and return the computed
big.Int to preserve behavior for other branches.
In `@internal/api/service/service_test.go`:
- Around line 126-130: The current mockStore.GetPolicy uses a shallow copy
(policy := *s.policy) which aliases map fields like LedgerQuery and
AssertionConfig to the original; change GetPolicy to perform a deep copy of
those map fields so the returned *models.Policy has independent maps: create a
new models.Policy value copying primitive fields from *s.policy, allocate new
maps for LedgerQuery and AssertionConfig and copy each key/value into them, set
policy.ID = id, and return &policy; this ensures mockStore.GetPolicy does not
share mutable map state with s.policy.
In `@internal/models/policy.go`:
- Around line 31-36: NormalizeAssertionMode currently returns non-empty invalid
AssertionMode values unchanged; update it to ensure it always returns a valid
mode by checking the value with the AssertionMode.IsValid() method and returning
AssertionModeCoverage when the input is either empty or not valid. Modify the
NormalizeAssertionMode function to call a.IsValid() (in addition to the existing
empty check) and return AssertionModeCoverage for any invalid input, and add a
brief doc comment to the function stating it guarantees a valid AssertionMode so
callers (e.g., reconciliation.go) no longer need to re-validate.
Comment on lines
+10
to
+16
| type AssertionMode string | ||
|
|
||
| const ( | ||
| AssertionModeCoverage AssertionMode = "COVERAGE" | ||
| AssertionModeEquality AssertionMode = "EQUALITY" | ||
| AssertionModeMinBuffer AssertionMode = "MIN_BUFFER" | ||
| ) |
There was a problem hiding this comment.
Typo: "Assertion" → "Assertion" throughout the public API surface.
"Assertion" is not a standard English word; the correct noun is "Assertion." Since this name will be persisted in the database (column names, stored values) and exposed in JSON responses, it will be increasingly costly to rename later. Consider fixing this now before it ships.
This affects AssertionMode, AssertionModeCoverage, AssertionModeEquality, AssertionModeMinBuffer, AssertionConfig, NormalizeAssertionMode, and the corresponding DB column/default values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/models/policy.go` around lines 10 - 16, The public API and persisted
names use the misspelled "Assertation" variant; rename all occurrences to the
correct "Assertion" form: rename the type (e.g., AssertationMode →
AssertionMode), its constants (e.g., AssertationModeCoverage →
AssertionModeCoverage, AssertationModeEquality → AssertionModeEquality,
AssertationModeMinBuffer → AssertionModeMinBuffer), related struct/type names
(e.g., AssertationConfig → AssertionConfig), and functions (e.g.,
NormalizeAssertationMode → NormalizeAssertionMode), update JSON tags and DB
column names/defaults and any migration or seed that persists the old string
values, and adjust all callers/tests to use the new identifiers so the API,
JSON, and DB values consistently use "Assertion".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/api/service/reconciliation.go (1)
98-113:⚠️ Potential issue | 🟠 MajorAssets present only in
paymentsBalancesare silently ignored.The loop iterates only over
ledgerBalances. Any asset that exists inpaymentsBalancesbut not inledgerBalancesis never processed — no drift is recorded and no error is raised. This is a reconciliation gap: unmatched payment-side assets (e.g., deposits in an unexpected currency) will go undetected.Consider iterating over the union of both key sets. A second pass over
paymentsBalancesfor keys absent fromledgerBalanceswould close this gap:Proposed fix — also iterate payment-only assets
for asset, ledgerBalance := range ledgerBalances { paymentBalance, ok := paymentsBalances[asset] if !ok { paymentBalance = nil } err := s.computeDrift(res, asset, ledgerBalance, paymentBalance, assertionMode, minBuffer) if err != nil { res.Status = models.ReconciliationNotOK if res.Error == "" { res.Error = err.Error() } else { res.Error = res.Error + "; " + err.Error() } } } + + // Process assets present only in paymentsBalances. + for asset, paymentBalance := range paymentsBalances { + if _, ok := ledgerBalances[asset]; ok { + continue // already handled above + } + + err := s.computeDrift(res, asset, nil, paymentBalance, assertionMode, minBuffer) + if err != nil { + res.Status = models.ReconciliationNotOK + if res.Error == "" { + res.Error = err.Error() + } else { + res.Error = res.Error + "; " + err.Error() + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/service/reconciliation.go` around lines 98 - 113, The loop only iterates ledgerBalances so assets that exist solely in paymentsBalances are skipped; update reconciliation to also process payment-only assets by iterating the union of keys (or add a second loop over paymentsBalances where the key is missing in ledgerBalances) and call s.computeDrift(res, asset, ledgerBalance, paymentBalance, assertionMode, minBuffer) with ledgerBalance set to nil (or zero value) for those payment-only assets, preserving the existing error handling that sets res.Status and appends res.Error when computeDrift returns an error.
🧹 Nitpick comments (2)
internal/api/service/reconciliation.go (2)
201-212: When bothAbsoluteandBPSare set,Absolutesilently wins.If a rule happens to have both fields populated, the
Absolutebranch returns first and theBPSvalue is never considered. If this is intentional, a comment clarifying the precedence would help. If it should be invalid, consider returning an error when both are non-nil.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/service/reconciliation.go` around lines 201 - 212, The current logic lets rule.Absolute silently take precedence over rule.BPS because the Absolute branch returns first; add an explicit check at the start of the calculation (before referencing rule.Absolute, rule.BPS, ledgerBalance) to handle the case where both are non-nil—either document the precedence with a clarifying comment or (preferred) return a clear error indicating both Absolute and BPS are set so callers must pick one; update the function that contains the shown branches to perform this validation and return the error rather than falling through.
130-143: Missing-balance cases bypass mode-specific evaluation.Lines 134–143 always return an error when one side is missing, regardless of the assertion mode. In COVERAGE mode, a ledger entry with no payment counterpart could still yield a non-negative drift (if the ledger amount is positive), which COVERAGE would consider OK. Routing these cases through
evaluateDrift(treating the missing side as zero) would make the behavior consistent across modes.If the intent is that a missing balance is always an error regardless of mode, a brief comment would help future readers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/service/reconciliation.go` around lines 130 - 143, The current switch returns an error immediately when one side is missing (ledgerBalance or paymentBalance) instead of running the same mode-aware logic; update the missing-balance branches in the reconciliation routine to call evaluateDrift (treating the missing side as zero by passing a zero big.Int) and populate res.DriftBalances with the absolute drift only after evaluateDrift returns its result, returning an error only if evaluateDrift indicates a violation for the current assertion mode (e.g., COVERAGE vs strict). Keep the existing behavior of storing the absolute drift in res.DriftBalances (use the same big.Int handling) but rely on evaluateDrift’s decision rather than unconditionally returning an error; if the design truly requires unconditional errors, add a clarifying comment above these cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/api/service/reconciliation.go`:
- Around line 98-113: The loop only iterates ledgerBalances so assets that exist
solely in paymentsBalances are skipped; update reconciliation to also process
payment-only assets by iterating the union of keys (or add a second loop over
paymentsBalances where the key is missing in ledgerBalances) and call
s.computeDrift(res, asset, ledgerBalance, paymentBalance, assertionMode,
minBuffer) with ledgerBalance set to nil (or zero value) for those payment-only
assets, preserving the existing error handling that sets res.Status and appends
res.Error when computeDrift returns an error.
---
Nitpick comments:
In `@internal/api/service/reconciliation.go`:
- Around line 201-212: The current logic lets rule.Absolute silently take
precedence over rule.BPS because the Absolute branch returns first; add an
explicit check at the start of the calculation (before referencing
rule.Absolute, rule.BPS, ledgerBalance) to handle the case where both are
non-nil—either document the precedence with a clarifying comment or (preferred)
return a clear error indicating both Absolute and BPS are set so callers must
pick one; update the function that contains the shown branches to perform this
validation and return the error rather than falling through.
- Around line 130-143: The current switch returns an error immediately when one
side is missing (ledgerBalance or paymentBalance) instead of running the same
mode-aware logic; update the missing-balance branches in the reconciliation
routine to call evaluateDrift (treating the missing side as zero by passing a
zero big.Int) and populate res.DriftBalances with the absolute drift only after
evaluateDrift returns its result, returning an error only if evaluateDrift
indicates a violation for the current assertion mode (e.g., COVERAGE vs strict).
Keep the existing behavior of storing the absolute drift in res.DriftBalances
(use the same big.Int handling) but rely on evaluateDrift’s decision rather than
unconditionally returning an error; if the design truly requires unconditional
errors, add a clarifying comment above these cases.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/api/policy.go (1)
44-53: Consider extracting atoPolicyResponsehelper to reduce repetition.The identical
models.Policy → policyResponsemapping is duplicated three times. A small helper would centralize the conversion and reduce the chance of future divergence when new fields are added.♻️ Suggested helper
+func toPolicyResponse(policy models.Policy) policyResponse { + return policyResponse{ + ID: policy.ID.String(), + Name: policy.Name, + CreatedAt: policy.CreatedAt, + LedgerName: policy.LedgerName, + LedgerQuery: policy.LedgerQuery, + PaymentsPoolID: policy.PaymentsPoolID.String(), + Mode: models.NormalizeAssertionMode(policy.AssertionMode).String(), + AssertionConfig: policy.AssertionConfig, + } +}Then each handler simply calls
toPolicyResponse(*policy)(ortoPolicyResponse(policy)in theMapCursorcallback).Also applies to: 83-92, 123-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/policy.go` around lines 44 - 53, Extract the repeated models.Policy → policyResponse mapping into a single helper function named toPolicyResponse(policy models.Policy) *policyResponse that constructs and returns a policyResponse using fields ID (policy.ID.String()), Name, CreatedAt, LedgerName, LedgerQuery, PaymentsPoolID (policy.PaymentsPoolID.String()), Mode (models.NormalizeAssertionMode(policy.AssertionMode).String()), and AssertionConfig; then replace the three duplicated mapping blocks with calls to toPolicyResponse(policy) (or toPolicyResponse(*policy) inside the MapCursor callback) to centralize the conversion and avoid divergence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/api/policy.go`:
- Around line 44-53: Extract the repeated models.Policy → policyResponse mapping
into a single helper function named toPolicyResponse(policy models.Policy)
*policyResponse that constructs and returns a policyResponse using fields ID
(policy.ID.String()), Name, CreatedAt, LedgerName, LedgerQuery, PaymentsPoolID
(policy.PaymentsPoolID.String()), Mode
(models.NormalizeAssertionMode(policy.AssertionMode).String()), and
AssertionConfig; then replace the three duplicated mapping blocks with calls to
toPolicyResponse(policy) (or toPolicyResponse(*policy) inside the MapCursor
callback) to centralize the conversion and avoid divergence.
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
1 participant
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.
Business Context
Regulated fintechs and financial institutions must regularly demonstrate that assets held in banking/payment rails are sufficient to cover liabilities represented in the ledger.
The current reconciliation flow applies a single implicit rule, which makes it harder to express different compliance policies across entities/assets (strict parity vs. coverage vs. required excess collateral).
This PR introduces explicit, policy-level modes so reconciliation results are easier to align with safeguarding and audit requirements.
What This Changes
COVERAGE,EQUALITY,MIN_BUFFERMIN_BUFFERusing anassetsrules map with UMN keys*(fallback) and strict-list behavior when*is absentmode/assertionConfigin model, migration, API responses, and OpenAPI schemaWhy It Matters
OK/NOT_OKoutcomes during controls and auditsMIN_BUFFER)Sign Convention Used in Examples
In this codebase reconciliation computes
drift = ledger + payments(per asset).For your setup:
world,pools:main) are negativeSo when fully backed, values cancel to zero.
Examples
1) Coverage policy (default / backward compatible)
{ "name": "safeguarding-main", "ledgerName": "default", "ledgerQuery": { "$or": [ {"$match": {"account": "world"}}, {"$match": {"account": "pools:main"}} ] }, "paymentsPoolID": "4f9a7f89-8fbe-41b6-bcdd-f43d39a8ad8e", "mode": "COVERAGE" }ledger + payments)drift >= 0OK2) Equality policy (strict parity)
{ "name": "daily-audit-parity", "ledgerName": "default", "ledgerQuery": { "$or": [ {"$match": {"account": "world"}}, {"$match": {"account": "pools:main"}} ] }, "paymentsPoolID": "4f9a7f89-8fbe-41b6-bcdd-f43d39a8ad8e", "mode": "EQUALITY" }ledger + payments)drift == 0NOT_OK3) Min buffer with catch-all fallback (
*){ "name": "regulated-buffer-policy", "ledgerName": "default", "ledgerQuery": { "$or": [ {"$match": {"account": "world"}}, {"$match": {"account": "pools:main"}} ] }, "paymentsPoolID": "4f9a7f89-8fbe-41b6-bcdd-f43d39a8ad8e", "mode": "MIN_BUFFER", "assertionConfig": { "assets": { "*": {"bps": 100}, "USD/2": {"absolute": 1000} } } }ledger + payments)absolute=1000(exact match)NOT_OKbps=100(fallback*)OK4) Min buffer strict list (no
*){ "mode": "MIN_BUFFER", "assertionConfig": { "assets": { "USD/2": {"bps": 80}, "EUR/2": {"absolute": 400} } } }NOT_OK)Compatibility
COVERAGEfor backward compatibilityassertion_modeandassertion_configtoreconciliations.policyTesting
gofmtappliedgo test ./...could not be run in this environment due restricted module download/network