Feat: Implement transaction logic models and utilities#40
Open
v1dit wants to merge 4 commits into
Open
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughIntroduces domain models (User, Expense, Split) and business logic for expense settlement calculations, including balance computation and transaction generation. Adds comprehensive test coverage and removes Changes
Sequence Diagram(s)sequenceDiagram
actor User as User/Client
participant ExpenseManager as Expense Manager
participant CalcUtils as CalculationUtils
participant Settlement as Settlement
User->>ExpenseManager: Create Expense with splits
User->>CalcUtils: calculateBalances(expenses)
activate CalcUtils
CalcUtils->>CalcUtils: Process each expense
CalcUtils->>CalcUtils: Credit payer, debit splitters
CalcUtils-->>User: Return Map<userId, balance>
deactivate CalcUtils
User->>Settlement: calculateSettlement(balances)
activate Settlement
Settlement->>Settlement: Separate debtors & creditors
Settlement->>Settlement: Sort by magnitude (desc)
Settlement->>Settlement: Match debtor to creditor
Settlement->>Settlement: Create transfer transactions
Settlement-->>User: Return List<Transaction>
deactivate Settlement
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Author
|
This PR introduces a validation and financial logic layer for Zplit transactions. MotivationWhile the data model and architecture are being actively developed, there is currently no centralized layer for:
This PR focuses on improving correctness and reliability of financial operations. What’s Included
NotesThe models included in this PR are minimal and intended only to support the logic layer. I’m happy to refactor them to align with the finalized architecture. Future Work
|
Author
|
Hi, I noticed there are ongoing efforts around architecture and core data models. This PR is mainly focused on building a validation and financial logic layer on top of transactions. The models included are minimal and I’m happy to refactor or align them with the final schema. Would appreciate any feedback on how best to integrate this with the current direction. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.dart_tool/package_config.json (1)
6-177:⚠️ Potential issue | 🟠 MajorRemove
.dart_tool/from version control—it contains machine-local paths and is a generated artifact.The repository is tracking
.dart_tool/package_config.jsonand other generated files that contain absolute paths (/Users/vidit/.pub-cache,/opt/homebrew/share/flutter) and environment-specific metadata. These should never be committed as they cause non-reproducible diffs, break CI builds, and expose local directory structures.Add
.dart_tool/to.gitignoreand remove from git tracking:Remediation steps
- Update
.gitignore:+.dart_tool/
- Remove from git index and history:
git rm -r --cached .dart_tool git commit -m "Remove .dart_tool from version control"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.dart_tool/package_config.json around lines 6 - 177, The repo is tracking generated, machine-local files under .dart_tool (e.g., .dart_tool/package_config.json) which should be ignored; add a .dart_tool/ entry to .gitignore, remove the tracked .dart_tool directory from git’s index (for example by removing it from the cache/index) and commit the removal so those files are no longer versioned, then ensure CI/local builds regenerate .dart_tool; reference: .dart_tool/package_config.json and the .dart_tool directory.
♻️ Duplicate comments (2)
build/native_assets/macos/native_assets.json (1)
1-1:⚠️ Potential issue | 🟠 MajorBuild artifacts should not be committed to version control.
This file has the same issue as
build/unit_test_assets/NativeAssetsManifest.json- build artifacts should be excluded via.gitignore.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build/native_assets/macos/native_assets.json` at line 1, Remove the committed build artifact build/native_assets/macos/native_assets.json from version control and add its path (and the similar build/unit_test_assets/NativeAssetsManifest.json pattern) to .gitignore; run git rm --cached for the tracked file(s) so they are removed from the repo while kept locally, then commit the updated .gitignore and the removal commit to prevent future commits of these build artifacts.build/unit_test_assets/FontManifest.json (1)
1-1:⚠️ Potential issue | 🟠 MajorBuild artifacts should not be committed to version control.
This file has the same issue as other files in the
build/directory - these are auto-generated artifacts that should be excluded via.gitignore.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build/unit_test_assets/FontManifest.json` at line 1, FontManifest.json is a generated build artifact and should not be committed; remove the committed FontManifest.json entry, add a rule to ignore build artifacts (e.g., add build/ or specifically FontManifest.json) to .gitignore, run git rm --cached FontManifest.json (or git rm -r --cached build/) to untrack it, and commit the .gitignore change and removal so future builds do not re-add the generated FontManifest.json file.
🧹 Nitpick comments (1)
test/models_test.dart (1)
7-32: Add assertions for model value semantics (==/hashCode).These tests validate constructor wiring, but not the overridden equality/hash behavior in the new models. A small test per model would protect against regressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/models_test.dart` around lines 7 - 32, Add tests that assert value semantics for User, Split, and Expense: create two User instances with identical fields and expect them to be equal (==) and have equal hashCode, and one with a differing field to assert inequality; repeat the same pattern for Split; for Expense create two Expense instances with identical primitive fields and equivalent splits (either identical Split objects or separately constructed equal Splits) and assert == and equal hashCode, plus an inequality case when a field or splits differ; add these as new tests (e.g., "User value semantics", "Split value semantics", "Expense value semantics") alongside the existing instantiation tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@build/unit_test_assets/NativeAssetsManifest.json`:
- Line 1: Remove the committed build artifacts under build/, specifically the
file NativeAssetsManifest.json in build/unit_test_assets, and update the
project's ignore rules so build/ is not tracked; run git rm -r build/ (or remove
that file) to delete the committed artifact and add "build/" to .gitignore so
future artifacts like NativeAssetsManifest.json are not committed.
In `@lib/src/logic/settlement.dart`:
- Around line 18-27: The Transaction equality and hashCode are inconsistent
because operator == uses a tolerance on amount while hashCode uses the raw
double; update both to normalize the amount to an integer cents representation
and use that for comparison and hashing: in Transaction.operator == compare
fromUserId, toUserId and (amount * 100).round() (or a fixed quantization) for
equality, and change Transaction.hashCode to combine fromUserId.hashCode,
toUserId.hashCode and the same integer cents value's hashCode so equal
Transactions produce identical hash codes.
- Around line 58-73: The settlement uses a double minAmount for the transaction
but subtracts the unrounded value from internal balances, causing drift; update
the algorithm in the settlement logic to operate entirely in integer cents:
convert debtor/creditor balances to cents at the start, compute minAmountCents =
min(debtCents, creditCents), record the transaction amount using rounded cents
(use minAmountCents) when calling transactions.add(Transaction(...)), subtract
minAmountCents from debtors and creditors entries (update MapEntry values in
cents), and use integer thresholds (e.g., <1 cent) when advancing i and j;
ensure you convert back to dollars only for any external/output display.
In `@lib/src/models/expense.dart`:
- Around line 9-18: The Expense model currently stores a mutable List<Split> by
reference (field splits set in the Expense constructor), making instances
mutable from outside; to fix, ensure the constructor (Expense) defensively
copies the incoming list and store an immutable view (e.g., wrap with
List.unmodifiable or UnmodifiableListView) and, if you expose splits via a
getter, return the unmodifiable view as well so external mutation cannot change
internal state; update any uses that pass the list to rely on the new immutable
storage.
In `@lib/src/models/split.dart`:
- Around line 3-20: Replace the floating-point monetary fields with integer
minor-unit fields and update equality/hash logic: change Split.amount to an
integer (e.g., cents or minorUnits) in the Split constructor and everywhere
Split is constructed, update the equality operator (operator ==) to compare the
new integer field and update hashCode to use that integer; likewise change other
monetary fields mentioned (Expense.amount, Settlement.amount) to integer
minor-units and update any consumers (calculation_utils.dart, settlement.dart,
validation_utils.dart) to perform arithmetic in integers (or use explicit
conversion helpers) so all comparisons and hash computations remain
deterministic.
In `@lib/src/utils/validation_utils.dart`:
- Around line 25-26: The current comparison (if ((totalSplitAmount -
expense.amount).abs() > 0.01)) lets an exact 0.01 difference pass; change the
comparison operator to >= so the condition becomes (if ((totalSplitAmount -
expense.amount).abs() >= 0.01)) to correctly flag one-cent mismatches for
validation (refer to totalSplitAmount, expense.amount and the errors.add(...)
call).
In `@test/validation_utils_test.dart`:
- Around line 7-41: Add a unit test in test/validation_utils_test.dart that
covers the negative split amount branch: construct an Expense (using Expense and
Split) where one Split has a negative amount, call
ValidationUtils.validateExpense(expense), and assert the returned errors
contains the specific validation message for negative split amounts (match the
exact message or a regex used by validateExpense) so the negative-split
validation path is exercised.
---
Outside diff comments:
In @.dart_tool/package_config.json:
- Around line 6-177: The repo is tracking generated, machine-local files under
.dart_tool (e.g., .dart_tool/package_config.json) which should be ignored; add a
.dart_tool/ entry to .gitignore, remove the tracked .dart_tool directory from
git’s index (for example by removing it from the cache/index) and commit the
removal so those files are no longer versioned, then ensure CI/local builds
regenerate .dart_tool; reference: .dart_tool/package_config.json and the
.dart_tool directory.
---
Duplicate comments:
In `@build/native_assets/macos/native_assets.json`:
- Line 1: Remove the committed build artifact
build/native_assets/macos/native_assets.json from version control and add its
path (and the similar build/unit_test_assets/NativeAssetsManifest.json pattern)
to .gitignore; run git rm --cached for the tracked file(s) so they are removed
from the repo while kept locally, then commit the updated .gitignore and the
removal commit to prevent future commits of these build artifacts.
In `@build/unit_test_assets/FontManifest.json`:
- Line 1: FontManifest.json is a generated build artifact and should not be
committed; remove the committed FontManifest.json entry, add a rule to ignore
build artifacts (e.g., add build/ or specifically FontManifest.json) to
.gitignore, run git rm --cached FontManifest.json (or git rm -r --cached build/)
to untrack it, and commit the .gitignore change and removal so future builds do
not re-add the generated FontManifest.json file.
---
Nitpick comments:
In `@test/models_test.dart`:
- Around line 7-32: Add tests that assert value semantics for User, Split, and
Expense: create two User instances with identical fields and expect them to be
equal (==) and have equal hashCode, and one with a differing field to assert
inequality; repeat the same pattern for Split; for Expense create two Expense
instances with identical primitive fields and equivalent splits (either
identical Split objects or separately constructed equal Splits) and assert ==
and equal hashCode, plus an inequality case when a field or splits differ; add
these as new tests (e.g., "User value semantics", "Split value semantics",
"Expense value semantics") alongside the existing instantiation tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ba1cc54c-e0d9-459c-93c4-d063bf0e3169
⛔ Files ignored due to path filters (4)
build/unit_test_assets/AssetManifest.binis excluded by!**/*.binbuild/unit_test_assets/fonts/MaterialIcons-Regular.otfis excluded by!**/*.otfbuild/unit_test_assets/packages/cupertino_icons/assets/CupertinoIcons.ttfis excluded by!**/*.ttfpubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
.dart_tool/package_config.json.dart_tool/package_graph.json.dart_tool/versionbuild/native_assets/macos/native_assets.jsonbuild/test_cache/build/99111e0c5b6228829e100ef67db14ea2.cache.dill.track.dillbuild/unit_test_assets/FontManifest.jsonbuild/unit_test_assets/NOTICES.Zbuild/unit_test_assets/NativeAssetsManifest.jsonbuild/unit_test_assets/shaders/ink_sparkle.fragbuild/unit_test_assets/shaders/stretch_effect.fraglib/src/logic/settlement.dartlib/src/models/expense.dartlib/src/models/split.dartlib/src/models/user.dartlib/src/utils/calculation_utils.dartlib/src/utils/validation_utils.darttest/calculation_utils_test.darttest/models_test.darttest/settlement_test.darttest/validation_utils_test.dart
Comment on lines
+3
to
+20
| final double amount; | ||
|
|
||
| Split({ | ||
| required this.userId, | ||
| required this.amount, | ||
| }); | ||
|
|
||
| @override | ||
| bool operator ==(Object other) => | ||
| identical(this, other) || | ||
| other is Split && | ||
| runtimeType == other.runtimeType && | ||
| userId == other.userId && | ||
| amount == other.amount; | ||
|
|
||
| @override | ||
| int get hashCode => userId.hashCode ^ amount.hashCode; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Monetary fields using double =="
rg -n -C2 '\bdouble\s+(amount|[a-zA-Z_]*Amount)\b'
echo
echo "== Exact equality checks on amount =="
rg -n -C2 '\bamount\s*==\s*'
echo
echo "== Balance maps using double =="
rg -n -C2 'Map<\s*String\s*,\s*double\s*>'Repository: StabilityNexus/Zplit
Length of output: 3067
Use a deterministic monetary representation instead of double with equality checks.
The codebase uses double for monetary amounts (Split.amount, Expense.amount, Settlement.amount) with direct equality comparisons (e.g., amount == other.amount in the == operator). This pattern is risky due to floating-point precision issues and will produce hard-to-reconcile defects in arithmetic-heavy workflows (calculations in calculation_utils.dart, settlement.dart, validation_utils.dart).
Switch to integer minor units (cents instead of dollars) for deterministic financial logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/src/models/split.dart` around lines 3 - 20, Replace the floating-point
monetary fields with integer minor-unit fields and update equality/hash logic:
change Split.amount to an integer (e.g., cents or minorUnits) in the Split
constructor and everywhere Split is constructed, update the equality operator
(operator ==) to compare the new integer field and update hashCode to use that
integer; likewise change other monetary fields mentioned (Expense.amount,
Settlement.amount) to integer minor-units and update any consumers
(calculation_utils.dart, settlement.dart, validation_utils.dart) to perform
arithmetic in integers (or use explicit conversion helpers) so all comparisons
and hash computations remain deterministic.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/src/logic/settlement.dart (1)
6-10:⚠️ Potential issue | 🟡 MinorGuard
Transactionagainst invalid states at construction time.Line 6 currently allows invalid objects (e.g.,
amount <= 0, empty IDs, or same sender/receiver) ifTransactionis created outsideSettlement.calculateSettlement.🔧 Proposed fix
Transaction({ required this.fromUserId, required this.toUserId, required this.amount, - }); + }) { + if (fromUserId.isEmpty || toUserId.isEmpty) { + throw ArgumentError('User IDs must be non-empty.'); + } + if (fromUserId == toUserId) { + throw ArgumentError('fromUserId and toUserId must be different.'); + } + if (amount <= 0) { + throw ArgumentError.value(amount, 'amount', 'must be greater than zero.'); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/logic/settlement.dart` around lines 6 - 10, The Transaction constructor allows invalid state creation; add input validation inside Transaction (in its constructor or a factory) to reject amount <= 0, empty or null fromUserId/toUserId, and fromUserId == toUserId by throwing ArgumentError (or using Dart asserts) with clear messages; update any code paths that construct Transaction (e.g., Settlement.calculateSettlement) to handle or propagate these errors as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/src/logic/settlement.dart`:
- Around line 32-33: Before generating transactions in calculateSettlement,
validate that the sum of all values in the balances Map equals zero; if it does
not, fail fast (throw an error or return a clear failure) so the algorithm in
calculateSettlement does not silently leave leftover credit/debt. Locate
calculateSettlement and add a check that computes balances.values.reduce(...)
(or equivalent) and if non-zero either throw an ArgumentError with a descriptive
message referencing balances or return an empty List/Result indicating
imbalance—ensure callers of calculateSettlement/Transaction handle this new
error/return contract.
---
Duplicate comments:
In `@lib/src/logic/settlement.dart`:
- Around line 6-10: The Transaction constructor allows invalid state creation;
add input validation inside Transaction (in its constructor or a factory) to
reject amount <= 0, empty or null fromUserId/toUserId, and fromUserId ==
toUserId by throwing ArgumentError (or using Dart asserts) with clear messages;
update any code paths that construct Transaction (e.g.,
Settlement.calculateSettlement) to handle or propagate these errors as needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f7e5aa1-8669-4adb-822c-2a7be0fccf10
📒 Files selected for processing (14)
.dart_tool/dartpad/web_plugin_registrant.dart.dart_tool/package_config.json.dart_tool/package_graph.json.dart_tool/version.gitignorelib/src/logic/settlement.dartlib/src/models/expense.dartlib/src/models/split.dartlib/src/utils/calculation_utils.dartlib/src/utils/validation_utils.darttest/calculation_utils_test.darttest/models_test.darttest/settlement_test.darttest/validation_utils_test.dart
💤 Files with no reviewable changes (4)
- .dart_tool/dartpad/web_plugin_registrant.dart
- .dart_tool/version
- .dart_tool/package_config.json
- .dart_tool/package_graph.json
✅ Files skipped from review due to trivial changes (4)
- .gitignore
- lib/src/utils/calculation_utils.dart
- test/calculation_utils_test.dart
- test/models_test.dart
🚧 Files skipped from review as they are similar to previous changes (5)
- test/settlement_test.dart
- lib/src/utils/validation_utils.dart
- lib/src/models/split.dart
- test/validation_utils_test.dart
- lib/src/models/expense.dart
|
@v1dit can u please open an issue for this as well also please open pr to dev |
|
@v1dit yes yes thats what i am saying change the branch to dev instead of main please |
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
2 participants
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.
Added models (User, Split, Expense) and utilities (ValidationUtils, CalculationUtils, Settlement) with comprehensive tests.
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores