Add technology fee interest paid aum#146
Conversation
WalkthroughAdds a new VaultAccount field Changes
Sequence Diagram(s)sequenceDiagram
participant Reconciler
participant Keeper
participant VaultAccount
participant BankModule
Reconciler->>Keeper: reconcileVaultInterest(ctx)
loop for each vault
Keeper->>Keeper: settleVaultPeriod(vault)
Keeper->>Keeper: PerformVaultTechFeeAccrualAndSweep(vault)
Note over Keeper,VaultAccount: fee = TVV × APR × duration
Keeper->>VaultAccount: Increment TechFeeAccrued
Keeper->>BankModule: Sweep payable amount to recipient
alt sweep success
Keeper->>VaultAccount: Update accrued/swept state
else sweep failure
Keeper->>Keeper: Log error, retain accrued amount
end
Keeper->>Keeper: PerformVaultInterestTransfer(vault)
Keeper->>Keeper: PublishVaultNav(vault)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
keeper/reconcile.go (2)
35-54: Consider logging unknown chain IDs before defaulting to testnet.The default case silently falls back to testnet for unknown chain IDs. Adding a log statement would help diagnose unexpected chain ID values in non-standard environments.
default: + // Unknown chain - defaulting to testnet recipient addrStr = TechFeeRecipientTestnet }
103-127: Redundant duration checks.Lines 104-106 and 124-127 perform the same check (
currentBlockTime <= vault.PeriodStartimpliesperiodDuration <= 0). Consider removing the redundant check.Also, lines 119-122 checking for empty rate is dead code since
GetTechFeeRatealways returnsHardcodedTechFeeAPR.func (k *Keeper) PerformVaultTechFeeAccrualAndSweep(ctx sdk.Context, vault *types.VaultAccount) error { currentBlockTime := ctx.BlockTime().Unix() - if currentBlockTime <= vault.PeriodStart { - return nil - } - denom := vault.UnderlyingAsset vaultAddr := vault.GetAddress() principalTvv, err := k.GetTVVInUnderlyingAsset(ctx, *vault) if err != nil { return fmt.Errorf("failed to get TVV for tech fee: %w", err) } if !principalTvv.IsPositive() { return nil } rate := k.GetTechFeeRate(ctx) - if rate == "" { - return nil - } periodDuration := currentBlockTime - vault.PeriodStart if periodDuration <= 0 { return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
types/vault.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (3)
api/provlabs/vault/v1/vault.pulsar.go(16 hunks)keeper/reconcile.go(3 hunks)proto/provlabs/vault/v1/vault.proto(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-08T17:21:24.992Z
Learnt from: nullpointer0x00
Repo: ProvLabs/vault PR: 0
File: :0-0
Timestamp: 2025-10-08T17:21:24.992Z
Learning: Documentation generation for the vault module is deferred. The module will be imported into the provenance project, which has a third-party docs script that will handle generating documentation from the proto comments.
Applied to files:
proto/provlabs/vault/v1/vault.proto
📚 Learning: 2025-11-03T20:27:23.755Z
Learnt from: nullpointer0x00
Repo: ProvLabs/vault PR: 113
File: keeper/msg_server.go:0-0
Timestamp: 2025-11-03T20:27:23.755Z
Learning: In keeper/msg_server.go, WithdrawInterestFunds only requires the authority address as a transfer agent (markertypes.WithTransferAgents(ctx, authorityAddr)) because funds transfer directly from the vault account to the authority. In contrast, WithdrawPrincipalFunds requires both the authority and vault addresses as transfer agents (markertypes.WithTransferAgents(ctx, authorityAddress, vaultAddr)) because the principal account is a marker account, and the vault needs explicit authorization to withdraw from it.
Applied to files:
api/provlabs/vault/v1/vault.pulsar.go
🧬 Code graph analysis (2)
keeper/reconcile.go (4)
keeper/keeper.go (1)
Keeper(20-34)api/provlabs/vault/v1/vault.pulsar.go (3)
VaultAccount(2888-2962)VaultAccount(2977-2977)VaultAccount(2980-2982)types/vault.pb.go (3)
VaultAccount(33-103)VaultAccount(107-107)VaultAccount(108-110)interest/interest.go (1)
CalculateInterestEarned(36-66)
api/provlabs/vault/v1/vault.pulsar.go (1)
types/vault.pb.go (3)
VaultAccount(33-103)VaultAccount(107-107)VaultAccount(108-110)
🪛 GitHub Actions: Vault Tests
keeper/reconcile.go
[error] 50-50: GetTechFeeRecipient: panic due to invalid hardcoded tech fee recipient address; bech32 decoding failed (invalid checksum)
⏰ 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). (8)
- GitHub Check: sim-test (benchmark-invariants)
- GitHub Check: sim-test (after-import)
- GitHub Check: sim-test (benchmark)
- GitHub Check: sim-test (simple)
- GitHub Check: sim-test (import-export)
- GitHub Check: sim-test (nondeterminism)
- GitHub Check: sim-test (multi-seed-short)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
proto/provlabs/vault/v1/vault.proto (1)
82-92: LGTM!The
tech_fee_accruedfield is properly defined with appropriate type annotations (cosmossdk.io/math.Int,cosmos.Int), non-nullable constraint, and comprehensive documentation explaining its purpose and lifecycle.keeper/reconcile.go (3)
86-100: LGTM!Good extraction of settlement orchestration into a helper. The sequence (tech fee → interest → NAV) is logical.
163-177: Silent failure on SendCoins may warrant consideration.When
SendCoinsfails, the error is logged but the function returnsnil, allowing settlement to proceed. While this prevents tech fee failures from blocking vault operations, it means:
TechFeeAccruedretains the full amount (not reduced bypayNow)- Interest transfer proceeds even if the vault had issues sending funds
Verify this is the intended behavior. If failures should propagate, return the error instead.
66-84: LGTM!Clean refactoring to delegate period settlement to the new
settleVaultPeriodhelper.api/provlabs/vault/v1/vault.pulsar.go (1)
1-2: Generated code acknowledged.This file is auto-generated from proto definitions. The changes correctly integrate the new
tech_fee_accruedfield into the Pulsar reflection, marshaling, and accessor logic.
| HardcodedTechFeeAPR = "0.0015" // 15 bps annualized | ||
| TechFeeRecipientMainnet = "pb1xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" | ||
| TechFeeRecipientTestnet = "tp1xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" | ||
|
|
||
| MainnetChainID = "pio-mainnet-1" | ||
| TestnetChainID = "pio-testnet-1" |
There was a problem hiding this comment.
Critical: Placeholder addresses cause panic.
The pipeline failure confirms that GetTechFeeRecipient panics because these placeholder addresses (pb1xxx..., tp1xxx...) fail bech32 decoding. Replace with valid addresses before merging.
🤖 Prompt for AI Agents
In keeper/reconcile.go around lines 23-28, the TechFeeRecipientMainnet/Testnet
constants are placeholder bech32 addresses causing GetTechFeeRecipient to panic;
replace these placeholders with the actual valid bech32 addresses for mainnet
and testnet (or load them from configuration/environment) and add a simple
validation step (e.g., decode/check bech32) at initialization to fail fast with
a clear error instead of panicking at runtime.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
keeper/reconcile.go (1)
22-27: Placeholder addresses still need to be replaced with valid bech32 addresses.The pipeline failures confirm these placeholder addresses (
pb1xxx...,tp1xxx...) fail bech32 decoding and cause panics. This blocks all tests and must be fixed before merge.
🧹 Nitpick comments (3)
keeper/reconcile.go (3)
30-32: Consider making the tech fee rate configurable via module params.Hardcoding the APR works for now, but future rate adjustments will require code changes and redeployment. This is acceptable for initial implementation.
137-140: Redundant period duration check.Lines 117-119 already ensure
currentBlockTime > vault.PeriodStart, makingperiodDuration <= 0impossible here.periodDuration := currentBlockTime - vault.PeriodStart - if periodDuration <= 0 { - return nil - }
176-189: Consider emitting an event for tech fee payments.Successful fee sweeps are only observable via balance changes. Adding an event (similar to
EventVaultReconcile) would improve auditability and monitoring.// After successful SendCoins and before updating TechFeeAccrued: k.emitEvent(ctx, types.NewEventTechFeePaid( vaultAddr.String(), sdk.NewCoin(denom, payNow), recipient.String(), ))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
keeper/reconcile.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
keeper/reconcile.go (5)
keeper/keeper.go (1)
Keeper(20-34)types/vault.pb.go (3)
VaultAccount(33-103)VaultAccount(107-107)VaultAccount(108-110)api/provlabs/vault/v1/vault.pulsar.go (3)
VaultAccount(2888-2962)VaultAccount(2977-2977)VaultAccount(2980-2982)interest/interest.go (1)
CalculateInterestEarned(36-66)types/expected_keepers.go (1)
BankKeeper(40-48)
🪛 GitHub Actions: Vault Tests
keeper/reconcile.go
[error] 49-49: Test failure: panic in GetTechFeeRecipient due to invalid hardcoded tech fee recipient address; bech32 decoding failed: invalid checksum (expected xe9ysh got xxxxxx).
[error] 170-170: Test failure: panic in PerformVaultTechFeeAccrualAndSweep during reconciliation; bech32 decoding of tech fee recipient failed (invalid checksum).
[error] 86-86: Test failure: panic in settleVaultPeriod during vault interest reconciliation; invalid tech fee recipient address decoded (bech32 checksum error).
⏰ 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). (7)
- GitHub Check: sim-test (benchmark)
- GitHub Check: sim-test (multi-seed-short)
- GitHub Check: sim-test (nondeterminism)
- GitHub Check: sim-test (import-export)
- GitHub Check: sim-test (after-import)
- GitHub Check: sim-test (benchmark-invariants)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
keeper/reconcile.go (3)
77-79: LGTM! Clean extraction of settlement logic.Delegating to
settleVaultPeriodimproves modularity and makes the period settlement sequence explicit.
85-99: LGTM! Settlement sequence is well-structured.The ordering (tech-fee → interest → NAV) ensures fees are accounted for before interest distribution. The error propagation is appropriate—only fatal errors (TVV calculation failures) abort the settlement.
152-155: Good defensive handling for nilTechFeeAccrued.This correctly handles vaults created before this field was added.
| default: | ||
| addrStr = TechFeeRecipientTestnet | ||
| } |
There was a problem hiding this comment.
Default fallback to testnet for unknown chains may misdirect funds.
If a new chain ID is introduced (e.g., a fork or upgrade), this silently routes tech fees to the testnet address. Consider panicking for unrecognized production chains or adding explicit logging.
default:
- addrStr = TechFeeRecipientTestnet
+ // For local/dev chains, fall back to testnet recipient
+ ctx.Logger().Warn("unknown chain ID for tech fee recipient, defaulting to testnet", "chainID", chainID)
+ addrStr = TechFeeRecipientTestnet
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| default: | |
| addrStr = TechFeeRecipientTestnet | |
| } | |
| default: | |
| // For local/dev chains, fall back to testnet recipient | |
| ctx.Logger().Warn("unknown chain ID for tech fee recipient, defaulting to testnet", "chainID", chainID) | |
| addrStr = TechFeeRecipientTestnet | |
| } |
🤖 Prompt for AI Agents
In keeper/reconcile.go around lines 43-45 the switch default silently falls back
to TechFeeRecipientTestnet which can misdirect production funds for unknown/new
chain IDs; change this to fail fast for production builds by returning an error
or calling panic/log.Fatalf when running on mainnet-like environments, or at
minimum log a clear, high-severity error that includes the unrecognized chain ID
and aborts processing; implement the chosen behavior (panic/return
error/log.Fatalf) instead of assigning the testnet address, ensuring callers
handle the error path appropriately.
| recipient := k.GetTechFeeRecipient(ctx) | ||
| if recipient.Empty() { | ||
| ctx.Logger().Error("tech fee recipient not configured", "vault", vaultAddr.String()) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Unreachable code: recipient.Empty() check is never true.
GetTechFeeRecipient panics on invalid addresses (line 49), so it never returns an empty address. This check provides no protection.
Once valid addresses are configured, consider whether you want to:
- Keep the panic for invalid config (fail-fast), or
- Return an error from
GetTechFeeRecipientand handle it here gracefully
🧰 Tools
🪛 GitHub Actions: Vault Tests
[error] 170-170: Test failure: panic in PerformVaultTechFeeAccrualAndSweep during reconciliation; bech32 decoding of tech fee recipient failed (invalid checksum).
🤖 Prompt for AI Agents
In keeper/reconcile.go around lines 170–174, the recipient.Empty() check is
unreachable because GetTechFeeRecipient panics on invalid addresses (see line
49); either remove the Empty() branch here if you accept the fail-fast panic, or
change GetTechFeeRecipient to return (address, error) and update callers:
propagate/handle the error in reconcile.go by logging the error and returning it
instead of relying on Empty(). If you choose the error-return approach, update
function signature, replace the panic with returning an error on invalid config,
and adjust all call sites to handle the new error.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
keeper/reconcile.go (3)
22-28: Placeholder addresses still causing pipeline failures.The pipeline confirms these placeholder addresses (
pb1xxx...,tp1xxx...) fail bech32 decoding and cause panics. This was flagged in a previous review and must be addressed before merging.
43-45: Default fallback to testnet already flagged.This was noted in a previous review. Consider adding logging or failing explicitly for unrecognized production chain IDs to prevent silent misdirection of funds.
170-174: Unreachable code:recipient.Empty()check already flagged.This was noted in a previous review.
GetTechFeeRecipientpanics on invalid addresses (line 49), so it never returns an empty address.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
keeper/reconcile.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
keeper/reconcile.go (3)
keeper/keeper.go (1)
Keeper(20-34)types/vault.pb.go (3)
VaultAccount(33-103)VaultAccount(107-107)VaultAccount(108-110)interest/interest.go (1)
CalculateInterestEarned(36-66)
🪛 GitHub Actions: Vault Tests
keeper/reconcile.go
[error] 49-49: Panic: invalid hardcoded tech fee recipient address for chain : decoding bech32 failed: invalid checksum (expected xe9ysh got xxxxxx)
[error] 170-170: PerformVaultTechFeeAccrualAndSweep panic traced from reconcile.go:170 during vault reconciliation flow
[error] 94-94: reconcileVaultInterest panic path encountered during test execution
⏰ 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). (7)
- GitHub Check: sim-test (nondeterminism)
- GitHub Check: sim-test (benchmark-invariants)
- GitHub Check: sim-test (after-import)
- GitHub Check: sim-test (benchmark)
- GitHub Check: sim-test (multi-seed-short)
- GitHub Check: sim-test (import-export)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
keeper/reconcile.go (4)
30-32: LGTM!Clean accessor pattern that allows future migration to dynamic rate configuration without API changes.
77-82: Clean refactor delegating to settleVaultPeriod.The extraction of period settlement logic into
settleVaultPeriodimproves readability and maintainability.
117-119: Minor: Redundant time checks provide defensive protection.The checks at lines 117-119 and 137-140 are redundant with the caller's validation, but acceptable since
PerformVaultTechFeeAccrualAndSweepis a public method that could be called directly.Also applies to: 137-140
152-155: No persistence issue exists. Thevault.TechFeeAccruedmodifications are properly persisted through theSafeAddVerificationcall at the end ofreconcileVaultInterest(line 83 ofkeeper/reconcile.go), which explicitly callsk.SetVaultAccount(sdkCtx, vault)(keeper/queue.go:35). The control flow ensures modifications only occur whenSafeAddVerificationwill be called and persist the vault state.
| func (k *Keeper) settleVaultPeriod(ctx sdk.Context, vault *types.VaultAccount) error { | ||
| if err := k.PerformVaultInterestTransfer(ctx, vault); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := k.publishShareNav(ctx, vault); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := k.PerformVaultTechFeeAccrualAndSweep(ctx, vault); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Tech fee accrual is bypassed in timeout-based reconciliation path.
settleVaultPeriod correctly orchestrates tech fee accrual, but handleVaultInterestTimeouts (line 448) calls PerformVaultInterestTransfer directly without going through settleVaultPeriod. This means tech fees are only accrued when transactions trigger reconcileVaultInterest, not during the periodic timeout-based reconciliation.
Consider updating handleVaultInterestTimeouts to use settleVaultPeriod or explicitly call PerformVaultTechFeeAccrualAndSweep:
if err := k.PerformVaultInterestTransfer(sdkCtx, vault); err != nil {
sdkCtx.Logger().Error("failed to reconcile interest", "vault", addr.String(), "err", err)
continue
}
+
+ if err := k.PerformVaultTechFeeAccrualAndSweep(sdkCtx, vault); err != nil {
+ sdkCtx.Logger().Error("failed to accrue tech fee", "vault", addr.String(), "err", err)
+ }
reconciled = append(reconciled, vault)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Vault Tests
[error] 94-94: reconcileVaultInterest panic path encountered during test execution
|
This was an old POC |
nullpointer0x00
deleted the
nullpointer0x00/17-add-technology-fee-interest-paid-AUM
branch
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.