Conversation
GAlexIHU
force-pushed
the
feat/negative-credit
branch
from
56fa779 to
5f30f96
Compare
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughRoute-level cost-basis handling was made explicit: Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CreditPurchaseHandler
participant Templates
participant Ledger
Client->>CreditPurchaseHandler: OnCreditPurchaseInitiated(amount, currency)
CreditPurchaseHandler->>CreditPurchaseHandler: outstandingAdvanceBalance(customer, currency)
alt outstanding advance > 0
CreditPurchaseHandler->>Templates: build attribution templates (advance, accrued) + issuableAmount = amount - advance
else no advance
CreditPurchaseHandler->>Templates: build issuance template for full amount
end
Templates->>Ledger: Resolve & Commit templates (per-cost-basis routes)
Ledger-->>Client: Commit result
sequenceDiagram
participant FlatFeeAdapter
participant Allocator
participant Templates
participant Ledger
FlatFeeAdapter->>Allocator: allocateCreditsToAccrued(charge, mode)
Allocator->>Templates: TransferCustomerFBOBucketToAccruedTemplate for each cost-basis
Templates->>Ledger: Commit per-cost-basis transfers
FlatFeeAdapter->>Templates: RecognizeEarningsFromAttributableAccruedTemplate
Templates->>Ledger: Debit accrued[costBasis], Credit earnings[costBasis] (loop)
Ledger-->>FlatFeeAdapter: Commit result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openmeter/ledger/chargeadapter/creditpurchase.go (1)
79-84:⚠️ Potential issue | 🟠 MajorBug: Should use
issuableAmountinstead ofcharge.Intent.CreditAmount.The code correctly calculates
issuableAmountat line 64 (reduced by advance), but then the template at line 81 still uses the originalcharge.Intent.CreditAmount. This means even when there's a partial advance, the full credit amount gets issued instead of just the excess.Compare with
OnCreditPurchaseInitiatedat line 149 which correctly usesissuableAmount.🐛 Proposed fix
inputs, err := transactions.ResolveTransactions( ctx, h.resolverDependencies(), transactions.ResolutionScope{ CustomerID: customerID, Namespace: charge.Namespace, }, transactions.IssueCustomerReceivableTemplate{ At: charge.CreatedAt, - Amount: charge.Intent.CreditAmount, + Amount: issuableAmount, Currency: charge.Intent.Currency, CostBasis: &costBasis, }, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/chargeadapter/creditpurchase.go` around lines 79 - 84, The IssueCustomerReceivableTemplate currently uses charge.Intent.CreditAmount which ignores the previously computed issuableAmount (reduced by advance); update the template to use issuableAmount instead of charge.Intent.CreditAmount so the issued receivable reflects the net credit; ensure the variable issuableAmount (from the earlier calculation) is in scope where transactions.IssueCustomerReceivableTemplate is constructed (same pattern as in OnCreditPurchaseInitiated).openmeter/ledger/routingrules/routingrules.go (1)
237-290:⚠️ Potential issue | 🟠 MajorThese translation rules don’t line up with the templates they’re validating.
Right now both rules only accept same-account moves where one side is
cost_basis=niland the other side is known. That makesTranslateCustomerFBOCostBasisTemplateinopenmeter/ledger/transactions/customer.goimpossible to commit underDefaultValidator, and it also rejects the known→known moves thatTranslateCustomerAccruedCostBasisTemplateinopenmeter/ledger/transactions/accrual.gostill allows throughValidate(). Please make the rule and template semantics match in one place, otherwise these transactions will fail at commit time withunknown_cost_basis_required.Also applies to: 332-352
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/routingrules/routingrules.go` around lines 237 - 290, The rules RequireAccruedCostBasisTranslationRule.Validate and RequireFBOCostBasisTranslationRule.Validate currently only allow same-account moves where one side has cost_basis=nil and the other known; update them to accept either (a) same-account translations where one side is known and the other nil (keep calling requireKnownToUnknownCostBasisTranslation as now) OR (b) same-account known→known cost-basis translations (allow when both entries’ cost_basis are non-nil and consistent). Concretely: in RequireAccruedCostBasisTranslationRule.Validate and RequireFBOCostBasisTranslationRule.Validate, after you split entries (negativeEntries, positiveEntries) detect whether both sides have non-nil cost_basis values and if so permit the transaction (return nil) — otherwise call requireKnownToUnknownCostBasisTranslation exactly as you do today; this will make the rules align with TranslateCustomerFBOCostBasisTemplate and TranslateCustomerAccruedCostBasisTemplate.
🧹 Nitpick comments (3)
openmeter/ledger/historical/adapter/ledger_test.go (1)
351-398: Consider adding explicit absent-vs-nil cost-basis filter cases.These tests already cover canonicalized numeric cost basis nicely. Adding cases for “cost basis absent” and “cost basis explicitly nil” would lock in the new semantics at both query and SQL expectation levels.
As per coding guidelines
**/*_test.go: Make sure the tests are comprehensive and cover the changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/historical/adapter/ledger_test.go` around lines 351 - 398, Add two explicit test cases covering (1) absent cost-basis filter and (2) cost-basis explicitly nil: update the existing tests around TestSumEntriesQuery_SQL and the earlier SumEntries usage to create queries using ledger.Query / ledger.Filters / ledger.RouteFilter where CostBasis is omitted entirely, and where CostBasis is set to an explicit nil option (e.g., mo.None or the package's nil representation). For each case assert both the generated SQL from sumEntriesQuery.SQL() and the args slice match the expected semantics (no cost_basis predicate when absent, and an explicit NULL check or parameter when cost-basis is explicitly nil) so the test suite locks in both behaviors. Reference sumEntriesQuery, ledger.Query, ledger.Filters, and RouteFilter.CostBasis when locating where to add the new assertions.openmeter/ledger/transactions/customer_test.go (1)
137-175: Please add one negative-path test for missingCostBasis.Great happy-path coverage here. It would be good to also assert that
FundCustomerAdvanceReceivableTemplatefails whenCostBasisis nil, since that contract is central to this feature.As per coding guidelines
**/*_test.go: Make sure the tests are comprehensive and cover the changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/transactions/customer_test.go` around lines 137 - 175, Add a negative-path test named like TestFundCustomerAdvanceReceivableTemplate_MissingCostBasis that constructs a new transactions test env, calls env.resolveAndCommit with a FundCustomerAdvanceReceivableTemplate whose CostBasis is nil, and assert the operation fails (use require.Error on the resolve call or assert no inputs were returned) and that balances remain unchanged by checking env.SumBalance for env.ReceivableSubAccountWithCostBasis and env.ReceivableSubAccount; reference FundCustomerAdvanceReceivableTemplate, env.resolveAndCommit, env.SumBalance, ReceivableSubAccountWithCostBasis, and ReceivableSubAccount to locate the relevant helpers.openmeter/ledger/transactions/collection.go (1)
116-142: Consider extracting common allocation logic.
collectFromPrioritizedSourcesandcollectFromBalanceSourceshave nearly identical allocation logic. If you wanted to DRY this up, you could use a common function with a balance-getter interface. That said, the duplication is pretty minimal (~20 lines), so this is totally optional.Also applies to: 194-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/transactions/collection.go` around lines 116 - 142, The two functions collectFromPrioritizedSources and collectFromBalanceSources duplicate allocation logic; extract a single allocator (e.g., allocateFromSources) that accepts a slice of a small interface or accessor functions for Balance() and SubAccount() (or two function params: getBalance(src) and getSubAccount(src)) plus the target Decimal, and reuse it from both collectFromPrioritizedSources and collectFromBalanceSources so the loop, positive checks, amount min(remaining,balance), append and remaining.Sub(amount) logic is centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/ledger/accounts.go`:
- Around line 104-109: The struct comment for CustomerAccruedRouteParams is
outdated—update it to state that routing is by Currency and an optional
CostBasis (pointer to alpacadecimal.Decimal) rather than currency-only; mention
that CostBasis is optional (nil when not used) so readers understand the pointer
semantics and routing behavior for the CustomerAccruedRouteParams type.
---
Outside diff comments:
In `@openmeter/ledger/chargeadapter/creditpurchase.go`:
- Around line 79-84: The IssueCustomerReceivableTemplate currently uses
charge.Intent.CreditAmount which ignores the previously computed issuableAmount
(reduced by advance); update the template to use issuableAmount instead of
charge.Intent.CreditAmount so the issued receivable reflects the net credit;
ensure the variable issuableAmount (from the earlier calculation) is in scope
where transactions.IssueCustomerReceivableTemplate is constructed (same pattern
as in OnCreditPurchaseInitiated).
In `@openmeter/ledger/routingrules/routingrules.go`:
- Around line 237-290: The rules RequireAccruedCostBasisTranslationRule.Validate
and RequireFBOCostBasisTranslationRule.Validate currently only allow
same-account moves where one side has cost_basis=nil and the other known; update
them to accept either (a) same-account translations where one side is known and
the other nil (keep calling requireKnownToUnknownCostBasisTranslation as now) OR
(b) same-account known→known cost-basis translations (allow when both entries’
cost_basis are non-nil and consistent). Concretely: in
RequireAccruedCostBasisTranslationRule.Validate and
RequireFBOCostBasisTranslationRule.Validate, after you split entries
(negativeEntries, positiveEntries) detect whether both sides have non-nil
cost_basis values and if so permit the transaction (return nil) — otherwise call
requireKnownToUnknownCostBasisTranslation exactly as you do today; this will
make the rules align with TranslateCustomerFBOCostBasisTemplate and
TranslateCustomerAccruedCostBasisTemplate.
---
Nitpick comments:
In `@openmeter/ledger/historical/adapter/ledger_test.go`:
- Around line 351-398: Add two explicit test cases covering (1) absent
cost-basis filter and (2) cost-basis explicitly nil: update the existing tests
around TestSumEntriesQuery_SQL and the earlier SumEntries usage to create
queries using ledger.Query / ledger.Filters / ledger.RouteFilter where CostBasis
is omitted entirely, and where CostBasis is set to an explicit nil option (e.g.,
mo.None or the package's nil representation). For each case assert both the
generated SQL from sumEntriesQuery.SQL() and the args slice match the expected
semantics (no cost_basis predicate when absent, and an explicit NULL check or
parameter when cost-basis is explicitly nil) so the test suite locks in both
behaviors. Reference sumEntriesQuery, ledger.Query, ledger.Filters, and
RouteFilter.CostBasis when locating where to add the new assertions.
In `@openmeter/ledger/transactions/collection.go`:
- Around line 116-142: The two functions collectFromPrioritizedSources and
collectFromBalanceSources duplicate allocation logic; extract a single allocator
(e.g., allocateFromSources) that accepts a slice of a small interface or
accessor functions for Balance() and SubAccount() (or two function params:
getBalance(src) and getSubAccount(src)) plus the target Decimal, and reuse it
from both collectFromPrioritizedSources and collectFromBalanceSources so the
loop, positive checks, amount min(remaining,balance), append and
remaining.Sub(amount) logic is centralized.
In `@openmeter/ledger/transactions/customer_test.go`:
- Around line 137-175: Add a negative-path test named like
TestFundCustomerAdvanceReceivableTemplate_MissingCostBasis that constructs a new
transactions test env, calls env.resolveAndCommit with a
FundCustomerAdvanceReceivableTemplate whose CostBasis is nil, and assert the
operation fails (use require.Error on the resolve call or assert no inputs were
returned) and that balances remain unchanged by checking env.SumBalance for
env.ReceivableSubAccountWithCostBasis and env.ReceivableSubAccount; reference
FundCustomerAdvanceReceivableTemplate, env.resolveAndCommit, env.SumBalance,
ReceivableSubAccountWithCostBasis, and ReceivableSubAccount to locate the
relevant helpers.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 97479a08-5fc3-462d-9b7d-0bb6505778e4
📒 Files selected for processing (23)
openmeter/ledger/account/adapter/repo_test.goopenmeter/ledger/account/adapter/subaccount.goopenmeter/ledger/accounts.goopenmeter/ledger/chargeadapter/creditpurchase.goopenmeter/ledger/chargeadapter/creditpurchase_test.goopenmeter/ledger/chargeadapter/flatfee.goopenmeter/ledger/chargeadapter/flatfee_test.goopenmeter/ledger/historical/adapter/ledger_test.goopenmeter/ledger/historical/adapter/sumentries_query.goopenmeter/ledger/primitives.goopenmeter/ledger/routing.goopenmeter/ledger/routingrules/defaults.goopenmeter/ledger/routingrules/routingrules.goopenmeter/ledger/testutils/integration.goopenmeter/ledger/transactions/accrual.goopenmeter/ledger/transactions/accrual_test.goopenmeter/ledger/transactions/collection.goopenmeter/ledger/transactions/customer.goopenmeter/ledger/transactions/customer_test.goopenmeter/ledger/transactions/earnings.goopenmeter/ledger/transactions/earnings_test.goopenmeter/ledger/transactions/testenv_test.gotest/credits/sanity_test.go
Comment on lines
104
to
109
| // CustomerAccruedRouteParams are routing parameters specific to customer accrued sub-accounts. | ||
| // Routed by currency only for now. | ||
| type CustomerAccruedRouteParams struct { | ||
| Currency currencyx.Code | ||
| Currency currencyx.Code | ||
| CostBasis *alpacadecimal.Decimal | ||
| } |
Contributor
There was a problem hiding this comment.
Comment is stale after adding CostBasis routing.
The struct now routes by currency and optional cost basis, but the comment still says currency-only.
Suggested doc fix
-// CustomerAccruedRouteParams are routing parameters specific to customer accrued sub-accounts.
-// Routed by currency only for now.
+// CustomerAccruedRouteParams are routing parameters specific to customer accrued sub-accounts.
+// Routed by currency and optional cost basis.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // CustomerAccruedRouteParams are routing parameters specific to customer accrued sub-accounts. | |
| // Routed by currency only for now. | |
| type CustomerAccruedRouteParams struct { | |
| Currency currencyx.Code | |
| Currency currencyx.Code | |
| CostBasis *alpacadecimal.Decimal | |
| } | |
| // CustomerAccruedRouteParams are routing parameters specific to customer accrued sub-accounts. | |
| // Routed by currency and optional cost basis. | |
| type CustomerAccruedRouteParams struct { | |
| Currency currencyx.Code | |
| CostBasis *alpacadecimal.Decimal | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/ledger/accounts.go` around lines 104 - 109, The struct comment for
CustomerAccruedRouteParams is outdated—update it to state that routing is by
Currency and an optional CostBasis (pointer to alpacadecimal.Decimal) rather
than currency-only; mention that CostBasis is optional (nil when not used) so
readers understand the pointer semantics and routing behavior for the
CustomerAccruedRouteParams type.
GAlexIHU
force-pushed
the
feat/negative-credit
branch
from
5f30f96 to
79448ba
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
openmeter/ledger/chargeadapter/flatfee_test.go (1)
710-713: MirrorsinvoiceCostBasis()from the main code.This test helper duplicates the logic from
flatfee.go. While acceptable for test isolation, if the value changes in one place, it needs to change in both.Consider exporting the constant or function to avoid drift, or add a comment noting the dependency:
func testInvoiceCostBasis() *alpacadecimal.Decimal { + // Must match invoiceCostBasis() in flatfee.go value := alpacadecimal.NewFromInt(1) return &value }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/chargeadapter/flatfee_test.go` around lines 710 - 713, The test helper testInvoiceCostBasis duplicates logic from invoiceCostBasis in flatfee.go which can drift; either export and reuse the shared constant/function from flatfee (e.g., make invoiceCostBasis or a named constant public and call it from testInvoiceCostBasis) or add a clear TODO/comment above testInvoiceCostBasis documenting it must mirror invoiceCostBasis and reference that function so future changes stay synced; update references to use the exported symbol (or add the comment) and remove the duplicated construction in testInvoiceCostBasis.openmeter/ledger/chargeadapter/flatfee.go (1)
414-417: Consider documenting why invoice cost basis is1.The magic number
1for invoice cost basis might benefit from a brief comment explaining the semantic meaning (e.g., "// Invoice-backed transactions use cost basis 1 to distinguish from credit-backed (0)").📝 Suggested documentation
func invoiceCostBasis() *alpacadecimal.Decimal { + // Invoice-backed transactions use cost basis 1 to distinguish from credit-backed (0). value := alpacadecimal.NewFromInt(1) return &value }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/chargeadapter/flatfee.go` around lines 414 - 417, Add a short explanatory comment to invoiceCostBasis explaining why it returns alpacadecimal.NewFromInt(1) (e.g., that invoice-backed transactions use cost basis 1 to distinguish from credit-backed 0), and place the comment directly above the invoiceCostBasis function so future readers see the semantic meaning; reference the function invoiceCostBasis and the call to alpacadecimal.NewFromInt(1) when updating the doc comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openmeter/ledger/chargeadapter/flatfee_test.go`:
- Around line 710-713: The test helper testInvoiceCostBasis duplicates logic
from invoiceCostBasis in flatfee.go which can drift; either export and reuse the
shared constant/function from flatfee (e.g., make invoiceCostBasis or a named
constant public and call it from testInvoiceCostBasis) or add a clear
TODO/comment above testInvoiceCostBasis documenting it must mirror
invoiceCostBasis and reference that function so future changes stay synced;
update references to use the exported symbol (or add the comment) and remove the
duplicated construction in testInvoiceCostBasis.
In `@openmeter/ledger/chargeadapter/flatfee.go`:
- Around line 414-417: Add a short explanatory comment to invoiceCostBasis
explaining why it returns alpacadecimal.NewFromInt(1) (e.g., that invoice-backed
transactions use cost basis 1 to distinguish from credit-backed 0), and place
the comment directly above the invoiceCostBasis function so future readers see
the semantic meaning; reference the function invoiceCostBasis and the call to
alpacadecimal.NewFromInt(1) when updating the doc comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 236065fe-938b-433b-99f1-a1f3f81e4996
📒 Files selected for processing (23)
openmeter/ledger/account/adapter/repo_test.goopenmeter/ledger/account/adapter/subaccount.goopenmeter/ledger/accounts.goopenmeter/ledger/chargeadapter/creditpurchase.goopenmeter/ledger/chargeadapter/creditpurchase_test.goopenmeter/ledger/chargeadapter/flatfee.goopenmeter/ledger/chargeadapter/flatfee_test.goopenmeter/ledger/historical/adapter/ledger_test.goopenmeter/ledger/historical/adapter/sumentries_query.goopenmeter/ledger/primitives.goopenmeter/ledger/routing.goopenmeter/ledger/routingrules/defaults.goopenmeter/ledger/routingrules/routingrules.goopenmeter/ledger/testutils/integration.goopenmeter/ledger/transactions/accrual.goopenmeter/ledger/transactions/accrual_test.goopenmeter/ledger/transactions/collection.goopenmeter/ledger/transactions/customer.goopenmeter/ledger/transactions/customer_test.goopenmeter/ledger/transactions/earnings.goopenmeter/ledger/transactions/earnings_test.goopenmeter/ledger/transactions/testenv_test.gotest/credits/sanity_test.go
✅ Files skipped from review due to trivial changes (5)
- openmeter/ledger/account/adapter/repo_test.go
- openmeter/ledger/transactions/customer_test.go
- openmeter/ledger/transactions/earnings.go
- openmeter/ledger/chargeadapter/creditpurchase.go
- openmeter/ledger/transactions/customer.go
🚧 Files skipped from review as they are similar to previous changes (13)
- openmeter/ledger/historical/adapter/sumentries_query.go
- openmeter/ledger/primitives.go
- openmeter/ledger/historical/adapter/ledger_test.go
- openmeter/ledger/routing.go
- openmeter/ledger/accounts.go
- openmeter/ledger/transactions/earnings_test.go
- openmeter/ledger/transactions/accrual_test.go
- openmeter/ledger/routingrules/defaults.go
- openmeter/ledger/transactions/collection.go
- openmeter/ledger/chargeadapter/creditpurchase_test.go
- openmeter/ledger/testutils/integration.go
- test/credits/sanity_test.go
- openmeter/ledger/routingrules/routingrules.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/credits/sanity_test.go (1)
1003-1038:⚠️ Potential issue | 🟠 MajorAdd a test case for
mo.Some(nil)cost-basis routing to ensure the query predicates don't regress.Your concern is spot-on. The helpers
mustCustomerAccruedBalanceWithCostBasisandmustCustomerReceivableRouteBalancecallGetSubAccountForRoute, which accepts the raw cost-basis pointer directly and bypasses therouteFilterCostBasisconverter. Meanwhile,mustCustomerReceivableBalanceandmustCustomerAuthorizedReceivableBalanceuseGetBalancewith aRouteFilterthat goes throughrouteFilterCostBasis(nil)→mo.None().The issue: in the ledger's query normalization and predicate building,
mo.Some(nil)andmo.None()generate different SQL:
mo.Some(nil)→CostBasisIsNil()predicate (explicit match for null cost basis)mo.None()→ no predicate at all (matches any cost basis)The test suite currently never exercises the
mo.Some(nil)path. Add a dedicated helper that explicitly constructsRouteFilter{CostBasis: mo.Some(nil)}and use it for at least one accrued or receivable assertion to cover this branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/credits/sanity_test.go` around lines 1003 - 1038, The current helpers mustCustomerAccruedBalanceWithCostBasis and mustCustomerReceivableRouteBalance call GetSubAccountForRoute with the raw cost-basis pointer and never exercise the mo.Some(nil) path produced by routeFilterCostBasis, so add a new helper (e.g., mustCustomerAccruedBalanceWithSomeNilCostBasis) that builds a RouteFilter with CostBasis: mo.Some(nil) (not mo.None()) and uses customerAccounts.AccruedAccount.GetBalance/GetSubAccountForRoute or the existing mustCustomerReceivableRouteBalance flow to assert a balance so at least one test exercises the mo.Some(nil) predicate branch; update one existing test to use this new helper to cover the routeFilterCostBasis(nil) → mo.Some(nil) SQL path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/credits/sanity_test.go`:
- Around line 677-695: Add explicit assertions immediately after the advance
that verify the promo vs purchased accrued buckets were recorded under the
correct cost bases: compare flatFeeStart.promoAccrued to
s.mustCustomerAccruedBalanceWithCostBasis(cust.GetID(), USD, &promoCostBasis),
and compare flatFeeStart.externalAccrued to
s.mustCustomerAccruedBalanceWithCostBasis(cust.GetID(), USD,
&externalCostBasis); also assert the nil/unattributed accrued route
(s.mustCustomerAccruedBalanceWithCostBasis(cust.GetID(), USD, nil)) reflects the
expected remaining accrued (e.g., equals flatFeeStart.accrued minus
promo+external) so a swap between promo and purchased buckets would fail.
---
Outside diff comments:
In `@test/credits/sanity_test.go`:
- Around line 1003-1038: The current helpers
mustCustomerAccruedBalanceWithCostBasis and mustCustomerReceivableRouteBalance
call GetSubAccountForRoute with the raw cost-basis pointer and never exercise
the mo.Some(nil) path produced by routeFilterCostBasis, so add a new helper
(e.g., mustCustomerAccruedBalanceWithSomeNilCostBasis) that builds a RouteFilter
with CostBasis: mo.Some(nil) (not mo.None()) and uses
customerAccounts.AccruedAccount.GetBalance/GetSubAccountForRoute or the existing
mustCustomerReceivableRouteBalance flow to assert a balance so at least one test
exercises the mo.Some(nil) predicate branch; update one existing test to use
this new helper to cover the routeFilterCostBasis(nil) → mo.Some(nil) SQL path.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 913f237d-c290-4155-b6d5-789e67b7ff15
📒 Files selected for processing (1)
test/credits/sanity_test.go
Comment on lines
+677
to
+695
| assertDelta("promo FBO after credit-only advance", flatFeeStart.promoFBO, alpacadecimal.NewFromInt(-30), s.mustCustomerFBOBalance(cust.GetID(), USD, &promoCostBasis)) | ||
| assertDelta("external FBO after credit-only advance", flatFeeStart.externalFBO, alpacadecimal.NewFromInt(-50), s.mustCustomerFBOBalance(cust.GetID(), USD, &externalCostBasis)) | ||
| assertDelta("unknown FBO after credit-only advance", flatFeeStart.unknownFBO, alpacadecimal.Zero, s.mustCustomerFBOBalance(cust.GetID(), USD, nil)) | ||
| assertDelta("promo receivable after credit-only advance", flatFeeStart.promoReceivable, alpacadecimal.Zero, s.mustCustomerReceivableBalance(cust.GetID(), USD, &promoCostBasis)) | ||
| assertDelta("external receivable after credit-only advance", flatFeeStart.externalReceivable, alpacadecimal.Zero, s.mustCustomerReceivableBalance(cust.GetID(), USD, &externalCostBasis)) | ||
| assertDelta("total open receivable after credit-only advance", flatFeeStart.totalOpenReceivable, alpacadecimal.NewFromInt(-20), s.mustCustomerReceivableBalance(cust.GetID(), USD, nil)) | ||
| assertDelta("authorized receivable after credit-only advance", flatFeeStart.authorizedReceivable, alpacadecimal.Zero, s.mustCustomerAuthorizedReceivableBalance(cust.GetID(), USD, nil)) | ||
| s.True( | ||
| s.mustCustomerReceivableRouteBalance(cust.GetID(), USD, nil, ledger.TransactionAuthorizationStatusOpen).Equal(alpacadecimal.NewFromInt(-20)), | ||
| "the uncovered credit_only shortfall should live in the exact open advance receivable route", | ||
| ) | ||
| s.True( | ||
| s.mustCustomerAccruedBalanceWithCostBasis(cust.GetID(), USD, nil).Equal(alpacadecimal.NewFromInt(20)), | ||
| "the uncovered shortfall should also remain in unattributed accrued until a later purchase backfills it", | ||
| ) | ||
| assertDelta("accrued after credit-only advance", flatFeeStart.accrued, alpacadecimal.NewFromInt(100), s.mustCustomerAccruedBalance(cust.GetID(), USD)) | ||
| assertDelta("total wash after credit-only advance", flatFeeStart.totalWash, alpacadecimal.Zero, s.mustWashBalance(ns, USD, nil)) | ||
| assertDelta("external wash after credit-only advance", flatFeeStart.externalWash, alpacadecimal.Zero, s.mustWashBalance(ns, USD, &externalCostBasis)) | ||
| assertDelta("earnings after credit-only advance", flatFeeStart.earnings, alpacadecimal.Zero, s.mustEarningsBalance(ns, USD)) |
Contributor
There was a problem hiding this comment.
Assert the promo vs purchased accrued buckets right after advance.
This block checks the nil-cost-basis shortfall, but it never proves that the original 30 promo credits and 50 purchased credits were accrued under the right non-nil routes. A swap between those two buckets would still pass, because start.externalAccrued is captured later from whatever state advance produced.
💡 Suggested assertions
+ s.True(
+ s.mustCustomerAccruedBalanceWithCostBasis(cust.GetID(), USD, &promoCostBasis).Equal(alpacadecimal.NewFromInt(30)),
+ "promotional credits should stay in the zero cost-basis accrued bucket",
+ )
+ s.True(
+ s.mustCustomerAccruedBalanceWithCostBasis(cust.GetID(), USD, &externalCostBasis).Equal(alpacadecimal.NewFromInt(50)),
+ "purchased credits should stay in the purchased cost-basis accrued bucket",
+ )
s.True(
s.mustCustomerAccruedBalanceWithCostBasis(cust.GetID(), USD, nil).Equal(alpacadecimal.NewFromInt(20)),
"the uncovered shortfall should also remain in unattributed accrued until a later purchase backfills it",
)As per coding guidelines, **/*_test.go: Make sure the tests are comprehensive and cover the changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/credits/sanity_test.go` around lines 677 - 695, Add explicit assertions
immediately after the advance that verify the promo vs purchased accrued buckets
were recorded under the correct cost bases: compare flatFeeStart.promoAccrued to
s.mustCustomerAccruedBalanceWithCostBasis(cust.GetID(), USD, &promoCostBasis),
and compare flatFeeStart.externalAccrued to
s.mustCustomerAccruedBalanceWithCostBasis(cust.GetID(), USD,
&externalCostBasis); also assert the nil/unattributed accrued route
(s.mustCustomerAccruedBalanceWithCostBasis(cust.GetID(), USD, nil)) reflects the
expected remaining accrued (e.g., equals flatFeeStart.accrued minus
promo+external) so a swap between promo and purchased buckets would fail.
turip
reviewed
Mar 31, 2026
|
|
||
| // TranslateCustomerFBOCostBasisTemplate moves customer FBO value between cost-basis buckets | ||
| // while keeping the same priority bucket. | ||
| type TranslateCustomerFBOCostBasisTemplate struct { |
Contributor
Author
There was a problem hiding this comment.
thanks, will clean up in a sec
turip
previously approved these changes
Mar 31, 2026
turip
approved these changes
Mar 31, 2026
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
2 participants
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.
Overview
Credit advance (negative credit) & credit_only scenarios:
also
Notes for reviewer
Summary by CodeRabbit
New Features
Improvements
Tests