Re: [PR] FINERACT-2455: Store and return annual effective interest rate on WC loans [fineract]

["oleksii-novikov-onix (via GitHub)" <gi...@apache.org>]

oleksii-novikov-onix commented on PR #6343:
URL: https://github.com/apache/fineract/pull/6343#issuecomment-5490977558

   > Replacing the hardcoded `pow(365)` with `npvDayCount` is a genuine correctness fix, and I like the legacy-model fallback in `annualEffectiveInterestRate()` for schedules persisted before this change. But there's a scope-collision problem that needs sorting out before anything else here.
   > 
   > **This directly overlaps with #6264 ("payment rate change EIR history"), on the same JIRA ticket.** Both PRs touch 9 of the same files, including `ProjectedAmortizationScheduleModel.java` and the WC account Avro schema. Both replace the exact same hardcoded `pow(365)` call in `WorkingCapitalLoanApplicationReadPlatformServiceImpl`, but with two different helper methods living in two different classes. And the two PRs make mutually exclusive contract changes: #6264 keeps `dailyEir` on the response/event and documents it in a new Avro record; this PR deletes `dailyEir` from the response, both mappers, and the existing Avro schema. Whichever of these two merges second is going to break the other. This needs to be resolved between the two PRs - either merge them, or have one explicitly build on top of the other, before either goes further.
   > 
   > Two issues on the substance, both of which also apply to #6264 (already flagged there) but are worse here since this PR feeds the rounded value back into the schedule math:
   > 
   > **Tenant money rounding mode applied to a rate, and that rate then drives the schedule.** `normalizedAnnualRate` rounds with `mc.getRoundingMode()`, which is the tenant-configurable money rounding mode - and the result is fed straight back through `deannualize` to become `effectiveInterestRate`, the value the whole amortization schedule discounts on. A tenant configured `FLOOR` and one configured `CEILING` will get materially different WC schedules for identical loan inputs. A rate isn't money; this should use a fixed rounding mode (#6264 has already been corrected to `RoundingMode.HALF_EVEN` for the same issue - worth reusing that constant if the two PRs get reconciled).
   > 
   > **The rate-segment's annual EIR is computed and then thrown away.** `computeScheduleParams` returns `annualEir` in `ScheduleParams`, but `applyRateChange` only pulls `segment.eir()` and drops `segment.annualEir()` on the floor - so after a payment rate change, the reported annual rate goes stale while the schedule itself is segment-aware. That directly contradicts the new Swagger text ("annual effective rate the schedule runs on").
   > 
   > One more concrete bug: `deannualize`'s Newton-Raphson seed (`Math.pow(...)`) can produce `Infinity` for an extreme daily IRR (reachable on a short schedule with a large discount fee relative to net disbursement), which then throws `NumberFormatException` out of `BigDecimal.valueOf`. The one existing caller of this path swallows it by accident (`NumberFormatException extends IllegalArgumentException`), but `applyRateChange`'s new call site has no such guard and would surface this as a 500. Also: zero unit tests for the new `TvmFunctions` numerical kernel (hand-rolled Newton-Raphson, no test class at all).
   > 
   > Recommendation: CHANGES_REQUESTED
   
   1. Scope collision with #6264. We touch 10 of the same files, and a test merge gives 3 conflicts: the shared feature file, the Swagger block around dailyEir, and enrichWithRateAndTerm, where we both replace the hardcoded 365. The helpers differ - annualiseEir does not round, normalizedAnnualRate rounds to six decimals and feeds the result back into the daily rate - and ProjectedAmortizationScheduleModel merges with no conflict at all, ending up with both. The rest is not exclusive: we drop dailyEir from the account, #6264 adds a daily rate to each history row.
   
   2. Tenant money rounding mode on a rate. Fixed: normalizedAnnualRate now rounds with a fixed HALF_EVEN constant, the same value #6264 uses.
   
   3. Segment annual EIR dropped. The annual rate is not dropped - computeScheduleParams derives the segment's daily rate from it, so every segment does run on its own normalised rate. The wrong part was the text: the value on the account is the rate the loan was priced at, matching the period payment rate next to it, which is also left as booked because what is in force on a date is read from the rate-change history. Swagger, the Avro doc and the javadoc now say that; the segment's own annual rate belongs on a history row, which is #6264's subject.
   
   4. Seed can overflow to Infinity. Fixed.
   
   5. No unit tests. Added TvmFunctionsTest.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org