-
Notifications
You must be signed in to change notification settings - Fork 4
fix: save-all when balance is negative #106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/interpreter/interpreter.go (1)
398-400: LGTM! Consider adding a clarifying comment.The optimization is correct: when saving "all", there's no need to reset the balance if it's already zero or negative (overdraft). This avoids unnecessary state mutations and is semantically accurate—you can't save funds from an overdrawn account.
Consider adding a brief inline comment to explain this behavior for future maintainers.
Apply this diff to add documentation:
if amt == nil { + // Only reset balance if positive; zero/negative balances are already "saved out" if balance.Sign() > 0 { balance.Set(big.NewInt(0)) } }internal/interpreter/testdata/script-tests/save/save-from-account__save-all-negative-balance.num (1)
1-6: Consider adding comments to clarify the test scenario.The test filename suggests it validates saving from an account with a negative balance, but the initial account state isn't explicit in the test file. Adding comments would help future maintainers understand:
- What is @alice's initial balance (positive, zero, or negative)?
- What behavior is being validated by this test?
- What are the expected outcomes after each operation?
For example:
+# Test: Save-all should be a no-op when balance is negative (overdraft) +# Initial: @alice has negative or zero balance +# Expected: save-all doesn't error, subsequent overdraft works save [USD *] from @alice
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
internal/interpreter/testdata/script-tests/save/save-from-account__save-all-negative-balance.num.specs.jsonis excluded by!**/*.json
📒 Files selected for processing (2)
internal/interpreter/interpreter.go(1 hunks)internal/interpreter/testdata/script-tests/save/save-from-account__save-all-negative-balance.num(1 hunks)
⏰ 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). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Tests
- GitHub Check: Dirty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 3 files
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #106 +/- ##
==========================================
- Coverage 68.13% 68.11% -0.02%
==========================================
Files 45 45
Lines 4290 4291 +1
==========================================
Hits 2923 2923
Misses 1209 1209
- Partials 158 159 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by cubic
Fixes save-all so negative balances are not reset to zero. This prevents incorrect fund availability and correctly triggers missingFunds for overdrawn accounts.
Written for commit 23909b2. Summary will update automatically on new commits.