feat(billing): sweep master wallet AKT above fee reserve into ACT - #3561
feat(billing): sweep master wallet AKT above fee reserve into ACT#3561baktun14 wants to merge 3 commits into
Conversation
Replaces the target-ACT-balance mint (MASTER_WALLET_TARGET_ACT_BALANCE) with a capped sweep: each mint-act run burns the AKT excess above a 2,000 AKT fee reserve (MASTER_WALLET_AKT_RESERVE), capped at 5,000 AKT per run (MASTER_WALLET_MAX_MINT_UAKT) so the backlog drains gradually. Runs skip without error while a prior mint settles, when there is no excess, or when the excess is below the BME minimum, so the command can run on a frequent cron to mint newly deposited AKT right away.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe master-wallet mint flow now burns excess AKT above a configured reserve, caps each run, and mints the calculated ACT amount. Configuration validation, service tests, controller delegation, and CLI descriptions now use the new flow. ChangesMaster-wallet excess AKT minting
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
apps/api/src/billing/config/env.config.spec.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/billing/config/env.config.ts`:
- Around line 54-55: Update the MASTER_WALLET_AKT_RESERVE and
MASTER_WALLET_MAX_MINT_UAKT schema fields to require integer, nonnegative values
by adding the corresponding Zod validations before their defaults. Add or update
tests covering rejection of negative and fractional inputs for both
configuration fields.
In
`@apps/api/src/billing/services/master-wallet-mint/master-wallet-mint.service.ts`:
- Around line 43-90: Serialize master-wallet mint execution with a durable
per-wallet lock shared by both the cron path and mint-act flow. Acquire the lock
before hasPendingSettlement in mintExcessAkt, hold it through balance
calculation and executeMint, and release it reliably on every return path; use
the existing persistent locking mechanism or add one backed by durable storage
rather than an in-process mutex.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cee18c4a-e5e4-48cb-a01f-de670ea28129
📒 Files selected for processing (6)
apps/api/src/app/console.tsapps/api/src/billing/config/env.config.tsapps/api/src/billing/controllers/master-wallet-mint/master-wallet-mint.controller.spec.tsapps/api/src/billing/controllers/master-wallet-mint/master-wallet-mint.controller.tsapps/api/src/billing/services/master-wallet-mint/master-wallet-mint.service.spec.tsapps/api/src/billing/services/master-wallet-mint/master-wallet-mint.service.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but I'm deferring given this rewrites the master wallet's mint/burn logic that moves real AKT/ACT value on mainnet — worth a human's eyes given the financial impact of a miscalculation.
What was reviewed: the reserve/excess/cap calculation, the BME-minimum skip path, the overlap guard against concurrent mints, and the dry-run/error-path behavior changes. I also checked whether the per-run mint cap could be set below the BME minimum mint and permanently stall minting — with the default AKT price range this isn't triggered, but it's a config-dependent edge case worth being aware of operationally.
Extended reasoning...
Overview
This PR reworks MasterWalletMintService (apps/api/src/billing/services/master-wallet-mint) from a 'top up ACT to a target' model to a 'burn AKT above a fee reserve, capped per run' model. It renames mintIfNeeded to mintExcessAkt, replaces MASTER_WALLET_TARGET_ACT_BALANCE with MASTER_WALLET_AKT_RESERVE and MASTER_WALLET_MAX_MINT_UAKT, adds an overlap guard against concurrent/overlapping mints via pending BME ledger records, and changes several previously-error conditions to info/warn-level skips. Tests were rewritten in step with the new behavior and cover the reserve threshold, per-run cap, BME-minimum skip, overlap guard, settlement polling, and balance verification paths.
Security risks
No auth/permission surface is touched. The main risk category is financial correctness rather than a classic security vulnerability: this service burns AKT and mints ACT on the master wallet, which also acts as the fee granter for all managed wallets. A miscalculation (e.g. burning more than the reserve allows, or a race between overlapping runs) could drain the fee reserve needed for gas across the platform. The new overlap guard (skip while pending BME ledger records exist) mitigates double-burns from concurrent runs, contingent on the ops-side non-overlapping cron/flock mentioned in the PR description actually being configured.
Level of scrutiny
This is billing logic that manages real on-chain value (AKT/ACT) for a production mainnet service, not just an internal config tweak. Even though the diff is well-scoped to one service plus its controller/CLI wiring, and the tests are thorough and mirror the new behavior precisely, the domain (mint/burn economics, reserve accounting) merits a human review pass rather than pure automated sign-off, given the consequence of a subtle calculation error is potential value loss or a stuck fee reserve.
Other factors
The bug-hunting system found no concrete bugs. One candidate issue was raised and investigated: whether the per-run mint cap (MASTER_WALLET_MAX_MINT_UAKT) being applied before the BME-minimum check could permanently stall minting if the cap is set below the BME minimum mint amount. Under the documented defaults (5,000 AKT cap vs. a BME minimum in the single-digit-AKT range at realistic AKT prices) this isn't triggered, so it wasn't treated as a blocking bug, but it does mean an operator misconfiguring the cap far below reserve/minimum could silently wedge minting forever with only info-level logs. Tests are comprehensive and match the new behavior; no leftover references to the removed mintIfNeeded/MASTER_WALLET_TARGET_ACT_BALANCE were found elsewhere in the codebase.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3561 +/- ##
==========================================
+ Coverage 75.27% 75.29% +0.01%
==========================================
Files 1163 1163
Lines 30165 30170 +5
Branches 7504 7510 +6
==========================================
+ Hits 22708 22716 +8
- Misses 6585 6589 +4
+ Partials 872 865 -7
🚀 New features to boost your workflow:
|
Constrain MASTER_WALLET_AKT_RESERVE and MASTER_WALLET_MAX_MINT_UAKT to non-negative integers. A negative reserve inflated the computed excess past the actual balance, and a fractional value reached getMintACTMsg as a non-integral coin amount.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/billing/config/env.config.spec.ts`:
- Around line 17-20: Extend the configuration tests in
apps/api/src/billing/config/env.config.spec.ts at lines 17-20 and 34-37: add
assertions that setup succeeds when MASTER_WALLET_AKT_RESERVE is "0" and when
MASTER_WALLET_MAX_MINT_UAKT is "0", preserving the existing positive-value
coverage.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 99a7f426-6744-4dcd-bb70-c43291b02568
📒 Files selected for processing (2)
apps/api/src/billing/config/env.config.spec.tsapps/api/src/billing/config/env.config.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/billing/config/env.config.ts
There was a problem hiding this comment.
No bugs found. This is a well-scoped, well-tested rework of the master wallet mint logic (deficit-fill → reserve-based excess burn), and the CodeRabbit-flagged config validation gap (negative/fractional reserve and cap values) was already fixed in a follow-up commit.
Extended reasoning...
Overview
The PR reworks MasterWalletMintService.mintIfNeeded into mintExcessAkt: instead of topping ACT up to a target, it now burns AKT above a configurable reserve, capped per run. Touches the service, its controller, CLI description text, and env schema (two new int/nonnegative env vars replacing one removed var). No changes to auth, external API surface, or database schema.
Security risks
None significant. This is an internal CLI/cron job operating on the master wallet, not exposed to end users. The one real risk CodeRabbit flagged — negative/fractional MASTER_WALLET_AKT_RESERVE/MASTER_WALLET_MAX_MINT_UAKT letting the burn amount exceed the wallet balance or reach the chain as a non-integral coin amount — was fixed in commit fe957be with .int().nonnegative() plus new tests. I re-verified the reserve math by hand: the code now checks aktBalance < reserve before computing excess = aktBalance - reserve, and caps aktToBurn to excess, so the reserve is provably preserved after a burn.
Level of scrutiny
Moderate: this moves real funds (mints ACT by burning AKT) but only in a manual/cron CLI context guarded by dry-run support, an overlap guard against concurrent runs, and post-broadcast balance verification with retries. The bug-hunting pass raised a TOCTOU concern on the overlap guard between the pending-ledger check and the broadcast, but verification concluded it is not exploitable given the intended non-overlapping cron deployment (concurrencyPolicy: Forbid).
Other factors
Test coverage is thorough — the service spec covers all branches (pending settlement skip, below/at reserve, below BME minimum, full burn, capped burn, price failure, tx failure, settlement timeout, balance-verification timeout, dry-run) and a new env.config.spec.ts covers the negative/fractional validation. Confirmed no other code references the removed MASTER_WALLET_TARGET_ACT_BALANCE env var. Given the fix for the one substantive review comment already landed and tests match the new behavior, I don't see anything additional worth blocking on.
Both fields accept 0 via .nonnegative(); assert each parses at the zero boundary.
There was a problem hiding this comment.
No new issues found, but this PR reworks the master-wallet AKT→ACT minting logic that moves real funds (burn/mint amounts, fee reserve sizing, overlap guard), so it's worth a human sign-off given the financial impact and the already-discussed accepted race-condition risk.
Reviewed: excess/reserve/cap arithmetic in mintExcessAkt, the BME minimum-mint fallback path, the pending-settlement overlap guard, and the new env schema validation (int/nonnegative, zero-boundary) added in response to CodeRabbit's earlier comments.
Extended reasoning...
Overview
This PR reworks MasterWalletMintService from a fill-to-target model to a reserve-based excess-burn model: it computes AKT above a configured reserve, caps the burn per run, mints ACT via MsgMintACT, and skips (rather than errors) on several previously-error states. It also adds two new validated env vars (MASTER_WALLET_AKT_RESERVE, MASTER_WALLET_MAX_MINT_UAKT) replacing the removed MASTER_WALLET_TARGET_ACT_BALANCE, and updates the controller/CLI wiring and tests accordingly.
Security risks
No auth/crypto/permission surface is touched. The main risk category is financial correctness rather than security: incorrect reserve/cap math or a race between concurrent cron runs could over-burn AKT or leave the fee-granter wallet under-funded. The PR author and CodeRabbit already discussed the check-to-broadcast race window and explicitly accepted it as a low-probability, self-healing risk (a failed on-chain tx or a one-cycle reserve dip) rather than adding a durable lock.
Level of scrutiny
This is money-moving logic in a fee-granter wallet used across all managed-wallet transactions, so I'd treat it as a critical-path change deserving human sign-off despite no bugs being found this run — the class of risk here (unrecoverable fund-flow bugs on a production wallet) is exactly what the approval guidelines call out for deferral, even when the diff itself is clean.
Other factors
The PR already went through a thorough CodeRabbit review cycle: schema validation gaps (missing int/nonnegative) and a zero-boundary test gap were both raised and fixed in follow-up commits (fe957be, 7f90712), and the concurrency question was explicitly raised, discussed, and accepted as a known tradeoff. Test coverage for the new skip/cap/reserve paths looks thorough. Given the financial blast radius, I'm deferring rather than approving.
2 participants
Why
The daily
mint-actjob currently tops the master wallet up to a 10k ACT target, leaving all remaining AKT idle in the wallet. We want the opposite: keep as little AKT in the master wallet as possible — only a fee reserve — and convert everything else to ACT, including AKT deposited later.What
Reworks
MasterWalletMintServicefrom "fill ACT deficit up to target" to "burn AKT excess above a fee reserve, capped per run":MASTER_WALLET_AKT_RESERVE(default 2,000 AKT — kept for tx fees; this wallet is the fee granter for all managed-wallet txs) andMASTER_WALLET_MAX_MINT_UAKT(default 5,000 AKT per run, so a large backlog drains gradually). RemovesMASTER_WALLET_TARGET_ACT_BALANCE(a stale value in prod env is harmless — the schema is not strict).MASTER_WALLET_BALANCE_BELOW_RESERVE). Only an invalid oracle price and tx/settlement failures remain errors, so a frequent cron doesn't page on healthy no-op states.mint-act(existing cron and manual invocation keep working); dry-run unchanged.Ops rollout (out of repo)
mint-act --dry-run/mint-actmanually to start the drain.MASTER_WALLET_MINT_SKIPPEDlogs).concurrencyPolicy: Forbid/ flock).Summary by CodeRabbit
New Features
Bug Fixes