feat: enhance precision handling for monetary values across schemas #523
Conversation
- Update various schemas to enforce DECIMAL(20,8) precision for monetary fields, ensuring consistent handling of high-precision amounts and rates. - Refactor related database creation logic to align with the new precision requirements. - Introduce tests to validate the precision handling in payment orders and related entities.
sundayonah
requested review from
5ran6,
chibie and
onahprosper
as code owners
WalkthroughStandardizes many financial columns to DECIMAL(20,8) via Ent schema updates and a new SQL migration; adds tests validating monetary precision. Removes ON CONFLICT/Upsert APIs across numerous Ent create builders, simplifying to plain inserts. Also drops public diff helpers from ent/migrate. No exported controller APIs were changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant S as SenderController
participant E as Ent ORM
participant DB as PostgreSQL
C->>S: POST /sender/orders {amount, rate, ...}
S->>E: Create PaymentOrder (decimal fields)
E->>DB: INSERT payment_orders (DECIMAL(20,8) columns)
DB-->>E: 201 id
E-->>S: Result {id, message}
S-->>C: 201 "Payment order initiated successfully"
Note over DB: Monetary fields stored as DECIMAL(20,8)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ent/migrate/schema.go (2)
39-49: Drift: beneficial_owners.ownership_percentage lacks DECIMAL(20,8) SchemaType mapping.SQL migration converts it, but the generated schema still exposes TypeFloat64 without SchemaType, risking future drift or reverts.
Do not edit this generated file. Update the schema and regenerate. For example, in ent/schema/beneficialowner.go:
func (BeneficialOwner) Fields() []ent.Field { return []ent.Field{ // ... - field.Float("ownership_percentage"), + field.Float("ownership_percentage"). + GoType(decimal.Decimal{}). + SchemaType(map[string]string{"postgres": "DECIMAL(20,8)"}), // ... } }Then run ent codegen and verify that this column in migrate/schema.go gains
SchemaType: {"postgres":"DECIMAL(20,8)"}.
292-309: Add DECIMAL(20,8) mapping for BeneficialOwners.ownership_percentage
26 DECIMAL(20,8) occurrences found in ent/migrate/schema.go, but ownership_percentage (ent/migrate/schema.go — BeneficialOwners) is defined as TypeFloat64 without a SchemaType. Add SchemaType: map[string]string{"postgres":"DECIMAL(20,8)"} to ownership_percentage and regenerate the schema/migrations.
🧹 Nitpick comments (12)
ent/schema/provisionbucket.go (1)
22-26: Postgres mapping looks good; add sqlite/mysql mappings to keep tests deterministic.Extend SchemaType so non‑Postgres envs don’t silently fall back to float. Also consider basic non‑negative and min<=max validation.
- SchemaType(map[string]string{"postgres": "DECIMAL(20,8)"}), + SchemaType(map[string]string{ + "postgres": "DECIMAL(20,8)", + "mysql": "DECIMAL(20,8)", + "sqlite": "NUMERIC", + }),Optionally add:
- field.Float("min_amount").NonNegative()
- field.Float("max_amount").NonNegative()
- a schema hook/validator to enforce min_amount <= max_amount.
ent/schema/network.go (1)
40-41: Align dialect mappings and fix potential precision loss for block_time.
- Add sqlite/mysql mappings here.
- block_time uses decimal.Decimal but lacks DECIMAL SchemaType; scanning via float can reintroduce precision loss. Either map it to DECIMAL too or drop GoType back to float64.
- SchemaType(map[string]string{"postgres": "DECIMAL(20,8)"}), + SchemaType(map[string]string{ + "postgres": "DECIMAL(20,8)", + "mysql": "DECIMAL(20,8)", + "sqlite": "NUMERIC", + }),Outside this hunk (for block_time), pick one:
Option A (preferred for consistency)
field.Float("block_time"). GoType(decimal.Decimal{}). SchemaType(map[string]string{ "postgres": "DECIMAL(20,8)", "mysql": "DECIMAL(20,8)", "sqlite": "NUMERIC", }),Option B (if exactness not required)
field.Float("block_time") // remove GoType(decimal.Decimal{})ent/schema/lockpaymentorder.go (1)
32-42: Great move to DECIMAL; add cross‑dialect mappings and tighten value ranges.
- Add sqlite/mysql mappings on all four fields.
- Consider .NonNegative() on amount/protocol_fee/rate and .Range(...) on order_percent (clarify 0..1 vs 0..100).
- SchemaType(map[string]string{"postgres": "DECIMAL(20,8)"}), + SchemaType(map[string]string{ + "postgres": "DECIMAL(20,8)", + "mysql": "DECIMAL(20,8)", + "sqlite": "NUMERIC", + }),(Apply to amount, protocol_fee, rate, order_percent.)
ent/schema/fiatcurrency.go (1)
35-36: DECIMAL mapping approved; add sqlite/mysql and verify scale suffices for extreme FX pairs.
- Extend SchemaType for all dialects.
- If you expect very small/large rates, confirm 20,8 is enough; otherwise bump precision/scale.
- SchemaType(map[string]string{"postgres": "DECIMAL(20,8)"}), + SchemaType(map[string]string{ + "postgres": "DECIMAL(20,8)", + "mysql": "DECIMAL(20,8)", + "sqlite": "NUMERIC", + }),ent/schema/senderordertoken.go (1)
27-28: Add sqlite/mysql mappings; also pin allowed range for fee_percent.
- Extend SchemaType for other dialects.
- Add .Range(0, 1) if fractional, or .Range(0, 100) if percentage.
- SchemaType(map[string]string{"postgres": "DECIMAL(20,8)"}), + SchemaType(map[string]string{ + "postgres": "DECIMAL(20,8)", + "mysql": "DECIMAL(20,8)", + "sqlite": "NUMERIC", + }),ent/schema/providerrating.go (1)
26-27: Add sqlite/mysql mappings; consider bounding trust_score.
- Extend SchemaType for all dialects.
- Add .Range(...) per domain (e.g., 0..5 or 0..100).
- SchemaType(map[string]string{"postgres": "DECIMAL(20,8)"}), + SchemaType(map[string]string{ + "postgres": "DECIMAL(20,8)", + "mysql": "DECIMAL(20,8)", + "sqlite": "NUMERIC", + }),ent/schema/providerordertoken.go (1)
27-42: Consistent DECIMAL mapping; add sqlite/mysql and validate cross‑field invariants.
- Extend SchemaType on all five fields.
- Add validators: non‑negative for amounts/ratios; min<=max.
- Enforce that only the rate matching conversion_rate_type is required (the other Optional()) via validators/hooks.
- SchemaType(map[string]string{"postgres": "DECIMAL(20,8)"}), + SchemaType(map[string]string{ + "postgres": "DECIMAL(20,8)", + "mysql": "DECIMAL(20,8)", + "sqlite": "NUMERIC", + }),(Apply to fixed_conversion_rate, floating_conversion_rate, max_order_amount, min_order_amount, rate_slippage.)
ent/schema/providercurrencies.go (1)
25-32: Looks good; add sqlite/mysql mappings and basic invariants for balances.
- Extend SchemaType for other dialects.
- Consider .NonNegative() on balances and a check/validator ensuring reserved <= total and available + reserved <= total.
- SchemaType(map[string]string{"postgres": "DECIMAL(20,8)"}), + SchemaType(map[string]string{ + "postgres": "DECIMAL(20,8)", + "mysql": "DECIMAL(20,8)", + "sqlite": "NUMERIC", + }),(Apply to available_balance, total_balance, reserved_balance.)
controllers/sender/sender_test.go (2)
903-911: Assert sender fee at 8‑dp instead of using a wide tolerance.Compute and compare at the DECIMAL(20,8) scale to actually validate precision and avoid masking errors with 0.01 tolerance.
Apply this diff:
- // Verify sender fee calculation precision (5% of amount) - expectedSenderFee := expectedAmount.Mul(decimal.NewFromFloat(0.05)) - // Use tolerance for rounding differences - diff := paymentOrder.SenderFee.Sub(expectedSenderFee).Abs() - tolerance := decimal.NewFromFloat(0.01) // Allow 0.01 tolerance - assert.True(t, diff.LessThanOrEqual(tolerance), - "Sender fee calculation precision mismatch. Expected: %s, Got: %s, Diff: %s", - expectedSenderFee.String(), paymentOrder.SenderFee.String(), diff.String()) + // Verify sender fee calculation at DB precision (8 dp) + expectedSenderFee := expectedAmount.Mul(decimal.NewFromFloat(0.05)).Round(8) + assert.Equal(t, 0, paymentOrder.SenderFee.Round(8).Cmp(expectedSenderFee), + "Sender fee precision mismatch. Expected: %s, Got: %s", + expectedSenderFee.String(), paymentOrder.SenderFee.String())
837-1038: Add boundary/overflow precision test cases and (optionally) a Postgres-backed run.
- Add subtests for 8‑decimal boundary (e.g., 1.23456789) and >8‑decimal input (e.g., 1.234567891) to assert rounding/truncation to 8 dp across Amount and Rate.
- Optional: gate a Postgres integration run (e.g., via env flag) to validate actual DECIMAL(20,8) semantics rather than SQLite’s flexible typing.
I can add the subtests and an env‑guarded Postgres test harness if you want me to push a patch.
ent/migrate/migrations/20250916172506_decimal_precision_update.sql (2)
1-120: Wrap the migration in a transaction for atomicity.If your migration runner doesn’t auto‑wrap in a single transaction, partial alters can leave the schema inconsistent on failure.
Apply this diff:
--- Update financial fields to use DECIMAL(20,8) precision for better financial accuracy +-- Update financial fields to use DECIMAL(20,8) precision for better financial accuracy +BEGIN; ... -ALTER TABLE "provider_ratings" -ALTER COLUMN "trust_score" TYPE DECIMAL(20,8); +ALTER TABLE "provider_ratings" +ALTER COLUMN "trust_score" TYPE DECIMAL(20,8); + +COMMIT;
4-34: Make rounding explicit during type change.Casting float→DECIMAL(20,8) relies on implicit rounding. Being explicit avoids behavioral surprises across environments.
Example for one column (apply pattern across all):
ALTER TABLE "payment_orders" ALTER COLUMN "amount" TYPE DECIMAL(20,8) USING ROUND("amount"::DECIMAL, 8);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ent/migrate/migrations/atlas.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sum
📒 Files selected for processing (39)
controllers/sender/sender_test.go(1 hunks)ent/apikey_create.go(0 hunks)ent/beneficialowner_create.go(0 hunks)ent/fiatcurrency_create.go(0 hunks)ent/identityverificationrequest_create.go(0 hunks)ent/institution_create.go(0 hunks)ent/kybprofile_create.go(0 hunks)ent/linkedaddress_create.go(0 hunks)ent/lockorderfulfillment_create.go(0 hunks)ent/lockpaymentorder_create.go(0 hunks)ent/migrate/migrate.go(0 hunks)ent/migrate/migrations/20250916172506_decimal_precision_update.sql(1 hunks)ent/migrate/schema.go(9 hunks)ent/network_create.go(0 hunks)ent/paymentorder_create.go(0 hunks)ent/paymentorderrecipient_create.go(0 hunks)ent/paymentwebhook_create.go(0 hunks)ent/providercurrencies_create.go(0 hunks)ent/providerordertoken_create.go(0 hunks)ent/providerprofile_create.go(0 hunks)ent/providerrating_create.go(0 hunks)ent/provisionbucket_create.go(0 hunks)ent/receiveaddress_create.go(0 hunks)ent/schema/fiatcurrency.go(1 hunks)ent/schema/lockpaymentorder.go(1 hunks)ent/schema/network.go(1 hunks)ent/schema/paymentorder.go(2 hunks)ent/schema/providercurrencies.go(1 hunks)ent/schema/providerordertoken.go(1 hunks)ent/schema/providerrating.go(1 hunks)ent/schema/provisionbucket.go(1 hunks)ent/schema/senderordertoken.go(1 hunks)ent/senderordertoken_create.go(0 hunks)ent/senderprofile_create.go(0 hunks)ent/token_create.go(0 hunks)ent/transactionlog_create.go(0 hunks)ent/user_create.go(0 hunks)ent/verificationtoken_create.go(0 hunks)ent/webhookretryattempt_create.go(0 hunks)
💤 Files with no reviewable changes (27)
- ent/migrate/migrate.go
- ent/providercurrencies_create.go
- ent/linkedaddress_create.go
- ent/identityverificationrequest_create.go
- ent/providerprofile_create.go
- ent/receiveaddress_create.go
- ent/paymentorder_create.go
- ent/lockpaymentorder_create.go
- ent/user_create.go
- ent/beneficialowner_create.go
- ent/kybprofile_create.go
- ent/transactionlog_create.go
- ent/apikey_create.go
- ent/network_create.go
- ent/webhookretryattempt_create.go
- ent/senderprofile_create.go
- ent/providerrating_create.go
- ent/paymentorderrecipient_create.go
- ent/verificationtoken_create.go
- ent/senderordertoken_create.go
- ent/institution_create.go
- ent/lockorderfulfillment_create.go
- ent/provisionbucket_create.go
- ent/providerordertoken_create.go
- ent/paymentwebhook_create.go
- ent/token_create.go
- ent/fiatcurrency_create.go
🧰 Additional context used
🧬 Code graph analysis (1)
controllers/sender/sender_test.go (7)
ent/paymentorder/where.go (6)
ID(16-18)IDEQ(21-23)Amount(71-73)Rate(101-103)NetworkFee(96-98)SenderFee(91-93)utils/test/test.go (1)
PerformRequest(14-25)types/types.go (1)
Response(558-562)ent/schema/paymentorder.go (4)
PaymentOrder(13-15)PaymentOrder(18-22)PaymentOrder(25-80)PaymentOrder(83-105)ent/paymentorder.go (2)
PaymentOrder(24-76)PaymentOrder(175-202)ent/schema/network.go (4)
Network(12-14)Network(17-21)Network(24-43)Network(46-53)ent/network.go (2)
Network(18-48)Network(82-101)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (3)
ent/schema/providerordertoken.go (1)
26-42: Action: Ensure all decimal.Decimal fields declare SchemaType for every supported SQL dialect (postgres, mysql, sqlite).Search shows SchemaType is present but only specifies "postgres" in these files — add mysql/sqlite entries or confirm they are intentionally omitted:
- ent/schema/paymentorder.go
- ent/schema/providerordertoken.go
- ent/schema/lockpaymentorder.go
- ent/schema/providercurrencies.go
- ent/schema/provisionbucket.go
- ent/schema/senderordertoken.go
- ent/schema/providerrating.go
- ent/schema/network.go
- ent/schema/fiatcurrency.go
The first presence-check failed due to a PCRE error. Re-run the corrected verification script below to (A) list any decimal.Decimal fields missing SchemaType and (B) list SchemaType maps that only contain "postgres":
#!/bin/bash set -euo pipefail echo "Fields using decimal.Decimal without SchemaType (should be none):" rg -n --type=go 'GoType\(decimal\.Decimal\{\}\)' ent/schema | while IFS=: read -r file line _; do start=$(( line > 3 ? line-3 : 1 )) if ! sed -n "${line},$((line+6))p" "$file" | rg -q 'SchemaType\('; then echo "MISSING SchemaType -> $file:$line" sed -n "${start},$((line+6))p" "$file" echo "----" fi done echo echo "SchemaType maps that only contain 'postgres' (should include mysql/sqlite if supported):" rg -n --type=go 'SchemaType\(\s*map\[string\]string\{' ent/schema | while IFS=: read -r file line _; do block=$(sed -n "${line},$((line+8))p" "$file" | tr '\n' ' ' | sed 's/ */ /g') if ! printf '%s' "$block" | rg -q '"mysql"|"sqlite"'; then echo "$file:$line: $block" fi doneent/schema/paymentorder.go (1)
28-49: PaymentOrder monetary fields correctly mapped to DECIMAL(20,8) (Postgres).Consistent SchemaType mapping with decimal.Decimal Go types. Looks good.
Please regenerate ent artifacts and run a quick migration diff to ensure no drift.
Also applies to: 61-63
ent/migrate/migrations/20250916172506_decimal_precision_update.sql (1)
1-120: Confirm there is a down/rollback migration.PR text mentions rollback, but this file contains only “up”. Ensure a corresponding “down” migration exists to revert DECIMAL(20,8)→double precision safely (with explicit casts).
…tempt creation - Implemented OnConflict handling in VerificationTokenCreate and WebhookRetryAttemptCreate to manage duplicate entries. - Added methods for configuring conflict resolution strategies, including Update, Ignore, and DoNothing. - Enhanced bulk creation with OnConflict options in VerificationTokenCreateBulk and WebhookRetryAttemptCreateBulk. - Updated related structures and methods to support upsert functionality for both entities.
Description
This PR implements
DECIMAL(20,8)precision for all financial fields across the Paycrest Aggregator schema to ensure financial accuracy and prevent floating-point precision errors that could lead to monetary losses and audit issues.Background
The Paycrest Aggregator is a critical financial system that processes cryptocurrency payment orders, manages provider balances, and handles fee calculations. Previously, all monetary amounts were stored using
double precisionwhich can introduce floating-point precision errors during financial calculations. These errors could compound over time and lead to significant monetary discrepancies in a production financial environment.Implementation Details
SchemaType(map[string]string{"postgres": "DECIMAL(20,8)"})for financial fieldsdecimal.Decimal{}for consistency and type safetyTables and Fields Updated
Total: 10 tables, 26 financial fields
amount,amount_paid,amount_returned,percent_settled,sender_fee,network_fee,rate,fee_percentamount,protocol_fee,rate,order_percentavailable_balance,total_balance,reserved_balancefixed_conversion_rate,floating_conversion_rate,max_order_amount,min_order_amount,rate_slippagemin_amount,max_amountfee_percentmarket_ratefeeownership_percentagetrust_scoreBreaking Changes
double precisiontoDECIMAL(20,8)API and Contract Changes
References
closes #506
Testing
Development Environment Testing
DECIMAL(20,8)Manual Testing Steps
Migration Application:
Validation Script:
Application Testing:
Test Coverage
DECIMAL(20,8)Checklist
feature/decimal-precision-update)Business Success
Next Steps
By submitting this PR, I agree to Paycrest's Contributor Code of Conduct and Contribution Guide.
Summary by CodeRabbit
Bug Fixes
Chores
Tests