Fix: check for 0 before division #3914
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Skipped Deployments
|
WalkthroughThe changes made in this pull request focus on improving the handling of percentage calculations within the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (1)
packages/server/src/queries/complex/portfolio/allocation.ts (1)
129-131: Consider simplifying the otherPercentage calculation.While the current implementation is correct, it could be more concise and consistent with the pattern used in other locations.
Consider this simplified version:
- const otherPercentage = totalCap.isZero() - ? new RatePretty(0) - : new RatePretty(otherAmount).quo(totalCap); + const otherPercentage = totalCap.isZero() + ? new RatePretty(0) + : new RatePretty(otherAmount.quo(totalCap));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/server/src/queries/complex/portfolio/allocation.ts (3 hunks)
🔇 Additional comments (3)
packages/server/src/queries/complex/portfolio/allocation.ts (3)
114-116: LGTM! Consistent zero check implementation.The zero check implementation is consistent with the pattern used elsewhere in the file and correctly protects the asset percentage calculations.
Line range hint
71-131: Consider adding comprehensive test coverage.While the zero check implementations are correct, it would be valuable to add test cases that verify:
- Zero total balance scenarios
- Mixed scenarios with some zero and some non-zero allocations
- Edge cases where only "Other" category has value
Let's check for existing test coverage:
#!/bin/bash # Look for test files related to allocation fd -e test.ts -e spec.ts allocationWould you like me to help generate comprehensive test cases for these scenarios?
71-73: LGTM! Zero check properly implemented.The implementation correctly handles the division by zero case by returning
RatePretty(0)whentotalCapis zero.Let's verify that all division operations in the file are protected:
✅ Verification successful
All division operations are properly protected against zero division
The verification shows that there are three instances of division operations using
quoin the file:
- Line 73: Protected by
totalCap.isZero()check- Line 116: Protected by similar zero check pattern
- Line 131: Also protected by zero check pattern
The test file confirms proper handling of zero cases and various allocation scenarios, including edge cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential division operations that might need zero checks rg -n 'quo\(' packages/server/src/queries/complex/portfolio/allocation.tsLength of output: 206
Script:
#!/bin/bash # Let's try a different approach to search for division operations rg -n "quo|div" packages/server/src/queries/complex/portfolio/allocation.ts # Also search for RatePretty to understand the context better rg -n "RatePretty" packages/server/src/queries/complex/portfolio/allocation.ts # Let's also check the test file to understand the test coverage cat packages/server/src/queries/complex/portfolio/__tests__/allocation.spec.tsLength of output: 11862
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/server/src/queries/complex/portfolio/__tests__/allocation.spec.ts (1)
190-245: Good test coverage for zero balance scenario!The test case effectively validates the division by zero fix by ensuring that:
- Zero balances are handled gracefully
- All assets return 0% percentage and $0 fiat value
- The behavior is consistent across different assets
Consider enhancing the test coverage with:
- Mixed scenarios where some assets have zero balance while others don't
- Explicit assertions that no errors are thrown
Example addition:
it("should handle mixed zero and non-zero balances correctly", () => { const mixedBalanceData = { ...MOCK_DATA.categories, "total-assets": { capitalization: "10.000000000000000000", account_coins_result: [ { coin: { denom: "factory/osmo1pfyxruwvtwk00y8z06dh2lqjdj82ldvy74wzm3/WOSMO", amount: "0", }, cap_value: "0", }, { coin: { denom: "factory/osmo1rckme96ptawr4zwexxj5g5gej9s2dmud8r2t9j0k0prn5mch5g4snzzwjv/sail", amount: "100", }, cap_value: "10.000000000000000000", }, ], is_best_effort: false, }, }; expect(() => calculatePercentAndFiatValues(mixedBalanceData, assetLists, "total-assets", 5) ).not.toThrow(); });
What is the purpose of the change:
Linear Task
Not the end of the world, as it only affects wallets connected with 0 balance. And is caught by error boundary. This is why I'm merging to stage.
Checks for 0 before calculating portfolio %.
Brief Changelog
Testing and Verifying
Added test case for 0 balance allocation.