refactor(billing): removes autoReloadThreshold and autoReloadAmount from wallet settings #2319
Conversation
ygrishajev
changed the title
WalkthroughRemoved auto-reload threshold/amount fields from schemas, DTOs, and service flows; validation now relies only on autoReloadEnabled. Adjusted payment-method defaulting to a sequential unmark/create flow. Replaced toFixed(2) with an Intl.NumberFormat-based ensure2floatingDigits and updated tests accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/billing/repositories/payment-method/payment-method.repository.ts (1)
105-119: Critical: Missingawaiton line 115 causes race condition.Line 115 is missing the
awaitkeyword, which means the second#unmarkAsDefaultExcludingcall is fire-and-forget. The function will return before this operation completes, potentially leaving multiple default payment methods in the database.Additionally, the double unmarking pattern (before and after creation) seems redundant:
- Line 108 unmarks all defaults excluding the new ID (which doesn't exist in the DB yet, so this effectively unmarks all defaults).
- Line 115 unmarks again after creation (intended to clean up any race conditions).
The missing
awaitdefeats the purpose of the second unmark and creates a concurrency bug.Apply this diff to fix the critical bug:
const output = await this.create({ ...input, isDefault: true, id }); - this.#unmarkAsDefaultExcluding(id, tx); + await this.#unmarkAsDefaultExcluding(id, tx); return this.toOutput(output);Optional refactor: Consider whether the second unmark is necessary. If the transaction guarantees isolation, the first unmark (line 108) should be sufficient:
async createAsDefault(input: Omit<PaymentMethodInput, "id" | "isDefault">) { return this.ensureTransaction(async tx => { const id = uuidv4(); await this.#unmarkAsDefaultExcluding(id, tx); const output = await this.create({ ...input, isDefault: true, id }); - await this.#unmarkAsDefaultExcluding(id, tx); return this.toOutput(output); }); }
🧹 Nitpick comments (2)
apps/api/src/billing/services/wallet-settings/wallet-settings.service.ts (1)
12-16: Remove unused fields from WalletSettingInput interface.The
autoReloadThresholdandautoReloadAmountfields are no longer used after this refactoring but remain in the interface definition. This creates confusion about the actual contract.Apply this diff to align the interface with actual usage:
export interface WalletSettingInput { autoReloadEnabled?: boolean; - autoReloadThreshold?: number; - autoReloadAmount?: number; }apps/api/src/billing/services/wallet-settings/wallet-settings.service.spec.ts (1)
219-262: Consider updating the test seeder to reflect removed fields.Based on the relevant code snippets,
generateWalletSettinginapps/api/test/seeders/wallet-setting.seeder.tsstill generatesautoReloadThresholdandautoReloadAmountfields. While this doesn't break tests (the fields are simply ignored), it could be misleading for developers reading the tests.Consider updating the seeder to remove these unused fields for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
apps/api/test/functional/__snapshots__/docs.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (9)
apps/api/src/billing/controllers/wallet-settings/wallet-settings.controller.spec.ts(2 hunks)apps/api/src/billing/http-schemas/wallet.schema.ts(2 hunks)apps/api/src/billing/repositories/payment-method/payment-method.repository.ts(2 hunks)apps/api/src/billing/routes/stripe-payment-methods/stripe-payment-methods.router.ts(1 hunks)apps/api/src/billing/services/balances/balances.service.ts(2 hunks)apps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.spec.ts(1 hunks)apps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.ts(1 hunks)apps/api/src/billing/services/wallet-settings/wallet-settings.service.spec.ts(6 hunks)apps/api/src/billing/services/wallet-settings/wallet-settings.service.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{ts,tsx,js}: Never use typeanyor cast to typeany. Always define the proper TypeScript types.
Never use deprecated methods from libraries.
Don't add unnecessary comments to the code.
Files:
apps/api/src/billing/routes/stripe-payment-methods/stripe-payment-methods.router.tsapps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.tsapps/api/src/billing/services/balances/balances.service.tsapps/api/src/billing/services/wallet-settings/wallet-settings.service.tsapps/api/src/billing/controllers/wallet-settings/wallet-settings.controller.spec.tsapps/api/src/billing/repositories/payment-method/payment-method.repository.tsapps/api/src/billing/services/wallet-settings/wallet-settings.service.spec.tsapps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.spec.tsapps/api/src/billing/http-schemas/wallet.schema.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under testUse
setupfunction instead ofbeforeEachin test files. Thesetupfunction must be at the bottom of the rootdescribeblock, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type.
**/*.spec.{ts,tsx}: Use<Subject>.namein the root describe suite description instead of hardcoded class/service name strings to enable automated refactoring tools to find all references
Use either a method name or a condition starting with 'when' for nested suite descriptions in tests
Use present simple, 3rd person singular for test descriptions without prepending 'should'
Files:
apps/api/src/billing/controllers/wallet-settings/wallet-settings.controller.spec.tsapps/api/src/billing/services/wallet-settings/wallet-settings.service.spec.tsapps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.spec.ts
🧠 Learnings (3)
📚 Learning: 2025-09-18T07:44:29.079Z
Learnt from: baktun14
Repo: akash-network/console PR: 1933
File: apps/deploy-web/src/pages/payment.tsx:92-116
Timestamp: 2025-09-18T07:44:29.079Z
Learning: In the payment system, discount calculation is not applied during payment processing. Instead, when users claim coupons/discounts, the discount amount is applied directly to their account balance. Therefore, the payment flow should charge parseFloat(amount) as entered, not a discounted amount.
Applied to files:
apps/api/src/billing/services/balances/balances.service.ts
📚 Learning: 2025-10-15T16:39:55.348Z
Learnt from: jzsfkzm
Repo: akash-network/console PR: 2039
File: apps/deploy-web/tests/ui/change-wallets.spec.ts:4-10
Timestamp: 2025-10-15T16:39:55.348Z
Learning: In the Akash Console E2E tests using the context-with-extension fixture, the first wallet is automatically created during fixture setup via `importWalletToLeap` in `apps/deploy-web/tests/ui/fixture/wallet-setup.ts`, so tests that call `frontPage.createWallet()` are creating a second wallet to test wallet switching functionality.
Applied to files:
apps/api/src/billing/controllers/wallet-settings/wallet-settings.controller.spec.tsapps/api/src/billing/services/wallet-settings/wallet-settings.service.spec.tsapps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.spec.ts
📚 Learning: 2025-09-25T14:31:44.914Z
Learnt from: baktun14
Repo: akash-network/console PR: 1969
File: apps/deploy-web/src/pages/payment.tsx:179-191
Timestamp: 2025-09-25T14:31:44.914Z
Learning: The payment confirmation endpoint in apps/api/src/billing/http-schemas/stripe.schema.ts uses zod schema validation with `amount: z.number().gte(20, "Amount must be greater or equal to $20")` to ensure all payment requests meet the minimum amount requirement, preventing zero-amount or invalid payments from reaching Stripe.
Applied to files:
apps/api/src/billing/http-schemas/wallet.schema.ts
🧬 Code graph analysis (2)
apps/api/src/billing/services/wallet-settings/wallet-settings.service.ts (1)
apps/api/src/billing/repositories/wallet-settings/wallet-settings.repository.ts (1)
WalletSettingInput(12-17)
apps/api/src/billing/services/wallet-settings/wallet-settings.service.spec.ts (1)
apps/api/test/seeders/wallet-setting.seeder.ts (1)
generateWalletSetting(5-18)
⏰ 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: validate / validate-app
- GitHub Check: test-build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (12)
apps/api/src/billing/services/balances/balances.service.ts (2)
14-18: LGTM! Currency formatter configured correctly.The use of
Intl.NumberFormatwith en-US locale and exactly 2 decimal places is appropriate for USD currency formatting. Disabling grouping (useGrouping: false) ensures the formatted output can be cleanly parsed back to a Number without locale-specific separators.
146-148: LGTM! Using Intl.NumberFormat for precision is sound.The roundabout conversion (Number → formatted string → Number) is intentional and provides standards-compliant CLDR rounding for currency values. This approach is more robust than
toFixed(2)for financial calculations and correctly handles edge cases like NaN and Infinity.apps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.ts (1)
169-169: LGTM! Threshold normalization ensures consistent precision.Wrapping the threshold calculation with
ensure2floatingDigitsensures both the balance and threshold are compared at the same 2-decimal precision, preventing potential floating-point comparison issues.apps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.spec.ts (1)
330-332: LGTM! Mock implementation aligns with handler usage.The explicit
ensure2floatingDigitsmock returning the input unchanged is appropriate for these tests, which use clean decimal values that don't require actual rounding. This follows the correct pattern withjest-mock-extended.apps/api/src/billing/routes/stripe-payment-methods/stripe-payment-methods.router.ts (1)
47-47: LGTM!The change from template literal to plain string is a good simplification. It's clearer and more consistent with other route declarations in the file.
apps/api/src/billing/controllers/wallet-settings/wallet-settings.controller.spec.ts (2)
39-51: LGTM! Tests correctly reflect simplified wallet settings payload.The test properly validates that
createWalletSettingsnow accepts onlyautoReloadEnabledand that the service is called with the correct simplified payload.
60-72: LGTM! Update test properly simplified.The test correctly validates the simplified update payload containing only
autoReloadEnabled.apps/api/src/billing/services/wallet-settings/wallet-settings.service.ts (1)
67-67: LGTM! Validation logic correctly simplified.The validation method signature has been properly updated to remove the
prevparameter, and the validation logic now correctly enforces only the prerequisites for auto-reload (user existence, Stripe customer setup, and default payment method) whenautoReloadEnabledis true.Also applies to: 107-122
apps/api/src/billing/http-schemas/wallet.schema.ts (1)
49-51: LGTM! Schema correctly simplified.The
WalletSettingsSchemanow correctly contains onlyautoReloadEnabled, aligning with the PR objectives to remove threshold and amount fields.apps/api/src/billing/services/wallet-settings/wallet-settings.service.spec.ts (3)
41-59: LGTM! Test correctly updated for simplified payload.The "updates existing wallet setting" test properly reflects that only
autoReloadEnabledis now passed to and expected by the service methods.
61-92: LGTM! Create test properly updated.The test correctly validates wallet setting creation with only the
autoReloadEnabledfield and verifies proper job scheduling.
204-216: LGTM! Good addition of job cancellation test.This new test case ensures that when a wallet setting with an associated
autoReloadJobIdis deleted, the reload job is also cancelled. This provides important coverage for the cleanup logic in the service'sdeleteWalletSettingmethod (line 142).
apps/api/src/billing/repositories/payment-method/payment-method.repository.ts
Show resolved
Hide resolved
ygrishajev
force-pushed
the
feature/billing
branch
from
dfa7f9b to
655b762
Compare
…rom wallet settings - Remove threshold/amount fields from WalletSettingsSchema - Simplify wallet settings validation to only check payment method - Update tests to reflect removed validation - Fix payment method default marking order - Use Intl.NumberFormat for currency formatting refs #1779
ygrishajev
force-pushed
the
feature/billing
branch
from
655b762 to
af0cda8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/api/src/billing/services/balances/balances.service.ts (1)
146-148:ensure2floatingDigitssemantics essentially preserved; only minor edge-case differences possibleSwitching to
Number(this.#currencyFormatter.format(amount))keeps the public behavior (number rounded to 2 decimals) consistent with the oldparseFloat(amount.toFixed(2))for normal inputs. This is cleaner and centralizes formatting via the shared formatter.Two small considerations:
- For extreme values /
Infinity/NaN,Intl.NumberFormatmay format to strings like"∞"whichNumber()turns intoNaN(previouslyparseFloat("Infinity")would returnInfinity). Likely not hit in practice, but worth being aware of.- The name
#currencyFormatterslightly suggests currency-symbol formatting, but the options are plain decimal; if this ever becomes confusing, renaming to something like#decimal2FractionFormattercould improve clarity.Function is otherwise correct and keeps the return type as
number.If you’d like to confirm no regressions, you can run a quick comparison against the old logic for a range of test values in a REPL (Node or browser console).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
apps/api/test/functional/__snapshots__/docs.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (9)
apps/api/src/billing/controllers/wallet-settings/wallet-settings.controller.spec.ts(2 hunks)apps/api/src/billing/http-schemas/wallet.schema.ts(2 hunks)apps/api/src/billing/repositories/payment-method/payment-method.repository.ts(1 hunks)apps/api/src/billing/routes/stripe-payment-methods/stripe-payment-methods.router.ts(1 hunks)apps/api/src/billing/services/balances/balances.service.ts(2 hunks)apps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.spec.ts(1 hunks)apps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.ts(1 hunks)apps/api/src/billing/services/wallet-settings/wallet-settings.service.spec.ts(6 hunks)apps/api/src/billing/services/wallet-settings/wallet-settings.service.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/api/src/billing/routes/stripe-payment-methods/stripe-payment-methods.router.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.ts
- apps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.spec.ts
- apps/api/src/billing/repositories/payment-method/payment-method.repository.ts
- apps/api/src/billing/services/wallet-settings/wallet-settings.service.ts
- apps/api/src/billing/http-schemas/wallet.schema.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{ts,tsx,js}: Never use typeanyor cast to typeany. Always define the proper TypeScript types.
Never use deprecated methods from libraries.
Don't add unnecessary comments to the code.
Files:
apps/api/src/billing/controllers/wallet-settings/wallet-settings.controller.spec.tsapps/api/src/billing/services/balances/balances.service.tsapps/api/src/billing/services/wallet-settings/wallet-settings.service.spec.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under testUse
setupfunction instead ofbeforeEachin test files. Thesetupfunction must be at the bottom of the rootdescribeblock, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type.
**/*.spec.{ts,tsx}: Use<Subject>.namein the root describe suite description instead of hardcoded class/service name strings to enable automated refactoring tools to find all references
Use either a method name or a condition starting with 'when' for nested suite descriptions in tests
Use present simple, 3rd person singular for test descriptions without prepending 'should'
Files:
apps/api/src/billing/controllers/wallet-settings/wallet-settings.controller.spec.tsapps/api/src/billing/services/wallet-settings/wallet-settings.service.spec.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: baktun14
Repo: akash-network/console PR: 1969
File: apps/deploy-web/src/pages/payment.tsx:179-191
Timestamp: 2025-09-25T14:31:44.914Z
Learning: The payment confirmation endpoint in apps/api/src/billing/http-schemas/stripe.schema.ts uses zod schema validation with `amount: z.number().gte(20, "Amount must be greater or equal to $20")` to ensure all payment requests meet the minimum amount requirement, preventing zero-amount or invalid payments from reaching Stripe.
📚 Learning: 2025-10-15T16:39:55.348Z
Learnt from: jzsfkzm
Repo: akash-network/console PR: 2039
File: apps/deploy-web/tests/ui/change-wallets.spec.ts:4-10
Timestamp: 2025-10-15T16:39:55.348Z
Learning: In the Akash Console E2E tests using the context-with-extension fixture, the first wallet is automatically created during fixture setup via `importWalletToLeap` in `apps/deploy-web/tests/ui/fixture/wallet-setup.ts`, so tests that call `frontPage.createWallet()` are creating a second wallet to test wallet switching functionality.
Applied to files:
apps/api/src/billing/controllers/wallet-settings/wallet-settings.controller.spec.tsapps/api/src/billing/services/wallet-settings/wallet-settings.service.spec.ts
📚 Learning: 2025-09-18T07:44:29.079Z
Learnt from: baktun14
Repo: akash-network/console PR: 1933
File: apps/deploy-web/src/pages/payment.tsx:92-116
Timestamp: 2025-09-18T07:44:29.079Z
Learning: In the payment system, discount calculation is not applied during payment processing. Instead, when users claim coupons/discounts, the discount amount is applied directly to their account balance. Therefore, the payment flow should charge parseFloat(amount) as entered, not a discounted amount.
Applied to files:
apps/api/src/billing/services/balances/balances.service.ts
🧬 Code graph analysis (1)
apps/api/src/billing/services/wallet-settings/wallet-settings.service.spec.ts (1)
apps/api/test/seeders/wallet-setting.seeder.ts (1)
generateWalletSetting(5-18)
⏰ 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). (1)
- GitHub Check: validate / validate-app
🔇 Additional comments (4)
apps/api/src/billing/services/balances/balances.service.ts (1)
14-18: Good reuse ofIntl.NumberFormatinstance for 2-decimal formattingUsing a shared
#currencyFormatterwith fixed 2 fraction digits anduseGrouping: falseavoids repeated formatter construction and keeps behavior consistent with previous 2-decimal rounding in one place. Implementation looks solid and type-safe.If you want to be extra-sure the behavior matches the old implementation, you could spot-check a few values (e.g., 0, 1.005, large numbers) against the previous
toFixed(2)-based logic in a small script.apps/api/src/billing/controllers/wallet-settings/wallet-settings.controller.spec.ts (1)
39-51: Tests correctly aligned to simplified wallet-settings payloadUsing only
autoReloadEnabledin both the controller request payloads and theupsertWalletSettingexpectations matches the new schema/service contract and keeps the tests focused on the single remaining setting. No issues here.Also applies to: 60-71
apps/api/src/billing/services/wallet-settings/wallet-settings.service.spec.ts (2)
41-59: Upsert tests now correctly exercise the simplified auto-reload contractAll updated
upsertWalletSettingtests consistently use{ autoReloadEnabled: boolean }as the input payload, and the expectations onupdateById/createreflect that shape. The additional case for transitioning from disabled → enabled and assertingwalletReloadJobService.scheduleForWalletSettinggives good coverage of the new job-scheduling behavior. This suite looks accurate and coherent with the refactored service.Also applies to: 61-118, 120-151, 181-191
203-216: Delete behavior test nicely covers job cancellationThe new
"deletes wallet setting and cancels job when autoReloadJobId exists"test verifies both repository calls andwalletReloadJobService.cancel(user.id, existingJobId), which is exactly the side effect you want on deletion. This meaningfully strengthens coverage of the delete path.
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (25.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2319 +/- ##
==========================================
- Coverage 48.03% 47.69% -0.34%
==========================================
Files 1038 1028 -10
Lines 29677 29325 -352
Branches 7630 7592 -38
==========================================
- Hits 14255 13988 -267
+ Misses 14946 14863 -83
+ Partials 476 474 -2
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
refs #1779
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.