feat(creditpurchase): Add statemachine to externally-settled creaditp…#4471
Open
mark-vass-konghq wants to merge 1 commit into
Open
feat(creditpurchase): Add statemachine to externally-settled creaditp…#4471mark-vass-konghq wants to merge 1 commit into
mark-vass-konghq wants to merge 1 commit into
Conversation
mark-vass-konghq
added
release-note/feature
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughExtracts external credit-purchase realizations into a realizations service, adds an ExternalCreditPurchaseStateMachine to orchestrate initiate→authorize→settle transitions, enforces strictly positive settlement cost basis, refactors handlers to delegate to the state machine, and adds comprehensive tests covering flows and validation. ChangesExternal Credit Purchase Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/billing/charges/creditpurchase/service/statemachine.go (1)
35-42: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse the repo’s aggregated validation pattern here.
This
Validate()still bails on the first failure, so callers miss combined config errors and don’t get the standard nillable validation shape. SinceStateMachineConfigjust picked up another dependency, it’d be good to switch this over now. As per coding guidelines, "ForValidate() errormethods in Go, collect all validation issues intovar errs []errorand returnmodels.NewNillableGenericValidationError(errors.Join(errs...))instead of returning on the first invalid field".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/charges/creditpurchase/service/statemachine.go` around lines 35 - 42, StateMachineConfig.Validate currently returns on the first error; change it to collect all field validation issues into a slice (e.g., var errs []error) checking Charge.ID and Adapter (and any newly added deps) and append errors for each invalid field, then return models.NewNillableGenericValidationError(errors.Join(errs...)) if len(errs)>0, otherwise return nil; update the function name StateMachineConfig.Validate and ensure you import and use errors.Join and models.NewNillableGenericValidationError to match the repo's aggregated validation pattern.
🧹 Nitpick comments (1)
openmeter/billing/charges/creditpurchase/service/external_test.go (1)
360-389: ⚡ Quick winHeads up: this setup is a near-clone of the previous test. 🪞
The charge fixture (lines 367-371) and the
externalStateMachineHandlercallbacks (lines 374-387) are byte-for-byte identical toTestExternalCreditPurchaseStateMachineAuthorizesAndSettlesPayment(lines 307-311 and 314-327). Only the trigger flow differs (sequential authorize→settle vs. the singleauthorized-and-settledtransition). Pulling the shared charge + handler into a tiny builder would keep the two tests focused on just the behavior that differs.♻️ Sketch of a shared builder
func newAuthorizeSettleFixture(t *testing.T) (creditpurchase.Charge, *externalStateMachineHandler) { t.Helper() charge := newExternalStateMachineTestCharge(creditpurchase.StatusActive, alpacadecimal.NewFromFloat(0.5)) charge.Realizations.CreditGrantRealization = &ledgertransaction.TimedGroupReference{ GroupReference: ledgertransaction.GroupReference{TransactionGroupID: "initiated-ledger-tx"}, Time: time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC), } handler := &externalStateMachineHandler{ onCreditPurchasePaymentAuthorized: func(ctx context.Context, input creditpurchase.PaymentEventInput) (ledgertransaction.GroupReference, error) { require.Equal(t, creditpurchase.StatusActive, input.Charge.Status) require.Nil(t, input.Charge.Realizations.ExternalPaymentSettlement) return ledgertransaction.GroupReference{TransactionGroupID: "authorized-ledger-tx"}, nil }, onCreditPurchasePaymentSettled: func(ctx context.Context, input creditpurchase.PaymentEventInput) (ledgertransaction.GroupReference, error) { require.Equal(t, creditpurchase.StatusActive, input.Charge.Status) require.NotNil(t, input.Charge.Realizations.ExternalPaymentSettlement) require.Equal(t, payment.StatusAuthorized, input.Charge.Realizations.ExternalPaymentSettlement.Status) require.Equal(t, "authorized-ledger-tx", input.Charge.Realizations.ExternalPaymentSettlement.Authorized.TransactionGroupID) return ledgertransaction.GroupReference{TransactionGroupID: "settled-ledger-tx"}, nil }, } return charge, handler }As per coding guidelines: "Avoid redundant test helpers and duplicate setup paths; parameterize helpers over maintaining near-identical ones."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/charges/creditpurchase/service/external_test.go` around lines 360 - 389, The two tests TestExternalCreditPurchaseStateMachineAuthorizesAndSettlesInSingleTransition and TestExternalCreditPurchaseStateMachineAuthorizesAndSettlesPayment duplicate the same charge setup and externalStateMachineHandler callbacks; extract that shared setup into a small helper (e.g. newAuthorizeSettleFixture) that returns the creditpurchase.Charge and *externalStateMachineHandler using the existing newExternalStateMachineTestCharge, externalStateMachineHandler (with onCreditPurchasePaymentAuthorized and onCreditPurchasePaymentSettled closures), then update both tests to call the helper and reuse lineageService/realizations creation (externalStateMachineLineage, newExternalStateMachineRealizations) so each test only sets the differing trigger flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openmeter/billing/charges/creditpurchase/service/external.go`:
- Around line 122-138: In AuthorizeExternalPayment on
ExternalCreditPurchaseStateMachine the branch that checks
s.Charge.Realizations.ExternalPaymentSettlement != nil calls
s.Realizations.AuthorizeExternalPayment but discards the returned charge,
leaving s.Charge stale; change that branch to capture the returned charge (from
Realizations.AuthorizeExternalPayment(ctx, s.Charge)), handle the error as
currently done, and assign the returned charge to s.Charge before returning so
the state machine retains the updated charge (refer to AuthorizeExternalPayment,
s.Charge, Realizations.AuthorizeExternalPayment, ExternalPaymentSettlement).
- Around line 44-58: The switch currently groups
creditpurchase.SettledInitialPaymentSettlementStatus with AuthorizedAndSettled
and fires triggerExternalPaymentAuthorizedAndSettled, which can replay
authorization when only settlement is requested; split those cases so
SettledInitialPaymentSettlementStatus calls stateMachine.FireAndActivate(ctx,
triggerExternalPaymentSettled) (the settle-only trigger) and keep
creditpurchase.AuthorizedAndSettledInitialPaymentSettlementStatus using
stateMachine.FireAndActivate(ctx, triggerExternalPaymentAuthorizedAndSettled),
leaving the Created and Authorized cases unchanged.
In `@openmeter/billing/charges/creditpurchase/service/realizations/service.go`:
- Around line 30-46: The Config.Validate function currently aggregates errors
into errs but returns errors.Join(errs...), so callers won't receive a
models.GenericValidationError; change the return to wrap the joined errors with
models.NewNillableGenericValidationError by returning
models.NewNillableGenericValidationError(errors.Join(errs...)) in the
Config.Validate method (referencing Config.Validate and
models.NewNillableGenericValidationError to locate and update the return).
---
Outside diff comments:
In `@openmeter/billing/charges/creditpurchase/service/statemachine.go`:
- Around line 35-42: StateMachineConfig.Validate currently returns on the first
error; change it to collect all field validation issues into a slice (e.g., var
errs []error) checking Charge.ID and Adapter (and any newly added deps) and
append errors for each invalid field, then return
models.NewNillableGenericValidationError(errors.Join(errs...)) if len(errs)>0,
otherwise return nil; update the function name StateMachineConfig.Validate and
ensure you import and use errors.Join and
models.NewNillableGenericValidationError to match the repo's aggregated
validation pattern.
---
Nitpick comments:
In `@openmeter/billing/charges/creditpurchase/service/external_test.go`:
- Around line 360-389: The two tests
TestExternalCreditPurchaseStateMachineAuthorizesAndSettlesInSingleTransition and
TestExternalCreditPurchaseStateMachineAuthorizesAndSettlesPayment duplicate the
same charge setup and externalStateMachineHandler callbacks; extract that shared
setup into a small helper (e.g. newAuthorizeSettleFixture) that returns the
creditpurchase.Charge and *externalStateMachineHandler using the existing
newExternalStateMachineTestCharge, externalStateMachineHandler (with
onCreditPurchasePaymentAuthorized and onCreditPurchasePaymentSettled closures),
then update both tests to call the helper and reuse lineageService/realizations
creation (externalStateMachineLineage, newExternalStateMachineRealizations) so
each test only sets the differing trigger flow.
🪄 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: 13e0a41c-0fcc-49f4-9126-ce5497ebe586
📒 Files selected for processing (8)
openmeter/billing/charges/creditpurchase/service/external.goopenmeter/billing/charges/creditpurchase/service/external_test.goopenmeter/billing/charges/creditpurchase/service/realizations/service.goopenmeter/billing/charges/creditpurchase/service/service.goopenmeter/billing/charges/creditpurchase/service/statemachine.goopenmeter/billing/charges/creditpurchase/settlement.goopenmeter/billing/charges/creditpurchase/settlement_test.goopenmeter/billing/charges/service/creditpurchase_test.go
mark-vass-konghq
force-pushed
the
feat/add-statemachine-to-external-settled-creditpurchase
branch
from
690b5c8 to
636dfda
Compare
turip
reviewed
Jun 2, 2026
| case creditpurchase.CreatedInitialPaymentSettlementStatus: | ||
| return charge, nil | ||
| case creditpurchase.AuthorizedInitialPaymentSettlementStatus: | ||
| if err := stateMachine.AuthorizeExternalPayment(ctx); err != nil { |
Comment on lines
+15
to
+16
| triggerExternalPaymentSettled meta.Trigger = "external_payment_settled" | ||
| triggerExternalPaymentAuthorizedAndSettled meta.Trigger = "external_payment_authorized_and_settled" |
Member
There was a problem hiding this comment.
let's instead use what's in billing:
// TriggerPaid is used to signify that the invoice has been paid
TriggerPaid InvoiceTrigger = "trigger_paid"
// TriggerAuthorized is used to signify that the invoice has an authorized payment method/funds hold
TriggerAuthorized InvoiceTrigger = "trigger_authorized"
I don't think it makes sense to differentiate between external settled and invoiced triggers as they will have seperate state machines.
Comment on lines
+97
to
+100
| s.Configure(creditpurchase.StatusActive). | ||
| OnEntryFrom(meta.TriggerNext, statelessx.EntryFunc(s.InitiateExternalCreditPurchaseFromCreated)). | ||
| Permit(triggerExternalPaymentSettled, creditpurchase.StatusFinal). | ||
| Permit(triggerExternalPaymentAuthorizedAndSettled, creditpurchase.StatusFinal) |
Member
There was a problem hiding this comment.
Let's have substates for active.payment.authorized, active.payment.pending
turip
reviewed
Jun 2, 2026
| // - the external state machine advances until stable | ||
| // then: | ||
| // - it grants credits, backfills lineage, and persists the active status | ||
| stateMachine, charge, adapter, lineageService := newExternalStateMachineTestMachine( |
Contributor
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)
openmeter/billing/charges/creditpurchase/service/external.go (1)
21-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate the requested initial status before advancing the machine.
Line 32 can already initiate the external purchase before Line 40 rejects an unsupported
InitialStatus. If invalid data slips through, this returns an error after the side effect has happened. Please resolveexternalInitialPaymentTrigger(...)first and fail fast beforeAdvanceUntilStateStable(ctx).💡 Suggested change
targetStatus := externalCreditPurchaseSettlement.InitialStatus + + trigger, hasTrigger, err := externalInitialPaymentTrigger(targetStatus) + if err != nil { + return creditpurchase.Charge{}, err + } stateMachine, err := NewExternalCreditPurchaseStateMachine(StateMachineConfig{ Charge: charge, Adapter: s.adapter, Realizations: s.realizations, @@ if advancedCharge != nil { charge = *advancedCharge } - - trigger, hasTrigger, err := externalInitialPaymentTrigger(targetStatus) - if err != nil { - return creditpurchase.Charge{}, err - } if !hasTrigger { return charge, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/charges/creditpurchase/service/external.go` around lines 21 - 50, Validate the requested initial status (call externalInitialPaymentTrigger(targetStatus) and handle its error/hasTrigger result) before advancing the state machine so we fail fast on unsupported InitialStatus; specifically, move the externalInitialPaymentTrigger call (and the early return when !hasTrigger or error) to occur prior to calling stateMachine.AdvanceUntilStateStable(ctx) (or even before NewExternalCreditPurchaseStateMachine if preferred), then only proceed to AdvanceUntilStateStable and stateMachine.FireAndActivate when the trigger is valid.
🧹 Nitpick comments (1)
openmeter/billing/charges/creditpurchase/service/external_test.go (1)
61-72: 💤 Low valueTiny optional thought on the float assertions.
Since you've already rounded
100.123→100.12, the equality checks like Line 72 could read with a plain literal (require.Equal(t, 100.12, charge.Intent.CreditAmount.InexactFloat64())). That said,expectedAmountis genuinely reused in the lineage matcher on Line 91 for.Equal(...), so keeping it as a decimal here is perfectly defensible — totally your call.As per coding guidelines: "When asserting
alpacadecimal.Decimalequality in tests, preferrequire.Equal(t, expectedFloat64, actual.InexactFloat64())... use simplefloat64(5)-style literals over verbose decimal construction".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/charges/creditpurchase/service/external_test.go` around lines 61 - 72, The test uses an alpacadecimal.Decimal variable expectedAmount for asserting the InexactFloat64 value, but per guidelines prefer a plain float64 literal when asserting Decimal.InexactFloat64(); update the assertion that compares charge.Intent.CreditAmount.InexactFloat64() to use a literal (e.g., 100.12) instead of expectedAmount.InexactFloat64(), leaving expectedAmount in place for its later use in the lineage matcher; modify the assertion around require.Equal(...) that references expectedAmount and charge.Intent.CreditAmount.InexactFloat64() accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@openmeter/billing/charges/creditpurchase/service/external.go`:
- Around line 21-50: Validate the requested initial status (call
externalInitialPaymentTrigger(targetStatus) and handle its error/hasTrigger
result) before advancing the state machine so we fail fast on unsupported
InitialStatus; specifically, move the externalInitialPaymentTrigger call (and
the early return when !hasTrigger or error) to occur prior to calling
stateMachine.AdvanceUntilStateStable(ctx) (or even before
NewExternalCreditPurchaseStateMachine if preferred), then only proceed to
AdvanceUntilStateStable and stateMachine.FireAndActivate when the trigger is
valid.
---
Nitpick comments:
In `@openmeter/billing/charges/creditpurchase/service/external_test.go`:
- Around line 61-72: The test uses an alpacadecimal.Decimal variable
expectedAmount for asserting the InexactFloat64 value, but per guidelines prefer
a plain float64 literal when asserting Decimal.InexactFloat64(); update the
assertion that compares charge.Intent.CreditAmount.InexactFloat64() to use a
literal (e.g., 100.12) instead of expectedAmount.InexactFloat64(), leaving
expectedAmount in place for its later use in the lineage matcher; modify the
assertion around require.Equal(...) that references expectedAmount and
charge.Intent.CreditAmount.InexactFloat64() accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5487f99e-3031-4fca-8b01-949a03561e1d
⛔ Files ignored due to path filters (2)
openmeter/ent/db/chargecreditpurchase/chargecreditpurchase.gois excluded by!**/ent/db/**openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**
📒 Files selected for processing (5)
openmeter/billing/charges/creditpurchase/service/external.goopenmeter/billing/charges/creditpurchase/service/external_test.goopenmeter/billing/charges/creditpurchase/statemachine.goopenmeter/billing/charges/service/creditpurchase_test.gotest/credits/creditgrant_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- openmeter/billing/charges/service/creditpurchase_test.go
turip
reviewed
Jun 2, 2026
|
|
||
| return charge, nil | ||
| }) | ||
| func externalInitialPaymentTrigger(status creditpurchase.InitialPaymentSettlementStatus) (meta.Trigger, bool, error) { |
Member
There was a problem hiding this comment.
meta.Trigger, error signature is enough, let's treat empty trigger (== "") as no trigger available.
Comment on lines
+137
to
+141
| switch s.Charge.Status { | ||
| case creditpurchase.StatusActive, creditpurchase.StatusActivePaymentPending, creditpurchase.StatusActivePaymentAuthorized: | ||
| default: | ||
| return fmt.Errorf("%w: authorize external payment [status=%s,id=%s]", chargestatemachine.ErrUnsupportedOperation, s.Charge.Status, s.Charge.ID) | ||
| } |
Member
There was a problem hiding this comment.
why do we need this?
the realization must error if we have already authorized the payment, right?
| } | ||
| currentBase := s.Charge.ChargeBase | ||
| eventCharge := s.Charge | ||
| eventCharge.Status = creditpurchase.StatusActivePaymentPending |
Member
There was a problem hiding this comment.
state machine should not be manipulated here, it's stateless' responsibility, why do we need this status change?
| func (s *ExternalCreditPurchaseStateMachine) SettleExternalPayment(ctx context.Context) error { | ||
| currentBase := s.Charge.ChargeBase | ||
| eventCharge := s.Charge | ||
| eventCharge.Status = creditpurchase.StatusActivePaymentAuthorized |
Member
There was a problem hiding this comment.
Same as with authorize, let's not do this, instead let's use different checks or statemachine states/triggering mecnahnisms as needed.
| CreatedInitialPaymentSettlementStatus InitialPaymentSettlementStatus = "created" | ||
| AuthorizedInitialPaymentSettlementStatus InitialPaymentSettlementStatus = "authorized" | ||
| SettledInitialPaymentSettlementStatus InitialPaymentSettlementStatus = "settled" | ||
| AuthorizedAndSettledInitialPaymentSettlementStatus InitialPaymentSettlementStatus = "authorized_and_settled" |
Member
There was a problem hiding this comment.
If initial payment status is settled, that means authorized -> settled transitions. Let's not add this redundant state.
| } | ||
|
|
||
| return meta.ChargeStatus(s), nil | ||
| switch s { |
Member
There was a problem hiding this comment.
there's a helper for this, feel free to update the charges skill, check flatfee or usagebased.
please never rely on explicit state enumerations outside of the statemachine transition matrix as these will diverge over time.
| }) | ||
| s.Require().NoError(err) | ||
| s.Equal(creditpurchase.StatusActive, grant.Status) | ||
| s.Equal(creditpurchase.StatusActivePaymentAuthorized, grant.Status) |
Member
There was a problem hiding this comment.
let's add integration level test for purchasing $1 credits with the costbasis of 0.005. I would expect the allocation to happen, and authroized/settled flow to fully work.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openmeter/billing/charges/meta/charge.go (1)
130-137: ⚡ Quick winAdd a docstring for
DetailedStatusToMetaStatus.Quick readability win: this helper encodes a subtle business contract (detailed status prefix mapping + invalid fallback behavior). A short doc comment will make call sites less error-prone.
As per coding guidelines: "Add a docstring to domain helpers when the name compresses important business semantics that are easy to misread at call sites; explain the observable business contract and why excluded cases are excluded".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/charges/meta/charge.go` around lines 130 - 137, Add a docstring for DetailedStatusToMetaStatus that succinctly documents the business contract: it derives a meta ChargeStatus by taking the prefix before the first '.' in the provided detailed status (e.g., "created.whatever" -> ChargeStatusCreated), validates the resulting ChargeStatus via metaStatus.Validate(), and on validation failure returns ChargeStatusCreated along with an error; also note that this intentionally ignores suffixes after the dot and that callers should pass non-empty detailed statuses or handle the error. Include the function name DetailedStatusToMetaStatus and mention the use of strings.SplitN and metaStatus.Validate to make the behavior and fallback explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@openmeter/billing/charges/meta/charge.go`:
- Around line 130-137: Add a docstring for DetailedStatusToMetaStatus that
succinctly documents the business contract: it derives a meta ChargeStatus by
taking the prefix before the first '.' in the provided detailed status (e.g.,
"created.whatever" -> ChargeStatusCreated), validates the resulting ChargeStatus
via metaStatus.Validate(), and on validation failure returns ChargeStatusCreated
along with an error; also note that this intentionally ignores suffixes after
the dot and that callers should pass non-empty detailed statuses or handle the
error. Include the function name DetailedStatusToMetaStatus and mention the use
of strings.SplitN and metaStatus.Validate to make the behavior and fallback
explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 826da269-ee96-4d39-bf5f-8a26910cc5d9
📒 Files selected for processing (9)
.agents/skills/charges/SKILL.mdopenmeter/billing/charges/creditpurchase/service/external.goopenmeter/billing/charges/creditpurchase/service/external_test.goopenmeter/billing/charges/creditpurchase/settlement.goopenmeter/billing/charges/creditpurchase/statemachine.goopenmeter/billing/charges/flatfee/statemachine.goopenmeter/billing/charges/meta/charge.goopenmeter/billing/charges/usagebased/statemachine.gotest/credits/creditgrant_test.go
✅ Files skipped from review due to trivial changes (1)
- .agents/skills/charges/SKILL.md
turip
reviewed
Jun 3, 2026
Comment on lines
161
to
170
| charge, err := s.Realizations.AuthorizeExternalPayment(ctx, s.Charge) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if paymentSettlement.Status != payment.StatusAuthorized { | ||
| return creditpurchase.Charge{}, payment.ErrPaymentAlreadySettled. | ||
| WithAttrs(charge.ErrorAttributes()). | ||
| WithAttrs(paymentSettlement.ErrorAttributes()) | ||
| } | ||
| charge, err = s.Realizations.SettleExternalPayment(ctx, charge) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| eventAt := clock.Now() | ||
| ledgerTransactionGroupReference, err := s.handler.OnCreditPurchasePaymentSettled(ctx, creditpurchase.PaymentEventInput{ | ||
| Charge: charge, | ||
| EventAt: eventAt, | ||
| }) | ||
| if err != nil { | ||
| return creditpurchase.Charge{}, err | ||
| } | ||
| charge.ChargeBase = currentBase | ||
| s.Charge = charge | ||
| return nil | ||
| } | ||
|
|
||
| paymentSettlement.Settled = &ledgertransaction.TimedGroupReference{ | ||
| GroupReference: ledgerTransactionGroupReference, | ||
| Time: eventAt, | ||
| } | ||
| func (s *ExternalCreditPurchaseStateMachine) HandleExternalPaymentPaid(ctx context.Context) error { | ||
| if s.Charge.Realizations.ExternalPaymentSettlement != nil { | ||
| return s.SettleExternalPayment(ctx) | ||
| } | ||
|
|
||
| paymentSettlement.Status = payment.StatusSettled | ||
| return s.AuthorizeAndSettleExternalPayment(ctx) | ||
| } | ||
|
|
||
| func (s *service) newExternalCreditPurchaseStateMachine(charge creditpurchase.Charge) (*ExternalCreditPurchaseStateMachine, error) { | ||
| return NewExternalCreditPurchaseStateMachine(StateMachineConfig{ | ||
| Charge: charge, | ||
| Adapter: s.adapter, | ||
| Realizations: s.realizations, | ||
| }) | ||
| } | ||
|
|
||
| paymentSettlement, err = s.adapter.UpdateExternalPayment(ctx, paymentSettlement) | ||
| func (s *service) HandleExternalPaymentAuthorized(ctx context.Context, charge creditpurchase.Charge) (creditpurchase.Charge, error) { | ||
| return transaction.Run(ctx, s.adapter, func(ctx context.Context) (creditpurchase.Charge, error) { | ||
| stateMachine, err := s.newExternalCreditPurchaseStateMachine(charge) | ||
| if err != nil { | ||
| return creditpurchase.Charge{}, err | ||
| } | ||
|
|
||
| charge.Realizations.ExternalPaymentSettlement = &paymentSettlement | ||
| if err := stateMachine.FireAndActivate(ctx, billing.TriggerAuthorized); err != nil { | ||
| return creditpurchase.Charge{}, err | ||
| } | ||
|
|
||
| // Let's update the charge status to final | ||
| charge.Status = creditpurchase.StatusFinal | ||
| return stateMachine.GetCharge(), nil | ||
| }) | ||
| } | ||
|
|
||
| updatedBase, err := s.adapter.UpdateCharge(ctx, charge.ChargeBase) | ||
| func (s *service) HandleExternalPaymentSettled(ctx context.Context, charge creditpurchase.Charge) (creditpurchase.Charge, error) { | ||
| return transaction.Run(ctx, s.adapter, func(ctx context.Context) (creditpurchase.Charge, error) { | ||
| stateMachine, err := s.newExternalCreditPurchaseStateMachine(charge) | ||
| if err != nil { |
Member
There was a problem hiding this comment.
there's a helper for this in realizations.
|
|
||
| s.Configure(creditpurchase.StatusActive). | ||
| Permit(billing.TriggerAuthorized, creditpurchase.StatusActivePaymentAuthorized). | ||
| Permit(billing.TriggerPaid, creditpurchase.StatusFinal) |
Member
There was a problem hiding this comment.
Why not OnActive(s.InitiateExternalCreditPurchaseFromCreated) and remove l 107?
Natural progression can be handled in OnActive.
| Permit(billing.TriggerPaid, creditpurchase.StatusFinal) | ||
|
|
||
| s.Configure(creditpurchase.StatusActivePaymentAuthorized). | ||
| OnEntryFrom(billing.TriggerAuthorized, statelessx.EntryFunc(s.AuthorizeExternalPayment)). |
Comment on lines
+115
to
+116
| s.Configure(creditpurchase.StatusFinal). | ||
| OnEntryFrom(billing.TriggerPaid, statelessx.EntryFunc(s.HandleExternalPaymentPaid)) |
Member
There was a problem hiding this comment.
Suggested change
| s.Configure(creditpurchase.StatusFinal). | |
| OnEntryFrom(billing.TriggerPaid, statelessx.EntryFunc(s.HandleExternalPaymentPaid)) | |
| s.Configure(creditpurchase.StatusActivePaid). | |
| Permit(billing.TriggerNext, creditpurchase.StatusFinal) | |
| OnActive(s.HandleExternalPaymentPaid) | |
| s.Configure(creditpurchase.StatusFinal). | |
| OnEntryFrom(billing.TriggerPaid, statelessx.EntryFunc(s.HandleExternalPaymentPaid)) |
I think it's easier to read that when in paid status execute that callback, then proceed to the next state.
| s.Configure(creditpurchase.StatusActivePaymentPending). | ||
| OnEntryFrom(meta.TriggerNext, statelessx.EntryFunc(s.InitiateExternalCreditPurchaseFromCreated)). | ||
| Permit(billing.TriggerAuthorized, creditpurchase.StatusActivePaymentAuthorized). | ||
| Permit(billing.TriggerPaid, creditpurchase.StatusFinal) |
Member
There was a problem hiding this comment.
Suggested change
| Permit(billing.TriggerPaid, creditpurchase.StatusFinal) | |
| Permit(billing.TriggerPaid, creditpurchase.StatusActivePaymentAuthorizedAndPaid) |
Then we can use active there to invoke the right callback instead of magicly deciding if authorization should happen.
Comment on lines
+662
to
+664
| if h.onCreditPurchaseInitiated == nil { | ||
| return ledgertransaction.GroupReference{}, nil | ||
| } |
Member
There was a problem hiding this comment.
let's return an error or panic here, as this is a not expected call.
Comment on lines
+656
to
+658
| onCreditPurchaseInitiated func(ctx context.Context, charge creditpurchase.Charge) (ledgertransaction.GroupReference, error) | ||
| onCreditPurchasePaymentAuthorized func(ctx context.Context, input creditpurchase.PaymentEventInput) (ledgertransaction.GroupReference, error) | ||
| onCreditPurchasePaymentSettled func(ctx context.Context, input creditpurchase.PaymentEventInput) (ledgertransaction.GroupReference, error) |
Member
There was a problem hiding this comment.
Use mock.Mock instead. It will fail the testcase if there's an unexpected call.
Comment on lines
+176
to
+181
| onCreditPurchaseInitiated: func(ctx context.Context, charge creditpurchase.Charge) (ledgertransaction.GroupReference, error) { | ||
| initiatedCalls++ | ||
| require.Equal(t, creditpurchase.StatusActivePaymentPending, charge.Status) | ||
| require.Nil(t, charge.Realizations.CreditGrantRealization) | ||
| require.Nil(t, charge.Realizations.ExternalPaymentSettlement) | ||
| return ledgertransaction.GroupReference{TransactionGroupID: "initiated-ledger-tx"}, nil |
Comment on lines
+226
to
+235
| s.CreditPurchaseTestHandler.onCreditPurchaseInitiated = func(ctx context.Context, charge creditpurchase.Charge) (ledgertransaction.GroupReference, error) { | ||
| callbackCalls++ | ||
| return ledgertransaction.GroupReference{TransactionGroupID: "unexpected-initiated-ledger-tx"}, nil | ||
| } | ||
| s.CreditPurchaseTestHandler.onCreditPurchasePaymentAuthorized = func(ctx context.Context, input creditpurchase.PaymentEventInput) (ledgertransaction.GroupReference, error) { | ||
| callbackCalls++ | ||
| return ledgertransaction.GroupReference{TransactionGroupID: "unexpected-authorized-ledger-tx"}, nil | ||
| } | ||
| s.CreditPurchaseTestHandler.onCreditPurchasePaymentSettled = func(ctx context.Context, input creditpurchase.PaymentEventInput) (ledgertransaction.GroupReference, error) { | ||
| callbackCalls++ |
Member
There was a problem hiding this comment.
these are not needed as they will return error if called without a callback set, so the testcase will fail.
| assert.Equal(t, initiatedCallback.id, charge.Realizations.CreditGrantRealization.GroupReference.TransactionGroupID) | ||
| assert.Nil(t, charge.Realizations.ExternalPaymentSettlement) | ||
| assert.Equal(t, creditpurchase.StatusActive, charge.Status, "charge status should be active") | ||
| assert.Equal(t, creditpurchase.StatusFinal, charge.Status, "charge status should be final") |
Member
There was a problem hiding this comment.
This should not be final state, we should only traverse to final after the callback has been invoked, not before.
So this should be active.payment.authorized.
mark-vass-konghq
force-pushed
the
feat/add-statemachine-to-external-settled-creditpurchase
branch
from
2cca225 to
a5b07dd
Compare
turip
reviewed
Jun 4, 2026
Comment on lines
+105
to
+107
| s.Configure(creditpurchase.StatusActive). | ||
| Permit(billing.TriggerAuthorized, creditpurchase.StatusActivePaymentAuthorized). | ||
| Permit(billing.TriggerPaid, creditpurchase.StatusActivePaymentAuthorized) |
Member
There was a problem hiding this comment.
Active should only allow Next, or we can skip initiate credit purchase.
| return s.settleAfterAuthorization | ||
| } | ||
|
|
||
| func (s *ExternalCreditPurchaseStateMachine) InitiateExternalCreditPurchaseFromCreated(ctx context.Context) error { |
Member
There was a problem hiding this comment.
please use some name that describes what it does, this seems like a bunch of random words.
| func (s *ExternalCreditPurchaseStateMachine) FireAndAdvanceUntilStateStable(ctx context.Context, trigger meta.Trigger) (creditpurchase.Charge, error) { | ||
| // Direct paid events still authorize first; the following next transition | ||
| // settles while leaving active.payment.authorized, before final is persisted. | ||
| s.settleAfterAuthorization = trigger == billing.TriggerPaid && |
Member
There was a problem hiding this comment.
don't: trigger is ephemeral, and this makes FireAndAdvanceUtilStable a custom implementation.
Comment on lines
+117
to
+123
| Permit(meta.TriggerNext, creditpurchase.StatusFinal, statelessx.BoolFn(s.ShouldSettleAfterAuthorization)). | ||
| OnExitWith(billing.TriggerPaid, func(ctx context.Context, _ ...any) error { | ||
| return s.SettleExternalPayment(ctx) | ||
| }). | ||
| OnExitWith(meta.TriggerNext, func(ctx context.Context, _ ...any) error { | ||
| return s.SettleExternalPayment(ctx) | ||
| }) |
Member
There was a problem hiding this comment.
This is super hard to read, let's refactor it together.
mark-vass-konghq
force-pushed
the
feat/add-statemachine-to-external-settled-creditpurchase
branch
from
5f822c7 to
30686e7
Compare
mark-vass-konghq
force-pushed
the
feat/add-statemachine-to-external-settled-creditpurchase
branch
from
30686e7 to
5b692e8
Compare
mark-vass-konghq
force-pushed
the
feat/add-statemachine-to-external-settled-creditpurchase
branch
from
5b692e8 to
77ab79c
Compare
mark-vass-konghq
force-pushed
the
feat/add-statemachine-to-external-settled-creditpurchase
branch
from
77ab79c to
ae425f3
Compare
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.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation