fix: don't throw an error when reserving zero #1856
Conversation
WalkthroughUpdates CachedBalance.reserveSufficientAmount to validate inputs: negative amounts now throw "Invalid amount"; zero returns 0. Positive amounts reserve up to available balance via Math.min, decrementing the cached value. Unit tests split zero and negative cases. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant CB as CachedBalance
Caller->>CB: reserveSufficientAmount(desiredAmount)
alt desiredAmount < 0
CB-->>Caller: throw Error("Invalid amount")
else desiredAmount == 0
CB-->>Caller: return 0
else desiredAmount > 0
CB->>CB: reserved = Math.min(desiredAmount, value)
CB->>CB: value -= reserved
CB-->>Caller: return reserved
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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
🧹 Nitpick comments (5)
apps/api/src/deployment/services/cached-balance/cached-balance.service.ts (2)
9-11: Good validation; add non-finite guard + explicit zero fast-pathCovers negatives. Consider rejecting NaN/Infinity and explicitly returning early for zero to make intent unambiguous.
public reserveSufficientAmount(desiredAmount: number) { - if (desiredAmount < 0) { + if (!Number.isFinite(desiredAmount)) { + throw new Error(`Invalid amount: ${desiredAmount}`); + } + if (desiredAmount < 0) { throw new Error(`Invalid amount: ${desiredAmount}`); } + if (desiredAmount === 0) { + return 0; + }
26-36: Unbounded address cache could grow indefinitelyIf many distinct addresses are touched, Map may retain entries for the process lifetime. Consider TTL/LRU eviction or an explicit invalidate API (e.g., after successful top-up).
apps/api/src/deployment/services/cached-balance/cached-balance.service.spec.ts (3)
67-72: Zero-amount case covered—niceTest captures the new contract. Optionally, add a follow-up reserve to assert balance remains untouched by zero reservation.
const amount = balance.reserveSufficientAmount(0); expect(amount).toBe(0); + const next = balance.reserveSufficientAmount(1000); + expect(next).toBe(1000);
74-78: Negative amount case aligned with new errorPrefer regex match to avoid coupling to full message text.
-expect(() => balance.reserveSufficientAmount(-100)).toThrow("Invalid amount"); +expect(() => balance.reserveSufficientAmount(-100)).toThrow(/Invalid amount/);
10-16: Refactor tests to repository guidelines (setup function + jest-mock-extended, no shared state)Current tests use beforeEach and manual jest.fn mocks. The repo guidelines require a setup() at the bottom of the root describe, no shared state, and using jest-mock-extended.
Example rewrite sketch (key parts only):
import { mockDeep } from "jest-mock-extended"; describe(CachedBalanceService.name, () => { describe("get", () => { it("should fetch and cache balance for new address", async () => { const { service, balancesService, address } = setup({ deploymentLimit: 1000 }); // ... }); // other its... function setup({ deploymentLimit = 1000 }: { deploymentLimit?: number }) { const balancesService = mockDeep<BalancesService>(); balancesService.getFreshLimits.mockResolvedValue({ deployment: deploymentLimit, fee: 100 }); const service = new CachedBalanceService(balancesService); const address = createAkashAddress(); return { service, balancesService, address }; } }); });Additionally, for consistency you can update other string-based toThrow usages (e.g., Line 57) to regex:
/Insufficient balance/.Also applies to: 22-27
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/api/src/deployment/services/cached-balance/cached-balance.service.spec.ts(1 hunks)apps/api/src/deployment/services/cached-balance/cached-balance.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()to mock dependencies in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under test.
**/*.spec.{ts,tsx}: Usesetupfunction instead ofbeforeEachin test files
setupfunction must be at the bottom of the rootdescribeblock in test files
setupfunction creates an object under test and returns it
setupfunction should accept a single parameter with inline type definition
Don't use shared state insetupfunction
Don't specify return type ofsetupfunction
Files:
apps/api/src/deployment/services/cached-balance/cached-balance.service.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/api/src/deployment/services/cached-balance/cached-balance.service.spec.tsapps/api/src/deployment/services/cached-balance/cached-balance.service.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/api/src/deployment/services/cached-balance/cached-balance.service.spec.tsapps/api/src/deployment/services/cached-balance/cached-balance.service.ts
⏰ 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). (2)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
🔇 Additional comments (2)
apps/api/src/deployment/services/cached-balance/cached-balance.service.ts (2)
13-15: Insufficient-at-zero behavior is consistent with testsThrowing only when balance is exactly 0 preserves prior semantics while allowing partial reservation otherwise. LGTM.
17-20: Reserve-up-to logic looks correctMath.min with in-place decrement prevents negatives and returns the actual reserved amount. LGTM.
jzsfkzm
force-pushed
the
bugfixes/1841-reserve-zero
branch
from
56fe101 to
347c02b
Compare
|
|
||
| public reserveSufficientAmount(desiredAmount: number) { | ||
| const value = Math.min(desiredAmount, this.value); | ||
| if (desiredAmount < 0) { |
There was a problem hiding this comment.
We should investigate a little further as to why there are negative amounts. It seems to be coming from this
There was a problem hiding this comment.
@baktun14 ok, let's put this PR aside while we check logs, I added a few lines of logging in this other PR. I suppose problem is with deployment.blockRate, as the other two factors of that product are constants.
There was a problem hiding this comment.
My change in this PR is not about negative values though, just made the change so 0 won't throw an error any more.
closes #1841
Summary by CodeRabbit
New Features
Bug Fixes
Tests