AUM dust accumulator#235
Conversation
|
Warning Review limit reached
Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds a persistent ChangesAUM Fee Dust Accumulation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Reconcile as PerformVaultFeeTransfer
participant Accrual as AccrueAUMFeePayment
participant Vault as VaultAccount
participant Bank
Caller->>Reconcile: trigger fee reconciliation
Reconcile->>Accrual: AccrueAUMFeePayment(ctx, vault, totalAssets)
Accrual->>Vault: parse FeeRemainder, compute decimal fee
Accrual->>Accrual: add remainder, truncate to whole underlying unit
Accrual->>Vault: write leftover fraction back to FeeRemainder
Accrual->>Accrual: convert whole units to PaymentDenom coin
Accrual-->>Reconcile: return collectible sdk.Coin
Reconcile->>Bank: transfer collected fee coin
Reconcile->>Vault: persist updated VaultAccount state
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 |
|
| Filename | Overview |
|---|---|
| keeper/reconcile.go | Adds decimal fee accrual, carry-forward logic, and shared collectible-fee calculation for collection and valuation. |
| types/vault.go | Adds fee_remainder validation, but coarse-denom vaults can still import an unbounded carry. |
| keeper/migrations.go | Normalizes empty legacy remainders to explicit zero while preserving existing values. |
| proto/provlabs/vault/v1/vault.proto | Adds the fee_remainder field and documents carry-forward behavior. |
Reviews (7): Last reviewed commit: "fix AUM fee dust valuation and bound imp..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
keeper/migrations_test.go (1)
80-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest is not table-driven.
This test exercises multiple scenarios (empty remainder normalization, existing remainder preserved, non-vault account untouched, idempotency on re-run) as a single linear sequence rather than a table-driven test with named cases. As per coding guidelines,
**/*_test.go: "Unit and integration tests must use table-driven test patterns to ensure exhaustive coverage of edge cases" and each case should have "a clear, descriptive name that explains the scenario being tested."That said, the idempotency check (running
Migrate1to2twice against shared state) is inherently sequential and doesn't map cleanly onto independent table cases, so this may be an acceptable exception if the shared-state assertions can't be cleanly decomposed.🤖 Prompt for 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. In `@keeper/migrations_test.go` around lines 80 - 133, `TestKeeper_MigrateVaultFeeRemainderDefaults` currently covers several scenarios in one linear flow; refactor the independent assertions into a table-driven test with named cases for empty remainder normalization, existing remainder preservation, and non-vault accounts left untouched. Use `TestKeeper_MigrateVaultFeeRemainderDefaults`, the local `createVault`/`getVault` helpers, and `keeper.NewMigrator(...).Migrate1to2` to structure per-case setup and expectations. Keep the second `Migrate1to2` idempotency check as a separate sequential subtest or clearly isolated exception since it depends on shared state.Source: Coding guidelines
types/vault.go (1)
298-307: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider enforcing the sub-unit invariant (
< 1), not just non-negativity.
FeeRemainderis documented and tested (seeAccrueAUMFeePayment's dust-accumulation test) as always a fraction in[0, 1), butValidate()only rejects negative values. A value>= 1(e.g., from a bad genesis import or future migration bug) would pass validation and get silently folded into the next fee calculation.♻️ Proposed fix to add an upper-bound check
if v.FeeRemainder != "" { remainder, err := sdkmath.LegacyNewDecFromStr(v.FeeRemainder) if err != nil { return fmt.Errorf("invalid fee remainder: %s", v.FeeRemainder) } if remainder.IsNegative() { return fmt.Errorf("fee remainder cannot be negative: %s", v.FeeRemainder) } + if remainder.GTE(sdkmath.LegacyOneDec()) { + return fmt.Errorf("fee remainder must be less than 1: %s", v.FeeRemainder) + } }🤖 Prompt for 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. In `@types/vault.go` around lines 298 - 307, In Vault.Validate, the FeeRemainder check only rejects negatives, so update the validation in that block to also enforce the documented sub-unit invariant for v.FeeRemainder; after parsing with sdkmath.LegacyNewDecFromStr, add an upper-bound check that rejects any remainder greater than or equal to 1.0 with a clear validation error, alongside the existing parse and non-negative checks.
🤖 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.
Nitpick comments:
In `@keeper/migrations_test.go`:
- Around line 80-133: `TestKeeper_MigrateVaultFeeRemainderDefaults` currently
covers several scenarios in one linear flow; refactor the independent assertions
into a table-driven test with named cases for empty remainder normalization,
existing remainder preservation, and non-vault accounts left untouched. Use
`TestKeeper_MigrateVaultFeeRemainderDefaults`, the local
`createVault`/`getVault` helpers, and `keeper.NewMigrator(...).Migrate1to2` to
structure per-case setup and expectations. Keep the second `Migrate1to2`
idempotency check as a separate sequential subtest or clearly isolated exception
since it depends on shared state.
In `@types/vault.go`:
- Around line 298-307: In Vault.Validate, the FeeRemainder check only rejects
negatives, so update the validation in that block to also enforce the documented
sub-unit invariant for v.FeeRemainder; after parsing with
sdkmath.LegacyNewDecFromStr, add an upper-bound check that rejects any remainder
greater than or equal to 1.0 with a clear validation error, alongside the
existing parse and non-negative checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01a73be4-4c20-4064-ae62-9376f76f310d
⛔ Files ignored due to path filters (1)
types/vault.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (15)
.changelog/unreleased/improvements/224-aum-fee-dust.mdapi/provlabs/vault/v1/vault.pulsar.gointerest/interest.gointerest/interest_test.gokeeper/migrations.gokeeper/migrations_test.gokeeper/migrator.gokeeper/reconcile.gokeeper/reconcile_test.gomodule.goproto/provlabs/vault/v1/vault.protospec/02_state.mdspec/06_blocker.mdtypes/vault.gotypes/vault_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
keeper/reconcile_test.go (1)
2285-2324: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueSolid regression test for the dust-carry fix.
Correctly proves naive per-window truncation collects nothing while the remainder-carrying path recovers the full expected fee, matching the issue's intent.
One nit: 10,000 iterations each performing a keeper
AccrueAUMFeePaymentcall (with state writes) is comparatively heavy for a unit test; a smaller iteration count (e.g., 500–1000) would likely still demonstrate the invariant with less runtime cost.🤖 Prompt for 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. In `@keeper/reconcile_test.go` around lines 2285 - 2324, The dust-carry regression test in AccrueAUMFeePayment is correct, but the loop count is unnecessarily high for a unit test and makes it slow. Reduce the iterations in the "AccrueAUMFeePayment dust accumulation preserves revenue across short windows" subtest to a smaller value that still proves naive truncation stays zero while the accumulated path reaches the expected whole fee, and keep the assertions tied to interest.CalculateAUMFee, interest.CalculateAUMFeeDec, and s.k.AccrueAUMFeePayment.
🤖 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.
Nitpick comments:
In `@keeper/reconcile_test.go`:
- Around line 2285-2324: The dust-carry regression test in AccrueAUMFeePayment
is correct, but the loop count is unnecessarily high for a unit test and makes
it slow. Reduce the iterations in the "AccrueAUMFeePayment dust accumulation
preserves revenue across short windows" subtest to a smaller value that still
proves naive truncation stays zero while the accumulated path reaches the
expected whole fee, and keep the assertions tied to interest.CalculateAUMFee,
interest.CalculateAUMFeeDec, and s.k.AccrueAUMFeePayment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78e0220d-9513-4c33-96d0-3478f10a17f1
📒 Files selected for processing (7)
.changelog/unreleased/improvements/224-aum-fee-dust.mddocs/superpowers/plans/2026-06-28-force-pause-flag.mdkeeper/migrations_test.gokeeper/reconcile.gokeeper/reconcile_test.gotypes/vault.gotypes/vault_test.go
✅ Files skipped from review due to trivial changes (2)
- .changelog/unreleased/improvements/224-aum-fee-dust.md
- docs/superpowers/plans/2026-06-28-force-pause-flag.md
🚧 Files skipped from review as they are similar to previous changes (3)
- types/vault.go
- keeper/migrations_test.go
- keeper/reconcile.go
| if v.FeeRemainder != "" { | ||
| remainder, err := sdkmath.LegacyNewDecFromStr(v.FeeRemainder) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid fee remainder: %s", v.FeeRemainder) | ||
| } | ||
| if remainder.IsNegative() { | ||
| return fmt.Errorf("fee remainder cannot be negative: %s", v.FeeRemainder) | ||
| } | ||
| } |
There was a problem hiding this comment.
fee_remainder, but reconciliation treats the parsed value as already accrued fee. A vault imported with a large value such as 1000000 passes validation, then the next fee reconciliation folds that amount into combined, converts it to a payment-denom fee, and adds it to the collectible or outstanding AUM fee even though the accumulator did not produce it. The validation needs to enforce the runtime carry invariant for the vault's payment denom, not just non-negativity.
| } | ||
| if v.PaymentDenom == v.UnderlyingAsset && !remainder.TruncateInt().IsZero() { | ||
| return fmt.Errorf("fee remainder must carry less than one whole unit when payment denom equals underlying asset: %s", v.FeeRemainder) |
There was a problem hiding this comment.
payment_denom equals underlying_asset. For a coarse payment denom, the carry still has a maximum valid value: it must be less than the underlying value needed to produce one more whole payment unit. With this validation, an imported vault using a coarse payment_denom can set fee_remainder to a large value such as 9999; the next fee reconciliation adds that stored value into combined, converts most of it into OutstandingAumFee, and collects fees the vault did not accrue. The coarse-denom branch needs its own bound based on the vault conversion rate, or it needs to reject imported values that cannot be safely bounded here.
1 participant
closes: #224
Summary by CodeRabbit
New Features
Bug Fixes
Tests