feat(s3,s4): project scaffolds — S3 Fiat On-Ramp + S4 Blockchain & Custody (STA-119, STA-131)#117
Merged
Conversation
…stody (STA-119, STA-131) Phase 3 kickoff: scaffold both services in parallel. S3 Fiat On-Ramp (fiat-on-ramp/): - 3 Gradle modules (api, client, app) - V1 migration: 7 tables (collection_orders, psp_transactions, refunds, reconciliation_records, 3 Namastack outbox) - 5 domain event records, 2 API DTOs, ApiError, Feign client - SecurityConfig, FallbackAdaptersConfig, 8 ArchUnit tests S4 Blockchain & Custody (blockchain-custody/): - 3 Gradle modules (api, client, app) - V1 migration: 11 tables (wallets, wallet_balances, chain_transfers, transfer_participants, transfer_lifecycle_events, wallet_nonces, chain_selection_log, wallet_status_audit_log, 3 Namastack outbox) - V2 seed: dev wallets for Base Sepolia testnet - 4 domain event records, 2 API DTOs, ApiError, Feign client - SecurityConfig, FallbackAdaptersConfig, 8 ArchUnit tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughIntroduces two new service modules (blockchain-custody and fiat-on-ramp) with complete infrastructure: API contracts, Spring Cloud OpenFeign clients, Spring Boot applications, security/Kafka configurations, database schemas with outbox pattern for event publishing, integration test infrastructure, and architecture enforcement rules. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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: 32
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@blockchain-custody/blockchain-custody-api/src/main/java/com/stablecoin/payments/custody/api/ApiError.java`:
- Around line 16-19: withErrors currently stores the caller's Map reference
directly which can be mutated externally; update withErrors (and/or the ApiError
constructor it calls) to defensively copy the input by producing an unmodifiable
copy such that both the map and its value lists are immutable (e.g., create a
new map from errors where each entry's value is wrapped with List.copyOf(...)
and then wrap the resulting map with Map.copyOf(...)) and pass that copy into
the ApiError instance so external mutations do not affect the ApiError state.
In
`@blockchain-custody/blockchain-custody-api/src/main/java/com/stablecoin/payments/custody/api/events/ChainReturnConfirmed.java`:
- Around line 6-20: The record currently allows callers to pass arbitrary
schemaVersion and eventType despite having fixed constants EVENT_TYPE and
SCHEMA_VERSION; add a compact record constructor in ChainReturnConfirmed that
validates schemaVersion.equals(SCHEMA_VERSION) and eventType.equals(EVENT_TYPE)
and throws an IllegalArgumentException (or NullPointerException for nulls) with
a clear message if they differ, so instances cannot carry inconsistent
type/version values; reference the record name ChainReturnConfirmed and the
component names schemaVersion and eventType when making this change.
In
`@blockchain-custody/blockchain-custody-api/src/main/java/com/stablecoin/payments/custody/api/events/ChainTransferConfirmed.java`:
- Around line 6-22: The record ChainTransferConfirmed currently allows callers
to pass arbitrary schemaVersion and eventType which can diverge from the
declared constants; add a canonical constructor for the record (or a static
factory) that validates schemaVersion equals SCHEMA_VERSION and eventType equals
EVENT_TYPE and throw an IllegalArgumentException (or similar) on mismatch, or
alternatively remove those two parameters from the public construction path and
populate them from the constants internally; target the ChainTransferConfirmed
record and its constructor/creation sites to enforce this invariant.
In
`@blockchain-custody/blockchain-custody-api/src/main/java/com/stablecoin/payments/custody/api/events/ChainTransferFailed.java`:
- Around line 6-20: The ChainTransferFailed record exposes caller-controlled
schemaVersion and eventType which should be fixed to the constants EVENT_TYPE
and SCHEMA_VERSION; change ChainTransferFailed to enforce those values by adding
a compact constructor or factory method that validates schemaVersion equals
SCHEMA_VERSION and eventType equals EVENT_TYPE (or omits them from the public
API and sets them internally), and throw IllegalArgumentException on mismatch so
invalid payloads cannot be created; update usages to call the new
factory/constructor or pass only the remaining fields (transferId, paymentId,
correlationId, chainId, reason, errorCode, failedAt).
In
`@blockchain-custody/blockchain-custody-api/src/main/java/com/stablecoin/payments/custody/api/events/ChainTransferSubmitted.java`:
- Around line 6-23: ChainTransferSubmitted currently allows producers to supply
schemaVersion and eventType; fix by preventing overrides: either remove those
fields from the record signature and always set SCHEMA_VERSION and EVENT_TYPE
internally, or add a compact constructor/validation that asserts the provided
schemaVersion equals SCHEMA_VERSION and eventType equals EVENT_TYPE and throw
IllegalArgumentException otherwise; alternatively add a static factory method
(e.g., ChainTransferSubmitted.create(...)) that omits these two parameters and
injects the constants. Ensure references to the constants SCHEMA_VERSION and
EVENT_TYPE and the record name ChainTransferSubmitted are used so no instance
can be created with mismatched metadata.
In
`@blockchain-custody/blockchain-custody-api/src/main/java/com/stablecoin/payments/custody/api/TransferResponse.java`:
- Around line 6-23: The TransferResponse record currently uses a raw String for
status; define a new enum TransferStatus (e.g., PENDING, CONFIRMED, FAILED,
REJECTED, etc.) and change the TransferResponse signature to use TransferStatus
status instead of String status; update all places that construct or deserialize
TransferResponse (mappers, builders, controllers, tests) to use TransferStatus
values and, if using Jackson, ensure enum serialization/deserialization behavior
is correct (e.g., `@JsonCreator` or default name mapping) so API consumers and
code get compile-time safety and explicit state documentation.
In
`@blockchain-custody/blockchain-custody-api/src/main/java/com/stablecoin/payments/custody/api/WalletBalanceResponse.java`:
- Around line 14-20: BalanceEntry currently models monetary fields as String
(availableBalance, reservedBalance, blockchainBalance) while
RefundResponse/refundAmount in another module uses BigDecimal; pick one
representation across APIs and make the DTOs consistent. Either change
BalanceEntry's monetary fields to BigDecimal (availableBalance, reservedBalance,
blockchainBalance) and ensure Jackson/serialization configuration accepts and
emits BigDecimal consistently, or change RefundResponse.refundAmount to String;
update any equals/hashCode/validation/tests and API consumers accordingly, and
run serialization/deserialization tests to confirm no precision or compatibility
regressions.
In
`@blockchain-custody/blockchain-custody-client/src/main/java/com/stablecoin/payments/custody/client/BlockchainCustodyClient.java`:
- Around line 11-19: Add production resilience to the BlockchainCustodyClient by
wiring in a Feign fallback/fallbackFactory and an ErrorDecoder and enabling a
circuit breaker: implement a fallback class (e.g.,
BlockchainCustodyClientFallback) that implements BlockchainCustodyClient and
returns safe defaults for getTransfer(UUID) and getWalletBalance(UUID), or
create a fallbackFactory for richer context; register it on the Feign client
annotation (use fallback=... or fallbackFactory=... on `@FeignClient`(name =
"blockchain-custody-service", ...)); implement a custom ErrorDecoder bean to
translate Feign errors into domain exceptions for getTransfer and
getWalletBalance; and integrate a circuit breaker (e.g., annotate
consumer/service methods that call BlockchainCustodyClient with Resilience4j
`@CircuitBreaker` or configure a Feign decorator) so calls degrade gracefully
under failure.
In `@blockchain-custody/blockchain-custody/build.gradle.kts`:
- Line 108: The dependency line
implementation("io.namastack:namastack-outbox-starter-jdbc:1.0.0") hardcodes the
version; move the version to the version catalog or gradle properties and
reference it from build.gradle.kts. Add an entry in libs.versions.toml (e.g.,
namastack-outbox = "1.0.0") or a property in gradle.properties (e.g.,
namastackOutboxVersion=1.0.0), then replace the implementation(...) call with
the catalog/property reference (e.g.,
implementation(libs.namastack.outbox.starter.jdbc) or
implementation("io.namastack:namastack-outbox-starter-jdbc:${namastackOutboxVersion}")).
Ensure the symbol
implementation("io.namastack:namastack-outbox-starter-jdbc:1.0.0") is updated
accordingly.
In
`@blockchain-custody/blockchain-custody/src/integration-test/java/com/stablecoin/payments/custody/AbstractIntegrationTest.java`:
- Around line 40-52: The TRUNCATE statement in the jdbcTemplate.execute call
inside AbstractIntegrationTest (cleanDatabase) omits the two Namastack outbox
companion tables; update the TRUNCATE TABLE list used by the
jdbcTemplate.execute (the multi-line SQL string) to also include
custody_outbox_instance and custody_outbox_partition so all three outbox tables
(custody_outbox_record, custody_outbox_instance, custody_outbox_partition) are
cleared during test cleanup.
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/config/SecurityConfig.java`:
- Around line 30-37: The current SecurityConfig exposes localSecurityFilterChain
(the bean method) and disables auth whenever app.security.enabled=false, which
is dangerous in production; update the bean so it is only loaded for
non-production contexts (e.g., annotate the configuration or the
localSecurityFilterChain method with a non-prod profile such as
`@Profile`("!prod") or move this bean into a test/dev-only configuration class),
remove or replace the `@ConditionalOnProperty` that can be flipped in prod, and
ensure production profiles rely on the standard secured SecurityFilterChain
instead; reference the SecurityConfig class and the localSecurityFilterChain
bean when making this change.
- Around line 18-27: The SecurityFilterChain built by
securityFilterChain(HttpSecurity http) currently disables CSRF and requires
authentication but does not configure any authentication mechanism; update the
chain to add the intended auth scheme (e.g., configure
http.oauth2ResourceServer(...) for JWT bearer token validation or
http.httpBasic(...) / http.authenticationProvider(...) for basic auth/custom
providers) so non-whitelisted requests can be authenticated; modify the
securityFilterChain method to call the chosen method (oauth2ResourceServer or
httpBasic or register a custom AuthenticationProvider/filter) on the
HttpSecurity instance before build().
In `@blockchain-custody/blockchain-custody/src/main/resources/application.yml`:
- Around line 103-105: Replace the hardcoded property app.security.enabled:
false with an environment-driven value so deployments can toggle security (use
the property key app.security.enabled and bind it to an env var like
APP_SECURITY_ENABLED with a sensible default, e.g. true); also add an
application-prod.yml profile file that explicitly sets app.security.enabled to
true for production so security can be enforced without rebuilding the artifact
and so Spring profiles can override the default at runtime.
In
`@blockchain-custody/blockchain-custody/src/main/resources/db/migration/V1__initial_schema.sql`:
- Around line 6-12: The migration V1__initial_schema.sql currently creates a
database role with a hardcoded plaintext password ('sp_pass') for role sp_user;
remove the sensitive credential from this SQL and stop embedding secrets in VCS
by either (a) deleting the CREATE ROLE ... WITH LOGIN PASSWORD 'sp_pass' block
and documenting that role creation/credentials must be provisioned via
environment vars or infrastructure (Terraform/Helm) or (b) moving role creation
into container/init scripts or TestContainers setup that read the password from
environment secrets; update references to the role sp_user in tests/docs to use
externally-provided credentials and add a note in the migration header that role
provisioning is out of scope for migrations.
In
`@blockchain-custody/blockchain-custody/src/main/resources/db/migration/V2__seed_wallets.sql`:
- Around line 8-16: The seed inserts currently select from the entire wallets
table (wallet_balances and wallet_nonces using SELECT ... FROM wallets w), which
will affect all existing wallets; change the inserts to target only the two
wallets created earlier by capturing those inserted rows and using them as the
source (e.g., convert the prior wallet INSERT into a WITH ... RETURNING CTE or
otherwise capture their wallet_id/chain_id/stablecoin) and then use that CTE as
the SELECT source for wallet_balances and wallet_nonces instead of SELECT ...
FROM wallets w so only the intended two wallets get seeded.
In
`@blockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/arch/ArchitectureTest.java`:
- Around line 28-39: The test domainShouldNotDependOnSpring currently allows
classes in ..domain.. to depend on
org.springframework.beans.factory.annotation.. (e.g., `@Autowired/`@Value) because
that package is exempted; remove that exemption so the rule only permits
org.springframework.stereotype.. and org.springframework.transaction.. as
intended. Edit the fluent rule in ArchitectureTest.java to drop the
.and(resideOutsideOfPackage("org.springframework.beans.factory.annotation.."))
exemption (or otherwise restrict dependencies to only the stereotype and
transaction packages) so no DI annotations from
org.springframework.beans.factory.annotation are allowed in ..domain...
In
`@blockchain-custody/blockchain-custody/src/testFixtures/java/com/stablecoin/payments/custody/fixtures/TestUtils.java`:
- Around line 11-34: Duplicate TestUtils matcher logic (methods
eqIgnoringTimestamps, eqIgnoring, and isEqualIgnoring) should be moved to a
single shared test-support module and the two copies removed; extract the class
TestUtils (preserving the method signatures and use of argThat, isEqualIgnoring,
and ignoringFieldsOfTypes with ZonedDateTime, LocalDateTime, LocalDate, Instant)
into the shared test utilities artifact, add required test-only dependencies
(AssertJ and Mockito) to that module, update both callers in the onramp and
custody test fixtures to import the new shared TestUtils, and remove the
duplicated TestUtils files from each module so both tests reference the single
shared implementation.
- Around line 23-32: The helper method isEqualIgnoring should not catch
Throwable; narrow the catch to assertion failures only by catching
AssertionError (or the specific assertion exception used by AssertJ) in the
isEqualIgnoring method so only comparison failures return false and any other
exceptions/errors propagate; update the catch block in isEqualIgnoring to catch
AssertionError (rethrow or let other Throwables bubble up) while keeping the
existing recursive comparison logic and ignored-field handling.
In
`@fiat-on-ramp/fiat-on-ramp-api/src/main/java/com/stablecoin/payments/onramp/api/events/FiatCollectionInitiated.java`:
- Line 10: The record FiatCollectionInitiated currently has a redundant instance
field eventType conflicting with the static EVENT_TYPE; either remove the
eventType component from the record signature and update all usages to reference
the static EVENT_TYPE, or add a compact constructor in FiatCollectionInitiated
that validates eventType.equals(EVENT_TYPE) and throws an
IllegalArgumentException when it doesn't match. Update constructors/call sites
accordingly so no code relies on a mutable or mismatched instance value and
ensure EVENT_TYPE remains the single source of truth.
- Around line 7-20: Add Jakarta validation annotations to the
FiatCollectionInitiated record fields so the event is validated at serialization
boundaries: annotate eventId and collectionId with `@NotNull`, amount with
`@Positive` (or `@DecimalMin`("0.01") if zero is invalid), and currency with
`@NotBlank`; ensure imports for jakarta.validation.constraints are added and keep
the record signature in FiatCollectionInitiated unchanged except for adding
these annotations to the corresponding parameters.
In
`@fiat-on-ramp/fiat-on-ramp-api/src/main/java/com/stablecoin/payments/onramp/api/events/FiatRefundCompleted.java`:
- Around line 7-19: The record FiatRefundCompleted should enforce that its
schemaVersion and eventType always equal the constants SCHEMA_VERSION and
EVENT_TYPE: add a compact constructor for FiatRefundCompleted that checks the
incoming schemaVersion and eventType (or simply ignores incoming values and
assigns the canonical ones) and throws an IllegalArgumentException if they
mismatch (or replaces them), and also validate non-null required fields (e.g.,
refundId, paymentId, refundAmount, currency, completedAt) as needed; update the
compact constructor to reference the record components schemaVersion and
eventType and the static SCHEMA_VERSION and EVENT_TYPE constants to locate the
code to change.
In `@fiat-on-ramp/fiat-on-ramp/build.gradle.kts`:
- Line 107: Replace the hardcoded dependency version on the implementation
declaration implementation("io.namastack:namastack-outbox-starter-jdbc:1.0.0")
by moving the version into the version catalog and referencing it from the build
script; add a libs.versions.toml entry named namastack-outbox = "1.0.0" and then
update the implementation line to use the cataloged version (e.g., reference
libs.versions.namastack-outbox) so the dependency uses the version from libs
rather than the inline "1.0.0".
In
`@fiat-on-ramp/fiat-on-ramp/src/integration-test/java/com/stablecoin/payments/onramp/AbstractIntegrationTest.java`:
- Around line 51-58: The configureProperties method currently registers an extra
Spring Cloud Stream property "spring.cloud.stream.kafka.binder.brokers" via
registry.add alongside other properties; remove that registry.add call (the one
adding "spring.cloud.stream.kafka.binder.brokers") from
AbstractIntegrationTest::configureProperties if this module does not use Spring
Cloud Stream so properties are consistent with blockchain-custody's
AbstractIntegrationTest, or alternatively guard its registration behind a
module-specific flag; update tests accordingly to ensure they still pick up only
spring.kafka.bootstrap-servers (KAFKA::getBootstrapServers).
In
`@fiat-on-ramp/fiat-on-ramp/src/integration-test/resources/application-integration-test.yml`:
- Around line 22-24: The integration-test profile is setting
outbox.relay.fixed-delay-ms but the application uses namastack.outbox.* so the
override never applies; update the profile to use the same root key
(namastack.outbox.relay.fixed-delay-ms) and set the desired low value (e.g., 50)
so the test profile overrides the base namastack.outbox.poll/relay interval;
ensure the property name exactly matches the existing configuration keys
(namastack.outbox.relay.fixed-delay-ms) so the test picks up the faster cadence.
In
`@fiat-on-ramp/fiat-on-ramp/src/main/java/com/stablecoin/payments/onramp/config/SecurityConfig.java`:
- Around line 16-28: The SecurityFilterChain bean (securityFilterChain)
configures anyRequest().authenticated() but doesn't register any authentication
mechanism on the HttpSecurity object, so requests will be rejected; update
securityFilterChain to configure an authentication method (e.g. enable JWT
validation by calling http.oauth2ResourceServer(...) or configure an
AuthenticationProvider) or add a clear TODO comment indicating which mechanism
to plug in; reference the SecurityFilterChain method and HttpSecurity invocation
so the implementer knows to modify that method to include
.oauth2ResourceServer(...) (or another provider) before .build().
In `@fiat-on-ramp/fiat-on-ramp/src/main/resources/application.yml`:
- Around line 115-117: The default config currently disables authentication via
the app.security.enabled: false setting; change the main resources default to
enable security (set app.security.enabled to true or remove the insecure
override) and move the false override into a non-production profile (e.g.,
application-local.yml or application-test.yml) so only explicit local/test
profiles disable auth; update any profile-specific config that relied on the
global false and ensure the management endpoints remain protected by the enabled
security setting.
In
`@fiat-on-ramp/fiat-on-ramp/src/main/resources/db/migration/V1__initial_schema.sql`:
- Around line 59-71: The FK on psp_transactions.collection_id currently has no
ON DELETE behavior (defaults to NO ACTION); update the CREATE TABLE for
psp_transactions to explicitly add the desired deletion rule (e.g., change the
REFERENCES clause for collection_id to "REFERENCES
collection_orders(collection_id) ON DELETE CASCADE" if you want child rows
removed when a collection_order is deleted, or ON DELETE RESTRICT/NO ACTION if
you want to prevent parent deletion). Apply the same explicit ON DELETE policy
to the foreign key definitions used by the refunds and reconciliation_records
tables so the database behavior is unambiguous for deletes.
- Around line 6-12: The SQL migration hardcodes the password 'sp_pass' when
creating role sp_user; replace the literal with a deployment-time placeholder or
remove the PASSWORD clause so credentials are injected externally (e.g., via
environment/config or TestContainers init) and document that sp_user's password
must be set by the deployment process; update the role creation logic around
sp_user to avoid committing secrets and ensure tests that need a password read
it from a provided test-only override.
- Around line 157-158: Rename the index to follow the project convention by
replacing the current index name idx_onramp_outbox_record_status with a
suffix-style name (e.g., onramp_outbox_record_status_idx) in the CREATE INDEX
statement that targets the onramp_outbox_record (status, next_retry_at) index;
update any other migration or SQL references to the old name to avoid conflicts
and ensure the new name is unique in the schema.
In
`@fiat-on-ramp/fiat-on-ramp/src/testFixtures/java/com/stablecoin/payments/onramp/fixtures/TestUtils.java`:
- Around line 15-21: Add unit tests for the public helpers eqIgnoringTimestamps
and eqIgnoring in TestUtils: (1) a test that eqIgnoringTimestamps(T) delegates
to eqIgnoring(T) by asserting it produces the same matcher behavior when
comparing objects that differ only in timestamp fields, and (2) tests for
eqIgnoring(T, String... fieldsToIgnore) that verify the matcher returns true
when the only differences between two objects are the provided fieldsToIgnore
(including timestamp field names) and false when other fields differ; exercise
at least one concrete POJO used in tests and assert both positive and negative
cases to ensure the matcher logic in isEqualIgnoring is validated.
- Around line 31-32: The catch block in TestUtils.java is too broad—replace
"catch (Throwable t)" with "catch (AssertionError e)" so only assertion failures
from AssertJ are handled; keep the body returning false and let other JVM errors
(e.g., OutOfMemoryError, StackOverflowError) propagate. Reference the existing
catch block currently catching Throwable and update it to catch AssertionError
instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b6b480ed-bd8f-46e1-ad1a-7f1545a29326
📒 Files selected for processing (45)
blockchain-custody/blockchain-custody-api/build.gradle.ktsblockchain-custody/blockchain-custody-api/src/main/java/com/stablecoin/payments/custody/api/ApiError.javablockchain-custody/blockchain-custody-api/src/main/java/com/stablecoin/payments/custody/api/TransferResponse.javablockchain-custody/blockchain-custody-api/src/main/java/com/stablecoin/payments/custody/api/WalletBalanceResponse.javablockchain-custody/blockchain-custody-api/src/main/java/com/stablecoin/payments/custody/api/events/ChainReturnConfirmed.javablockchain-custody/blockchain-custody-api/src/main/java/com/stablecoin/payments/custody/api/events/ChainTransferConfirmed.javablockchain-custody/blockchain-custody-api/src/main/java/com/stablecoin/payments/custody/api/events/ChainTransferFailed.javablockchain-custody/blockchain-custody-api/src/main/java/com/stablecoin/payments/custody/api/events/ChainTransferSubmitted.javablockchain-custody/blockchain-custody-client/build.gradle.ktsblockchain-custody/blockchain-custody-client/src/main/java/com/stablecoin/payments/custody/client/BlockchainCustodyClient.javablockchain-custody/blockchain-custody/build.gradle.ktsblockchain-custody/blockchain-custody/src/integration-test/java/com/stablecoin/payments/custody/AbstractIntegrationTest.javablockchain-custody/blockchain-custody/src/integration-test/java/com/stablecoin/payments/custody/config/TestKafkaConfig.javablockchain-custody/blockchain-custody/src/integration-test/resources/application-integration-test.ymlblockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/BlockchainCustodyApplication.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/config/FallbackAdaptersConfig.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/config/SecurityConfig.javablockchain-custody/blockchain-custody/src/main/resources/application.ymlblockchain-custody/blockchain-custody/src/main/resources/db/migration/V1__initial_schema.sqlblockchain-custody/blockchain-custody/src/main/resources/db/migration/V2__seed_wallets.sqlblockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/arch/ArchitectureTest.javablockchain-custody/blockchain-custody/src/testFixtures/java/com/stablecoin/payments/custody/fixtures/TestUtils.javafiat-on-ramp/fiat-on-ramp-api/build.gradle.ktsfiat-on-ramp/fiat-on-ramp-api/src/main/java/com/stablecoin/payments/onramp/api/ApiError.javafiat-on-ramp/fiat-on-ramp-api/src/main/java/com/stablecoin/payments/onramp/api/CollectionResponse.javafiat-on-ramp/fiat-on-ramp-api/src/main/java/com/stablecoin/payments/onramp/api/RefundResponse.javafiat-on-ramp/fiat-on-ramp-api/src/main/java/com/stablecoin/payments/onramp/api/events/FiatCollected.javafiat-on-ramp/fiat-on-ramp-api/src/main/java/com/stablecoin/payments/onramp/api/events/FiatCollectionFailed.javafiat-on-ramp/fiat-on-ramp-api/src/main/java/com/stablecoin/payments/onramp/api/events/FiatCollectionInitiated.javafiat-on-ramp/fiat-on-ramp-api/src/main/java/com/stablecoin/payments/onramp/api/events/FiatRefundCompleted.javafiat-on-ramp/fiat-on-ramp-api/src/main/java/com/stablecoin/payments/onramp/api/events/FiatRefundInitiated.javafiat-on-ramp/fiat-on-ramp-client/build.gradle.ktsfiat-on-ramp/fiat-on-ramp-client/src/main/java/com/stablecoin/payments/onramp/client/FiatOnRampClient.javafiat-on-ramp/fiat-on-ramp/build.gradle.ktsfiat-on-ramp/fiat-on-ramp/src/integration-test/java/com/stablecoin/payments/onramp/AbstractIntegrationTest.javafiat-on-ramp/fiat-on-ramp/src/integration-test/resources/application-integration-test.ymlfiat-on-ramp/fiat-on-ramp/src/main/java/com/stablecoin/payments/onramp/FiatOnRampApplication.javafiat-on-ramp/fiat-on-ramp/src/main/java/com/stablecoin/payments/onramp/config/FallbackAdaptersConfig.javafiat-on-ramp/fiat-on-ramp/src/main/java/com/stablecoin/payments/onramp/config/SecurityConfig.javafiat-on-ramp/fiat-on-ramp/src/main/resources/application.ymlfiat-on-ramp/fiat-on-ramp/src/main/resources/db/migration/V1__initial_schema.sqlfiat-on-ramp/fiat-on-ramp/src/test/java/com/stablecoin/payments/onramp/arch/ArchitectureTest.javafiat-on-ramp/fiat-on-ramp/src/test/java/com/stablecoin/payments/onramp/config/TestKafkaConfig.javafiat-on-ramp/fiat-on-ramp/src/testFixtures/java/com/stablecoin/payments/onramp/fixtures/TestUtils.javasettings.gradle.kts
Comment on lines
+16
to
+19
| public static ApiError withErrors(String code, String status, String message, | ||
| Map<String, List<String>> errors) { | ||
| return new ApiError(code, status, message, errors); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider defensive copy of errors map.
withErrors passes the map directly. If the caller retains a reference and mutates it, the ApiError instance becomes inconsistent. Wrap with Map.copyOf():
♻️ Suggested fix
public static ApiError withErrors(String code, String status, String message,
Map<String, List<String>> errors) {
- return new ApiError(code, status, message, errors);
+ return new ApiError(code, status, message, errors == null ? Map.of() : Map.copyOf(errors));
}📝 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
| public static ApiError withErrors(String code, String status, String message, | |
| Map<String, List<String>> errors) { | |
| return new ApiError(code, status, message, errors); | |
| } | |
| public static ApiError withErrors(String code, String status, String message, | |
| Map<String, List<String>> errors) { | |
| return new ApiError(code, status, message, errors == null ? Map.of() : Map.copyOf(errors)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody-api/src/main/java/com/stablecoin/payments/custody/api/ApiError.java`
around lines 16 - 19, withErrors currently stores the caller's Map reference
directly which can be mutated externally; update withErrors (and/or the ApiError
constructor it calls) to defensively copy the input by producing an unmodifiable
copy such that both the map and its value lists are immutable (e.g., create a
new map from errors where each entry's value is wrapped with List.copyOf(...)
and then wrap the resulting map with Map.copyOf(...)) and pass that copy into
the ApiError instance so external mutations do not affect the ApiError state.
Comment on lines
+6
to
+20
| public record ChainReturnConfirmed( | ||
| String schemaVersion, | ||
| UUID eventId, | ||
| String eventType, | ||
| UUID transferId, | ||
| UUID parentTransferId, | ||
| UUID paymentId, | ||
| UUID correlationId, | ||
| String chainId, | ||
| String txHash, | ||
| Instant confirmedAt | ||
| ) { | ||
| public static final String EVENT_TYPE = "chain.return.confirmed"; | ||
| public static final String SCHEMA_VERSION = "1.0"; | ||
| } |
There was a problem hiding this comment.
Lock down the event identity fields.
Line 7 and Line 9 duplicate metadata that is already fixed by Line 18 and Line 19. Leaving those fields unconstrained means ChainReturnConfirmed instances can carry inconsistent type/version values, which weakens the contract for downstream consumers. Enforce the constants in the constructor path instead of trusting callers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody-api/src/main/java/com/stablecoin/payments/custody/api/events/ChainReturnConfirmed.java`
around lines 6 - 20, The record currently allows callers to pass arbitrary
schemaVersion and eventType despite having fixed constants EVENT_TYPE and
SCHEMA_VERSION; add a compact record constructor in ChainReturnConfirmed that
validates schemaVersion.equals(SCHEMA_VERSION) and eventType.equals(EVENT_TYPE)
and throws an IllegalArgumentException (or NullPointerException for nulls) with
a clear message if they differ, so instances cannot carry inconsistent
type/version values; reference the record name ChainReturnConfirmed and the
component names schemaVersion and eventType when making this change.
Comment on lines
+6
to
+22
| public record ChainTransferConfirmed( | ||
| String schemaVersion, | ||
| UUID eventId, | ||
| String eventType, | ||
| UUID transferId, | ||
| UUID paymentId, | ||
| UUID correlationId, | ||
| String chainId, | ||
| String txHash, | ||
| String blockNumber, | ||
| Integer confirmations, | ||
| String gasUsed, | ||
| Instant confirmedAt | ||
| ) { | ||
| public static final String EVENT_TYPE = "chain.transfer.confirmed"; | ||
| public static final String SCHEMA_VERSION = "1.0"; | ||
| } |
There was a problem hiding this comment.
Protect the event contract from mismatched metadata.
Line 7 and Line 9 should not be freely settable when this record already defines SCHEMA_VERSION and EVENT_TYPE. A mismatched ChainTransferConfirmed payload is easy to construct here and hard to diagnose once it reaches Kafka consumers. Validate those fields in the canonical constructor or remove them from the caller-facing constructor path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody-api/src/main/java/com/stablecoin/payments/custody/api/events/ChainTransferConfirmed.java`
around lines 6 - 22, The record ChainTransferConfirmed currently allows callers
to pass arbitrary schemaVersion and eventType which can diverge from the
declared constants; add a canonical constructor for the record (or a static
factory) that validates schemaVersion equals SCHEMA_VERSION and eventType equals
EVENT_TYPE and throw an IllegalArgumentException (or similar) on mismatch, or
alternatively remove those two parameters from the public construction path and
populate them from the constants internally; target the ChainTransferConfirmed
record and its constructor/creation sites to enforce this invariant.
Comment on lines
+6
to
+20
| public record ChainTransferFailed( | ||
| String schemaVersion, | ||
| UUID eventId, | ||
| String eventType, | ||
| UUID transferId, | ||
| UUID paymentId, | ||
| UUID correlationId, | ||
| String chainId, | ||
| String reason, | ||
| String errorCode, | ||
| Instant failedAt | ||
| ) { | ||
| public static final String EVENT_TYPE = "chain.transfer.failed"; | ||
| public static final String SCHEMA_VERSION = "1.0"; | ||
| } |
There was a problem hiding this comment.
Enforce fixed event metadata in the record API.
Line 7 and Line 9 make schemaVersion and eventType caller-controlled even though this type already declares fixed constants. That allows invalid ChainTransferFailed payloads to be created and can break consumer routing/deserialization. Prefer a compact constructor that validates both fields, or a secondary constructor/factory that hard-codes them.
Suggested fix
public record ChainTransferFailed(
String schemaVersion,
UUID eventId,
String eventType,
UUID transferId,
UUID paymentId,
UUID correlationId,
String chainId,
String reason,
String errorCode,
Instant failedAt
) {
public static final String EVENT_TYPE = "chain.transfer.failed";
public static final String SCHEMA_VERSION = "1.0";
+
+ public ChainTransferFailed {
+ if (!SCHEMA_VERSION.equals(schemaVersion)) {
+ throw new IllegalArgumentException("Unsupported schemaVersion: " + schemaVersion);
+ }
+ if (!EVENT_TYPE.equals(eventType)) {
+ throw new IllegalArgumentException("Unexpected eventType: " + eventType);
+ }
+ }
}📝 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
| public record ChainTransferFailed( | |
| String schemaVersion, | |
| UUID eventId, | |
| String eventType, | |
| UUID transferId, | |
| UUID paymentId, | |
| UUID correlationId, | |
| String chainId, | |
| String reason, | |
| String errorCode, | |
| Instant failedAt | |
| ) { | |
| public static final String EVENT_TYPE = "chain.transfer.failed"; | |
| public static final String SCHEMA_VERSION = "1.0"; | |
| } | |
| public record ChainTransferFailed( | |
| String schemaVersion, | |
| UUID eventId, | |
| String eventType, | |
| UUID transferId, | |
| UUID paymentId, | |
| UUID correlationId, | |
| String chainId, | |
| String reason, | |
| String errorCode, | |
| Instant failedAt | |
| ) { | |
| public static final String EVENT_TYPE = "chain.transfer.failed"; | |
| public static final String SCHEMA_VERSION = "1.0"; | |
| public ChainTransferFailed { | |
| if (!SCHEMA_VERSION.equals(schemaVersion)) { | |
| throw new IllegalArgumentException("Unsupported schemaVersion: " + schemaVersion); | |
| } | |
| if (!EVENT_TYPE.equals(eventType)) { | |
| throw new IllegalArgumentException("Unexpected eventType: " + eventType); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody-api/src/main/java/com/stablecoin/payments/custody/api/events/ChainTransferFailed.java`
around lines 6 - 20, The ChainTransferFailed record exposes caller-controlled
schemaVersion and eventType which should be fixed to the constants EVENT_TYPE
and SCHEMA_VERSION; change ChainTransferFailed to enforce those values by adding
a compact constructor or factory method that validates schemaVersion equals
SCHEMA_VERSION and eventType equals EVENT_TYPE (or omits them from the public
API and sets them internally), and throw IllegalArgumentException on mismatch so
invalid payloads cannot be created; update usages to call the new
factory/constructor or pass only the remaining fields (transferId, paymentId,
correlationId, chainId, reason, errorCode, failedAt).
Comment on lines
+6
to
+23
| public record ChainTransferSubmitted( | ||
| String schemaVersion, | ||
| UUID eventId, | ||
| String eventType, | ||
| UUID transferId, | ||
| UUID paymentId, | ||
| UUID correlationId, | ||
| String chainId, | ||
| String stablecoin, | ||
| String amount, | ||
| String txHash, | ||
| String fromAddress, | ||
| String toAddress, | ||
| Instant submittedAt | ||
| ) { | ||
| public static final String EVENT_TYPE = "chain.transfer.submitted"; | ||
| public static final String SCHEMA_VERSION = "1.0"; | ||
| } |
There was a problem hiding this comment.
Do not let producers override eventType/schemaVersion.
Line 7 and Line 9 should be invariant contract metadata for this event, not arbitrary inputs. As written, a producer can instantiate ChainTransferSubmitted with values that disagree with SCHEMA_VERSION and EVENT_TYPE, which is risky for event dispatch and schema handling. Add constructor validation or expose a constructor/factory that always injects the constants.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody-api/src/main/java/com/stablecoin/payments/custody/api/events/ChainTransferSubmitted.java`
around lines 6 - 23, ChainTransferSubmitted currently allows producers to supply
schemaVersion and eventType; fix by preventing overrides: either remove those
fields from the record signature and always set SCHEMA_VERSION and EVENT_TYPE
internally, or add a compact constructor/validation that asserts the provided
schemaVersion equals SCHEMA_VERSION and eventType equals EVENT_TYPE and throw
IllegalArgumentException otherwise; alternatively add a static factory method
(e.g., ChainTransferSubmitted.create(...)) that omits these two parameters and
injects the constants. Ensure references to the constants SCHEMA_VERSION and
EVENT_TYPE and the record name ChainTransferSubmitted are used so no instance
can be created with mismatched metadata.
Comment on lines
+6
to
+12
| DO $$ | ||
| BEGIN | ||
| IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'sp_user') THEN | ||
| CREATE ROLE sp_user WITH LOGIN PASSWORD 'sp_pass'; | ||
| END IF; | ||
| END | ||
| $$; |
There was a problem hiding this comment.
Hardcoded password in role creation.
sp_pass is committed to version control. Even for dev/test environments, consider using a placeholder that's overridden at deployment, or document this is TestContainers-only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-on-ramp/fiat-on-ramp/src/main/resources/db/migration/V1__initial_schema.sql`
around lines 6 - 12, The SQL migration hardcodes the password 'sp_pass' when
creating role sp_user; replace the literal with a deployment-time placeholder or
remove the PASSWORD clause so credentials are injected externally (e.g., via
environment/config or TestContainers init) and document that sp_user's password
must be set by the deployment process; update the role creation logic around
sp_user to avoid committing secrets and ensure tests that need a password read
it from a provided test-only override.
Comment on lines
+59
to
+71
| CREATE TABLE psp_transactions ( | ||
| psp_transaction_id UUID NOT NULL DEFAULT gen_random_uuid(), | ||
| collection_id UUID NOT NULL REFERENCES collection_orders(collection_id), | ||
| psp VARCHAR(50) NOT NULL, | ||
| psp_reference VARCHAR(200) NOT NULL, | ||
| event_type VARCHAR(50) NOT NULL, | ||
| status VARCHAR(30) NOT NULL, | ||
| amount NUMERIC(20, 8) NULL, | ||
| currency VARCHAR(3) NULL, | ||
| raw_response JSONB NOT NULL DEFAULT '{}', | ||
| received_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), | ||
| CONSTRAINT psp_transactions_pkey PRIMARY KEY (psp_transaction_id) | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider ON DELETE behavior for FK constraints.
psp_transactions.collection_id references collection_orders without explicit ON DELETE. Default is NO ACTION, which prevents parent deletion if children exist. If cascading deletes are intended (e.g., for test cleanup), specify ON DELETE CASCADE. Same applies to refunds and reconciliation_records FKs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-on-ramp/fiat-on-ramp/src/main/resources/db/migration/V1__initial_schema.sql`
around lines 59 - 71, The FK on psp_transactions.collection_id currently has no
ON DELETE behavior (defaults to NO ACTION); update the CREATE TABLE for
psp_transactions to explicitly add the desired deletion rule (e.g., change the
REFERENCES clause for collection_id to "REFERENCES
collection_orders(collection_id) ON DELETE CASCADE" if you want child rows
removed when a collection_order is deleted, or ON DELETE RESTRICT/NO ACTION if
you want to prevent parent deletion). Apply the same explicit ON DELETE policy
to the foreign key definitions used by the refunds and reconciliation_records
tables so the database behavior is unambiguous for deletes.
Comment on lines
+157
to
+158
| CREATE INDEX idx_onramp_outbox_record_status | ||
| ON onramp_outbox_record (status, next_retry_at); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Index naming inconsistency.
Outbox index uses idx_ prefix (idx_onramp_outbox_record_status) while domain tables use _idx suffix (collection_orders_status_idx). Align with project convention.
As per coding guidelines: "Verify index naming follows the project convention."
Proposed fix
-CREATE INDEX idx_onramp_outbox_record_status
+CREATE INDEX onramp_outbox_record_status_idx
ON onramp_outbox_record (status, next_retry_at);📝 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
| CREATE INDEX idx_onramp_outbox_record_status | |
| ON onramp_outbox_record (status, next_retry_at); | |
| CREATE INDEX onramp_outbox_record_status_idx | |
| ON onramp_outbox_record (status, next_retry_at); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-on-ramp/fiat-on-ramp/src/main/resources/db/migration/V1__initial_schema.sql`
around lines 157 - 158, Rename the index to follow the project convention by
replacing the current index name idx_onramp_outbox_record_status with a
suffix-style name (e.g., onramp_outbox_record_status_idx) in the CREATE INDEX
statement that targets the onramp_outbox_record (status, next_retry_at) index;
update any other migration or SQL references to the old name to avoid conflicts
and ensure the new name is unique in the schema.
Comment on lines
+15
to
+21
| public static <T> T eqIgnoringTimestamps(T expected) { | ||
| return eqIgnoring(expected); | ||
| } | ||
|
|
||
| public static <T> T eqIgnoring(T expected, String... fieldsToIgnore) { | ||
| return argThat(it -> isEqualIgnoring(it, expected, fieldsToIgnore)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding tests for this utility class.
Two public methods (eqIgnoringTimestamps, eqIgnoring) lack test coverage. Utility classes used across test fixtures benefit from self-validation to catch regressions early. As per coding guidelines: "Every new public method should have at least one test".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-on-ramp/fiat-on-ramp/src/testFixtures/java/com/stablecoin/payments/onramp/fixtures/TestUtils.java`
around lines 15 - 21, Add unit tests for the public helpers eqIgnoringTimestamps
and eqIgnoring in TestUtils: (1) a test that eqIgnoringTimestamps(T) delegates
to eqIgnoring(T) by asserting it produces the same matcher behavior when
comparing objects that differ only in timestamp fields, and (2) tests for
eqIgnoring(T, String... fieldsToIgnore) that verify the matcher returns true
when the only differences between two objects are the provided fieldsToIgnore
(including timestamp field names) and false when other fields differ; exercise
at least one concrete POJO used in tests and assert both positive and negative
cases to ensure the matcher logic in isEqualIgnoring is validated.
Comment on lines
+31
to
+32
| } catch (Throwable t) { | ||
| return false; |
There was a problem hiding this comment.
Catch AssertionError instead of Throwable.
AssertJ throws AssertionError on comparison failure. Catching Throwable is overly broad and could mask JVM-level errors like OutOfMemoryError or StackOverflowError.
Proposed fix
- } catch (Throwable t) {
+ } catch (AssertionError e) {
return false;
}📝 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
| } catch (Throwable t) { | |
| return false; | |
| } catch (AssertionError e) { | |
| return false; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-on-ramp/fiat-on-ramp/src/testFixtures/java/com/stablecoin/payments/onramp/fixtures/TestUtils.java`
around lines 31 - 32, The catch block in TestUtils.java is too broad—replace
"catch (Throwable t)" with "catch (AssertionError e)" so only assertion failures
from AssertJ are handled; keep the body returning false and let other JVM errors
(e.g., OutOfMemoryError, StackOverflowError) propagate. Reference the existing
catch block currently catching Throwable and update it to catch AssertionError
instead.
This was referenced Mar 8, 2026
Merged
Merged
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
fiat-on-ramp/): 3 Gradle modules, V1 migration (7 tables: collection_orders, psp_transactions, refunds, reconciliation_records + 3 Namastack outbox), 5 event records, 2 API DTOs, Feign client, 8 ArchUnit testsblockchain-custody/): 3 Gradle modules, V1 migration (11 tables: wallets, wallet_balances, chain_transfers, transfer_participants, transfer_lifecycle_events, wallet_nonces, chain_selection_log, wallet_status_audit_log + 3 Namastack outbox), V2 seed (dev wallets), 4 event records, 2 API DTOs, Feign client, 8 ArchUnit testsCloses STA-119
Closes STA-131
Test plan
./gradlew :fiat-on-ramp:fiat-on-ramp:test— 8 ArchUnit tests pass./gradlew :blockchain-custody:blockchain-custody:test— 8 ArchUnit tests pass🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Chores