feat(*): support v2 events as well as v3 events #394
Conversation
WalkthroughThe changes update the event publishing mechanism across the codebase. Functions in the activities use variadic syntax to publish multiple events at once instead of a single event, and several functions have been updated to accept additional parameters (e.g., payment initiation, timestamps). Moreover, the event payloads and types have been versioned into V2 and V3 for accounts, balances, bank accounts, payments, payment initiations, pools, and connectors, with corresponding updates in the tests and workflow methods. Changes
Sequence Diagram(s)sequenceDiagram
participant A as Activity
participant E as Events
participant P as Publisher
A->>E: Call NewEvent...(returns slice of event messages)
A->>E: Call Publish(...events)
E->>P: For each event: NewMessage(event)
E->>P: Publish all messages using variadic syntax
P-->>E: Acknowledge published events
Assessment against linked issues
Possibly related PRs
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
63b2c1b to
675221c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #394 +/- ##
==========================================
+ Coverage 59.71% 60.07% +0.35%
==========================================
Files 538 538
Lines 26680 27040 +360
==========================================
+ Hits 15933 16245 +312
- Misses 9907 9949 +42
- Partials 840 846 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
f189d26 to
930455d
Compare
930455d to
f4b2232
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (12)
internal/models/connectors.go (1)
111-141: Well-structured implementation for v2 provider compatibility.The
ToV2Providerfunction provides a clean mapping between lowercase v3 provider names and uppercase v2 provider names, enabling backward compatibility with v2 API. The implementation mirrors the existingToV3Providerfunction and includes all current providers with consistent formatting.Consider adding unit tests to verify these mappings work as expected in both directions to prevent regressions when adding new providers in the future.
// Example test for both provider conversion functions func TestProviderConversions(t *testing.T) { testCases := []struct { v2Provider string v3Provider string }{ {"ADYEN", "adyen"}, {"ATLAR", "atlar"}, {"BANKING-CIRCLE", "bankingcircle"}, // Add other provider pairs here } for _, tc := range testCases { t.Run(tc.v2Provider, func(t *testing.T) { // Test v3 to v2 conversion assert.Equal(t, tc.v2Provider, ToV2Provider(tc.v3Provider)) // Test v2 to v3 conversion assert.Equal(t, tc.v3Provider, ToV3Provider(tc.v2Provider)) // Test idempotency assert.Equal(t, tc.v2Provider, ToV2Provider(ToV3Provider(tc.v2Provider))) assert.Equal(t, tc.v3Provider, ToV3Provider(ToV2Provider(tc.v3Provider))) }) } }internal/events/connector.go (1)
17-20: Check theconnectorIdvs.connectorIDdiscrepancy.
For the V2 payload, you’re usingjson:"connectorId"whereas the V3 payload usesconnectorID. If this difference is intentional for backward compatibility, that’s fine; otherwise consider unifying the casing and naming to minimize confusion.- ConnectorID string `json:"connectorId"` + ConnectorID string `json:"connectorID"`internal/events/bank_account.go (1)
33-42: Clarify naming for V2 related accounts JSON key.
In V2,RelatedAccountsis mapped tojson:"adjustments", which could be confusing if these are truly additional or related accounts. If it’s intentional, consider clarifying in documentation. Otherwise, rename for consistency.- RelatedAccounts []V2BankAccountRelatedAccountsPayload `json:"adjustments"` + RelatedAccounts []V2BankAccountRelatedAccountsPayload `json:"relatedAccounts"`internal/events/pool.go (2)
55-74: Consider adding IdempotencyKey for V2 events.Unlike the V3 event message, the V2 event omits
IdempotencyKey. If consistent deduplication is desired for V2, think about adding it.return publish.EventMessage{ + IdempotencyKey: pool.IdempotencyKey(), Date: time.Now().UTC(), ... }
107-118: toV2PoolDeleteEvent consistency check.Similar to other V2 events, consider whether
IdempotencyKeyis beneficial here. Maintaining parity with V3 might reduce confusion.internal/events/payment_initiation.go (1)
172-201: toV2TransferInitiationAdjustmentEvent approach is consistent with V3.Retaining a similar shape for the V2 payload while differing only where necessary fosters clarity. Consider also whether
IdempotencyKeyis needed for dedup.internal/connectors/engine/activities/events_send_balance.go (1)
1-19: Consistent pattern across event publishingThe changes across all event publishing activities follow a consistent pattern, which is good for maintainability and understanding the codebase.
While the current implementation is sufficient for supporting both v2 and v3 events, consider adding version configuration to allow selectively disabling v2 events in the future when they're no longer needed. This would help with a smooth transition strategy and prevent unnecessary event publishing when v2 is deprecated.
Also, ensure there's appropriate error handling if one version of an event fails to publish but others succeed. Consider using a transaction-like approach if all events must be published successfully or none at all.
internal/connectors/engine/workflow/create_bank_account.go (1)
97-97: Improved workflow execution handlingThe addition of
GetChildWorkflowExecution()ensures proper access to the child workflow execution context. This change, combined with the comment indicating not to wait for events to be sent, suggests a more flexible approach to child workflow execution that allows events to be processed asynchronously.However, I recommend adding additional error logging for the child workflow execution, especially since you're explicitly not waiting for completion:
- ).GetChildWorkflowExecution().Get(ctx, nil); err != nil { + ).GetChildWorkflowExecution().Get(ctx, nil); err != nil { + // Log the error but continue since we don't want to block on event sending + workflow.GetLogger(ctx).Error("Failed to send events for bank account", "error", err) return "", err }Also applies to: 113-113
test/e2e/api_bank_accounts_test.go (3)
158-175: Consider adjusting the 2-second timeout or adding retry logic.Using
Eventually(...).WithTimeout(2 * time.Second)can be sufficient for local tests, but it might be prone to flakiness in slower environments or CI pipelines. Consider extending the timeout if you observe intermittent failures.
176-192: Parallel validation for V3 events looks good.The new block ensures both V2 and V3 payloads are ultimately tested. Restating the same verification logic for the V3 payload is consistent with your versioned approach. If the logic is repeated elsewhere, consider factoring it into a common test helper to reduce duplication.
231-232: Validate payload structure for these simpler checks.Unlike the previous blocks, you are only asserting the event name here. Consider verifying payload details as well, to ensure correct content in addition to the event type.
pkg/events/event.go (1)
6-7: Consider updating or clarifying the EventVersion constantSince the code now supports both V2 and V3 events, keeping the static
EventVersion = "v3"constant might be confusing. Consider either:
- Adding a comment explaining that this is the default/latest version
- Creating a mechanism to set the version dynamically
- Adding a separate constant for V2 event handling
This would make it clearer how version selection works in the system.
🛑 Comments failed to post (2)
internal/events/payment.go (1)
116-166: 🛠️ Refactor suggestion
Confirm consistent version usage for V2 events.
As with the connector events, yourVersionfield is set toevents.EventVersionwhich is likely"v3". This may confuse consumers looking for a “v2” presence.- Version: events.EventVersion, + Version: "v2", // or define an explicit V2 event version constant📝 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.func (e Events) toV2PaymentEvent(payment models.Payment, adjustment models.PaymentAdjustment) publish.EventMessage { payload := V2PaymentMessagePayload{ ID: payment.ID.String(), Reference: payment.Reference, Type: payment.Type, Status: payment.Status, InitialAmount: payment.InitialAmount, Amount: payment.Amount, Scheme: payment.Scheme, Asset: payment.Asset, CreatedAt: payment.CreatedAt, ConnectorID: payment.ConnectorID.String(), Provider: models.ToV2Provider(payment.ConnectorID.Provider), SourceAccountID: func() string { if payment.SourceAccountID == nil { return "" } return payment.SourceAccountID.String() }(), DestinationAccountID: func() string { if payment.DestinationAccountID == nil { return "" } return payment.DestinationAccountID.String() }(), RawData: adjustment.Raw, Metadata: payment.Metadata, } if payment.SourceAccountID != nil { payload.Links = append(payload.Links, api.Link{ Name: "source_account", URI: e.stackURL + "/api/payments/accounts/" + payment.SourceAccountID.String(), }) } if payment.DestinationAccountID != nil { payload.Links = append(payload.Links, api.Link{ Name: "destination_account", URI: e.stackURL + "/api/payments/accounts/" + payment.DestinationAccountID.String(), }) } return publish.EventMessage{ Date: time.Now().UTC(), App: events.EventApp, Version: "v2", // or define an explicit V2 event version constant Type: events.V2EventTypeSavedPayments, Payload: payload, } }internal/connectors/engine/activities/events_send_payment_initiation_related_payments.go (1)
21-27: 💡 Verification agent
🧩 Analysis chain
Workflow function signature updated properly.
Ensuring consistency with the wrapped activity’s new parameters is crucial. Confirm that all workflow calls to
EventsSendPaymentInitiationRelatedPaymentsupply thepiandstatusarguments to avoid runtime issues.
🏁 Script executed:
#!/bin/bash # Confirm all references to EventsSendPaymentInitiationRelatedPayment are updated with pi and status rg -A 3 "EventsSendPaymentInitiationRelatedPayment(\("Length of output: 193
Below is an updated verification command to properly search for occurrences of the workflow function call with its new parameters. Please run this revised script manually to confirm that every call to
EventsSendPaymentInitiationRelatedPaymentincludes the additionalpiandstatusarguments:Once verified, ensure that any outdated calls are updated accordingly to prevent runtime errors.
Next steps:
- Run the above script and inspect each usage to confirm it passes both
piandstatus.- If any outdated invocations are found, update them to match the new function signature.
🏁 Script executed:
#!/bin/bash # Confirm that all references to EventsSendPaymentInitiationRelatedPayment use the updated parameters (pi, status) rg -A 3 'EventsSendPaymentInitiationRelatedPayment\('Length of output: 1694
Action Required: Missing
statusParameter in Workflow InvocationThe workflow function’s signature now expects four arguments (including
piandstatus), but our search shows that in internal/connectors/engine/workflow/send_events.go the call toEventsSendPaymentInitiationRelatedPaymentonly supplies three arguments:
infiniteRetryContext(ctx)*sendEvents.SendEventPaymentInitiationRelatedPayment.PaymentInitiationRelatedPayment*sendEvents.SendEventPaymentInitiationRelatedPayment.PaymentInitiationPlease update the invocation to pass a valid
statusparameter to ensure consistency with the updated function signature and to avoid potential runtime issues.
09d2417 to
b8645af
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
internal/events/connector.go (1)
43-54:⚠️ Potential issueMissing idempotency key in v2 event.
The
toV2ConnectorEventfunction doesn't set theIdempotencyKeyfield whiletoV3ConnectorEventdoes. This inconsistency could potentially lead to idempotency issues with v2 events.func toV2ConnectorEvent(connectorID models.ConnectorID, at time.Time) publish.EventMessage { return publish.EventMessage{ + IdempotencyKey: resetConnectorIdempotencyKey(connectorID, at), Date: time.Now().UTC(), App: events.EventApp, Version: events.V2EventVersion, Type: events.V2EventTypeConnectorReset, Payload: V2ConnectorMessagePayload{ CreatedAt: at, ConnectorID: connectorID.String(), }, } }
🧹 Nitpick comments (7)
internal/events/payment.go (2)
107-108: V2 event missing idempotency key.The V3 event message includes an idempotency key (line 107) but the V2 event message does not. If idempotency is essential for event processing, consider adding it to the V2 event message as well.
return publish.EventMessage{ + IdempotencyKey: adjustment.IdempotencyKey(), Date: time.Now().UTC(), App: events.EventApp,Also applies to: 160-161
116-166: Consider extracting common code between V2 and V3 event creation.There's significant duplication between
toV2PaymentEventandtoV3PaymentEvent. Consider extracting common logic into helper functions to reduce duplication while maintaining version-specific differences.internal/events/payment_initiation.go (5)
50-74: Review field naming inconsistencies between V2 and V3 payloads.There are naming inconsistencies between the V2 and V3 payloads that might cause confusion:
- V3 uses
ConnectorID, V2 usesConnectorId- V3 uses
SourceAccountID/DestinationAccountID, V2 usesSourceAccountId/DestinationAccountIdWhile this might be intentional for compatibility reasons, consider documenting the reason for these differences or standardizing if possible.
126-126: Document the default status choice.Consider adding a comment explaining why
PAYMENT_INITIATION_ADJUSTMENT_STATUS_WAITING_FOR_VALIDATIONis used as the default status for saved transfer initiations.
172-201: Different error handling approaches between V2 and V3.Note the different approaches to error handling:
- V3 uses optional pointers to strings (
*string)- V2 uses empty strings as defaults
While both approaches work, consider unifying the error handling approach for better maintainability.
186-191: Simplify V2 error handling with a ternary-like expression.The error handling logic can be simplified:
-Error: func() string { - if adj.Error == nil { - return "" - } - return adj.Error.Error() -}(), +Error: pointer.ForString(adj.Error, ""),This assumes a
ForStringhelper exists or can be created similar to theForhelper used elsewhere.
249-255: Optimize RelatedPayments initialization.Instead of initializing an empty slice and then appending to it, consider direct initialization:
-payload.RelatedPayments = append(payload.RelatedPayments, &V2TransferInitiationsPaymentsMessagePayload{ +payload.RelatedPayments = []*V2TransferInitiationsPaymentsMessagePayload{{ TransferInitiationID: pi.ID.String(), PaymentID: relatedPayment.PaymentID.String(), CreatedAt: pi.CreatedAt, Status: status.String(), -}) +}}This is more efficient and clearer when you know the exact contents of the slice upfront.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
internal/connectors/engine/workflow/create_bank_account.go(2 hunks)internal/connectors/engine/workflow/create_bank_account_test.go(1 hunks)internal/connectors/engine/workflow/create_payout_test.go(8 hunks)internal/connectors/engine/workflow/create_transfer_test.go(7 hunks)internal/connectors/engine/workflow/poll_payout_test.go(12 hunks)internal/connectors/engine/workflow/poll_transfer_test.go(12 hunks)internal/connectors/engine/workflow/reverse.go(2 hunks)internal/connectors/engine/workflow/reverse_payout_test.go(5 hunks)internal/connectors/engine/workflow/reverse_transfer_test.go(5 hunks)internal/connectors/engine/workflow/utils.go(6 hunks)internal/events/account.go(3 hunks)internal/events/balance.go(3 hunks)internal/events/bank_account.go(3 hunks)internal/events/connector.go(1 hunks)internal/events/payment.go(3 hunks)internal/events/payment_initiation.go(6 hunks)internal/events/pool.go(2 hunks)pkg/events/event.go(1 hunks)test/e2e/api_payment_initiations_test.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/connectors/engine/workflow/create_bank_account.go
- test/e2e/api_payment_initiations_test.go
- internal/connectors/engine/workflow/poll_payout_test.go
- internal/events/account.go
- internal/connectors/engine/workflow/reverse.go
- internal/connectors/engine/workflow/poll_transfer_test.go
- internal/connectors/engine/workflow/utils.go
- pkg/events/event.go
- internal/events/pool.go
🧰 Additional context used
🧬 Code Definitions (9)
internal/connectors/engine/workflow/reverse_transfer_test.go (1)
internal/connectors/engine/workflow/send_events.go (2)
SendEventPaymentInitiationAdjustment(15-18)SendEventPaymentInitiationRelatedPayment(20-24)
internal/events/balance.go (3)
internal/events/events.go (1)
Events(11-15)pkg/events/event.go (4)
V3EventVersion(7-7)V3EventTypeSavedBalances(24-24)V2EventVersion(6-6)V2EventTypeSavedBalances(14-14)internal/models/connectors.go (1)
ToV2Provider(114-141)
internal/connectors/engine/workflow/create_transfer_test.go (1)
internal/connectors/engine/workflow/send_events.go (2)
SendEventPaymentInitiationAdjustment(15-18)SendEventPaymentInitiationRelatedPayment(20-24)
internal/connectors/engine/workflow/reverse_payout_test.go (1)
internal/connectors/engine/workflow/send_events.go (2)
SendEventPaymentInitiationAdjustment(15-18)SendEventPaymentInitiationRelatedPayment(20-24)
internal/events/connector.go (2)
internal/events/events.go (1)
Events(11-15)pkg/events/event.go (5)
EventApp(8-8)V3EventVersion(7-7)V3EventTypeConnectorReset(26-26)V2EventVersion(6-6)V2EventTypeConnectorReset(18-18)
internal/events/bank_account.go (3)
internal/models/bank_accounts.go (1)
BankAccount(26-39)pkg/events/event.go (5)
V3EventVersion(7-7)V3EventTypeSavedBankAccount(25-25)EventApp(8-8)V2EventVersion(6-6)V2EventTypeSavedBankAccount(15-15)internal/models/connectors.go (1)
ToV2Provider(114-141)
internal/connectors/engine/workflow/create_payout_test.go (1)
internal/connectors/engine/workflow/send_events.go (2)
SendEventPaymentInitiationAdjustment(15-18)SendEventPaymentInitiationRelatedPayment(20-24)
internal/events/payment.go (4)
internal/events/events.go (1)
Events(11-15)internal/models/payments.go (1)
Payment(57-97)pkg/events/event.go (5)
V3EventVersion(7-7)V3EventTypeSavedPayments(22-22)EventApp(8-8)V2EventVersion(6-6)V2EventTypeSavedPayments(12-12)internal/models/connectors.go (1)
ToV2Provider(114-141)
internal/events/payment_initiation.go (5)
internal/events/events.go (1)
Events(11-15)internal/models/payment_initiations.go (1)
PaymentInitiation(35-66)pkg/events/event.go (4)
V3EventVersion(7-7)V3EventTypeSavedPaymentInitiation(27-27)V3EventTypeSavedPaymentInitiationAdjustment(28-28)V3EventTypeSavedPaymentInitiationRelatedPayment(29-29)internal/models/payment_initiation_adjustments.go (1)
PaymentInitiationAdjustment(12-28)internal/models/payment_initation_related_payments.go (1)
PaymentInitiationRelatedPayments(3-9)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (32)
internal/connectors/engine/workflow/create_transfer_test.go (1)
31-31: Field reference updates to match new SendEvents struct fields.The test assertions have been updated to use the new field names
SendEventPaymentInitiationAdjustmentandSendEventPaymentInitiationRelatedPaymentinstead of the previousPaymentInitiationAdjustmentandPaymentInitiationRelatedPayment. This change is consistent with the updated structure insend_events.goand supports the addition of v2 events alongside v3 events as mentioned in the PR objectives.Also applies to: 53-53, 72-72, 108-108, 124-124, 198-198
internal/connectors/engine/workflow/reverse_transfer_test.go (1)
60-60: Field reference updates to match new SendEvents struct fields.The test assertions have been correctly updated to use the new field names
SendEventPaymentInitiationAdjustmentandSendEventPaymentInitiationRelatedPaymentinstead of the previousPaymentInitiationAdjustmentandPaymentInitiationRelatedPayment. This change aligns with the refactored SendEvents structure insend_events.goand ensures test compatibility with the dual versioning support (v2/v3) implemented in this PR.Also applies to: 93-93, 108-108, 173-173, 196-196
internal/connectors/engine/workflow/reverse_payout_test.go (1)
60-60: Field reference updates to match new SendEvents struct fields.The assertions have been properly updated to reference
SendEventPaymentInitiationAdjustmentandSendEventPaymentInitiationRelatedPaymentfields instead of the previousPaymentInitiationAdjustmentandPaymentInitiationRelatedPayment. These changes maintain consistency with the updated SendEvents structure insend_events.goand support the feature goal of handling both v2 and v3 event types.Also applies to: 93-93, 108-108, 173-173, 196-196
internal/connectors/engine/workflow/create_bank_account_test.go (1)
173-173: Simplified error handling in test mock.The code now directly returns
temporal.NewNonRetryableApplicationErrorinstead of using a function to build and return the error. This makes the mock setup more concise while maintaining the same test behavior.internal/connectors/engine/workflow/create_payout_test.go (1)
31-31: Field name updates align with versioned event structure.The test code has been updated to use the new field names in the
SendEventsstruct, replacing direct references toPaymentInitiationAdjustmentandPaymentInitiationRelatedPaymentwithSendEventPaymentInitiationAdjustmentandSendEventPaymentInitiationRelatedPayment. These changes match the new structured types defined insend_events.goand support the PR's objective of enabling support for both v2 and v3 events.Also applies to: 53-53, 72-72, 107-107, 122-122, 144-144, 163-163, 197-197
internal/events/connector.go (4)
17-20: Type naming convention is consistent with versioning approach.The
V2ConnectorMessagePayloadstruct is correctly defined with JSON field names in camelCase format (connectorId), which maintains consistency with v2 API conventions.
22-27: Signature update properly handles multiple event versions.The function now returns a slice of events instead of a single event, correctly implementing the dual-version support. This allows publishing both v2 and v3 formats of the same event.
12-15: V3 implementation correctly maintains the original behavior.The
V3ConnectorMessagePayloadandtoV3ConnectorEventimplementations properly preserve the original behavior while allowing for the v2 event support to be added.Also applies to: 29-41
56-58:❓ Verification inconclusive
Verify idempotency key usage across both versions.
The
resetConnectorIdempotencyKeyfunction is used for v3 events but not for v2 events. If both event versions should be idempotent, this function should be used consistently.
🏁 Script executed:
#!/bin/bash # Check if idempotency keys are used consistently across other v2/v3 event pairs # Search for other event generation functions that might have similar issues rg "func to.*Event" -A 10 --glob "internal/events/*.go" | grep -E "Idempotency|idempotency"Length of output: 331
Ensure uniform idempotency key generation across event versions
Our investigation shows that the mechanism for generating idempotency keys differs between events. Specifically:
- In internal/events/connector.go, v3 events use
resetConnectorIdempotencyKey(connectorID, at).- In internal/events/pool.go, the key is generated with
id.String().- In internal/events/payment_initiation.go, the key comes from
relatedPayment.IdempotencyKey().Please verify whether both v2 and v3 events are intended to have equivalent idempotency behavior. If uniform idempotency is desired across all event versions, consider standardizing the key generation approach (for instance, by utilizing
resetConnectorIdempotencyKey) consistently.internal/events/balance.go (4)
12-20: LGTM: Well-structured payload type for V3 balance eventsThe
V3BalanceMessagePayloadstructure properly encapsulates all necessary fields for balance events, including temporal fields for tracking creation and updates. The JSON field naming is consistent with standard conventions using camelCase.
22-29: Note the difference in JSON tag for ConnectorID fieldThe
V2BalanceMessagePayloadstructure usesconnectorId(lowercase 'd') as the JSON tag, while the V3 version usesconnectorID(uppercase 'ID'). This difference is important for maintaining backward compatibility with existing systems consuming these events.Also note that this structure doesn't include the
LastUpdatedAtfield present in the V3 version.
38-57: LGTM: Clean implementation of V3 event generationThe
toV3BalanceEventfunction correctly constructs a V3 balance event with all necessary fields, including proper version and type values. It's good that you're using the specific V3 event type and version constants from the events package.
31-36:✅ Verification successful
API change: Function now returns multiple events
The function signature has changed from returning a single
publish.EventMessageto returning a slice[]publish.EventMessage, which now includes both V2 and V3 versions of the event. Ensure that all callers of this function are updated to handle multiple events.
🏁 Script executed:
#!/bin/bash # Find all places where NewEventSavedBalances is called to verify they can handle multiple events rg -A 3 "NewEventSavedBalances" --glob '!internal/events/balance.go'Length of output: 452
Verified API Change: Multiple Event Handling is Correct
The changes made in
internal/events/balance.go—returning a slice of events instead of a single event—are correctly handled by the call site ininternal/connectors/engine/activities/events_send_balance.go, which uses the spread operator (...) to pass the events. Please confirm that no other call sites require similar adjustments.internal/events/payment.go (5)
14-34: Well-structured V3PaymentMessagePayload with clear field organization.The new payload struct for V3 events has a clean separation between mandatory and optional fields, with appropriate JSON tags and clear type definitions.
36-54: Clear type differentiation in V2PaymentMessagePayload.The V2 payload struct uses model types (
models.PaymentType,models.PaymentStatus, etc.) instead of strings as in V3, which properly preserves the original schema. Also note that field naming follows V2 conventions with "Id" instead of "ID" in JSON tags.
148-149: V2 events use different API paths for account links.The links to accounts in V2 events use
/api/payments/accounts/path instead of/api/payments/v3/accounts/used in V3 events. This is the correct approach for maintaining version-specific API paths.Also applies to: 154-156
128-134: Consistent approach for handling nil account IDs.Both V2 and V3 implementations handle nil account IDs in the same way, using anonymous functions to convert them to empty strings. This is a good pattern for handling optional fields.
Also applies to: 135-143
56-61:✅ Verification successful
Good implementation of multi-version event publishing.
The function now correctly returns both V2 and V3 event messages in a slice, supporting backward compatibility while allowing for the new format.
🏁 Script executed:
#!/bin/bash # Description: Verify that all callers of NewEventSavedPayments have been updated to handle multiple events # Find all places where NewEventSavedPayments is called rg -l "NewEventSavedPayments" --type goLength of output: 127
Multi-version event publishing verified and approved.
- The function in
internal/events/payment.gonow correctly returns both V2 and V3 event messages in a slice.- Verification confirms that its callers (notably in
internal/connectors/engine/activities/events_send_payment.go) have been updated to handle multiple events as expected.Overall, the implementation supports backward compatibility while introducing the new format. Approved with no further changes required.
internal/events/payment_initiation.go (8)
13-48: Consistent versioning and naming updates for V3 payloads.The renaming of payload types with a
V3prefix clearly indicates the versioning strategy, which is a good practice. This explicit versioning makes it easier to maintain backward compatibility while introducing new features.
76-81: Appropriate multi-event publishing pattern for backward compatibility.The function now correctly returns both V2 and V3 events as a slice, which is a clean approach for supporting backward compatibility with existing consumers while introducing a new version.
83-108: Clean extraction of V3 event creation logic.The
toV3PaymentInitiationEventfunction nicely encapsulates the logic for creating V3 events, making the code more maintainable and easier to test.
138-143: Appropriate multi-event publishing for adjustments.Similar to the base payment initiation events, the adjustment events now correctly publish both V2 and V3 versions.
145-170: Clean implementation for V3 adjustment events.The V3 adjustment event creation is well-implemented with proper error handling for optional fields.
203-211: Comprehensive related payment event handling.The function now appropriately supports both V2 and V3 events for related payments, with the V2 version requiring additional context (payment initiation and status).
213-227: Clean V3 implementation for related payment events.The V3 implementation is clean and follows the pattern established by other V3 event functions.
110-136:✅ Verification successful
Verify hardcoded status in V2 event creation.
The V2 event creation includes a hardcoded status:
Status: models.PAYMENT_INITIATION_ADJUSTMENT_STATUS_WAITING_FOR_VALIDATION.String(). Verify if this default status is appropriate for all payment initiations or if it should be derived from the input model.
🏁 Script executed:
#!/bin/bash # Search for occurrences of this status to understand its usage rg "PAYMENT_INITIATION_ADJUSTMENT_STATUS_WAITING_FOR_VALIDATION" -A 2 -B 2Length of output: 9754
Default status usage verified: 'WAITING_FOR_VALIDATION' is consistently applied.
After reviewing the search results:
- The status
models.PAYMENT_INITIATION_ADJUSTMENT_STATUS_WAITING_FOR_VALIDATIONis used throughout tests (e.g., in e2e, API creation, and migration tests) to indicate the default state for new payment initiations.- Other parts of the codebase (such as in API service creation and deletion) also rely on this same default status.
It appears that the design intentionally hardcodes this value rather than deriving it from the input model. No adjustments are needed at this time.
internal/events/bank_account.go (6)
11-24: LGTM: Event payload versioning looks goodThe renaming of
BankAccountMessagePayloadtoV3BankAccountMessagePayloadand related changes correctly implement versioning for bank account event payloads, aligning with the PR objective to support both v2 and v3 events.
26-31: LGTM: Related accounts payload versioning looks goodThe renaming of
BankAccountRelatedAccountsPayloadtoV3BankAccountRelatedAccountsPayloadis consistent with the versioning approach.
44-49: LGTM: V2 related accounts payload structureThe
V2BankAccountRelatedAccountsPayloadstruct has the same fields as its V3 counterpart, which helps maintain consistent data structures across versions.
51-60: LGTM: Event generation now returns multiple versioned eventsThe function properly returns an array of events (both V2 and V3) instead of a single event, which correctly implements the multi-version support.
62-105: LGTM: V3 event generation function updatesThe
toV3BankAccountEventfunction has been correctly updated to use the versioned types and constants. The function now properly referencesV3EventVersionandV3EventTypeSavedBankAccount.
130-139:✅ Verification successful
Verify that models.ToV2Provider works correctly
The code properly uses
models.ToV2Providerfor V2 events andmodels.ToV3Providerfor V3 events, but let's verify that the V2 provider function handles all providers correctly.
🏁 Script executed:
#!/bin/bash # Check implementation of ToV2Provider vs ToV3Provider to ensure consistency # List the code in ToV2Provider and ToV3Provider functions echo "ToV2Provider function:" cat internal/models/connectors.go | grep -A 30 "ToV2Provider" echo -e "\nToV3Provider function:" cat internal/models/connectors.go | grep -A 30 "ToV3Provider" # Check if there are any providers in one function but not the other echo -e "\nChecking for inconsistencies between provider lists:" grep "case \"" internal/models/connectors.go | grep -A 1 "ToV2Provider" | grep "return" | sort > /tmp/v2_providers.txt grep "case \"" internal/models/connectors.go | grep -A 1 "ToV3Provider" | grep "return" | sort > /tmp/v3_providers.txt diff /tmp/v2_providers.txt /tmp/v3_providers.txt || echo "Differences found in provider lists"Length of output: 1773
Verified: Provider Conversion Functions Consistency
The mappings in both
models.ToV2Providerandmodels.ToV3Providerhave been confirmed to be consistent. The V2 function correctly converts provider names to their uppercase counterparts for V2 events, and the V3 function reverses this conversion for V3 events. The verification script shows that all providers are handled as expected with no discrepancies found.
5141f34 to
a8564d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/events/payment_initiation.go (1)
215-266: Evaluate event type naming consistency in V2 for related-payment.
You’re reusingV2EventTypeSavedTransferInitiationfor related payment events (lines 231–266). While it likely works, consider using a distinct event type for clarity. This helps potential consumers differentiate “transfer initiation” from “payment initiation related payment” in V2 logs.internal/events/connector.go (1)
12-20: Check JSON field naming consistency across V2 and V3.
V3 usesconnectorIDwhile V2 usesconnectorId. Mismatched casing in API fields may sometimes cause confusion. If you want to keep consistent naming with other V2 vs. V3 payload structures, this is fine. Otherwise, consider standardizing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
internal/events/account.go(3 hunks)internal/events/balance.go(3 hunks)internal/events/bank_account.go(3 hunks)internal/events/connector.go(1 hunks)internal/events/payment.go(3 hunks)internal/events/payment_initiation.go(6 hunks)internal/events/pool.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/events/pool.go
🧰 Additional context used
🧠 Learnings (1)
internal/events/bank_account.go (1)
Learnt from: paul-nicolas
PR: formancehq/payments#394
File: internal/events/bank_account.go:33-42
Timestamp: 2025-04-01T11:39:55.104Z
Learning: In the V2BankAccountMessagePayload struct, the RelatedAccounts field is intentionally mapped to JSON field "adjustments" rather than "relatedAccounts" for backward compatibility with v2 API.
🧬 Code Definitions (6)
internal/events/balance.go (3)
internal/events/events.go (1)
Events(11-15)pkg/events/event.go (5)
V3EventVersion(7-7)V3EventTypeSavedBalances(24-24)EventApp(8-8)V2EventVersion(6-6)V2EventTypeSavedBalances(14-14)internal/models/connectors.go (1)
ToV2Provider(114-141)
internal/events/connector.go (2)
internal/events/events.go (1)
Events(11-15)pkg/events/event.go (5)
EventApp(8-8)V3EventVersion(7-7)V3EventTypeConnectorReset(26-26)V2EventVersion(6-6)V2EventTypeConnectorReset(18-18)
internal/events/bank_account.go (3)
internal/models/bank_accounts.go (1)
BankAccount(26-39)pkg/events/event.go (5)
V3EventVersion(7-7)V3EventTypeSavedBankAccount(25-25)EventApp(8-8)V2EventVersion(6-6)V2EventTypeSavedBankAccount(15-15)internal/models/connectors.go (1)
ToV2Provider(114-141)
internal/events/payment.go (3)
internal/events/events.go (1)
Events(11-15)internal/models/payments.go (1)
Payment(57-97)pkg/events/event.go (5)
V3EventVersion(7-7)V3EventTypeSavedPayments(22-22)EventApp(8-8)V2EventVersion(6-6)V2EventTypeSavedPayments(12-12)
internal/events/account.go (4)
internal/events/events.go (1)
Events(11-15)internal/models/accounts.go (1)
Account(29-55)pkg/events/event.go (5)
V3EventVersion(7-7)V3EventTypeSavedAccounts(23-23)EventApp(8-8)V2EventVersion(6-6)V2EventTypeSavedAccounts(13-13)internal/models/connectors.go (1)
ToV2Provider(114-141)
internal/events/payment_initiation.go (6)
internal/events/events.go (1)
Events(11-15)internal/models/payment_initiations.go (1)
PaymentInitiation(35-66)pkg/events/event.go (7)
V3EventVersion(7-7)V3EventTypeSavedPaymentInitiation(27-27)EventApp(8-8)V2EventVersion(6-6)V2EventTypeSavedTransferInitiation(16-16)V3EventTypeSavedPaymentInitiationAdjustment(28-28)V3EventTypeSavedPaymentInitiationRelatedPayment(29-29)internal/models/connectors.go (1)
ToV2Provider(114-141)internal/models/payment_initiation_adjustments.go (1)
PaymentInitiationAdjustment(12-28)internal/models/payment_initation_related_payments.go (1)
PaymentInitiationRelatedPayments(3-9)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (21)
internal/events/account.go (5)
12-26: Structure looks goodThe V3AccountMessagePayload is well-organized with clear separation between mandatory and optional fields. The JSON tags are properly defined with appropriate omitempty options for optional fields.
28-37: Consistent V2 API format maintainedThe V2AccountMessagePayload correctly uses
"connectorId"(camelCase) in the JSON tag, which differs from V3's"connectorID"(with uppercase ID). This maintains consistency with the V2 API formatting convention.
39-44: Clean implementation of multi-version event publishingThe function now correctly returns an array of events containing both V2 and V3 formats, allowing consumers to handle whichever version they support. This is a good approach for maintaining backward compatibility.
46-74: V3 event construction looks goodThe function properly constructs the V3 payload with the correct event version and type. The provider conversion and optional field handling are implemented correctly.
76-102: V2 event construction is properly implementedGood use of the
models.ToV2Providerfunction to ensure the provider name follows the V2 format convention. The event is correctly constructed with the appropriate version and type for V2.internal/events/balance.go (4)
12-20: V3 balance payload structure looks goodThe V3BalanceMessagePayload includes all necessary fields including LastUpdatedAt, which provides important temporal information for balance tracking.
22-29: V2 balance payload maintains backward compatibilityThe V2BalanceMessagePayload correctly uses the camelCase format for "connectorId" in the JSON tag, consistent with V2 API conventions. Also, it properly omits the LastUpdatedAt field which wasn't present in V2.
31-36: Good implementation of multi-version event publishingThe function elegantly returns both V2 and V3 balance events, maintaining backward compatibility while enabling new features.
59-77: Previous idempotency key issue correctly addressedThe V2 balance event now includes the idempotency key at line 70, as recommended in previous review comments. This ensures consistent event processing across versions.
internal/events/payment.go (4)
14-34: V3 payment payload structure is well-definedThe V3PaymentMessagePayload has a clear separation between mandatory and optional fields with appropriate JSON tags. The structure captures all the necessary payment information.
36-54: V2 payment payload maintains type consistencyThe V2PaymentMessagePayload properly maintains the original types for fields like Type, Status, and Scheme (using models.PaymentType instead of string), maintaining type safety and backward compatibility.
56-61: Function signature updated to support multiple versionsThe NewEventSavedPayments function now accepts a payment adjustment parameter and returns multiple event messages, correctly implementing the multi-version support requirement.
116-158: Different API paths in V2 and V3 linksGood attention to detail in using different API paths for links in the V2 payload:
- V3 uses
/api/payments/v3/accounts/- V2 uses
/api/payments/accounts/This ensures that links direct to the appropriate API version endpoints.
internal/events/bank_account.go (3)
33-42: V2BankAccountMessagePayload field mapping is intentionalThe RelatedAccounts field is intentionally mapped to JSON field "adjustments" rather than "relatedAccounts" for backward compatibility with the V2 API, as noted in the retrieved learning.
51-54: Improved error handlingThe error handling in NewEventSavedBankAccounts has been improved to return nil instead of an empty event message in case of an error, which is more idiomatic Go pattern.
107-149: V2 bank account event properly implementedThe toV2BankAccountEvent function correctly:
- Builds the V2 payload with the appropriate fields
- Handles optional fields properly
- Uses ToV2Provider for proper provider name formatting
- Includes the idempotency key (addressing a previous review comment)
- Sets the correct version and event type
The function is well-implemented and maintains backward compatibility.
internal/events/payment_initiation.go (3)
13-49: Validate serialization strategy for these V3 message payload structs.
All fields appear correctly typed and consistent with the existing naming patterns. It's good that large integer values are stored in*big.Intto avoid overflow. However, ensure that the JSON serialization for*big.Intis handled gracefully and tested at runtime, especially for edge cases involving very large numbers or nil values.
50-74: Confirm error handling approach in V2 payloads.
TheErrorfield is declared asstringrather than*string, which means an empty string will represent the absence of an error. This works fine so long as the downstream consumers of these events treat empty errors as “no error.” Verify that no conflicting assumptions exist in downstream integrations expecting a null or omitted field.
76-214: Verify correctness of newly introduced multi-event return pattern.
These methods now return both V2 and V3 events ([]publish.EventMessage). The approach is consistent with supporting both event versions in parallel. Ensure that callers expecting a single event are updated accordingly and that no code path incorrectly relies on the previous single-event behavior.internal/events/connector.go (2)
22-27: Multi-event publication for connector reset is a good improvement.
Returning both V2 and V3 event messages inNewEventResetConnectoraligns well with the approach in other event-related functions. No issues spotted.
29-55: Consider verifying envir 6D40 onment date usage.
Each event’sDateis set totime.Now().UTC()rather than usingatfrom the caller. This is a typical practice; however, if accurate historical timestamps are required for resets, ensure that using the current time here is consistent with business needs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/events/payment.go (1)
63-114: Helper functions are methods instead of package functions.Unlike the implementation in account.go and balance.go where helper functions are package functions, here they're implemented as methods on the Events struct. While functionally equivalent, this creates an inconsistency in the implementation pattern across files.
Consider either:
- Changing these to package functions to match the implementation in other files, or
- Updating the other files to use methods for consistency
I also note that the API paths in the Links are version-specific:
- V3:
/api/payments/v3/accounts/- V2:
/api/payments/accounts/This is correct and maintains backward compatibility.
Also applies to: 116-167
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
internal/events/account.go(3 hunks)internal/events/balance.go(3 hunks)internal/events/bank_account.go(3 hunks)internal/events/connector.go(1 hunks)internal/events/payment.go(3 hunks)internal/events/payment_initiation.go(6 hunks)internal/events/pool.go(2 hunks)test/e2e/api_payment_initiations_test.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/api_payment_initiations_test.go
- internal/events/pool.go
🧰 Additional context used
🧠 Learnings (1)
internal/events/bank_account.go (1)
Learnt from: paul-nicolas
PR: formancehq/payments#394
File: internal/events/bank_account.go:33-42
Timestamp: 2025-04-01T11:39:55.104Z
Learning: In the V2BankAccountMessagePayload struct, the RelatedAccounts field is intentionally mapped to JSON field "adjustments" rather than "relatedAccounts" for backward compatibility with v2 API.
🧬 Code Definitions (6)
internal/events/account.go (3)
internal/models/accounts.go (1)
Account(29-55)pkg/events/event.go (4)
V3EventVersion(7-7)V3EventTypeSavedAccounts(23-23)V2EventVersion(6-6)V2EventTypeSavedAccounts(13-13)internal/models/connectors.go (1)
ToV2Provider(114-141)
internal/events/balance.go (3)
internal/events/events.go (1)
Events(11-15)pkg/events/event.go (5)
V3EventVersion(7-7)V3EventTypeSavedBalances(24-24)EventApp(8-8)V2EventVersion(6-6)V2EventTypeSavedBalances(14-14)internal/models/connectors.go (1)
ToV2Provider(114-141)
internal/events/payment.go (4)
internal/events/events.go (1)
Events(11-15)internal/models/payments.go (1)
Payment(57-97)pkg/events/event.go (5)
V3EventVersion(7-7)V3EventTypeSavedPayments(22-22)EventApp(8-8)V2EventVersion(6-6)V2EventTypeSavedPayments(12-12)internal/models/connectors.go (1)
ToV2Provider(114-141)
internal/events/connector.go (2)
internal/events/events.go (1)
Events(11-15)pkg/events/event.go (5)
EventApp(8-8)V3EventVersion(7-7)V3EventTypeConnectorReset(26-26)V2EventVersion(6-6)V2EventTypeConnectorReset(18-18)
internal/events/bank_account.go (3)
internal/models/bank_accounts.go (1)
BankAccount(26-39)pkg/events/event.go (5)
V3EventVersion(7-7)V3EventTypeSavedBankAccount(25-25)EventApp(8-8)V2EventVersion(6-6)V2EventTypeSavedBankAccount(15-15)internal/models/connectors.go (1)
ToV2Provider(114-141)
internal/events/payment_initiation.go (6)
internal/events/events.go (1)
Events(11-15)internal/models/payment_initiations.go (1)
PaymentInitiation(35-66)pkg/events/event.go (7)
V3EventVersion(7-7)V3EventTypeSavedPaymentInitiation(27-27)EventApp(8-8)V2EventVersion(6-6)V2EventTypeSavedTransferInitiation(16-16)V3EventTypeSavedPaymentInitiationAdjustment(28-28)V3EventTypeSavedPaymentInitiationRelatedPayment(29-29)internal/models/connectors.go (1)
ToV2Provider(114-141)internal/models/payment_initiation_adjustments.go (1)
PaymentInitiationAdjustment(12-28)internal/models/payment_initation_related_payments.go (1)
PaymentInitiationRelatedPayments(3-9)
🔇 Additional comments (30)
internal/events/payment_initiation.go (8)
13-30: Naming convention change reflects versioning strategy effectivelyThe renaming of payload structs to include version prefixes provides clear indication of which event version each struct supports. This improves code readability and makes the versioning strategy explicit throughout the codebase.
Also applies to: 32-43, 45-48
50-66: New V2 transfer initiation payload structure looks well-designedThe V2 payload follows a slightly different naming convention (e.g.,
ConnectorIdinstead ofConnectorID) which appears to be intentional to maintain backward compatibility with existing V2 API consumers. The additional fields likeAttempts,Status, andRelatedPaymentsprovide necessary context for V2 events.
76-81: Good approach to supporting dual event publishingConverting the function to return a slice of events enables publishing both V2 and V3 events simultaneously, which aligns well with the PR objective of supporting both event versions.
173-193: Different error handling between V2 and V3 event payloadsI notice that error handling differs between V2 and V3 payloads:
- V2 returns empty string for null errors (lines 187-192)
- V3 returns nil for null errors (lines 153-159)
This difference appears intentional to match the respective API expectations, but worth confirming this is the desired behavior.
251-257: RelatedPayments implementation as an arrayThe V2 payload structure uses an array for
RelatedPaymentswhich is initialized and populated with a single entry. This suggests the design supports multiple related payments, though the current implementation only adds one. This gives flexibility for future extensions if needed.
205-213:✅ Verification successful
API enhancement with additional parameters
This function now requires both the payment initiation and a status parameter to generate complete V2 events. Make sure all callers have been updated accordingly.
🏁 Script executed:
#!/bin/bash # Find all calls to NewEventSavedPaymentInitiationRelatedPayment to verify they've been updated rg "NewEventSavedPaymentInitiationRelatedPayment" -A 3 -B 3Length of output: 1501
Action Required: Confirm Call Sites Update for Enhanced API Parameters
The function
NewEventSavedPaymentInitiationRelatedPayment(ininternal/events/payment_initiation.go) now accepts both a payment initiation (pi) and a status parameter to produce complete V2 events. I verified that the update is reflected in its usage—most notably, ininternal/connectors/engine/activities/events_send_payment_initiation_related_payments.go, where the 9E88 function is correctly called with the updated signature.
- Location 1:
internal/events/payment_initiation.go– The function implementation now returns both V2 and V3 events.- Location 2:
internal/connectors/engine/activities/events_send_payment_initiation_related_payments.go– The function call correctly passes therelatedPayment,pi, andstatusparameters.It appears that all call sites have been updated accordingly. Please review to ensure no other instances or callers were missed.
110-127:✅ Verification successful
Verify the hardcoded status value in V2 events
The V2 event generation hardcodes the status to
WAITING_FOR_VALIDATION. Is this always the correct initial status for all transfer initiations? Consider making this more dynamic if there are cases where a different initial status might be appropriate.
🏁 Script executed:
#!/bin/bash # Find where PAYMENT_INITIATION_ADJUSTMENT_STATUS_WAITING_FOR_VALIDATION is used # to verify if it's always the initial status # Search for other possible status values and their usage rg "PAYMENT_INITIATION_ADJUSTMENT_STATUS_" -A 2 -B 2Length of output: 65589
Confirmation: The initial status remains correctly hardcoded to WAITING_FOR_VALIDATION
After reviewing the usage across tests, migrations, and various workflow implementations, it’s clear that WAITING_FOR_VALIDATION is consistently used as the default initial status for transfer initiation events. There’s no evidence in the codebase suggesting that a different initial status is expected or required at this stage. The dynamic setting of this status isn’t necessary under the current design.
139-144:✅ Verification successful
API change requiring additional parameter
The function now requires the full payment initiation object to generate V2 events. This change is necessary but could impact existing callers of this function, so ensure all call sites have been updated.
🏁 Script executed:
#!/bin/bash # Find all calls to NewEventSavedPaymentInitiationAdjustment to verify they've been updated rg "NewEventSavedPaymentInitiationAdjustment" -A 3 -B 3Length of output: 1547
API Change Verified: Additional Parameter Update
We’ve confirmed that the call to
NewEventSavedPaymentInitiationAdjustment(now requiring bothadjand the fullpiparameters) is updated appropriately. In particular, the call ininternal/connectors/engine/activities/events_send_payment_initiation_adjustment.gocorrectly passes thepiparameter. Please ensure that any additional call sites (if applicable) are aligned with this updated API.internal/events/account.go (5)
12-26: Account payload structures look good, with appropriate field definitions.The V3AccountMessagePayload structure correctly defines the expected fields with appropriate JSON tags and field types. The optional fields are properly marked with
omitempty.
28-37: V2AccountMessagePayload follows the expected V2 naming conventions.The V2 payload structure appropriately uses different JSON field naming conventions (e.g.,
connectorIdinstead ofconnectorID) to maintain backward compatibility with V2 clients. Note that the V2 payload doesn't include theRawDatafield that's in the V3 payload, which is fine for backward compatibility.
39-44: Return value signature change supports both V2 and V3 events.Good implementation of the function to return both V2 and V3 event messages as a slice. This allows the system to publish events in both formats for backward compatibility.
46-74: V3 event generation is properly implemented.The V3 event generation correctly:
- Constructs the payload with all required fields
- Handles optional fields with appropriate null checks
- Uses the V3 event version and type constants
- Sets the proper IdempotencyKey
76-102: V2 event generation properly handles different field naming requirements.The V2 event generation function appropriately:
- Uses the V2Provider conversion function
- Maps the Name field to AccountName for V2 compatibility
- Uses the correct V2 event version and type constants
internal/events/balance.go (4)
12-20: Balance payload structures follow the same pattern as the account payloads.The V3BalanceMessagePayload structure correctly includes all necessary fields with appropriate JSON tags. The structure is well-defined and aligned with the model structure.
22-29: V2BalanceMessagePayload properly implements V2 naming conventions.The structure appropriately follows V2 naming convention (
connectorIdinstead ofconnectorID). However, it notably doesn't include theLastUpdatedAtfield present in the V3 payload, which is likely an intentional difference between versions.
31-36: The function signature change appropriately returns both event formats.The updated function returns both V2 and V3 events as a slice, maintaining the same pattern used in other event types for consistency.
59-76: V2 balance event now includes the IdempotencyKey.The implementation now includes the IdempotencyKey in the V2 balance event, addressing the issue flagged in a previous review comment. This is important for ensuring event idempotency across event versions.
internal/events/payment.go (3)
14-35: V3PaymentMessagePayload covers all necessary payment properties.The structure is well-designed with clear separation between mandatory and optional fields, using
omitemptyappropriately for optional fields. The JSON tags follow V3 naming conventions.
36-54: V2PaymentMessagePayload correctly maintains type information.Notably, this V2 structure maintains the original Go types (models.PaymentType, models.PaymentStatus, models.PaymentScheme) while the V3 structure uses string types. This preserves the existing behavior of V2 events while allowing V3 to use a more flexible approach.
56-61: The function signature change returns both event formats.The function now returns a slice of events, maintaining consistency with other event types in the codebase.
internal/events/connector.go (3)
12-20: Connector payload structures appropriately differentiate between V2 and V3.The structures are simple and the only difference is the JSON tag casing convention (
connectorIdvsconnectorID), which is consistent with other V2/V3 payload definitions in the codebase.
22-27: Function signature change returns both event versions.The updated function returns both V2 and V3 events, maintaining consistency with the pattern used for other event types.
29-55:✅ Verification successful
Helper functions use the same idempotency key generation.
Both event versions correctly use the same idempotency key generation function, ensuring that the events are treated as duplicates by consumers that might receive both versions.
Note that unlike in payment.go, these are package functions rather than methods on the Events struct, which is consistent with account.go and balance.go but creates an inconsistency with payment.go.
To verify the consistency of helper function implementation across event files:
🏁 Script executed:
#!/bin/bash # Check if other event files use methods or package functions for event creation helpers echo "Checking implementation pattern in event files:" grep -A 2 "func toV" internal/events/*.go | grep -v "_test.go"Length of output: 5034
Consistency Verified for Connector Event Helpers
- The connector event functions in internal/events/connector.go consistently use the same
resetConnectorIdempotencyKeyfor both V2 and V3 events. This ensures that duplicate events are recognized by downstream consumers.- A grep across event files confirms that the package-level helper functions in files like account.go, balance.go, and bank_account.go follow a similar implementation pattern.
- Note that the pattern differs from payment.go, where event helpers are implemented as methods on the
Eventsstruct. While this inconsistency is intentional and aligns with the design in several modules, you might want to review if standardizing the pattern across all event types could improve code clarity.internal/events/bank_account.go (7)
11-24: Type refactoring to support versioning looks goodThe renaming of the existing types to include the V3 prefix aligns well with the PR objective to support both V2 and V3 events. The field types and JSON mappings are preserved which ensures backward compatibility.
26-31: Clean renaming of related accounts payloadThe V3BankAccountRelatedAccountsPayload maintains the same structure as before, just with updated naming.
33-42: V2 payload structure matches V2 API expectationsThe V2BankAccountMessagePayload correctly maps the RelatedAccounts field to "adjustments" in JSON as needed for backward compatibility with V2 API. I also notice that there's no Metadata field in the V2 payload, which appears to be a deliberate difference between V2 and V3.
44-49: V2 related accounts payload structure is correctThe structure matches the V3 version, ensuring consistency in the data model while maintaining different serialization formats.
51-60: Return type change enables multi-version event publishingThe function now correctly returns an array of events to support both V2 and V3 formats simultaneously. The error handling has also been improved by returning
nilinstead of an empty message when an error occurs.
62-105: V3 event creation function updated correctlyThe function has been appropriately renamed and updated to use the V3-specific types and constants. The core logic remains the same, maintaining the existing behavior while clearly indicating this is for V3 events.
107-149: V2 event creation function looks goodThe implementation for creating V2 events follows the same pattern as the V3 version with appropriate adjustments:
- Uses V2-specific payload types
- Correctly maps providers using ToV2Provider
- Uses V2-specific event constants
- Properly omits the Metadata field which isn't part of V2
The IdempotencyKey is also correctly included, addressing a concern from a previous review.
This reverts commit 8aa0af1.
Fixes PMNT-88