feat(s5): infrastructure persistence — JPA entities, repos, adapters, 24 ITs (STA-149, STA-150)#154
Merged
Puneethkumarck merged 2 commits intoMar 10, 2026
Conversation
… 24 ITs (STA-149, STA-150) 3 JPA entities (PayoutOrder with @Version optimistic locking, StablecoinRedemption, OffRampTransaction with @JdbcTypeCode(SqlTypes.JSON) for JSONB), 3 Spring Data JPA repositories, 4 MapStruct mappers (including PayoutOrderEntityUpdater for in-place Hibernate update), 3 persistence adapters implementing domain ports, V2 migration adding BankAccount (4 cols) and MobileMoneyAccount (3 cols) to payout_orders. 24 integration tests: 13 PayoutOrder (CRUD, state machine happy/failure paths, unique constraints, findBy queries), 5 StablecoinRedemption (save, findById, findByPayoutId, ticker round-trip), 6 OffRampTransaction (append-only, JSONB round-trip, multiple per payout). 181 total tests (149 unit + 8 ArchUnit + 24 IT). Closes STA-149 Closes STA-150 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughAdds persistent storage for OffRampTransaction, PayoutOrder, and StablecoinRedemption: JPA entities, Spring Data repositories, MapStruct mappers/updaters, repository adapters, integration tests, and a Flyway migration to flatten bank/mobile-money fields on payout_orders. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@fiat-off-ramp/fiat-off-ramp/src/integration-test/java/com/stablecoin/payments/offramp/infrastructure/persistence/PayoutOrderPersistenceAdapterIT.java`:
- Around line 156-221: Add a new optimistic-locking integration test in
PayoutOrderPersistenceAdapterIT (e.g., shouldThrowOnStaleUpdate) that exercises
PayoutOrderEntity's `@Version` behavior: save an initial entity via
adapter.save(aPendingOrder()), load the same entity twice into two variables
(e.g., first = adapter.findById(...).get() and second =
adapter.findById(...).get()), mutate and save the first
(adapter.save(first.startRedemption())), then mutate the stale second and assert
that saving it (adapter.save(second.startRedemption()) or similar) throws
org.springframework.orm.ObjectOptimisticLockingFailureException using
assertThrows; ensure you reference adapter.save and the entity's
payoutId()/findById lookup so the test mirrors the existing tests' style.
In
`@fiat-off-ramp/fiat-off-ramp/src/integration-test/java/com/stablecoin/payments/offramp/infrastructure/persistence/StablecoinRedemptionPersistenceAdapterIT.java`:
- Around line 31-92: Add an integration test that verifies adapter.save()
behavior when saving a StablecoinRedemption with an existing redemptionId:
create a redemption via StablecoinRedemption.create(...), save it with
adapter.save(redemption), then attempt to save a second object with the same
redemptionId (either by reusing the same instance or by constructing another
StablecoinRedemption with the same redemptionId) and assert the intended
insert-only contract—either expect an exception from adapter.save(...) or call
adapter.findById(saved.redemptionId()) afterwards and assert the row was not
changed (compare to the original saved using usingRecursiveComparison with
BigDecimal comparator and ignoring Instant). This test should live alongside the
other ITs and reference StablecoinRedemption.create, adapter.save, and
adapter.findById to locate the code under test.
In
`@fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/entity/OffRampTransactionEntity.java`:
- Around line 19-26: Add an explicit equals/hashCode implementation for the JPA
entity by annotating OffRampTransactionEntity with
`@EqualsAndHashCode`(onlyExplicitlyIncluded = true) and mark the primary key field
(e.g., the UUID id field) with `@EqualsAndHashCode.Include`; ensure the id is used
for equality (and remains immutable once assigned) so Set/Map behavior and
Hibernate identity semantics are correct, and remove or avoid using other
mutable fields in equals/hashCode implementations.
- Around line 50-53: OffRampTransactionEntity currently persists rawResponse
(field rawResponse) as plaintext JSONB; update persistence to either encrypt
this column or minimize stored data: implement a JPA AttributeConverter (e.g.,
RawResponseEncryptor) to encrypt/decrypt rawResponse at rest (use Vault/KMS) or
change the entity mapping to store only a reduced reconciliation DTO (extract
non-PII fields before persisting) in the methods that create
OffRampTransactionEntity; add an automatic purge/retention mechanism (e.g.,
annotate entity with a TTL filter or `@Where` clause tied to an expiration
timestamp, or implement a DB trigger/job) and ensure storage is gated by
documented partner contract compliance checks before persisting.
In
`@fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/entity/OffRampTransactionJpaRepository.java`:
- Around line 8-10: The repository method findByPayoutId in
OffRampTransactionJpaRepository returns an append-only set without an order
guarantee; change the contract to enforce ordering by timestamp (e.g.,
rename/replace findByPayoutId to findByPayoutIdOrderByReceivedAtAsc) or add a
Sort parameter (keep method name and signature like findByPayoutId(UUID
payoutId, Sort sort)) so callers receive a deterministic event timeline based on
OffRampTransactionEntity.receivedAt.
In
`@fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/mapper/OffRampTransactionPersistenceMapper.java`:
- Around line 7-42: The mapper interfaces OffRampTransactionPersistenceMapper
and PayoutOrderPersistenceMapper are missing MapStruct's Spring component model
and therefore won't be registered as beans; update the `@Mapper` annotation on
both interfaces (e.g., on OffRampTransactionPersistenceMapper) to include
componentModel = "spring" so MapStruct generates Spring-managed implementations
(i.e., change `@Mapper` to `@Mapper`(componentModel = "spring") on the
OffRampTransactionPersistenceMapper and the PayoutOrderPersistenceMapper).
In
`@fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/mapper/PayoutOrderPersistenceMapper.java`:
- Around line 67-85: The mapper currently silences partially populated
bank/mobile fields by returning null; instead, in the toDomain mapping method
(the block constructing BankAccount and MobileMoneyAccount) detect partial
groups and throw an exception: if any of entity.getBankAccountNumber(),
getBankCode(), getBankAccountType(), getBankCountry() are non-null but not all
present, throw an IllegalStateException with a clear message identifying the
missing bank fields; likewise for the mobile group using
entity.getMobileMoneyProvider(), getMobileMoneyPhone(), getMobileMoneyCountry()
before calling AccountType.valueOf(...) or MobileMoneyProvider.valueOf(...).
This fail-fast behavior prevents masking corrupted/half-populated rows.
In
`@fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/PayoutOrderPersistenceAdapter.java`:
- Around line 27-33: The save method in PayoutOrderPersistenceAdapter currently
reloads the entity and overwrites it with the caller's domain state (via
updater.updateEntity and mapper.toEntity) without preserving the caller's
version, which allows stale writes to bypass optimistic locking; change save so
the domain object's version is propagated and enforced on write (e.g., require
PayoutOrder to expose a version, map that version onto the JPA entity before
calling jpa.save or use a repository save that checks the version), or instead
perform the load-and-state-transition inside the persistence layer (load current
entity, apply allowed state transition logic and increment/version the entity
within PayoutOrderPersistenceAdapter) so that concurrent updates will raise
OptimisticLockException rather than silently overwriting newer state.
In
`@fiat-off-ramp/fiat-off-ramp/src/main/resources/db/migration/V2__add_bank_and_mobile_money_columns.sql`:
- Around line 20-27: Remove the DO $$ ... END $$; block that grants SELECT to
role 'sp_readonly' (the IF EXISTS (SELECT 1 FROM pg_roles WHERE rolname =
'sp_readonly') and the GRANT SELECT ON ALL TABLES IN SCHEMA public) from this
migration; either move that TestContainers-only permission to test/bootstrap SQL
or replace the blanket GRANT with explicit GRANT statements targeted to this
service's tables only (e.g., grant on each service table), ensuring the
migration no longer alters permissions across the entire public schema.
- Around line 6-17: Existing rows will have NULLs and violate the documented
invariant; update the migration to first backfill a valid placeholder payout
destination for all existing payout_orders (e.g., run an UPDATE that sets one
group of columns to a sentinel/placeholder value such as
mobile_money_provider='UNSPECIFIED' and mobile_money_phone='UNKNOWN' or a real
default where appropriate) so every row has either bank_account_number or
mobile_money_phone non-NULL, then add a CHECK constraint on payout_orders
enforcing the invariant (e.g., CHECK (bank_account_number IS NOT NULL OR
mobile_money_phone IS NOT NULL)); finally, if desired, set sensible DEFAULTs/NOT
NULL on the new columns for future rows. Ensure you reference the payout_orders
table and the new columns (bank_account_number, bank_code, bank_account_type,
bank_country, mobile_money_provider, mobile_money_phone, mobile_money_country)
in the migration and perform backfill before adding the constraint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3e98ceb1-5373-4887-af80-48b718babd86
📒 Files selected for processing (17)
fiat-off-ramp/fiat-off-ramp/src/integration-test/java/com/stablecoin/payments/offramp/infrastructure/persistence/OffRampTransactionPersistenceAdapterIT.javafiat-off-ramp/fiat-off-ramp/src/integration-test/java/com/stablecoin/payments/offramp/infrastructure/persistence/PayoutOrderPersistenceAdapterIT.javafiat-off-ramp/fiat-off-ramp/src/integration-test/java/com/stablecoin/payments/offramp/infrastructure/persistence/StablecoinRedemptionPersistenceAdapterIT.javafiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/OffRampTransactionPersistenceAdapter.javafiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/PayoutOrderPersistenceAdapter.javafiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/StablecoinRedemptionPersistenceAdapter.javafiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/entity/OffRampTransactionEntity.javafiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/entity/OffRampTransactionJpaRepository.javafiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/entity/PayoutOrderEntity.javafiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/entity/PayoutOrderJpaRepository.javafiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/entity/StablecoinRedemptionEntity.javafiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/entity/StablecoinRedemptionJpaRepository.javafiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/mapper/OffRampTransactionPersistenceMapper.javafiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/mapper/PayoutOrderEntityUpdater.javafiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/mapper/PayoutOrderPersistenceMapper.javafiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/mapper/StablecoinRedemptionPersistenceMapper.javafiat-off-ramp/fiat-off-ramp/src/main/resources/db/migration/V2__add_bank_and_mobile_money_columns.sql
Comment on lines
+31
to
+92
| @Test | ||
| @DisplayName("should save and retrieve redemption by id") | ||
| void shouldSaveAndRetrieveById() { | ||
| var order = savePayoutOrder(); | ||
| var redemption = StablecoinRedemption.create( | ||
| order.payoutId(), aStablecoinTicker(), | ||
| new BigDecimal("1000.00"), new BigDecimal("920.00"), | ||
| "EUR", "circle", "circle_ref_001"); | ||
| var saved = adapter.save(redemption); | ||
|
|
||
| assertThat(adapter.findById(saved.redemptionId())).isPresent().get() | ||
| .usingRecursiveComparison() | ||
| .withComparatorForType(BigDecimal::compareTo, BigDecimal.class) | ||
| .ignoringFieldsOfTypes(Instant.class) | ||
| .isEqualTo(saved); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("should find redemption by payout id") | ||
| void shouldFindByPayoutId() { | ||
| var order = savePayoutOrder(); | ||
| var redemption = StablecoinRedemption.create( | ||
| order.payoutId(), aStablecoinTicker(), | ||
| new BigDecimal("1000.00"), new BigDecimal("920.00"), | ||
| "EUR", "circle", "circle_ref_002"); | ||
| var saved = adapter.save(redemption); | ||
|
|
||
| assertThat(adapter.findByPayoutId(order.payoutId())).isPresent().get() | ||
| .usingRecursiveComparison() | ||
| .withComparatorForType(BigDecimal::compareTo, BigDecimal.class) | ||
| .ignoringFieldsOfTypes(Instant.class) | ||
| .isEqualTo(saved); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("should return empty when id not found") | ||
| void shouldReturnEmptyWhenIdNotFound() { | ||
| assertThat(adapter.findById(UUID.randomUUID())).isEmpty(); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("should return empty when payout id not found") | ||
| void shouldReturnEmptyWhenPayoutIdNotFound() { | ||
| assertThat(adapter.findByPayoutId(UUID.randomUUID())).isEmpty(); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("should persist all stablecoin ticker fields via round-trip") | ||
| void shouldPersistStablecoinTickerRoundTrip() { | ||
| var order = savePayoutOrder(); | ||
| var redemption = StablecoinRedemption.create( | ||
| order.payoutId(), aStablecoinTicker(), | ||
| new BigDecimal("500.00"), new BigDecimal("460.00"), | ||
| "EUR", "circle", "circle_ref_003"); | ||
| var saved = adapter.save(redemption); | ||
|
|
||
| assertThat(adapter.findById(saved.redemptionId())).isPresent().get() | ||
| .usingRecursiveComparison() | ||
| .withComparatorForType(BigDecimal::compareTo, BigDecimal.class) | ||
| .ignoringFieldsOfTypes(Instant.class) | ||
| .isEqualTo(saved); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "StablecoinRedemption.java" | head -5Repository: Puneethkumarck/stablebridge-platform
Length of output: 194
🏁 Script executed:
find . -type f -name "*StablecoinRedemptionPersistenceAdapter*" | head -10Repository: Puneethkumarck/stablebridge-platform
Length of output: 387
🏁 Script executed:
find . -type f -name "StablecoinRedemptionPersistenceAdapterIT.java" | head -5Repository: Puneethkumarck/stablebridge-platform
Length of output: 240
🏁 Script executed:
cat -n "./fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/domain/model/StablecoinRedemption.java"Repository: Puneethkumarck/stablebridge-platform
Length of output: 3222
🏁 Script executed:
cat -n "./fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/StablecoinRedemptionPersistenceAdapter.java"Repository: Puneethkumarck/stablebridge-platform
Length of output: 1700
🏁 Script executed:
cat -n "./fiat-off-ramp/fiat-off-ramp/src/integration-test/java/com/stablecoin/payments/offramp/infrastructure/persistence/StablecoinRedemptionPersistenceAdapterIT.java"Repository: Puneethkumarck/stablebridge-platform
Length of output: 4793
🏁 Script executed:
find . -type f -name "*StablecoinRedemption*Entity*" -o -name "*StablecoinRedemption*JpaRepository*" | head -10Repository: Puneethkumarck/stablebridge-platform
Length of output: 370
🏁 Script executed:
find . -type f -name "*.sql" -o -name "*schema*" -o -name "*migration*" | grep -i stablecoin | head -10Repository: Puneethkumarck/stablebridge-platform
Length of output: 62
🏁 Script executed:
find . -path "*/db/migration*" -o -path "*/resources/db*" | head -5Repository: Puneethkumarck/stablebridge-platform
Length of output: 495
🏁 Script executed:
find ./fiat-off-ramp -path "*/db/migration*" -type f | head -20Repository: Puneethkumarck/stablebridge-platform
Length of output: 268
🏁 Script executed:
cat -n "./fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/entity/StablecoinRedemptionEntity.java"Repository: Puneethkumarck/stablebridge-platform
Length of output: 1946
🏁 Script executed:
cat -n "./fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/entity/StablecoinRedemptionJpaRepository.java"Repository: Puneethkumarck/stablebridge-platform
Length of output: 530
🏁 Script executed:
cat -n "./fiat-off-ramp/fiat-off-ramp/src/main/resources/db/migration/V1__initial_schema.sql"Repository: Puneethkumarck/stablebridge-platform
Length of output: 7491
🏁 Script executed:
cat -n "./fiat-off-ramp/fiat-off-ramp/src/main/resources/db/migration/V2__add_bank_and_mobile_money_columns.sql"Repository: Puneethkumarck/stablebridge-platform
Length of output: 1595
🏁 Script executed:
rg -A 5 "insert.only|write.once|immutable" --type java ./fiat-off-ramp/fiat-off-ramp/src/main/javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 62
🏁 Script executed:
rg -B 3 -A 3 "findByPayoutId" ./fiat-off-ramp/fiat-off-ramp/src/main/java --type javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 5997
🏁 Script executed:
rg "StablecoinRedemption" ./fiat-off-ramp/fiat-off-ramp/src/main/java --type java -lRepository: Puneethkumarck/stablebridge-platform
Length of output: 909
🏁 Script executed:
cat -n "./fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/domain/port/StablecoinRedemptionRepository.java"Repository: Puneethkumarck/stablebridge-platform
Length of output: 613
🏁 Script executed:
rg "\.save\(" ./fiat-off-ramp/fiat-off-ramp/src/main/java -B 2 -A 2 --type java | grep -A 4 -B 4 "StablecoinRedemption"Repository: Puneethkumarck/stablebridge-platform
Length of output: 1550
🏁 Script executed:
rg -i "insert.only|write.once|idempotent|immutable" ./fiat-off-ramp --type java --type md | head -30Repository: Puneethkumarck/stablebridge-platform
Length of output: 275
🏁 Script executed:
find ./fiat-off-ramp -name "README*" -o -name "*.md" | xargs grep -l "redemption" 2>/dev/null | head -5Repository: Puneethkumarck/stablebridge-platform
Length of output: 62
Add an integration test for duplicate redemption saves.
The adapter uses standard JpaRepository.save(), which performs an UPDATE when the primary key already exists. Since StablecoinRedemption.create() generates a new redemptionId, the suite never proves whether a second save() with an existing ID is rejected or overwrites the original. If the insert-only contract is intended, lock it down with an IT—either asserting that it throws an exception on duplicate redemptionId or that it returns the original row unchanged.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-off-ramp/fiat-off-ramp/src/integration-test/java/com/stablecoin/payments/offramp/infrastructure/persistence/StablecoinRedemptionPersistenceAdapterIT.java`
around lines 31 - 92, Add an integration test that verifies adapter.save()
behavior when saving a StablecoinRedemption with an existing redemptionId:
create a redemption via StablecoinRedemption.create(...), save it with
adapter.save(redemption), then attempt to save a second object with the same
redemptionId (either by reusing the same instance or by constructing another
StablecoinRedemption with the same redemptionId) and assert the intended
insert-only contract—either expect an exception from adapter.save(...) or call
adapter.findById(saved.redemptionId()) afterwards and assert the row was not
changed (compare to the original saved using usingRecursiveComparison with
BigDecimal comparator and ignoring Instant). This test should live alongside the
other ITs and reference StablecoinRedemption.create, adapter.save, and
adapter.findById to locate the code under test.
Comment on lines
+19
to
+26
| @Entity | ||
| @Table(name = "off_ramp_transactions") | ||
| @Getter | ||
| @Setter | ||
| @NoArgsConstructor | ||
| @AllArgsConstructor | ||
| @Builder | ||
| public class OffRampTransactionEntity { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding @EqualsAndHashCode(onlyExplicitlyIncluded = true) with ID-based identity.
JPA entities without proper equals/hashCode can misbehave in Set/Map collections and Hibernate's persistence context, especially during merge operations. Standard approach for entities with application-assigned UUIDs:
♻️ Proposed fix
+import lombok.EqualsAndHashCode; `@Getter`
`@Setter`
`@NoArgsConstructor`
`@AllArgsConstructor`
`@Builder`
+@EqualsAndHashCode(onlyExplicitlyIncluded = true)
public class OffRampTransactionEntity {
`@Id`
`@Column`(name = "offramp_txn_id", updatable = false, nullable = false)
+ `@EqualsAndHashCode.Include`
private UUID offRampTxnId;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Entity | |
| @Table(name = "off_ramp_transactions") | |
| @Getter | |
| @Setter | |
| @NoArgsConstructor | |
| @AllArgsConstructor | |
| @Builder | |
| public class OffRampTransactionEntity { | |
| `@Entity` | |
| `@Table`(name = "off_ramp_transactions") | |
| `@Getter` | |
| `@Setter` | |
| `@NoArgsConstructor` | |
| `@AllArgsConstructor` | |
| `@Builder` | |
| `@EqualsAndHashCode`(onlyExplicitlyIncluded = true) | |
| public class OffRampTransactionEntity { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/entity/OffRampTransactionEntity.java`
around lines 19 - 26, Add an explicit equals/hashCode implementation for the JPA
entity by annotating OffRampTransactionEntity with
`@EqualsAndHashCode`(onlyExplicitlyIncluded = true) and mark the primary key field
(e.g., the UUID id field) with `@EqualsAndHashCode.Include`; ensure the id is used
for equality (and remains immutable once assigned) so Set/Map behavior and
Hibernate identity semantics are correct, and remove or avoid using other
mutable fields in equals/hashCode implementations.
Comment on lines
+50
to
+53
| @Builder.Default | ||
| @JdbcTypeCode(SqlTypes.JSON) | ||
| @Column(name = "raw_response", nullable = false, columnDefinition = "JSONB") | ||
| private String rawResponse = "{}"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's encryption or masking logic applied before persistence
rg -n "rawResponse|raw_response" --type java -C3Repository: Puneethkumarck/stablebridge-platform
Length of output: 26087
🏁 Script executed:
# Check for encryption configuration in application properties/yml files
find . -name "application*.yml" -o -name "application*.properties" | head -20Repository: Puneethkumarck/stablebridge-platform
Length of output: 1762
🏁 Script executed:
# Search for any encryption-related configuration or annotations
rg -i "encrypt|cipher|pgp|tde" --type yaml --type properties --type java | head -40Repository: Puneethkumarck/stablebridge-platform
Length of output: 114
🏁 Script executed:
# Check for data retention or purge policies
rg -i "retention|purge|ttl|delete.*older|gdpr|ccpa" --type java | head -40Repository: Puneethkumarck/stablebridge-platform
Length of output: 7605
🏁 Script executed:
# Look at the mapper to see if there's any sanitization before persistence
cat fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/mapper/OffRampTransactionPersistenceMapper.javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 1669
🏁 Script executed:
# Check if there's any documentation or comments about data handling
rg -i "pii|sensitive|personal.*information|account.*number|payment.*data" --type java -B2 -A2 | head -60Repository: Puneethkumarck/stablebridge-platform
Length of output: 9246
🏁 Script executed:
# Search for `@Convert`, AttributeConverter, or encryption handlers
rg "@Convert|AttributeConverter|Converter.*encrypt" --type javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 62
🏁 Script executed:
# Check if the domain model applies any masking before mapper
cat fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/domain/model/OffRampTransaction.java | head -70Repository: Puneethkumarck/stablebridge-platform
Length of output: 2418
Encrypt rawResponse JSONB or implement strict data minimization policy.
Partner responses are persisted unencrypted containing transaction IDs and sensitive identifiers (confirmed by integration tests). Payment data of this nature requires:
- Column-level encryption (TDE, Vault integration, or JPA
@Converterwith encryption) OR data minimization (store only non-PII fields required for reconciliation) - Document partner contract terms permitting storage and compliance with GDPR Article 5 (data minimization) and CCPA
- Implement automatic purge policy (e.g., via Hibernate
@Wherewith expiration or database trigger)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/entity/OffRampTransactionEntity.java`
around lines 50 - 53, OffRampTransactionEntity currently persists rawResponse
(field rawResponse) as plaintext JSONB; update persistence to either encrypt
this column or minimize stored data: implement a JPA AttributeConverter (e.g.,
RawResponseEncryptor) to encrypt/decrypt rawResponse at rest (use Vault/KMS) or
change the entity mapping to store only a reduced reconciliation DTO (extract
non-PII fields before persisting) in the methods that create
OffRampTransactionEntity; add an automatic purge/retention mechanism (e.g.,
annotate entity with a TTL filter or `@Where` clause tied to an expiration
timestamp, or implement a DB trigger/job) and ensure storage is gated by
documented partner contract compliance checks before persisting.
Comment on lines
+7
to
+42
| @Mapper | ||
| public interface OffRampTransactionPersistenceMapper { | ||
|
|
||
| default OffRampTransactionEntity toEntity(OffRampTransaction transaction) { | ||
| if (transaction == null) { | ||
| return null; | ||
| } | ||
| return OffRampTransactionEntity.builder() | ||
| .offRampTxnId(transaction.offRampTxnId()) | ||
| .payoutId(transaction.payoutId()) | ||
| .partnerName(transaction.partnerName()) | ||
| .eventType(transaction.eventType()) | ||
| .amount(transaction.amount()) | ||
| .currency(transaction.currency()) | ||
| .status(transaction.status()) | ||
| .rawResponse(transaction.rawResponse()) | ||
| .receivedAt(transaction.receivedAt()) | ||
| .build(); | ||
| } | ||
|
|
||
| default OffRampTransaction toDomain(OffRampTransactionEntity entity) { | ||
| if (entity == null) { | ||
| return null; | ||
| } | ||
| return new OffRampTransaction( | ||
| entity.getOffRampTxnId(), | ||
| entity.getPayoutId(), | ||
| entity.getPartnerName(), | ||
| entity.getEventType(), | ||
| entity.getAmount(), | ||
| entity.getCurrency(), | ||
| entity.getStatus(), | ||
| entity.getRawResponse(), | ||
| entity.getReceivedAt() | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how the persistence mappers are instantiated.
rg -n -C2 'OffRampTransactionPersistenceMapper|PayoutOrderPersistenceMapper' fiat-off-ramp/fiat-off-ramp/src/main/java
rg -n -C2 'Mappers\.getMapper|componentModel\s*=\s*"spring"' fiat-off-ramp/fiat-off-ramp/src/main/javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 5713
Configure MapStruct mappers for Spring dependency injection.
Both OffRampTransactionPersistenceMapper and PayoutOrderPersistenceMapper are injected as Spring dependencies but lack componentModel = "spring". MapStruct will not register them as beans, causing a NoSuchBeanDefinitionException at runtime. Add componentModel = "spring" to the @Mapper annotation:
`@Mapper`(componentModel = "spring")
public interface OffRampTransactionPersistenceMapper {
// ...
}Apply the same fix to PayoutOrderPersistenceMapper.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/mapper/OffRampTransactionPersistenceMapper.java`
around lines 7 - 42, The mapper interfaces OffRampTransactionPersistenceMapper
and PayoutOrderPersistenceMapper are missing MapStruct's Spring component model
and therefore won't be registered as beans; update the `@Mapper` annotation on
both interfaces (e.g., on OffRampTransactionPersistenceMapper) to include
componentModel = "spring" so MapStruct generates Spring-managed implementations
(i.e., change `@Mapper` to `@Mapper`(componentModel = "spring") on the
OffRampTransactionPersistenceMapper and the PayoutOrderPersistenceMapper).
Comment on lines
+27
to
+33
| public PayoutOrder save(PayoutOrder order) { | ||
| var existing = jpa.findById(order.payoutId()); | ||
| if (existing.isPresent()) { | ||
| updater.updateEntity(existing.get(), order); | ||
| return mapper.toDomain(jpa.save(existing.get())); | ||
| } | ||
| return mapper.toDomain(jpa.save(mapper.toEntity(order))); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the PayoutOrderEntity class
find . -name "PayoutOrderEntity.java" -type f 2>/dev/null | head -20Repository: Puneethkumarck/stablebridge-platform
Length of output: 212
🏁 Script executed:
# Find the PayoutOrder domain class
find . -name "PayoutOrder.java" -type f 2>/dev/null | head -20Repository: Puneethkumarck/stablebridge-platform
Length of output: 185
🏁 Script executed:
# Find the PayoutOrderPersistenceAdapter to confirm the file path and see full context
find . -name "PayoutOrderPersistenceAdapter.java" -type f 2>/dev/nullRepository: Puneethkumarck/stablebridge-platform
Length of output: 217
🏁 Script executed:
cat -n ./fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/entity/PayoutOrderEntity.javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 4854
🏁 Script executed:
cat -n ./fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/domain/model/PayoutOrder.javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 18382
🏁 Script executed:
cat -n ./fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/PayoutOrderPersistenceAdapter.javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 2835
🏁 Script executed:
find . -name "PayoutOrderEntityUpdater.java" -type f 2>/dev/nullRepository: Puneethkumarck/stablebridge-platform
Length of output: 219
🏁 Script executed:
find . -name "*Updater*.java" -path "*/offramp/*" -type f 2>/dev/null | head -20Repository: Puneethkumarck/stablebridge-platform
Length of output: 219
🏁 Script executed:
cat -n ./fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/mapper/PayoutOrderEntityUpdater.javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 2924
@Version bypass enables stale writes on state machine transitions.
The reload-then-update pattern defeats optimistic locking. You fetch the current entity (gaining its latest version), overwrite its state with the caller's domain object (which carries no version), then save using the DB's current version token. If concurrent modifications occur, the stale caller's state overwrites the newer transition without triggering OptimisticLockException. Since PayoutOrder enforces strict state transitions (PENDING → REDEEMING → REDEEMED → ...) with terminal states, lost updates corrupt the order lifecycle. Pass the version through the domain object and enforce it on the write, or load-and-transition entirely within persistence boundaries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/PayoutOrderPersistenceAdapter.java`
around lines 27 - 33, The save method in PayoutOrderPersistenceAdapter currently
reloads the entity and overwrites it with the caller's domain state (via
updater.updateEntity and mapper.toEntity) without preserving the caller's
version, which allows stale writes to bypass optimistic locking; change save so
the domain object's version is propagated and enforced on write (e.g., require
PayoutOrder to expose a version, map that version onto the JPA entity before
calling jpa.save or use a repository save that checks the version), or instead
perform the load-and-state-transition inside the persistence layer (load current
entity, apply allowed state transition logic and increment/version the entity
within PayoutOrderPersistenceAdapter) so that concurrent updates will raise
OptimisticLockException rather than silently overwriting newer state.
Comment on lines
+6
to
+17
| -- At least one of bank or mobile money must be set (application-level invariant). | ||
|
|
||
| -- BankAccount VO columns | ||
| ALTER TABLE payout_orders ADD COLUMN bank_account_number VARCHAR(50); | ||
| ALTER TABLE payout_orders ADD COLUMN bank_code VARCHAR(50); | ||
| ALTER TABLE payout_orders ADD COLUMN bank_account_type VARCHAR(20); | ||
| ALTER TABLE payout_orders ADD COLUMN bank_country VARCHAR(2); | ||
|
|
||
| -- MobileMoneyAccount VO columns | ||
| ALTER TABLE payout_orders ADD COLUMN mobile_money_provider VARCHAR(30); | ||
| ALTER TABLE payout_orders ADD COLUMN mobile_money_phone VARCHAR(30); | ||
| ALTER TABLE payout_orders ADD COLUMN mobile_money_country VARCHAR(2); |
There was a problem hiding this comment.
Backfill or constrain the new payout-destination columns.
All existing payout_orders rows will get these fields as NULL, which violates the invariant documented in this script. Add a backfill/default plan plus a check constraint before relying on these columns at read time.
As per coding guidelines, "Check that new columns have sensible defaults for existing rows".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-off-ramp/fiat-off-ramp/src/main/resources/db/migration/V2__add_bank_and_mobile_money_columns.sql`
around lines 6 - 17, Existing rows will have NULLs and violate the documented
invariant; update the migration to first backfill a valid placeholder payout
destination for all existing payout_orders (e.g., run an UPDATE that sets one
group of columns to a sentinel/placeholder value such as
mobile_money_provider='UNSPECIFIED' and mobile_money_phone='UNKNOWN' or a real
default where appropriate) so every row has either bank_account_number or
mobile_money_phone non-NULL, then add a CHECK constraint on payout_orders
enforcing the invariant (e.g., CHECK (bank_account_number IS NOT NULL OR
mobile_money_phone IS NOT NULL)); finally, if desired, set sensible DEFAULTs/NOT
NULL on the new columns for future rows. Ensure you reference the payout_orders
table and the new columns (bank_account_number, bank_code, bank_account_type,
bank_country, mobile_money_provider, mobile_money_phone, mobile_money_country)
in the migration and perform backfill before adding the constraint.
… ordered queries, new ITs (STA-150) - Fail-fast on partial bank/mobile columns in PayoutOrderPersistenceMapper - Scope sp_readonly GRANT to service-specific tables in V2 migration - Add deterministic ordering to OffRampTransactionJpaRepository (receivedAt ASC) - Add optimistic locking IT for PayoutOrderEntity @Version - Add FK constraint IT for StablecoinRedemption Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
fiat-off-ramp/fiat-off-ramp/src/main/resources/db/migration/V2__add_bank_and_mobile_money_columns.sql (1)
6-17:⚠️ Potential issue | 🟠 MajorBackfill legacy rows before relying on the new payout-destination fields.
Line 6 declares an invariant, but this migration leaves every pre-existing
payout_ordersrow with both destination variants unset because all seven new columns are nullable and have no backfill/default. That creates invalid persisted state as soon as the migration runs. Add a data backfill plan for existing rows and enforce the invariant with aCHECKconstraint once the legacy data is valid.As per coding guidelines, "Check that new columns have sensible defaults for existing rows".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fiat-off-ramp/fiat-off-ramp/src/main/resources/db/migration/V2__add_bank_and_mobile_money_columns.sql` around lines 6 - 17, Backfill existing payout_orders rows before adding the invariant: write an UPDATE that populates the new columns (bank_account_number, bank_code, bank_account_type, bank_country, mobile_money_provider, mobile_money_phone, mobile_money_country) from whatever legacy destination fields exist for each row or set a safe sentinel/default for one side per row (e.g. a known default phone or default bank representation) so every row has at least one destination populated; then add a CHECK constraint on table payout_orders (e.g. CHECK (bank_account_number IS NOT NULL OR mobile_money_phone IS NOT NULL) or a more complete expression referencing the new columns) to enforce the invariant, and only after the backfill optionally ALTER the new columns to NOT NULL if appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@fiat-off-ramp/fiat-off-ramp/src/integration-test/java/com/stablecoin/payments/offramp/infrastructure/persistence/PayoutOrderPersistenceAdapterIT.java`:
- Around line 234-250: The test loads two references from the same persistence
context so they are the same managed instance; instead, after saving the updated
first instance you must clear the persistence context and then reload the second
to get a stale-version instance. Modify shouldThrowOnStaleUpdate so you call
jpa.findById(...) to get first, update and jpa.saveAndFlush(first), then call
entityManager.clear() (not entityManager.detach(second)) and only then call
jpa.findById(...) to obtain second, change its status and assert that
jpa.saveAndFlush(second) throws ObjectOptimisticLockingFailureException.
In
`@fiat-off-ramp/fiat-off-ramp/src/integration-test/java/com/stablecoin/payments/offramp/infrastructure/persistence/StablecoinRedemptionPersistenceAdapterIT.java`:
- Around line 79-94: This test duplicates the logic in
shouldSaveAndRetrieveById; either delete shouldPersistStablecoinTickerRoundTrip
or change it to assert unique StablecoinTicker serialization details: keep the
setup using StablecoinRedemption.create and aStablecoinTicker() but replace the
broad recursive comparison with focused assertions on the StablecoinTicker
fields (e.g., compare
saved.redemptionId()/adapter.findById(saved.redemptionId()).get().ticker()
fields explicitly) so the test verifies the specific ticker serialization rather
than repeating the full round-trip used in shouldSaveAndRetrieveById.
In
`@fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/mapper/PayoutOrderPersistenceMapper.java`:
- Around line 97-145: The IllegalStateException messages in mapBankAccount and
mapMobileMoneyAccount only include boolean flags and lack the payout identifier;
update both methods to include the entity's payoutId in the thrown message by
appending entity.getPayoutId() (or formatting it into the existing formatted
string) so the exceptions read something like "Corrupted payout_orders row:
partial ... — payoutId=%s" with the payoutId included alongside the current
flags.
- Around line 62-65: The mapping currently returns null for partial
PartnerIdentifier data, causing inconsistent behavior with
BankAccount/MobileMoneyAccount; add a private helper method
mapPartnerIdentifier(PayoutOrderEntity entity) that checks
entity.getOffRampPartnerId() and entity.getOffRampPartnerName(), returns null
only if both are null, throws an IllegalStateException when exactly one is
present (use a message like "Corrupted payout_orders row %s: partial partner
columns — id=%s, name=%s".formatted(entity.getPayoutId(), hasId, hasName)), and
otherwise returns new PartnerIdentifier(entity.getOffRampPartnerId(),
entity.getOffRampPartnerName()); then replace the existing inline logic that
sets offRampPartner with a call to mapPartnerIdentifier(entity).
In
`@fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/OffRampTransactionPersistenceAdapter.java`:
- Line 15: The class-level `@Slf4j` annotation on
OffRampTransactionPersistenceAdapter is unused; either add meaningful logging
calls inside methods like save(...) and findById(...) or remove the `@Slf4j`
annotation and its import to avoid dead code—update the
OffRampTransactionPersistenceAdapter class by deleting the `@Slf4j` annotation
(and related lombok import) if you choose not to add log statements, or
alternatively add appropriate log.debug/info/error calls where operations occur
and keep the annotation.
---
Duplicate comments:
In
`@fiat-off-ramp/fiat-off-ramp/src/main/resources/db/migration/V2__add_bank_and_mobile_money_columns.sql`:
- Around line 6-17: Backfill existing payout_orders rows before adding the
invariant: write an UPDATE that populates the new columns (bank_account_number,
bank_code, bank_account_type, bank_country, mobile_money_provider,
mobile_money_phone, mobile_money_country) from whatever legacy destination
fields exist for each row or set a safe sentinel/default for one side per row
(e.g. a known default phone or default bank representation) so every row has at
least one destination populated; then add a CHECK constraint on table
payout_orders (e.g. CHECK (bank_account_number IS NOT NULL OR mobile_money_phone
IS NOT NULL) or a more complete expression referencing the new columns) to
enforce the invariant, and only after the backfill optionally ALTER the new
columns to NOT NULL if appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4b3bc224-e933-4e85-b5df-bc0b283de280
📒 Files selected for processing (6)
fiat-off-ramp/fiat-off-ramp/src/integration-test/java/com/stablecoin/payments/offramp/infrastructure/persistence/PayoutOrderPersistenceAdapterIT.javafiat-off-ramp/fiat-off-ramp/src/integration-test/java/com/stablecoin/payments/offramp/infrastructure/persistence/StablecoinRedemptionPersistenceAdapterIT.javafiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/OffRampTransactionPersistenceAdapter.javafiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/entity/OffRampTransactionJpaRepository.javafiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/mapper/PayoutOrderPersistenceMapper.javafiat-off-ramp/fiat-off-ramp/src/main/resources/db/migration/V2__add_bank_and_mobile_money_columns.sql
Comment on lines
+234
to
+250
| @Test | ||
| @DisplayName("should throw on stale version update (optimistic locking)") | ||
| void shouldThrowOnStaleUpdate() { | ||
| var saved = adapter.save(aPendingOrder()); | ||
|
|
||
| var first = jpa.findById(saved.payoutId()).orElseThrow(); | ||
| var second = jpa.findById(saved.payoutId()).orElseThrow(); | ||
|
|
||
| first.setStatus(PayoutStatus.REDEEMING); | ||
| jpa.saveAndFlush(first); | ||
|
|
||
| entityManager.detach(second); | ||
| second.setStatus(PayoutStatus.REDEEMING); | ||
|
|
||
| assertThatThrownBy(() -> jpa.saveAndFlush(second)) | ||
| .isInstanceOf(ObjectOptimisticLockingFailureException.class); | ||
| } |
There was a problem hiding this comment.
Optimistic locking test does not exercise stale-version conflict.
first and second are loaded in the same persistence context—JPA returns the same managed instance. After saveAndFlush(first), the version is already incremented on that instance. detach(second) then detaches the up-to-date entity, so the subsequent save won't conflict.
To properly test @Version behavior, detach/clear before the second load:
🐛 Proposed fix
`@Test`
`@DisplayName`("should throw on stale version update (optimistic locking)")
void shouldThrowOnStaleUpdate() {
var saved = adapter.save(aPendingOrder());
- var first = jpa.findById(saved.payoutId()).orElseThrow();
- var second = jpa.findById(saved.payoutId()).orElseThrow();
+ var stale = jpa.findById(saved.payoutId()).orElseThrow();
+ entityManager.detach(stale); // snapshot at version 0
- first.setStatus(PayoutStatus.REDEEMING);
- jpa.saveAndFlush(first);
+ var current = jpa.findById(saved.payoutId()).orElseThrow();
+ current.setStatus(PayoutStatus.REDEEMING);
+ jpa.saveAndFlush(current); // version now 1
- entityManager.detach(second);
- second.setStatus(PayoutStatus.REDEEMING);
+ stale.setStatus(PayoutStatus.REDEEMED); // still version 0
- assertThatThrownBy(() -> jpa.saveAndFlush(second))
+ assertThatThrownBy(() -> jpa.saveAndFlush(stale))
.isInstanceOf(ObjectOptimisticLockingFailureException.class);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Test | |
| @DisplayName("should throw on stale version update (optimistic locking)") | |
| void shouldThrowOnStaleUpdate() { | |
| var saved = adapter.save(aPendingOrder()); | |
| var first = jpa.findById(saved.payoutId()).orElseThrow(); | |
| var second = jpa.findById(saved.payoutId()).orElseThrow(); | |
| first.setStatus(PayoutStatus.REDEEMING); | |
| jpa.saveAndFlush(first); | |
| entityManager.detach(second); | |
| second.setStatus(PayoutStatus.REDEEMING); | |
| assertThatThrownBy(() -> jpa.saveAndFlush(second)) | |
| .isInstanceOf(ObjectOptimisticLockingFailureException.class); | |
| } | |
| `@Test` | |
| `@DisplayName`("should throw on stale version update (optimistic locking)") | |
| void shouldThrowOnStaleUpdate() { | |
| var saved = adapter.save(aPendingOrder()); | |
| var stale = jpa.findById(saved.payoutId()).orElseThrow(); | |
| entityManager.detach(stale); // snapshot at version 0 | |
| var current = jpa.findById(saved.payoutId()).orElseThrow(); | |
| current.setStatus(PayoutStatus.REDEEMING); | |
| jpa.saveAndFlush(current); // version now 1 | |
| stale.setStatus(PayoutStatus.REDEEMED); // still version 0 | |
| assertThatThrownBy(() -> jpa.saveAndFlush(stale)) | |
| .isInstanceOf(ObjectOptimisticLockingFailureException.class); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-off-ramp/fiat-off-ramp/src/integration-test/java/com/stablecoin/payments/offramp/infrastructure/persistence/PayoutOrderPersistenceAdapterIT.java`
around lines 234 - 250, The test loads two references from the same persistence
context so they are the same managed instance; instead, after saving the updated
first instance you must clear the persistence context and then reload the second
to get a stale-version instance. Modify shouldThrowOnStaleUpdate so you call
jpa.findById(...) to get first, update and jpa.saveAndFlush(first), then call
entityManager.clear() (not entityManager.detach(second)) and only then call
jpa.findById(...) to obtain second, change its status and assert that
jpa.saveAndFlush(second) throws ObjectOptimisticLockingFailureException.
Comment on lines
+79
to
+94
| @Test | ||
| @DisplayName("should persist all stablecoin ticker fields via round-trip") | ||
| void shouldPersistStablecoinTickerRoundTrip() { | ||
| var order = savePayoutOrder(); | ||
| var redemption = StablecoinRedemption.create( | ||
| order.payoutId(), aStablecoinTicker(), | ||
| new BigDecimal("500.00"), new BigDecimal("460.00"), | ||
| "EUR", "circle", "circle_ref_003"); | ||
| var saved = adapter.save(redemption); | ||
|
|
||
| assertThat(adapter.findById(saved.redemptionId())).isPresent().get() | ||
| .usingRecursiveComparison() | ||
| .withComparatorForType(BigDecimal::compareTo, BigDecimal.class) | ||
| .ignoringFieldsOfTypes(Instant.class) | ||
| .isEqualTo(saved); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant test—duplicates shouldSaveAndRetrieveById logic.
Both tests save a redemption and retrieve by id with recursive comparison. Unless you're specifically asserting something unique about StablecoinTicker serialization beyond a standard round-trip, consider removing or differentiating this test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-off-ramp/fiat-off-ramp/src/integration-test/java/com/stablecoin/payments/offramp/infrastructure/persistence/StablecoinRedemptionPersistenceAdapterIT.java`
around lines 79 - 94, This test duplicates the logic in
shouldSaveAndRetrieveById; either delete shouldPersistStablecoinTickerRoundTrip
or change it to assert unique StablecoinTicker serialization details: keep the
setup using StablecoinRedemption.create and aStablecoinTicker() but replace the
broad recursive comparison with focused assertions on the StablecoinTicker
fields (e.g., compare
saved.redemptionId()/adapter.findById(saved.redemptionId()).get().ticker()
fields explicitly) so the test verifies the specific ticker serialization rather
than repeating the full round-trip used in shouldSaveAndRetrieveById.
Comment on lines
+62
to
+65
| PartnerIdentifier offRampPartner = null; | ||
| if (entity.getOffRampPartnerId() != null && entity.getOffRampPartnerName() != null) { | ||
| offRampPartner = new PartnerIdentifier(entity.getOffRampPartnerId(), entity.getOffRampPartnerName()); | ||
| } |
There was a problem hiding this comment.
Inconsistent partial-data handling for PartnerIdentifier.
BankAccount and MobileMoneyAccount throw IllegalStateException on partial columns, but PartnerIdentifier silently returns null when only one of offRampPartnerId or offRampPartnerName is present. Apply the same fail-fast pattern for consistency.
🛡️ Proposed fix
- PartnerIdentifier offRampPartner = null;
- if (entity.getOffRampPartnerId() != null && entity.getOffRampPartnerName() != null) {
- offRampPartner = new PartnerIdentifier(entity.getOffRampPartnerId(), entity.getOffRampPartnerName());
- }
+ PartnerIdentifier offRampPartner = mapPartnerIdentifier(entity);Add helper method:
private static PartnerIdentifier mapPartnerIdentifier(PayoutOrderEntity entity) {
boolean hasId = entity.getOffRampPartnerId() != null;
boolean hasName = entity.getOffRampPartnerName() != null;
if (!hasId && !hasName) {
return null;
}
if (hasId != hasName) {
throw new IllegalStateException(
"Corrupted payout_orders row %s: partial partner columns — id=%s, name=%s"
.formatted(entity.getPayoutId(), hasId, hasName));
}
return new PartnerIdentifier(entity.getOffRampPartnerId(), entity.getOffRampPartnerName());
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/mapper/PayoutOrderPersistenceMapper.java`
around lines 62 - 65, The mapping currently returns null for partial
PartnerIdentifier data, causing inconsistent behavior with
BankAccount/MobileMoneyAccount; add a private helper method
mapPartnerIdentifier(PayoutOrderEntity entity) that checks
entity.getOffRampPartnerId() and entity.getOffRampPartnerName(), returns null
only if both are null, throws an IllegalStateException when exactly one is
present (use a message like "Corrupted payout_orders row %s: partial partner
columns — id=%s, name=%s".formatted(entity.getPayoutId(), hasId, hasName)), and
otherwise returns new PartnerIdentifier(entity.getOffRampPartnerId(),
entity.getOffRampPartnerName()); then replace the existing inline logic that
sets offRampPartner with a call to mapPartnerIdentifier(entity).
Comment on lines
+97
to
+145
| private static BankAccount mapBankAccount(PayoutOrderEntity entity) { | ||
| boolean hasAccountNumber = entity.getBankAccountNumber() != null; | ||
| boolean hasCode = entity.getBankCode() != null; | ||
| boolean hasType = entity.getBankAccountType() != null; | ||
| boolean hasCountry = entity.getBankCountry() != null; | ||
|
|
||
| boolean hasAny = hasAccountNumber || hasCode || hasType || hasCountry; | ||
| boolean hasAll = hasAccountNumber && hasCode && hasType && hasCountry; | ||
|
|
||
| if (!hasAny) { | ||
| return null; | ||
| } | ||
| if (!hasAll) { | ||
| throw new IllegalStateException( | ||
| "Corrupted payout_orders row: partial bank account columns populated — " | ||
| + "accountNumber=%s, bankCode=%s, accountType=%s, country=%s".formatted( | ||
| hasAccountNumber, hasCode, hasType, hasCountry)); | ||
| } | ||
| return new BankAccount( | ||
| entity.getBankAccountNumber(), | ||
| entity.getBankCode(), | ||
| AccountType.valueOf(entity.getBankAccountType()), | ||
| entity.getBankCountry() | ||
| ); | ||
| } | ||
|
|
||
| private static MobileMoneyAccount mapMobileMoneyAccount(PayoutOrderEntity entity) { | ||
| boolean hasProvider = entity.getMobileMoneyProvider() != null; | ||
| boolean hasPhone = entity.getMobileMoneyPhone() != null; | ||
| boolean hasCountry = entity.getMobileMoneyCountry() != null; | ||
|
|
||
| boolean hasAny = hasProvider || hasPhone || hasCountry; | ||
| boolean hasAll = hasProvider && hasPhone && hasCountry; | ||
|
|
||
| if (!hasAny) { | ||
| return null; | ||
| } | ||
| if (!hasAll) { | ||
| throw new IllegalStateException( | ||
| "Corrupted payout_orders row: partial mobile money columns populated — " | ||
| + "provider=%s, phone=%s, country=%s".formatted( | ||
| hasProvider, hasPhone, hasCountry)); | ||
| } | ||
| return new MobileMoneyAccount( | ||
| MobileMoneyProvider.valueOf(entity.getMobileMoneyProvider()), | ||
| entity.getMobileMoneyPhone(), | ||
| entity.getMobileMoneyCountry() | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Include payoutId in corruption error messages for traceability.
The fail-fast guards are solid, but error messages only log boolean flags. Adding the payoutId aids incident triage when scanning logs.
♻️ Proposed refinement
throw new IllegalStateException(
- "Corrupted payout_orders row: partial bank account columns populated — "
+ "Corrupted payout_orders row %s: partial bank account columns — "
+ "accountNumber=%s, bankCode=%s, accountType=%s, country=%s".formatted(
- hasAccountNumber, hasCode, hasType, hasCountry));
+ entity.getPayoutId(), hasAccountNumber, hasCode, hasType, hasCountry));Same for mapMobileMoneyAccount:
throw new IllegalStateException(
- "Corrupted payout_orders row: partial mobile money columns populated — "
+ "Corrupted payout_orders row %s: partial mobile money columns — "
+ "provider=%s, phone=%s, country=%s".formatted(
- hasProvider, hasPhone, hasCountry));
+ entity.getPayoutId(), hasProvider, hasPhone, hasCountry));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/mapper/PayoutOrderPersistenceMapper.java`
around lines 97 - 145, The IllegalStateException messages in mapBankAccount and
mapMobileMoneyAccount only include boolean flags and lack the payout identifier;
update both methods to include the entity's payoutId in the thrown message by
appending entity.getPayoutId() (or formatting it into the existing formatted
string) so the exceptions read something like "Corrupted payout_orders row:
partial ... — payoutId=%s" with the payoutId included alongside the current
flags.
| import java.util.Optional; | ||
| import java.util.UUID; | ||
|
|
||
| @Slf4j |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unused @Slf4j annotation.
No logging statements in this adapter. Either add operational logging (e.g., debug on save/find operations) or remove the annotation for cleaner code.
♻️ Remove unused annotation
-@Slf4j
`@Repository`
`@RequiredArgsConstructor`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Slf4j | |
| `@Repository` | |
| `@RequiredArgsConstructor` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/persistence/OffRampTransactionPersistenceAdapter.java`
at line 15, The class-level `@Slf4j` annotation on
OffRampTransactionPersistenceAdapter is unused; either add meaningful logging
calls inside methods like save(...) and findById(...) or remove the `@Slf4j`
annotation and its import to avoid dead code—update the
OffRampTransactionPersistenceAdapter class by deleting the `@Slf4j` annotation
(and related lombok import) if you choose not to add log statements, or
alternatively add appropriate log.debug/info/error calls where operations occur
and keep the annotation.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PayoutOrderEntitywith@Versionoptimistic locking,StablecoinRedemptionEntity,OffRampTransactionEntitywith@JdbcTypeCode(SqlTypes.JSON)for JSONB)payout_ordersTest plan
PayoutOrderPersistenceAdapterIT— 13 tests (save/retrieve bank + mobile money, findByPaymentId/Status/RecipientId/PartnerReference, unique constraint, fiat happy path, hold path, redemption failure, payout failure)StablecoinRedemptionPersistenceAdapterIT— 5 tests (save/retrieve, findByPayoutId, StablecoinTicker round-trip)OffRampTransactionPersistenceAdapterIT— 6 tests (save/retrieve, findByPayoutId, append-only multiple per payout, JSONB round-trip)usingRecursiveComparison().withComparatorForType(BigDecimal::compareTo)Closes STA-149
Closes STA-150
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Database Migration