Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds AUM-fee support across the module: new protobuf event and generated bindings, genesis/state fields and validation for fee queue and AUM fee address, FeeTimeoutQueue type and tests, keeper state for AUM fee address and timeout queue, fee calculation/collection and timeout handling, valuation/interest helpers, and widespread test updates. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant BeginBlocker as "BeginBlocker"
participant FeeHandler as "handleVaultFeeTimeouts"
participant FeeQueue as "FeeTimeoutQueue"
participant Reconciler as "reconcileVault"
participant FeeCollector as "PerformVaultFeeTransfer"
participant Events as "Event system"
BeginBlocker->>FeeHandler: trigger processing
FeeHandler->>FeeQueue: WalkDue(now)
FeeQueue-->>FeeHandler: due vault entries
FeeHandler->>Reconciler: atomicallyReconcileFee(vault)
Reconciler->>FeeCollector: compute fee & transfer (uses TVV/NAV)
FeeCollector->>Events: emit EventVaultFeeCollected
FeeCollector-->>Reconciler: update vault state (OutstandingAumFee, FeePeriodStart)
Reconciler->>FeeQueue: Dequeue processed entry
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
keeper/payout_test.go (1)
150-175:⚠️ Potential issue | 🟡 MinorAvoid hardcoded epoch-derived fee expectations and ignored errors in this test path.
expectedAssets := 46and"4ylds"depend on current epoch assumptions, and the ignored errors (_) can mask breakage. Compute expected fee from the same formula and assert both error returns.As per coding guidelines: "Use descriptive variable names and clear logic flow in test code that reads like a description of the test case, avoiding internal comments".✅ Suggested robust expectation setup
import ( "fmt" "time" "cosmossdk.io/math" sdk "github.com/cosmos/cosmos-sdk/types" banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" markertypes "github.com/provenance-io/provenance/x/marker/types" + "github.com/provlabs/vault/interest" "github.com/provlabs/vault/keeper" "github.com/provlabs/vault/types" )- // Fee calculation: 50 * 0.0015 * testBlockTime.Unix() / 31,536,000 - // For current epoch (~1.7B), fee is 4. - expectedAssets := sdk.NewInt64Coin(underlyingDenom, 46) + feeAmount, err := interest.CalculateAUMFee(assets.Amount, testBlockTime.Unix()-1) + s.Require().NoError(err, "should calculate expected AUM fee") + expectedAssets := sdk.NewCoin(underlyingDenom, assets.Amount.Sub(feeAmount)) s.assertBalance(ownerAddr, underlyingDenom, expectedAssets.Amount) ... - provlabsAddr, _ := types.GetProvLabsFeeAddress(s.ctx.ChainID()) - expectedEvents = append(expectedEvents, createSendCoinEvents(principalAddress.String(), provlabsAddr.String(), "4ylds")...) - feeEvent, _ := sdk.TypedEventToEvent(&types.EventVaultFeeCollected{ + provlabsAddr, err := types.GetProvLabsFeeAddress(s.ctx.ChainID()) + s.Require().NoError(err, "should resolve ProvLabs fee address") + expectedFee := sdk.NewCoin(underlyingDenom, feeAmount) + expectedEvents = append(expectedEvents, createSendCoinEvents(principalAddress.String(), provlabsAddr.String(), expectedFee.String())...) + feeEvent, err := sdk.TypedEventToEvent(&types.EventVaultFeeCollected{ VaultAddress: vaultAddr.String(), - FeeAmount: "4ylds", - AumSnapshot: "50", + FeeAmount: expectedFee.String(), + AumSnapshot: assets.Amount.String(), DurationSeconds: testBlockTime.Unix() - 1, }) + s.Require().NoError(err, "should not error converting typed EventVaultFeeCollected")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/payout_test.go` around lines 150 - 175, The test currently uses hardcoded fee expectations (expectedAssets := 46 and "4ylds") and ignores error returns, which can break across epochs; compute the expected fee and resulting expectedAssets using the same formula used in production (e.g., fee = principal * feeRate * durationSeconds / secondsPerYear) and derive expectedAssets from that calculation, replace the hardcoded "4ylds" with the computed fee string, and assert equality; also stop ignoring errors from types.GetProvLabsFeeAddress and sdk.TypedEventToEvent (check and require.NoError on their returned errs) and use those returned values (provlabsAddr, feeEvent, etc.) when building expectedEvents so failures surface instead of being masked.keeper/reconcile.go (1)
98-116:⚠️ Potential issue | 🟠 MajorPublish fee-adjusted TVV here, not raw principal.
When
PerformVaultFeeTransferleaves a non-zeroOutstandingAumFee, Lines 106-114 still push gross marker balances into NAV. That keeps the published share NAV overstated until the liability is actually paid, which contradicts the fee-liability model in this PR. The same adjustment likely needs to be shared with any other pricing path that still reads gross TVV.💡 Suggested adjustment
tvv, err := k.GetTVVInUnderlyingAsset(ctx, *vault) if err != nil { return fmt.Errorf("failed to get TVV: %w", err) } + outstanding, err := k.CalculateOutstandingFeeUnderlying(ctx, *vault) + if err != nil { + return fmt.Errorf("failed to convert outstanding AUM fee: %w", err) + } + tvv = tvv.Sub(outstanding) + if tvv.IsNegative() { + tvv = sdkmath.ZeroInt() + } if !tvv.IsPositive() { return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/reconcile.go` around lines 98 - 116, publishShareNav currently uses the raw TVV from GetTVVInUnderlyingAsset when calling setShareDenomNAV, which will overstate NAV if there is an OutstandingAumFee left after PerformVaultFeeTransfer; adjust the TVV by subtracting the vault's OutstandingAumFee (e.g. tvv = tvv.Sub(vault.OutstandingAumFee) or equivalent) and clamp to zero before calling setShareDenomNAV so the published NAV reflects fee liabilities, and apply the same fee-adjustment logic in any other pricing paths that read gross TVV.
🧹 Nitpick comments (8)
keeper/query_server_test.go (2)
472-476: Consider validatingHeightandTimefields for complete test coverage.The
ManualEqualityhook only assertsAssets, butQueryEstimateSwapInResponsealso containsHeightandTimefields (pertypes/query.pb.go:592-599). SinceManualEqualityfully replaces the default deep-equality check, these fields are never validated.Other tests in this file (e.g.,
VaultPendingSwapOutsat line 787) useWithinDurationfor time fields. Consider applying a similar pattern here to improve coverage without introducing flakiness.♻️ Suggested improvement
ManualEquality: func(s querytest.TestSuiter, expected, actual *types.QueryEstimateSwapInResponse) { s.Require().NotNil(actual, "actual response should not be nil") s.Require().NotNil(expected, "expected response should not be nil") s.Assert().Equal(expected.Assets, actual.Assets, "assets mismatch") + s.Assert().Equal(expected.Height, actual.Height, "height mismatch") + s.Assert().WithinDuration(expected.Time, actual.Time, time.Second, "time mismatch") },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/query_server_test.go` around lines 472 - 476, ManualEquality currently only compares Assets and omits Height and Time on QueryEstimateSwapInResponse; update the ManualEquality hook (used with TestSuiter) to also assert expected.Height == actual.Height and to compare timestamps using a tolerance (e.g., s.Assert().WithinDuration(expected.Time, actual.Time, acceptableDelta)) similar to VaultPendingSwapOuts so tests remain stable while fully validating Height and Time fields.
608-612: Same coverage gap:HeightandTimefields are not validated.Similar to
EstimateSwapInabove,QueryEstimateSwapOutResponsecontainsHeightandTimefields (pertypes/query.pb.go:720-727) that are not validated when using thisManualEqualityoverride. Consider applying the same fix for consistency.♻️ Suggested improvement
ManualEquality: func(s querytest.TestSuiter, expected, actual *types.QueryEstimateSwapOutResponse) { s.Require().NotNil(actual, "actual response should not be nil") s.Require().NotNil(expected, "expected response should not be nil") s.Assert().Equal(expected.Assets, actual.Assets, "assets mismatch") + s.Assert().Equal(expected.Height, actual.Height, "height mismatch") + s.Assert().WithinDuration(expected.Time, actual.Time, time.Second, "time mismatch") },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/query_server_test.go` around lines 608 - 612, The ManualEquality override for QueryEstimateSwapOutResponse currently only checks Assets and misses Height and Time; update the ManualEquality function (the lambda taking s querytest.TestSuiter, expected, actual *types.QueryEstimateSwapOutResponse) to also assert that expected.Height == actual.Height and that expected.Time equals actual.Time (use the same equality approach you used for EstimateSwapIn—e.g., s.Assert().Equal for scalar Height and an appropriate equality/comparison for the Time/timestamp field) so Height and Time are validated alongside Assets.simulation/operations_test.go (1)
39-39: Prefer deterministic block time inSetupTestto avoid time-coupled simulation behavior.Using
time.Now()in suite setup makes this test context non-reproducible across runs; a fixed timestamp keeps failures easier to debug.♻️ Suggested adjustment
- s.ctx = s.app.BaseApp.NewContext(false).WithBlockTime(time.Now()) + s.ctx = s.app.BaseApp.NewContext(false).WithBlockTime(time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulation/operations_test.go` at line 39, The test setup uses a non-deterministic timestamp (time.Now()) when creating the context (s.ctx = s.app.BaseApp.NewContext(false).WithBlockTime(...)); change SetupTest to use a fixed deterministic time constant (e.g., a time.Date or time.Unix value) and pass that into WithBlockTime so s.ctx is reproducible across runs; update the assignment in the SetupTest function where s.ctx is set (reference: s.ctx, s.app.BaseApp.NewContext, WithBlockTime) to use the fixed timestamp.types/vault_test.go (1)
33-57: Extract a baseVaultAccountfixture for this table.This pattern repeats through most of the table: start from the same valid vault, then flip one field for the case under test. Pulling that into a helper would make the new
OutstandingAumFeecoverage much easier to extend and read. Based on learnings, "Applies to **/*_test.go : Abstract repeated test operations (e.g., account setup, vault creation, state assertions) into reusable test helper functions".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@types/vault_test.go` around lines 33 - 57, Tests duplicate the same valid VaultAccount setup across cases; create a helper (e.g., newVaultFixture or baseVaultAccountFixture) that returns a types.VaultAccount pre-populated with BaseAccount: baseAcc, Admin: validAdmin, TotalShares: sdk.NewInt64Coin(validDenom,0), UnderlyingAsset: "uusd", PaymentDenom: "uusd", CurrentInterestRate: validInterest, DesiredInterestRate: validInterest, OutstandingAumFee: sdk.NewInt64Coin("uusd",0), then update the table entries to call this helper and override only the field under test (e.g., set BaseAccount = nil or change OutstandingAumFee) so cases reference the fixture and mutate the specific field to cover each scenario.keeper/valuation_engine_test.go (1)
182-212: Consider converting this into a table-driven test with shared setup helpers.The assertions are good, but this block will be easier to extend/maintain as a case table (success, identity, missing NAV, zero-price).
As per coding guidelines: "Use table-driven test patterns for unit and integration tests to ensure exhaustive coverage of edge cases" and "Abstract repeated test operations (e.g., account setup, vault creation, state assertions) into reusable test helper functions".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/valuation_engine_test.go` around lines 182 - 212, Refactor TestFromUnderlyingAssetAmount into a table-driven test: create a slice of test cases (fields: name, inputAmt, targetDenom, wantVal *math.Int or nil, wantErr string or empty, preTest func()) and iterate over them with t.Run or s.Run, calling setupSinglePaymentDenomVault once (or per-case if isolation needed) and invoking keeper.Keeper.FromUnderlyingAssetAmount in the loop; move case-specific setup like s.bumpHeight() and s.setReverseNAV(underlyingDenom,"zeroprice",0,1) into the test case's preTest so the "zero price" case triggers the zero-price precondition, and replace the repeated s.Require() assertions with table-driven checks that assert either expected value equality or that the returned error contains wantErr. Ensure you reference TestFromUnderlyingAssetAmount, setupSinglePaymentDenomVault, FromUnderlyingAssetAmount, bumpHeight, and setReverseNAV when locating code to change.keeper/reconcile_test.go (1)
1369-1377: Remove the temporary event/balance logging before merge.Lines 1369-1377 are debug artifacts now; they'll add a lot of noise to normal test runs without strengthening the assertions.
🧹 Suggested cleanup
endVault := s.simApp.BankKeeper.GetBalance(s.ctx, vaultAddr, underlying.Denom) - s.T().Logf("End vault balance: %s", endVault.String()) events := normalizeEvents(s.ctx.EventManager().Events()) - for _, ev := range events { - s.T().Logf("Event: %s", ev.Type) - for _, attr := range ev.Attributes { - s.T().Logf(" %s: %s", attr.Key, attr.Value) - } - }As per coding guidelines, "Proactively eliminate unused code and obsolete comments to maintain code cleanliness".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/reconcile_test.go` around lines 1369 - 1377, The test contains leftover debug logging (s.T().Logf calls printing endVault.String() and iterating events from s.ctx.EventManager().Events() after calling normalizeEvents) that should be removed; delete the temporary Logf blocks and the for-loop that logs each event/attribute so the test relies on assertions only (remove references to endVault.String(), normalizeEvents logging loop, and the event iteration) while keeping any real assertions intact.proto/provlabs/vault/v1/vault.proto (1)
99-107: Document the denom and queue semantics of the new fee fields.The current comments still leave two important API details implicit:
fee_period_timeoutis an absolute queue-managed timestamp, andoutstanding_aum_feeis stored inpayment_denom, not the underlying asset. Those details matter for migrations and client implementations.📝 Suggested doc wording
- // fee_period_timeout is the end time (in Unix seconds) of the current AUM fee collection period. + // fee_period_timeout is the absolute Unix timestamp at which the current AUM + // fee collection period becomes due. This value is managed together with the + // fee timeout queue. int64 fee_period_timeout = 22; - // outstanding_aum_fee is the amount of AUM fee that has been calculated but not yet collected - // due to insufficient liquidity in the principal marker. + // outstanding_aum_fee is the unpaid AUM fee balance stored in payment_denom. + // It accumulates when the principal marker does not have enough payment_denom + // liquidity to satisfy the full collection amount and remains a liability + // until collected. cosmos.base.v1beta1.Coin outstanding_aum_fee = 23 [(gogoproto.nullable) = false];As per coding guidelines, "Ensure all fields and messages in .proto files are thoroughly documented, as these propagate to generated API specs".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proto/provlabs/vault/v1/vault.proto` around lines 99 - 107, The comments for the new fields are ambiguous: clarify that fee_period_timeout is an absolute timestamp managed as a queue key (i.e., a Unix-second deadline used by the queue scheduler) and that outstanding_aum_fee is recorded using the vault's payment_denom (not the underlying asset denom) so migrations/clients know which denom to read/write; update the comment for fee_period_timeout to state it is an absolute queue-managed Unix timestamp and update the comment for outstanding_aum_fee to state it is stored in payment_denom (reference the payment_denom field) and represents AUM fees calculated but not yet collected due to insufficient liquidity.keeper/reconcile.go (1)
285-333: The Godoc still describes fee liquidity as a hard requirement.The implementation below only checks the interest leg, and the new deferred-fee behavior intentionally allows zero
PaymentDenomliquidity by carrying the shortfall inOutstandingAumFee. Please update the comment so it matches the actual contract.📝 Suggested doc update
-// CanPayoutDuration determines whether the vault can fulfill both the projected -// interest payment/refund AND the AUM fee collection over the given duration -// based on current reserves and principal TVV. +// CanPayoutDuration determines whether the vault can fulfill the projected +// interest payment/refund over the given duration based on current reserves +// and principal TVV. // // Interest is checked against vault reserves (positive interest) or principal // marker underlying balance (negative interest). -// AUM fee is checked against the principal marker's PaymentDenom balance. +// Unpaid AUM fees are intentionally not treated as blocking here; they are +// deferred into OutstandingAumFee during collection. // -// It returns true only if all checks pass. +// It returns true when the interest leg is payable.As per coding guidelines, "Every exported function, interface, and type must have clear Godoc comments describing the why and architectural context".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/reconcile.go` around lines 285 - 333, Update the Godoc for CanPayoutDuration to reflect the actual contract: state that this function only verifies the vault can cover projected interest (positive interest from vault reserves, negative interest from principal marker underlying balance) over the given duration and does NOT require immediate PaymentDenom liquidity for AUM fees because deferred-fee behavior allows shortfalls to be carried in OutstandingAumFee; remove or reword any language claiming fee liquidity is a hard requirement and add a brief note about why fees are deferred and where shortfalls are tracked (OutstandingAumFee) so callers understand the architectural context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/provlabs/vault/v1/events.pulsar.go`:
- Around line 19706-19713: The event fields FeeAmount and AumSnapshot are
ambiguous about units and partial collections; update the proto message that
defines these fields to include explicit denom and requested-vs-collected
information (either replace string fields with a structured coin type or add
fields like PaymentDenom, UnderlyingAssetDenom, OutstandingAumFee (string/coin)
and FeeRequested vs FeeCollected) while keeping VaultAddress and DurationSeconds
intact, then regenerate the Go bindings (which will update events.pulsar.go) so
consumers can tell which denom each amount uses and whether fee_amount is the
full requested fee or the transferred amount.
In `@interest/interest.go`:
- Around line 76-95: CalculateAUMFee currently computes a negative fee when the
input aum is negative; add a guard at the start of CalculateAUMFee to reject
negative AUMs (e.g. if aum.IsNegative() return an error like "aum cannot be
negative") before any fee math runs so the function never returns a negative
fee. Reference the CalculateAUMFee function and the aum parameter when adding
this check.
In `@keeper/abci.go`:
- Around line 18-21: BeginBlocker now calls handleVaultFeeTimeouts every block
but handleVaultFeeTimeouts dequeues vaults and only re-enqueues them on
successful fee transfer, allowing failed vaults to be permanently dropped;
modify handleVaultFeeTimeouts so it does not permanently remove vaults on
transient failures—either only pop a vault from the queue after a successful
transfer or ensure any vault that fails a transfer is pushed back onto the fee
queue (or scheduled for retry) before returning; update the logic in
handleVaultFeeTimeouts (and any helper like the dequeue/requeue functions it
uses) to re-enqueue failed vault IDs and preserve retry metadata so vaults
continue to be attempted in future BeginBlocker calls.
In `@keeper/genesis.go`:
- Around line 68-75: The code casts entry.Time (uint64) to int64 when calling
k.FeeTimeoutQueue.Enqueue without checking bounds; add a guard that verifies
entry.Time <= math.MaxInt64 before casting and calling k.FeeTimeoutQueue.Enqueue
(and similarly for payout timeout imports). If the check fails, return or panic
with a clear error indicating the invalid timeout value and the offending
entry.Addr; update GenesisState.Validate to enforce the same bound for future
genesis validation so invalid genesis cannot be created.
In `@keeper/queue.go`:
- Around line 66-79: The update should be made atomic: create a cache context
via ctx.CacheContext() (e.g., cacheCtx, write := ctx.CacheContext()), perform
Dequeue, update vault.FeePeriodStart and FeePeriodTimeout, call
k.SetVaultAccount using cacheCtx, and Enqueue on k.FeeTimeoutQueue using
cacheCtx; only if Enqueue succeeds call write() to commit; on any error return a
wrapped error using fmt.Errorf("%w", err) and log via k.getLogger(ctx) with
structured fields (e.g., "vault", vault.GetAddress()) instead of ctx.Logger();
keep references to SafeEnqueueFeeTimeout, FeeTimeoutQueue, SetVaultAccount, and
getLogger to locate the changes.
In `@keeper/reconcile.go`:
- Around line 488-491: The code currently continues on a fee-collection error
inside the timeout handling, which dequeues the item and skips calling
resetVaultInterestPeriods/resetVaultFeePeriods and re-queuing the timeout;
update the error path in the PerformVaultFeeTransfer and analogous block (also
lines ~642-645) so that on error you still call resetVaultInterestPeriods and
resetVaultFeePeriods for the vault and re-schedule the timeout (or requeue the
item) instead of using continue; locate the failing block by the
PerformVaultFeeTransfer call and ensure both
resetVaultInterestPeriods/resetVaultFeePeriods are invoked and the timeout item
is re-added or retried when an error occurs.
- Around line 404-423: The CalculateVaultTotalAssets function can return a
negative sdkmath.Int when accrued fees exceed remaining assets; after applying
interest (EstimateAccruedInterest), AUM fee (EstimateAccruedAUMFee) and
outstanding fee conversion (CalculateOutstandingFeeUnderlying) clamp the
resulting estimated value to a non-negative value by flooring it at zero before
returning (i.e., if estimated.IsNegative() then set estimated =
sdkmath.ZeroInt()); ensure you still return the clamped sdkmath.Int and nil
error from CalculateVaultTotalAssets.
In `@keeper/vault.go`:
- Around line 106-108: CreateVault currently persists intermediate writes (those
at the earlier write and the write before calling SafeEnqueueFeeTimeout),
leaving a partially-created vault if SafeEnqueueFeeTimeout fails; to make
creation atomic, perform all state mutations and the SafeEnqueueFeeTimeout call
using a cached context obtained via ctx.CacheContext() (use the cached context
when calling the same keeper methods that perform the writes and pass it into
SafeEnqueueFeeTimeout), then only commit the cache to the real context when
SafeEnqueueFeeTimeout returns nil; if it errors, discard the cache so no partial
state (orphan vault) is persisted.
In `@queue/fee_timeout_test.go`:
- Around line 75-77: Test variables a1 and a2 are both created from the same
bech32 address so the test does not exercise cross-vault behavior; update the
test to use distinct addresses (e.g., derive a2 from a different
TestProvlabsAddress call or a second helper) so that the assertions around
WalkDue and RemoveAllForVault actually validate per-vault isolation; locate the
allocations of a1 and a2 in fee_timeout_test.go and replace the second identical
address creation with a distinct address creation used by the subsequent test
blocks (also fix the similar occurrences around lines 93-95).
In `@queue/fee_timeout.go`:
- Around line 13-24: Add Godoc comments for the exported type FeeTimeoutQueue
and its constructor NewFeeTimeoutQueue: document the purpose of the queue in the
fee-reconciliation flow (what entries are stored, why entries are keyed by
(uint64, sdk.AccAddress)), explain callers’ responsibilities when using the
queue (e.g., how items are enqueued/consumed and when timeouts are processed),
and provide architectural context tying it to Vault fee timeout handling. Place
the comment immediately above the type FeeTimeoutQueue and the constructor
NewFeeTimeoutQueue, using concise sentences that describe the “why” and how this
queue fits into the larger fee reconciliation workflow.
- Around line 48-54: WalkDue currently casts nowSec to uint64 without checking
for negative values, which turns negative timestamps into large unsigned numbers
and breaks the time comparison; add input validation at the start of
FeeTimeoutQueue.WalkDue to return an error if nowSec is negative (e.g., nowSec <
0) to avoid the unsafe uint64 cast, mirroring the validation behavior used in
Enqueue and Dequeue, then proceed to call p.keyset.Walk with the safe
uint64(nowSec) once validated.
In `@types/keys.go`:
- Around line 78-93: GetProvLabsFeeAddress currently calls
sdk.AccAddressFromBech32 which enforces the SDK global Bech32 HRP; instead
decode the hardcoded addresses using an explicit Bech32 decode or raw bytes so
the global prefix won’t cause "invalid Bech32 prefix" errors. For the cases
using ProvLabsMainnetFeeAddress and ProvLabsTestnetFeeAddress, call a Bech32
decoder (e.g., bech32.Decode) or use sdk.GetFromBech32 with the expected HRP
inferred from chainID (or replace the constants with raw address bytes) and then
construct the sdk.AccAddress from the decoded data, returning a wrapped error on
decode/HRP mismatch; leave the default branch creating
sdk.AccAddress(crypto.AddressHash(...)) unchanged. Ensure you update only
GetProvLabsFeeAddress and reference the constants ProvLabsMainnetFeeAddress and
ProvLabsTestnetFeeAddress when locating the code.
In `@types/vault_test.go`:
- Around line 553-560: The table-driven test calls tc.vaultAccount.Validate()
and immediately uses err.Error() when expecting an error, which can panic if err
is nil; change the assertions inside the t.Run to use fatal assertions so the
subtest stops on failure: use require.Errorf (or require.Errorf) when
tc.expectedErr != "" to assert err is non-nil before inspecting err.Error(), and
use require.NoErrorf when tc.expectedErr == "" to fail fast on unexpected
errors; update the block that references tc.vaultAccount.Validate(), err, and
tc.expectedErr to use require.* functions (from the same testify package) and
only call err.Error() after require.Errorf has confirmed err is non-nil.
In `@types/vault.go`:
- Around line 195-200: When OutstandingAumFee has a non-zero amount (i.e.,
Amount is not nil and not zero), enforce that its Denom is non-empty and equals
v.PaymentDenom; return a clear error if not. Update the existing validation
around v.OutstandingAumFee.Amount/Denom (the checks using
v.OutstandingAumFee.Denom, v.PaymentDenom, v.OutstandingAumFee.Amount.IsNil, and
v.OutstandingAumFee.IsNegative) so that a non-zero amount cannot have an empty
or mismatched denom, and keep the negative-amount check intact.
---
Outside diff comments:
In `@keeper/payout_test.go`:
- Around line 150-175: The test currently uses hardcoded fee expectations
(expectedAssets := 46 and "4ylds") and ignores error returns, which can break
across epochs; compute the expected fee and resulting expectedAssets using the
same formula used in production (e.g., fee = principal * feeRate *
durationSeconds / secondsPerYear) and derive expectedAssets from that
calculation, replace the hardcoded "4ylds" with the computed fee string, and
assert equality; also stop ignoring errors from types.GetProvLabsFeeAddress and
sdk.TypedEventToEvent (check and require.NoError on their returned errs) and use
those returned values (provlabsAddr, feeEvent, etc.) when building
expectedEvents so failures surface instead of being masked.
In `@keeper/reconcile.go`:
- Around line 98-116: publishShareNav currently uses the raw TVV from
GetTVVInUnderlyingAsset when calling setShareDenomNAV, which will overstate NAV
if there is an OutstandingAumFee left after PerformVaultFeeTransfer; adjust the
TVV by subtracting the vault's OutstandingAumFee (e.g. tvv =
tvv.Sub(vault.OutstandingAumFee) or equivalent) and clamp to zero before calling
setShareDenomNAV so the published NAV reflects fee liabilities, and apply the
same fee-adjustment logic in any other pricing paths that read gross TVV.
---
Nitpick comments:
In `@keeper/query_server_test.go`:
- Around line 472-476: ManualEquality currently only compares Assets and omits
Height and Time on QueryEstimateSwapInResponse; update the ManualEquality hook
(used with TestSuiter) to also assert expected.Height == actual.Height and to
compare timestamps using a tolerance (e.g.,
s.Assert().WithinDuration(expected.Time, actual.Time, acceptableDelta)) similar
to VaultPendingSwapOuts so tests remain stable while fully validating Height and
Time fields.
- Around line 608-612: The ManualEquality override for
QueryEstimateSwapOutResponse currently only checks Assets and misses Height and
Time; update the ManualEquality function (the lambda taking s
querytest.TestSuiter, expected, actual *types.QueryEstimateSwapOutResponse) to
also assert that expected.Height == actual.Height and that expected.Time equals
actual.Time (use the same equality approach you used for EstimateSwapIn—e.g.,
s.Assert().Equal for scalar Height and an appropriate equality/comparison for
the Time/timestamp field) so Height and Time are validated alongside Assets.
In `@keeper/reconcile_test.go`:
- Around line 1369-1377: The test contains leftover debug logging (s.T().Logf
calls printing endVault.String() and iterating events from
s.ctx.EventManager().Events() after calling normalizeEvents) that should be
removed; delete the temporary Logf blocks and the for-loop that logs each
event/attribute so the test relies on assertions only (remove references to
endVault.String(), normalizeEvents logging loop, and the event iteration) while
keeping any real assertions intact.
In `@keeper/reconcile.go`:
- Around line 285-333: Update the Godoc for CanPayoutDuration to reflect the
actual contract: state that this function only verifies the vault can cover
projected interest (positive interest from vault reserves, negative interest
from principal marker underlying balance) over the given duration and does NOT
require immediate PaymentDenom liquidity for AUM fees because deferred-fee
behavior allows shortfalls to be carried in OutstandingAumFee; remove or reword
any language claiming fee liquidity is a hard requirement and add a brief note
about why fees are deferred and where shortfalls are tracked (OutstandingAumFee)
so callers understand the architectural context.
In `@keeper/valuation_engine_test.go`:
- Around line 182-212: Refactor TestFromUnderlyingAssetAmount into a
table-driven test: create a slice of test cases (fields: name, inputAmt,
targetDenom, wantVal *math.Int or nil, wantErr string or empty, preTest func())
and iterate over them with t.Run or s.Run, calling setupSinglePaymentDenomVault
once (or per-case if isolation needed) and invoking
keeper.Keeper.FromUnderlyingAssetAmount in the loop; move case-specific setup
like s.bumpHeight() and s.setReverseNAV(underlyingDenom,"zeroprice",0,1) into
the test case's preTest so the "zero price" case triggers the zero-price
precondition, and replace the repeated s.Require() assertions with table-driven
checks that assert either expected value equality or that the returned error
contains wantErr. Ensure you reference TestFromUnderlyingAssetAmount,
setupSinglePaymentDenomVault, FromUnderlyingAssetAmount, bumpHeight, and
setReverseNAV when locating code to change.
In `@proto/provlabs/vault/v1/vault.proto`:
- Around line 99-107: The comments for the new fields are ambiguous: clarify
that fee_period_timeout is an absolute timestamp managed as a queue key (i.e., a
Unix-second deadline used by the queue scheduler) and that outstanding_aum_fee
is recorded using the vault's payment_denom (not the underlying asset denom) so
migrations/clients know which denom to read/write; update the comment for
fee_period_timeout to state it is an absolute queue-managed Unix timestamp and
update the comment for outstanding_aum_fee to state it is stored in
payment_denom (reference the payment_denom field) and represents AUM fees
calculated but not yet collected due to insufficient liquidity.
In `@simulation/operations_test.go`:
- Line 39: The test setup uses a non-deterministic timestamp (time.Now()) when
creating the context (s.ctx =
s.app.BaseApp.NewContext(false).WithBlockTime(...)); change SetupTest to use a
fixed deterministic time constant (e.g., a time.Date or time.Unix value) and
pass that into WithBlockTime so s.ctx is reproducible across runs; update the
assignment in the SetupTest function where s.ctx is set (reference: s.ctx,
s.app.BaseApp.NewContext, WithBlockTime) to use the fixed timestamp.
In `@types/vault_test.go`:
- Around line 33-57: Tests duplicate the same valid VaultAccount setup across
cases; create a helper (e.g., newVaultFixture or baseVaultAccountFixture) that
returns a types.VaultAccount pre-populated with BaseAccount: baseAcc, Admin:
validAdmin, TotalShares: sdk.NewInt64Coin(validDenom,0), UnderlyingAsset:
"uusd", PaymentDenom: "uusd", CurrentInterestRate: validInterest,
DesiredInterestRate: validInterest, OutstandingAumFee:
sdk.NewInt64Coin("uusd",0), then update the table entries to call this helper
and override only the field under test (e.g., set BaseAccount = nil or change
OutstandingAumFee) so cases reference the fixture and mutate the specific field
to cover each scenario.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f8711168-7338-4d43-bb45-2dcf474f50e8
⛔ Files ignored due to path filters (3)
types/events.pb.gois excluded by!**/*.pb.gotypes/genesis.pb.gois excluded by!**/*.pb.gotypes/vault.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (28)
api/provlabs/vault/v1/events.pulsar.goapi/provlabs/vault/v1/genesis.pulsar.goapi/provlabs/vault/v1/vault.pulsar.gointerest/interest.gointerest/interest_test.gokeeper/abci.gokeeper/export_test.gokeeper/genesis.gokeeper/keeper.gokeeper/payout_test.gokeeper/query_server_test.gokeeper/queue.gokeeper/queue_test.gokeeper/reconcile.gokeeper/reconcile_test.gokeeper/suite_test.gokeeper/valuation_engine.gokeeper/valuation_engine_test.gokeeper/vault.goproto/provlabs/vault/v1/events.protoproto/provlabs/vault/v1/genesis.protoproto/provlabs/vault/v1/vault.protoqueue/fee_timeout.goqueue/fee_timeout_test.gosimulation/operations_test.gotypes/keys.gotypes/vault.gotypes/vault_test.go
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
keeper/reconcile.go (2)
407-425:⚠️ Potential issue | 🟠 MajorClamp net total assets at zero before returning.
When accrued + outstanding fees exceed assets, this function can return a negative value; downstream valuation paths should not consume negative TVV.
Proposed fix
estimated = estimated.Sub(outstandingUnderlying) + if estimated.IsNegative() { + return sdkmath.ZeroInt(), nil + } return estimated, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/reconcile.go` around lines 407 - 425, The computed "estimated" net assets can go negative; clamp it to zero before returning by checking if estimated.IsNegative() and, if so, set estimated to the zero sdkmath.Int value. Update the function that calls EstimateAccruedInterest, EstimateAccruedAUMFee and CalculateOutstandingFeeUnderlying (the block using variables interestEarned, feeAccrued, outstandingUnderlying and returning estimated) to perform: if estimated.IsNegative() { estimated = sdkmath.Int{} } right before the final return so downstream code never receives a negative value.
490-493:⚠️ Potential issue | 🔴 CriticalFee-transfer error path drops dequeued work and can break reconciliation continuity.
After dequeue,
continueonPerformVaultFeeTransfererror loses automatic retry scheduling. In the interest-timeout flow, interest may already be applied before this branch, so period reset is skipped and the vault can be left in an inconsistent retry state.Suggested correction direction
- if err := k.PerformVaultFeeTransfer(ctx, vault); err != nil { - ctx.Logger().Error("failed to collect AUM fee", "vault", addr.String(), "err", err) - continue - } + if err := k.PerformVaultFeeTransfer(ctx, vault); err != nil { + ctx.Logger().Error("failed to collect AUM fee", "vault", addr.String(), "err", err) + // ensure already-applied interest is not replayed and fee work is retried + if requeueErr := k.SafeEnqueueTimeout(ctx, vault); requeueErr != nil { + ctx.Logger().Error("failed to requeue interest timeout after fee failure", "vault", addr.String(), "err", requeueErr) + } + if requeueErr := k.FeeTimeoutQueue.Enqueue(ctx, now, addr); requeueErr != nil { + ctx.Logger().Error("failed to requeue fee timeout after fee failure", "vault", addr.String(), "err", requeueErr) + } + continue + }- if err := k.PerformVaultFeeTransfer(ctx, vault); err != nil { - ctx.Logger().Error("failed to collect AUM fee", "vault", addr.String(), "err", err) - continue - } + if err := k.PerformVaultFeeTransfer(ctx, vault); err != nil { + ctx.Logger().Error("failed to collect AUM fee", "vault", addr.String(), "err", err) + if requeueErr := k.FeeTimeoutQueue.Enqueue(ctx, int64(timeout), addr); requeueErr != nil { + ctx.Logger().Error("failed to requeue fee timeout", "vault", addr.String(), "err", requeueErr) + } + continue + }Also applies to: 644-647
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/reconcile.go` around lines 490 - 493, The error path that logs a failed PerformVaultFeeTransfer and then uses continue drops the dequeued vault and prevents automatic retry/period-reset, so replace the bare continue with logic that re-schedules the vault for retry (e.g., call the keeper's enqueue/requeue method or otherwise push the vault back onto the work queue) and/or invoke the existing retry scheduling helper so this vault will be retried later; also ensure the fee-period reset/interest-timeout bookkeeping runs even on transfer errors (either move the period-reset before the error branch or call the reset helper before requeueing). Apply the same change at the other occurrence referenced (lines ~644-647) so both error paths requeue the vault instead of dropping it.
🧹 Nitpick comments (3)
keeper/payout_test.go (1)
150-153: Replace epoch-coupled comments/magic amount with named expected values.At Line 150-153, use named fixtures (e.g.,
expectedCollectedFee) instead of inline epoch math comments plus hardcoded46; this keeps the test narrative clearer and easier to maintain.As per coding guidelines, "Use descriptive variable names and clear logic flow in test code that reads like a description of the test case, avoiding internal comments".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/payout_test.go` around lines 150 - 153, Replace the hardcoded magic amount and epoch-math comment by introducing a descriptive test fixture (e.g., expectedCollectedFee or expectedAssetsAmount) and use it when constructing expectedAssets and asserting balances; locate the assertion using symbols expectedAssets, sdk.NewInt64Coin, s.assertBalance, ownerAddr and underlyingDenom, create a named variable (expectedCollectedFee) holding the expected int64 value (previously 46) with a short clarifying name, and use that variable instead of the literal 46 so the test reads clearly without epoch-dependent inline math.types/events.go (1)
288-289: Expand the Godoc to include architectural intent.Line 288’s comment currently restates the function name. Please add why this constructor exists (canonical fee-event serialization for downstream consumers).
♻️ Suggested update
-// NewEventVaultFeeCollected creates a new EventVaultFeeCollected event. +// NewEventVaultFeeCollected builds the typed fee-collection event emitted by vault reconciliation. +// It normalizes sdk.Coin fields to canonical string amounts for consistent event consumption.As per coding guidelines, "Every exported function, interface, and type must have clear Godoc comments describing the why and architectural context".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@types/events.go` around lines 288 - 289, Update the Godoc for the exported constructor NewEventVaultFeeCollected to explain its architectural intent: state that it is the canonical constructor used to serialize vault fee events for downstream consumers and event routing, describe why callers should use this constructor (ensures consistent field ordering/format and metadata like duration/aumSnapshot), and briefly document the key parameters (vaultAddress, collected, requested, aumSnapshot, outstanding, duration) to clarify their roles in the canonical fee-event payload.keeper/reconcile_test.go (1)
1375-1383: Remove debug logging artifacts from the test body.These
s.T().Logf(...)loops make test output noisy and should not ship in committed test logic.As per coding guidelines
**/*.go: “Proactively eliminate unused code and obsolete comments to maintain code cleanliness”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/reconcile_test.go` around lines 1375 - 1383, Remove the test debug logging: delete the s.T().Logf("End vault balance: %s", endVault.String()) call and the subsequent loop that logs events and attributes (the for _, ev := range events { ... } block that iterates over normalizeEvents(s.ctx.EventManager().Events())). Leave the events := normalizeEvents(...) assignment if the test uses it for assertions, but eliminate any printing/logging of events or attributes so the test output is not noisy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@keeper/payout_test.go`:
- Around line 168-177: The test currently ignores errors from
types.GetProvLabsFeeAddress and sdk.TypedEventToEvent; update the code to
capture both return errors and assert they are nil using the test assertion
helpers (e.g., require.NoError or require.Nil with a descriptive message) when
calling types.GetProvLabsFeeAddress(...) and
sdk.TypedEventToEvent(&types.EventVaultFeeCollected{...}); ensure
createSendCoinEvents still receives the resolved provlabs address variable
(provLabsAddr) after a successful resolution and fail the test immediately if
either call returns an error, referencing the functions/types
GetProvLabsFeeAddress, TypedEventToEvent, createSendCoinEvents, and
EventVaultFeeCollected in the assertions for clarity.
In `@keeper/reconcile_test.go`:
- Around line 1625-1626: The test currently ignores errors returned by calls
like types.GetProvLabsFeeAddress(s.ctx.ChainID()) and similar helper functions;
update the test to check and fail on those errors (e.g.,
require.NoError/require.Nil or t.Fatalf) and include descriptive failure
messages for each assertion. Specifically, replace discarded-error patterns
around GetProvLabsFeeAddress, s.ctx.ChainID(), and
s.simApp.BankKeeper.GetBalance usage in reconcile_test.go (and the similar
occurrences at the noted ranges 1659-1660, 1749-1751, 1755-1761) so every
setup/verification call returns no error before proceeding and each assertion
uses a clear message explaining what failed.
- Around line 1576-1768: The tests TestKeeper_PerformVaultFeeTransfer,
TestKeeper_CanPayoutDuration_WithAUMFee, and TestKeeper_HandleVaultFeeTimeouts
duplicate vault creation, marker setup, funding, and context wiring; extract
reusable helpers (e.g., helper
CreateAndActivateMarker/requireAddFinalizeAndActivateMarker wrapper,
CreateVaultWithParams, FundMarker(markerAddr, coins),
SetVaultRatesAndPeriod(vault, currentRate, desiredRate, feeStart, feeTimeout),
and AdvanceCtxWithTime(ctx, t)) and replace repeated blocks with calls to those
helpers and a table-driven loop for permutations (different
paymentDenom/liquidity/times) so tests become concise and maintainable while
keeping existing assertions intact referencing functions CreateVault,
PerformVaultFeeTransfer, CanPayoutDuration, FeeTimeoutQueue, and
TestAccessor_handleVaultFeeTimeouts.
In `@keeper/reconcile.go`:
- Around line 287-295: The comment for CanPayoutDuration is out of sync with its
implementation: it claims an AUM fee feasibility check is performed but the
function only validates interest-side liquidity and then returns true; update
the function CanPayoutDuration to actually compute the AUM fee for the given
duration (e.g., aumFee) and verify that the principal marker's PaymentDenom
balance (principalMarker.PaymentDenom or equivalent balance lookup used
elsewhere) can cover that fee; if it cannot, return false, otherwise proceed to
return true; ensure you reference the same variables used for interest checks
(reserves, interest) and add the AUM fee check before the final true return so
the behavior matches the comment.
- Around line 635-646: Replace use of ctx.Logger() in the fee-timeout handlers
with the module-scoped logger k.getLogger(ctx): specifically, in the error paths
around the dequeue fee timeout handling and the PerformVaultFeeTransfer call
(references: tryGetVault, PerformVaultFeeTransfer), call k.getLogger(ctx).Error
instead of ctx.Logger().Error and keep the structured fields (e.g., "vault",
addr.String(), "err", err) so logs remain module-scoped and structured; do the
same for the equivalent error sites mentioned (the other fee-timeout log paths).
- Around line 40-50: The code is returning raw errors from calls like
PerformVaultInterestTransfer, PerformVaultFeeTransfer, SafeEnqueueFeeTimeout,
and publishShareNav; wrap each propagated error with contextual text using
fmt.Errorf and the %w verb (e.g. fmt.Errorf("performing vault interest transfer
for vault %v: %w", vault.ID, err)) so callers get actionable context; apply the
same wrapping pattern to the other similar raw-err returns flagged in the review
as well.
---
Duplicate comments:
In `@keeper/reconcile.go`:
- Around line 407-425: The computed "estimated" net assets can go negative;
clamp it to zero before returning by checking if estimated.IsNegative() and, if
so, set estimated to the zero sdkmath.Int value. Update the function that calls
EstimateAccruedInterest, EstimateAccruedAUMFee and
CalculateOutstandingFeeUnderlying (the block using variables interestEarned,
feeAccrued, outstandingUnderlying and returning estimated) to perform: if
estimated.IsNegative() { estimated = sdkmath.Int{} } right before the final
return so downstream code never receives a negative value.
- Around line 490-493: The error path that logs a failed PerformVaultFeeTransfer
and then uses continue drops the dequeued vault and prevents automatic
retry/period-reset, so replace the bare continue with logic that re-schedules
the vault for retry (e.g., call the keeper's enqueue/requeue method or otherwise
push the vault back onto the work queue) and/or invoke the existing retry
scheduling helper so this vault will be retried later; also ensure the
fee-period reset/interest-timeout bookkeeping runs even on transfer errors
(either move the period-reset before the error branch or call the reset helper
before requeueing). Apply the same change at the other occurrence referenced
(lines ~644-647) so both error paths requeue the vault instead of dropping it.
---
Nitpick comments:
In `@keeper/payout_test.go`:
- Around line 150-153: Replace the hardcoded magic amount and epoch-math comment
by introducing a descriptive test fixture (e.g., expectedCollectedFee or
expectedAssetsAmount) and use it when constructing expectedAssets and asserting
balances; locate the assertion using symbols expectedAssets, sdk.NewInt64Coin,
s.assertBalance, ownerAddr and underlyingDenom, create a named variable
(expectedCollectedFee) holding the expected int64 value (previously 46) with a
short clarifying name, and use that variable instead of the literal 46 so the
test reads clearly without epoch-dependent inline math.
In `@keeper/reconcile_test.go`:
- Around line 1375-1383: Remove the test debug logging: delete the
s.T().Logf("End vault balance: %s", endVault.String()) call and the subsequent
loop that logs events and attributes (the for _, ev := range events { ... }
block that iterates over normalizeEvents(s.ctx.EventManager().Events())). Leave
the events := normalizeEvents(...) assignment if the test uses it for
assertions, but eliminate any printing/logging of events or attributes so the
test output is not noisy.
In `@types/events.go`:
- Around line 288-289: Update the Godoc for the exported constructor
NewEventVaultFeeCollected to explain its architectural intent: state that it is
the canonical constructor used to serialize vault fee events for downstream
consumers and event routing, describe why callers should use this constructor
(ensures consistent field ordering/format and metadata like
duration/aumSnapshot), and briefly document the key parameters (vaultAddress,
collected, requested, aumSnapshot, outstanding, duration) to clarify their roles
in the canonical fee-event payload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 486653c4-81b2-4d48-acaf-24a603b4deef
⛔ Files ignored due to path filters (1)
types/events.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (6)
api/provlabs/vault/v1/events.pulsar.gokeeper/payout_test.gokeeper/reconcile.gokeeper/reconcile_test.goproto/provlabs/vault/v1/events.prototypes/events.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.gitignore (1)
69-69: Anchor ignore rule to repository root to avoid overmatching.
issues/ignores any folder namedissuesat any depth. If this is meant to be only the root local workspace folder, prefer/issues/.Proposed change
-issues/ +/issues/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 69, The .gitignore entry "issues/" is unanchored and will ignore any directory named "issues" at any depth; change it to an anchored pattern so it only ignores the repository root folder by replacing the unanchored rule with a root-anchored one (use the pattern that begins with a slash, e.g., "/issues/") to avoid overmatching.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@keeper/genesis_test.go`:
- Around line 85-88: The subtests currently only assert that InitGenesis panics
but not that it panics with the expected message; update the t.Run body for each
entry in tests (the tests slice using name, genState, panicMsg) to capture and
compare the panic text to panicMsg (e.g., using require.PanicsWithValue or
recovering the panic and asserting equality) and include a descriptive failure
message that references the test name and expected panicMsg; do the same for the
other subtest block referenced (lines ~110-114) so the new MaxInt64 guard is
verified specifically.
---
Nitpick comments:
In @.gitignore:
- Line 69: The .gitignore entry "issues/" is unanchored and will ignore any
directory named "issues" at any depth; change it to an anchored pattern so it
only ignores the repository root folder by replacing the unanchored rule with a
root-anchored one (use the pattern that begins with a slash, e.g., "/issues/")
to avoid overmatching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4bd2c0e7-731c-439d-9cc4-193c4f63b4a4
📒 Files selected for processing (7)
.gitignoreinterest/interest.gointerest/interest_test.gokeeper/genesis.gokeeper/genesis_test.gotypes/genesis.gotypes/genesis_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- interest/interest_test.go
- interest/interest.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
keeper/queue.go (1)
79-104: Implementation is correct and addresses prior atomicity concerns.The cache context pattern ensures that
FeePeriodStart,FeePeriodTimeout, vault persistence, and queue enqueue are committed atomically. Error handling and logging follow coding guidelines.Optional consideration:
SafeEnqueueFeeTimeoutandSafeEnqueueTimeoutshare nearly identical structure. If more timeout queues emerge, consider extracting a generic helper to reduce duplication. For now, the explicitness is acceptable.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/queue.go` around lines 79 - 104, SafeEnqueueFeeTimeout largely duplicates SafeEnqueueTimeout; extract the shared cache-context + dequeue, set period start/timeout, persist vault, enqueue, and write logic into a single helper (e.g., safeEnqueueTimeout) that accepts the queue interface (FeeTimeoutQueue-like), the vault, and a small strategy/parameters to set the specific timeout fields (FeePeriodStart/FeePeriodTimeout), then replace SafeEnqueueFeeTimeout and SafeEnqueueTimeout to call that helper to remove duplication while preserving current error handling and logging semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@keeper/queue.go`:
- Around line 79-104: SafeEnqueueFeeTimeout largely duplicates
SafeEnqueueTimeout; extract the shared cache-context + dequeue, set period
start/timeout, persist vault, enqueue, and write logic into a single helper
(e.g., safeEnqueueTimeout) that accepts the queue interface
(FeeTimeoutQueue-like), the vault, and a small strategy/parameters to set the
specific timeout fields (FeePeriodStart/FeePeriodTimeout), then replace
SafeEnqueueFeeTimeout and SafeEnqueueTimeout to call that helper to remove
duplication while preserving current error handling and logging semantics.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
keeper/reconcile.go (1)
227-235: Wrap error with context on line 229.The error return on line 229 returns raw
errwithout context, inconsistent with the error wrapping elsewhere in this method.Proposed fix
tvv, err := k.GetTVVInUnderlyingAsset(ctx, *vault) if err != nil { - return err + return fmt.Errorf("failed to get TVV for fee calculation: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/reconcile.go` around lines 227 - 235, The call to GetTVVInUnderlyingAsset returns a raw err that should be wrapped with context like other errors in this method; modify the error return after GetTVVInUnderlyingAsset(ctx, *vault) to wrap the error (e.g., fmt.Errorf or errors.Wrap) with a descriptive message including the vault identifier and action (e.g., "getting TVV for vault %v"), keeping consistency with how errors from EstimateAccruedAUMFeePayment are handled; update the error return site in reconcile.go near the GetTVVInUnderlyingAsset invocation accordingly.types/vault_test.go (2)
463-564: Extract a reusable vault fixture builder for these added cases.The newly added validation cases repeat the same vault setup many times; a helper/builder would make case intent clearer and reduce maintenance churn.
♻️ Refactor sketch
+func validVaultAccountFixture(baseAcc *authtypes.BaseAccount, admin, denom string) types.VaultAccount { + return types.VaultAccount{ + BaseAccount: baseAcc, + Admin: admin, + TotalShares: sdk.NewInt64Coin(denom, 0), + UnderlyingAsset: "uusd", + PaymentDenom: "uusd", + CurrentInterestRate: "0.0", + DesiredInterestRate: "0.0", + OutstandingAumFee: sdk.NewInt64Coin("uusd", 0), + } +}Then each case mutates only relevant fields (
PeriodStart,FeePeriodTimeout,OutstandingAumFee, etc.).As per coding guidelines, “Abstract repeated test operations (e.g., account setup, vault creation, state assertions) into reusable test helper functions”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@types/vault_test.go` around lines 463 - 564, Add a reusable fixture builder (e.g., makeVaultAccount or newVaultFixture) that returns a types.VaultAccount pre-populated with the common defaults used across these cases (BaseAccount: baseAcc, Admin: validAdmin, TotalShares: sdk.NewInt64Coin(validDenom, 0), UnderlyingAsset: "uusd", PaymentDenom: "uusd", CurrentInterestRate: "0.0", DesiredInterestRate: "0.0", OutstandingAumFee: sdk.NewInt64Coin("uusd", 0)); update each test case to call this builder and then mutate only the differing fields (PeriodStart, PeriodTimeout, FeePeriodStart, FeePeriodTimeout, OutstandingAumFee) instead of repeating the full types.VaultAccount literal—use the unique symbol names (types.VaultAccount, makeVaultAccount/newVaultFixture, OutstandingAumFee, PeriodStart, FeePeriodTimeout) so future changes to defaults are centralized and tests are clearer.
679-715: Strengthen fee-address test assertions to validate exact branch outputs.For mainnet/testnet, the test currently accepts any non-nil address on success. Assert exact expected address strings when
err == nil, so regressions in hardcoded constants/branch behavior are caught.✅ Stronger table assertions
func TestGetProvLabsFeeAddress(t *testing.T) { tests := []struct { - name string - chainID string + name string + chainID string + expectedAddr string }{ { - name: "mainnet", - chainID: "pio-mainnet-1", + name: "mainnet", + chainID: "pio-mainnet-1", + expectedAddr: types.ProvLabsMainnetFeeAddress, }, { - name: "testnet", - chainID: "pio-testnet-1", + name: "testnet", + chainID: "pio-testnet-1", + expectedAddr: types.ProvLabsTestnetFeeAddress, }, { - name: "other/local", - chainID: "vaulty-1", + name: "other/local", + chainID: "vaulty-1", }, } @@ addr, err := types.GetProvLabsFeeAddress(tc.chainID) if tc.chainID == "pio-mainnet-1" || tc.chainID == "pio-testnet-1" { if err != nil { require.Containsf(t, err.Error(), "invalid Bech32 prefix", "test case %q: unexpected error; want Bech32 prefix error, got %v", tc.name, err) } else { require.NotNilf(t, addr, "test case %q: fee address should not be nil", tc.name) + require.Equalf(t, tc.expectedAddr, addr.String(), "test case %q: unexpected fee address", tc.name) } } else { require.NoErrorf(t, err, "test case %q: unexpected error for chain %s", tc.name, tc.chainID) require.NotNilf(t, addr, "test case %q: fee address should not be nil", tc.name) }As per coding guidelines, “Use table-driven test patterns for unit and integration tests to ensure exhaustive coverage of edge cases”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@types/vault_test.go` around lines 679 - 715, Update TestGetProvLabsFeeAddress to assert exact expected outputs by adding an expectedAddr string to each test case and comparing it when err == nil; locate the table-driven tests in TestGetProvLabsFeeAddress and modify each case (for "pio-mainnet-1" and "pio-testnet-1" set the expected bech32 address strings, for "vaulty-1" set its expected address or empty if not applicable), then replace the loose require.NotNilf checks with require.Equalf(t, tc.expectedAddr, addr.String(), "test case %q: unexpected fee address", tc.name) when err is nil, and retain the existing error-prefix checks for prefix-mismatch paths; ensure you update the test struct to include expectedAddr and handle nil/empty addr safely before calling addr.String().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@types/vault.go`:
- Around line 198-200: The current validation treats an empty-denom coin with
zero amount as acceptable, which can lead to a missing-denom being used later by
PerformVaultFeeTransfer; update the validation around the OutstandingAumFee
field so that a fee is considered present only when Denom is non-empty AND
Amount is non-nil AND Amount is non-zero, and if present its Denom must equal
v.PaymentDenom; in practice change the condition around OutstandingAumFee to
explicitly require Denom != "" for any non-zero amount and return an error when
a non-zero OutstandingAumFee has an empty denom or when the denom does not match
v.PaymentDenom so PerformVaultFeeTransfer's OutstandingAumFee.Add(newFeePayment)
can safely assume a valid denom.
---
Nitpick comments:
In `@keeper/reconcile.go`:
- Around line 227-235: The call to GetTVVInUnderlyingAsset returns a raw err
that should be wrapped with context like other errors in this method; modify the
error return after GetTVVInUnderlyingAsset(ctx, *vault) to wrap the error (e.g.,
fmt.Errorf or errors.Wrap) with a descriptive message including the vault
identifier and action (e.g., "getting TVV for vault %v"), keeping consistency
with how errors from EstimateAccruedAUMFeePayment are handled; update the error
return site in reconcile.go near the GetTVVInUnderlyingAsset invocation
accordingly.
In `@types/vault_test.go`:
- Around line 463-564: Add a reusable fixture builder (e.g., makeVaultAccount or
newVaultFixture) that returns a types.VaultAccount pre-populated with the common
defaults used across these cases (BaseAccount: baseAcc, Admin: validAdmin,
TotalShares: sdk.NewInt64Coin(validDenom, 0), UnderlyingAsset: "uusd",
PaymentDenom: "uusd", CurrentInterestRate: "0.0", DesiredInterestRate: "0.0",
OutstandingAumFee: sdk.NewInt64Coin("uusd", 0)); update each test case to call
this builder and then mutate only the differing fields (PeriodStart,
PeriodTimeout, FeePeriodStart, FeePeriodTimeout, OutstandingAumFee) instead of
repeating the full types.VaultAccount literal—use the unique symbol names
(types.VaultAccount, makeVaultAccount/newVaultFixture, OutstandingAumFee,
PeriodStart, FeePeriodTimeout) so future changes to defaults are centralized and
tests are clearer.
- Around line 679-715: Update TestGetProvLabsFeeAddress to assert exact expected
outputs by adding an expectedAddr string to each test case and comparing it when
err == nil; locate the table-driven tests in TestGetProvLabsFeeAddress and
modify each case (for "pio-mainnet-1" and "pio-testnet-1" set the expected
bech32 address strings, for "vaulty-1" set its expected address or empty if not
applicable), then replace the loose require.NotNilf checks with
require.Equalf(t, tc.expectedAddr, addr.String(), "test case %q: unexpected fee
address", tc.name) when err is nil, and retain the existing error-prefix checks
for prefix-mismatch paths; ensure you update the test struct to include
expectedAddr and handle nil/empty addr safely before calling addr.String().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c1eab96-7ef3-4e75-bb07-b45c7930122d
📒 Files selected for processing (5)
keeper/reconcile.goqueue/fee_timeout.goqueue/payout_timeout.gotypes/vault.gotypes/vault_test.go
Collaborator
Author
|
@coderabbitai Can you go through all your suggested changes in this PR and resolve conversations that are fixed. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll fetch all my review comments on this PR to identify which ones have been addressed. 🏁 Script executed: Length of output: 0 Let me try different API endpoints to find my comments. 🏁 Script executed: Length of output: 7850 Good, I can see 12 threads already resolved and 10 still open. Let me check the current code to see which unresolved ones may have been fixed. 🏁 Script executed: Length of output: 8699 🏁 Script executed: Length of output: 24253
None of these have been addressed in the current branch, so there is nothing to resolve at this time. Let me know if any of these have been fixed in a recent commit that isn't yet pushed, and I'll re-check and resolve accordingly. 🧠 Learnings used |
…FeeTransfer failure the vault is permanently dropped from schedulin
10 tasks
Collaborator
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
Taztingo
reviewed
Mar 19, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
types/vault_test.go (1)
1-578:⚠️ Potential issue | 🔴 CriticalAdd test coverage for
ValidateManagementAuthoritymethod.The
ValidateManagementAuthoritymethod exists intypes/vault.go(line 293) and is actively used in 9 locations acrosskeeper/msg_server.go(lines 165, 235, 308, 344, 377, 420, 471, 496, 541), but currently has no unit test coverage. As an exported, security-related validation method, it requires comprehensive table-driven test cases covering valid/invalid authority addresses and edge cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@types/vault_test.go` around lines 1 - 578, Add table-driven tests for the exported method ValidateManagementAuthority on types.VaultAccount by creating a new test (e.g., TestVaultAccount_ValidateManagementAuthority) that covers valid authority addresses, invalid formats, empty strings, and edge cases where authority equals Admin or AssetManager; for each case construct a types.VaultAccount (use utils.TestAddress().Bech32 for valid addresses and malformed strings for invalid ones), call vaultAccount.ValidateManagementAuthority(authority) and assert expected error/no-error using require/ assert; reference the method name ValidateManagementAuthority and the types.VaultAccount struct so reviewers can locate and run the new tests alongside the existing TestVaultAccount_Validate in the same package.
♻️ Duplicate comments (1)
keeper/vault.go (1)
91-108:⚠️ Potential issue | 🟠 MajorVault creation can still leave partial state on
SafeEnqueueFeeTimeoutfailure.The writes at Line 91 (
SetVaultLookup) and Line 104 (SetAccount) occur on the main context beforeSafeEnqueueFeeTimeoutis called. IfSafeEnqueueFeeTimeoutfails, these writes are already persisted, leaving an orphan vault without a fee timeout entry.While
SafeEnqueueFeeTimeoutuses its ownCacheContextinternally (perkeeper/queue.go:79-103), that only protects the fee timeout operations—not the vault creation steps preceding it.Wrap vault creation in CacheContext for atomicity
func (k *Keeper) createVaultAccount(ctx sdk.Context, admin, shareDenom, underlyingAsset, paymentDenom string, withdrawalDelay uint64) (*types.VaultAccount, error) { + cacheCtx, write := ctx.CacheContext() + vaultAddr := types.GetVaultAddress(shareDenom) vault := types.NewVaultAccount( - authtypes.NewBaseAccountWithAddress(vaultAddr), + authtypes.NewBaseAccountWithAddress(vaultAddr), admin, shareDenom, underlyingAsset, paymentDenom, withdrawalDelay, ) if err := vault.Validate(); err != nil { return nil, fmt.Errorf("failed to validate vault account: %w", err) } - if err := k.SetVaultLookup(ctx, vault); err != nil { + if err := k.SetVaultLookup(cacheCtx, vault); err != nil { return nil, fmt.Errorf("failed to store new vault: %w", err) } - vaultAcc := k.AuthKeeper.GetAccount(ctx, vault.GetAddress()) + vaultAcc := k.AuthKeeper.GetAccount(cacheCtx, vault.GetAddress()) if vaultAcc != nil { if _, ok := vaultAcc.(types.VaultAccountI); ok { return nil, fmt.Errorf("vault address already exists for %s", vaultAddr.String()) } else if vaultAcc.GetSequence() > 0 { return nil, fmt.Errorf("account at %s is not a vault account", vaultAddr.String()) } } - vaultAcc = k.AuthKeeper.NewAccount(ctx, vault).(types.VaultAccountI) - k.AuthKeeper.SetAccount(ctx, vaultAcc) + vaultAcc = k.AuthKeeper.NewAccount(cacheCtx, vault).(types.VaultAccountI) + k.AuthKeeper.SetAccount(cacheCtx, vaultAcc) - if err := k.SafeEnqueueFeeTimeout(ctx, vault); err != nil { + if err := k.SafeEnqueueFeeTimeout(cacheCtx, vault); err != nil { return nil, fmt.Errorf("failed to enqueue initial fee timeout: %w", err) } + write() return vault, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/vault.go` around lines 91 - 108, Wrap the vault creation, account setup, and fee timeout enqueue in a single CacheContext so all writes are atomic: create a cache context (e.g. cacheCtx, write := ctx.CacheContext()), perform k.SetVaultLookup(cacheCtx, vault), the AuthKeeper.GetAccount/validation against vaultAddr, k.AuthKeeper.NewAccount(cacheCtx, vault) and k.AuthKeeper.SetAccount(cacheCtx, vaultAcc), then call k.SafeEnqueueFeeTimeout(cacheCtx, vault); if all succeed call write() (or commit the cache) to persist, otherwise do not call write() so no partial state (SetVaultLookup/SetAccount) is persisted; keep existing error returns unchanged.
🧹 Nitpick comments (6)
keeper/payout.go (2)
139-140: Error message inconsistency: method renamed but error text unchanged.The method was renamed from
reconcileVaultInteresttoreconcileVault, but the error message still says "failed to reconcile vault interest". Consider updating for consistency.Update error message
if err := k.reconcileVault(ctx, &vault); err != nil { - return fmt.Errorf("failed to reconcile vault interest: %w", err) + return fmt.Errorf("failed to reconcile vault: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/payout.go` around lines 139 - 140, Update the error text to match the renamed method: replace the outdated message "failed to reconcile vault interest" used in the k.reconcileVault(ctx, &vault) error return with a consistent message referencing reconcileVault (e.g., "failed to reconcile vault"); ensure the changed string appears in the error returned from the k.reconcileVault call so logs match the method name and intent.
84-89: Use module-scoped loggerk.getLogger(ctx)instead ofctx.Logger().Per coding guidelines for keeper files, use the module-scoped logger via
k.getLogger(ctx)for structured and informative log messages.Use k.getLogger(ctx)
- ctx.Logger().Error( + k.getLogger(ctx).Error( "failed to dequeue withdrawal request", "request_id", j.ID, "vault_address", j.VaultAddr.String(), "error", err, )- ctx.Logger().Error( + k.getLogger(ctx).Error( "failed to process withdrawal, issuing refund", "withdrawal_id", j.ID, "reason", reason, "error", err, )Also applies to: 101-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/payout.go` around lines 84 - 89, Replace use of ctx.Logger() with the module-scoped logger k.getLogger(ctx) in the keeper payout code: where the error log is emitted for dequeueing withdrawal requests (the call that currently passes "failed to dequeue withdrawal request", "request_id", j.ID, "vault_address", j.VaultAddr.String(), "error", err) swap ctx.Logger().Error(...) to k.getLogger(ctx).Error(...). Do the same replacement for the other similar logging calls around the dequeue logic (the block referenced also at the later occurrences around lines 101-106) so all structured logs in this keeper use k.getLogger(ctx) instead of ctx.Logger().keeper/reconcile.go (3)
74-76: Wrap error with context.The error from
SafeAddVerificationis returned unwrapped, inconsistent with the other error paths in this function that properly wrap errors.Proposed fix
if err := k.SafeAddVerification(cacheCtx, v); err != nil { - return err + return fmt.Errorf("add verification: %w", err) }As per coding guidelines
**/*.go: "Always wrap errors with context using fmt.Errorf with %w verb".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/reconcile.go` around lines 74 - 76, The call to SafeAddVerification returns an error that is forwarded directly; update the error handling in the function containing this call so it wraps the returned error with context using fmt.Errorf and the %w verb (e.g., replace returning err with return fmt.Errorf("adding verification %s failed: %w", v.ID, err) or similar). Ensure you import fmt if needed and keep the message consistent with other wrapped errors in this file.
540-552: DirectPeriodTimeoutmodification in error recovery path.This function directly modifies
vault.PeriodTimeout(Line 545), which per learnings should normally be managed through queuing structures. However, this appears intentional for error recovery scenarios where the normal atomic flow has already failed.Consider adding a brief comment explaining this is an exceptional recovery path to prevent future confusion.
Suggested documentation
func (k Keeper) rescheduleInterestTimeout(ctx sdk.Context, vault *types.VaultAccount, oldTimeout int64) { + // Note: Direct PeriodTimeout modification is intentional here for error recovery. + // The normal SafeEnqueueTimeout path has already failed, so we manually + // dequeue/set/enqueue to prevent the vault from being dropped from scheduling. newTimeout := ctx.BlockTime().Unix() + AutoReconcileTimeout🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/reconcile.go` around lines 540 - 552, rescheduleInterestTimeout currently sets vault.PeriodTimeout directly (and calls SetVaultAccount) as part of its error-recovery flow after dequeuing/enqueuing via PayoutTimeoutQueue; add a short clarifying comment above the direct assignment and persistence that this is an exceptional recovery path performed when the normal atomic queue flow failed so future readers know the direct mutation is intentional (reference rescheduleInterestTimeout, PeriodTimeout, PayoutTimeoutQueue, and SetVaultAccount).
286-292: Consider consolidating vault persistence.
PerformVaultFeeTransferpersists the vault at Line 290 after updatingOutstandingAumFeeandFeePeriodStart. However,SafeEnqueueFeeTimeout(called afterward inatomicallyReconcileFee) also persists the vault with updatedFeePeriodStartandFeePeriodTimeout.This double-write works because of the cache context, but the vault state is written twice with slightly different values. Consider either:
- Removing the
SetVaultAccountcall here and lettingSafeEnqueueFeeTimeouthandle persistence, or- Documenting that this is intentional for event emission ordering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/reconcile.go` around lines 286 - 292, PerformVaultFeeTransfer currently calls SetVaultAccount to persist vault.OutstandingAumFee and vault.FeePeriodStart, but atomicallyReconcileFee calls SafeEnqueueFeeTimeout afterward which also persists the vault with FeePeriodStart and FeePeriodTimeout; remove the redundant SetVaultAccount call inside PerformVaultFeeTransfer and ensure PerformVaultFeeTransfer only updates vault.OutstandingAumFee and vault.FeePeriodStart in-memory before returning so that SafeEnqueueFeeTimeout (called by atomicallyReconcileFee) performs the single write and event emission; if you must keep the write for ordering reasons, instead add a clear comment in PerformVaultFeeTransfer explaining why SetVaultAccount is required and that SafeEnqueueFeeTimeout will update FeePeriodTimeout and re-persist the vault.keeper/reconcile_test.go (1)
1507-1515: Remove debug logging from test.These
s.T().Logfcalls appear to be leftover debugging artifacts. They clutter test output and should be removed for cleaner test runs.Proposed cleanup
endVault := s.simApp.BankKeeper.GetBalance(s.ctx, vaultAddr, underlying.Denom) - s.T().Logf("End vault balance: %s", endVault.String()) events := normalizeEvents(s.ctx.EventManager().Events()) - for _, ev := range events { - s.T().Logf("Event: %s", ev.Type) - for _, attr := range ev.Attributes { - s.T().Logf(" %s: %s", attr.Key, attr.Value) - } - } - expectedVaultBalance := underlying.Amount.Add(smallPrincipal.Amount)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/reconcile_test.go` around lines 1507 - 1515, Remove the leftover debug logging in the test by deleting the s.T().Logf calls that print "End vault balance" and each Event/Attribute inside the loop; keep the existing normalization and iteration (normalizeEvents and events variable) but remove the three s.T().Logf lines so the test no longer logs debug output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@keeper/reconcile_test.go`:
- Around line 268-269: Tests currently discard errors from calls like
MarkerKeeper.GetMarker, s.k.GetVault, and s.k.GetTVVInUnderlyingAsset (e.g.,
paymentMarkerAccount, _ := ..., updatedVault, _ := ..., tvv, _ := ...); change
these to capture the error and assert it is nil using s.Require().NoError(err,
"<descriptive message>") before using the returned value (e.g.,
paymentMarkerAccount, err := s.k.MarkerKeeper.GetMarker(...);
s.Require().NoError(err, "failed to get payment marker account"); updatedVault,
err := s.k.GetVault(...); s.Require().NoError(err, "failed to get vault"); tvv,
err := s.k.GetTVVInUnderlyingAsset(...); s.Require().NoError(err, "failed to get
TVV in underlying asset") so test failures show precise error details.
---
Outside diff comments:
In `@types/vault_test.go`:
- Around line 1-578: Add table-driven tests for the exported method
ValidateManagementAuthority on types.VaultAccount by creating a new test (e.g.,
TestVaultAccount_ValidateManagementAuthority) that covers valid authority
addresses, invalid formats, empty strings, and edge cases where authority equals
Admin or AssetManager; for each case construct a types.VaultAccount (use
utils.TestAddress().Bech32 for valid addresses and malformed strings for invalid
ones), call vaultAccount.ValidateManagementAuthority(authority) and assert
expected error/no-error using require/ assert; reference the method name
ValidateManagementAuthority and the types.VaultAccount struct so reviewers can
locate and run the new tests alongside the existing TestVaultAccount_Validate in
the same package.
---
Duplicate comments:
In `@keeper/vault.go`:
- Around line 91-108: Wrap the vault creation, account setup, and fee timeout
enqueue in a single CacheContext so all writes are atomic: create a cache
context (e.g. cacheCtx, write := ctx.CacheContext()), perform
k.SetVaultLookup(cacheCtx, vault), the AuthKeeper.GetAccount/validation against
vaultAddr, k.AuthKeeper.NewAccount(cacheCtx, vault) and
k.AuthKeeper.SetAccount(cacheCtx, vaultAcc), then call
k.SafeEnqueueFeeTimeout(cacheCtx, vault); if all succeed call write() (or commit
the cache) to persist, otherwise do not call write() so no partial state
(SetVaultLookup/SetAccount) is persisted; keep existing error returns unchanged.
---
Nitpick comments:
In `@keeper/payout.go`:
- Around line 139-140: Update the error text to match the renamed method:
replace the outdated message "failed to reconcile vault interest" used in the
k.reconcileVault(ctx, &vault) error return with a consistent message referencing
reconcileVault (e.g., "failed to reconcile vault"); ensure the changed string
appears in the error returned from the k.reconcileVault call so logs match the
method name and intent.
- Around line 84-89: Replace use of ctx.Logger() with the module-scoped logger
k.getLogger(ctx) in the keeper payout code: where the error log is emitted for
dequeueing withdrawal requests (the call that currently passes "failed to
dequeue withdrawal request", "request_id", j.ID, "vault_address",
j.VaultAddr.String(), "error", err) swap ctx.Logger().Error(...) to
k.getLogger(ctx).Error(...). Do the same replacement for the other similar
logging calls around the dequeue logic (the block referenced also at the later
occurrences around lines 101-106) so all structured logs in this keeper use
k.getLogger(ctx) instead of ctx.Logger().
In `@keeper/reconcile_test.go`:
- Around line 1507-1515: Remove the leftover debug logging in the test by
deleting the s.T().Logf calls that print "End vault balance" and each
Event/Attribute inside the loop; keep the existing normalization and iteration
(normalizeEvents and events variable) but remove the three s.T().Logf lines so
the test no longer logs debug output.
In `@keeper/reconcile.go`:
- Around line 74-76: The call to SafeAddVerification returns an error that is
forwarded directly; update the error handling in the function containing this
call so it wraps the returned error with context using fmt.Errorf and the %w
verb (e.g., replace returning err with return fmt.Errorf("adding verification %s
failed: %w", v.ID, err) or similar). Ensure you import fmt if needed and keep
the message consistent with other wrapped errors in this file.
- Around line 540-552: rescheduleInterestTimeout currently sets
vault.PeriodTimeout directly (and calls SetVaultAccount) as part of its
error-recovery flow after dequeuing/enqueuing via PayoutTimeoutQueue; add a
short clarifying comment above the direct assignment and persistence that this
is an exceptional recovery path performed when the normal atomic queue flow
failed so future readers know the direct mutation is intentional (reference
rescheduleInterestTimeout, PeriodTimeout, PayoutTimeoutQueue, and
SetVaultAccount).
- Around line 286-292: PerformVaultFeeTransfer currently calls SetVaultAccount
to persist vault.OutstandingAumFee and vault.FeePeriodStart, but
atomicallyReconcileFee calls SafeEnqueueFeeTimeout afterward which also persists
the vault with FeePeriodStart and FeePeriodTimeout; remove the redundant
SetVaultAccount call inside PerformVaultFeeTransfer and ensure
PerformVaultFeeTransfer only updates vault.OutstandingAumFee and
vault.FeePeriodStart in-memory before returning so that SafeEnqueueFeeTimeout
(called by atomicallyReconcileFee) performs the single write and event emission;
if you must keep the write for ordering reasons, instead add a clear comment in
PerformVaultFeeTransfer explaining why SetVaultAccount is required and that
SafeEnqueueFeeTimeout will update FeePeriodTimeout and re-persist the vault.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d20df1a-845a-408d-97ce-700adcb478c7
⛔ Files ignored due to path filters (1)
types/genesis.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (36)
.changelog/unreleased/features/178-aum-fee-schedule.md.changelog/unreleased/improvements/131-withdrawal-atomicity.mdCONTRIBUTING.mdGEMINI.mdapi/provlabs/vault/v1/genesis.pulsar.gokeeper/abci.gokeeper/export_test.gokeeper/genesis.gokeeper/genesis_test.gokeeper/keeper.gokeeper/migrations.gokeeper/msg_server.gokeeper/payout.gokeeper/payout_test.gokeeper/query_server.gokeeper/reconcile.gokeeper/reconcile_test.gokeeper/state.gokeeper/suite_test.gokeeper/valuation_engine.gokeeper/valuation_engine_test.gokeeper/vault.gokeeper/vault_test.goproto/generate.shproto/provlabs/vault/v1/genesis.protoqueue/fee_timeout.goqueue/fee_timeout_test.goqueue/payout_timeout.goqueue/payout_timeout_test.gosimapp/test_helpers.gospec/01_concepts.mdspec/06_blocker.mdtypes/genesis.gotypes/keys.gotypes/vault.gotypes/vault_test.go
💤 Files with no reviewable changes (1)
- proto/generate.sh
✅ Files skipped from review due to trivial changes (7)
- .changelog/unreleased/features/178-aum-fee-schedule.md
- CONTRIBUTING.md
- keeper/migrations.go
- queue/payout_timeout_test.go
- .changelog/unreleased/improvements/131-withdrawal-atomicity.md
- GEMINI.md
- spec/01_concepts.md
🚧 Files skipped from review as they are similar to previous changes (10)
- keeper/abci.go
- queue/payout_timeout.go
- types/genesis.go
- keeper/export_test.go
- keeper/keeper.go
- types/keys.go
- keeper/genesis_test.go
- proto/provlabs/vault/v1/genesis.proto
- api/provlabs/vault/v1/genesis.pulsar.go
- keeper/valuation_engine.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
keeper/reconcile_test.go (1)
1849-1855:⚠️ Potential issue | 🟡 MinorCapture and assert errors from
Walkcalls.Multiple
Walkcalls discard the returned error, which could hide test failures. As per coding guidelines, every assertion should check errors with descriptive messages.Proposed fix for Walk error handling
found := false - s.k.FeeTimeoutQueue.Walk(s.ctx, func(timeout uint64, addr sdk.AccAddress) (bool, error) { + err = s.k.FeeTimeoutQueue.Walk(s.ctx, func(timeout uint64, addr sdk.AccAddress) (bool, error) { if addr.Equals(vaultAddr) { found = true s.Require().Greater(int64(timeout), now.Unix()) } return false, nil }) + s.Require().NoError(err, "failed walking fee timeout queue")Apply similar changes to the Walk calls at lines 2156-2162, 2204-2213, and 2260-2266.
As per coding guidelines
**/*_test.go: "Every assertion in tests must include a descriptive failure message that provides context".Also applies to: 2156-2162, 2204-2213, 2260-2266
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/reconcile_test.go` around lines 1849 - 1855, The Walk calls (e.g., s.k.FeeTimeoutQueue.Walk) currently ignore the returned error and assert without descriptive messages; update each Walk invocation to capture its error (err := s.k.FeeTimeoutQueue.Walk(...)) and assert no error with a descriptive message using s.Require().NoError(err, "Walk failed for FeeTimeoutQueue while looking for vaultAddr"), then keep the existing assertions (like s.Require().Greater(int64(timeout), now.Unix())) but add descriptive failure messages (e.g., s.Require().Greater(..., "timeout for vaultAddr should be greater than now")); apply the same pattern to the other Walk calls mentioned (lines 2156-2162, 2204-2213, 2260-2266) so every Walk error is checked and every test assertion includes context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@keeper/reconcile_test.go`:
- Line 2167: Add descriptive failure messages to the NoError assertions that
currently call s.Require().NoError(err) so failures give context: update the
assertion after the fee-timeout flow to s.Require().NoError(err, "failed to get
vault after fee timeout handling"), the assertion after calling
handleVaultFeeTimeouts to s.Require().NoError(err, "handleVaultFeeTimeouts
should succeed"), the assertion after calling handleVaultInterestTimeouts to
s.Require().NoError(err, "handleVaultInterestTimeouts should not return error"),
and the later vault retrieval assertion to s.Require().NoError(err, "failed to
get vault after interest timeout retry"); locate these uses by searching for the
NoError calls in reconcile_test.go and the surrounding calls to
handleVaultFeeTimeouts and handleVaultInterestTimeouts to apply the message
strings.
---
Duplicate comments:
In `@keeper/reconcile_test.go`:
- Around line 1849-1855: The Walk calls (e.g., s.k.FeeTimeoutQueue.Walk)
currently ignore the returned error and assert without descriptive messages;
update each Walk invocation to capture its error (err :=
s.k.FeeTimeoutQueue.Walk(...)) and assert no error with a descriptive message
using s.Require().NoError(err, "Walk failed for FeeTimeoutQueue while looking
for vaultAddr"), then keep the existing assertions (like
s.Require().Greater(int64(timeout), now.Unix())) but add descriptive failure
messages (e.g., s.Require().Greater(..., "timeout for vaultAddr should be
greater than now")); apply the same pattern to the other Walk calls mentioned
(lines 2156-2162, 2204-2213, 2260-2266) so every Walk error is checked and every
test assertion includes context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8fb5c37f-efc0-4767-966f-4aa07b73e4d2
📒 Files selected for processing (2)
GEMINI.mdkeeper/reconcile_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- GEMINI.md
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
keeper/reconcile_test.go (1)
1511-1519: Consider removing debug logging.The
s.T().Logfstatements will produce output during test runs. If these are temporary debugging aids, consider removing them before merge; if they're intentional for CI visibility, this is acceptable.Optional cleanup
endVault := s.simApp.BankKeeper.GetBalance(s.ctx, vaultAddr, underlying.Denom) - s.T().Logf("End vault balance: %s", endVault.String()) events := normalizeEvents(s.ctx.EventManager().Events()) - for _, ev := range events { - s.T().Logf("Event: %s", ev.Type) - for _, attr := range ev.Attributes { - s.T().Logf(" %s: %s", attr.Key, attr.Value) - } - } - expectedVaultBalance := underlying.Amount.Add(smallPrincipal.Amount)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/reconcile_test.go` around lines 1511 - 1519, Remove the temporary test debug logging: delete the s.T().Logf(...) calls used to print "End vault balance" and the per-event/type/attribute logs in the test (the s.T().Logf calls inside the loop over events from s.ctx.EventManager().Events()), leaving the normalizeEvents(s.ctx.EventManager().Events()) call and the events iteration logic intact; if CI visibility is required, replace those logs with t.Logf guarded by a test-verbosity flag or add a comment explaining why they must remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@keeper/reconcile_test.go`:
- Around line 1849-1856: The test currently ignores the error returned by
s.k.FeeTimeoutQueue.Walk; update the call to capture its returned error (e.g.,
err := s.k.FeeTimeoutQueue.Walk(s.ctx, ...)) and add an assertion that err is
nil (s.Require().NoError(err)) before asserting found, keeping the existing
lambda logic that sets found and checks timeout; reference the
FeeTimeoutQueue.Walk call and the found variable in your change.
- Around line 2156-2163: The call to FeeTimeoutQueue.Walk on s.k
(s.k.FeeTimeoutQueue.Walk(s.ctx, ...)) currently ignores its returned error;
change the call to capture the error (e.g., err :=
s.k.FeeTimeoutQueue.Walk(s.ctx, func(timeout uint64, addr sdk.AccAddress) (bool,
error) { ... })) and add an assertion that err is nil (e.g.,
s.Require().NoError(err)) after the Walk so any Walk failure is surfaced in the
test; keep the existing body that sets found and checks expectedTimeout
unchanged.
- Around line 2204-2213: The call to s.k.FeeTimeoutQueue.Walk ignores its
returned error; change to capture and check the error (e.g. err :=
s.k.FeeTimeoutQueue.Walk(...); if err != nil { t.Fatalf("FeeTimeoutQueue.Walk
failed: %v", err) } or use testify/require.NoError) so failures in
FeeTimeoutQueue.Walk are surfaced; keep the existing callback logic (checking
vaultAddr, foundOld, foundNew) unchanged.
- Around line 2260-2267: The call to s.k.PayoutTimeoutQueue.Walk ignores its
returned error; update the block calling s.k.PayoutTimeoutQueue.Walk to capture
the returned error (e.g., err := s.k.PayoutTimeoutQueue.Walk(...)) and assert no
error using the test helper (e.g., s.Require().NoError(err)) before asserting
found and timeout expectations; keep the inner callback unchanged except for
preserving its (bool, error) return and ensure the error check happens
immediately after the Walk call.
---
Nitpick comments:
In `@keeper/reconcile_test.go`:
- Around line 1511-1519: Remove the temporary test debug logging: delete the
s.T().Logf(...) calls used to print "End vault balance" and the
per-event/type/attribute logs in the test (the s.T().Logf calls inside the loop
over events from s.ctx.EventManager().Events()), leaving the
normalizeEvents(s.ctx.EventManager().Events()) call and the events iteration
logic intact; if CI visibility is required, replace those logs with t.Logf
guarded by a test-verbosity flag or add a comment explaining why they must
remain.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
keeper/reconcile_test.go (2)
1849-1857:⚠️ Potential issue | 🟡 MinorAdd descriptive message to
NoErrorassertion afterWalk.The error is correctly captured and asserted, but the message is missing for consistency with guidelines.
🛡️ Proposed fix
err = s.k.FeeTimeoutQueue.Walk(s.ctx, func(timeout uint64, addr sdk.AccAddress) (bool, error) { if addr.Equals(vaultAddr) { found = true s.Require().Greater(int64(timeout), now.Unix()) } return false, nil }) - s.Require().NoError(err) + s.Require().NoError(err, "failed to walk fee timeout queue") s.Require().True(found, "new fee timeout should be enqueued")As per coding guidelines
**/*_test.go: "Every assertion in tests must include a descriptive failure message".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/reconcile_test.go` around lines 1849 - 1857, The test calls FeeTimeoutQueue.Walk and asserts no error with s.Require().NoError(err) but lacks the required descriptive failure message; update the assertion at the end of the Walk block (the s.Require().NoError(err) line) to include a clear context string (e.g., "failed walking FeeTimeoutQueue" or similar) so the message explains what operation failed when the assertion fails, referencing FeeTimeoutQueue.Walk and the err variable.
2206-2217:⚠️ Potential issue | 🟡 MinorAdd descriptive message to
NoErrorassertion afterWalk.Same pattern - the error is captured but the message is missing.
🛡️ Proposed fix
err = s.k.FeeTimeoutQueue.Walk(s.ctx, func(timeout uint64, addr sdk.AccAddress) (bool, error) { if addr.Equals(vaultAddr) { if timeout == uint64(twoMonthsAgo.Unix()) { foundOld = true } else if timeout > uint64(now.Unix()) { foundNew = true } } return false, nil }) - s.Require().NoError(err) + s.Require().NoError(err, "failed to walk fee timeout queue") s.Require().False(foundOld, "vault should be dequeued from old timeout")As per coding guidelines
**/*_test.go: "Every assertion in tests must include a descriptive failure message".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/reconcile_test.go` around lines 2206 - 2217, The NoError assertion after calling s.k.FeeTimeoutQueue.Walk (the s.Require().NoError(err) immediately following the Walk) is missing a descriptive failure message; update that call to include a clear message (e.g., "Walk over FeeTimeoutQueue returned unexpected error") so test failures explain the context, keeping the rest of the Walk callback logic and the subsequent assertions unchanged.
🧹 Nitpick comments (1)
keeper/reconcile_test.go (1)
1510-1522: Consider removing debug logging from test.These
s.T().Logfcalls appear to be debug artifacts that add noise to test output. The assertions that follow already validate the expected behavior.♻️ Proposed cleanup
endVault := s.simApp.BankKeeper.GetBalance(s.ctx, vaultAddr, underlying.Denom) - s.T().Logf("End vault balance: %s", endVault.String()) - - events := normalizeEvents(s.ctx.EventManager().Events()) - for _, ev := range events { - s.T().Logf("Event: %s", ev.Type) - for _, attr := range ev.Attributes { - s.T().Logf(" %s: %s", attr.Key, attr.Value) - } - } expectedVaultBalance := underlying.Amount.Add(smallPrincipal.Amount) s.Require().Equal(expectedVaultBalance, endVault.Amount, "Vault should receive exactly the available marker balance") + + events := normalizeEvents(s.ctx.EventManager().Events()) found := false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/reconcile_test.go` around lines 1510 - 1522, Remove the debug logging calls that print the vault balance and events (the s.T().Logf lines around endVault, the loop over events from normalizeEvents(s.ctx.EventManager().Events()), and their inner s.T().Logf calls); keep the balance computation and assertion using expectedVaultBalance and endVault.Amount as-is so the test remains deterministic and noise-free. Ensure no other logging is left behind in the same block (only remove the debug Logf calls, not the calls to normalizeEvents, events variable, or the final s.Require().Equal assertion).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@keeper/reconcile_test.go`:
- Around line 1817-1819: The NoError assertion on s.Require().NoError(err) in
the CanPayInterestDuration test lacks a descriptive failure message; update the
call to include a clear message (e.g., "CanPayInterestDuration returned error
for vault=<vault> year=<year>") so failures explain context. Locate the
assertion where s.k.CanPayInterestDuration(s.ctx, vault, year) is called and
change s.Require().NoError(err) to include an informative string mentioning the
function and relevant variables (vault/year) to aid debugging.
- Around line 2157-2165: The test calls s.k.FeeTimeoutQueue.Walk and asserts no
error with s.Require().NoError(err) but lacks a descriptive message; update the
assertion to include a clear failure message (for example: "FeeTimeoutQueue.Walk
returned unexpected error") so failures explain what operation failed; locate
the call to s.k.FeeTimeoutQueue.Walk and change the s.Require().NoError
invocation to include that descriptive message while leaving the rest of the
Walk callback and the s.Require().True(found, ...) assertion as-is.
- Around line 2263-2271: The NoError assertion after calling
PayoutTimeoutQueue.Walk lacks a descriptive failure message; update the
s.Require().NoError(err) call (used after s.k.PayoutTimeoutQueue.Walk in
reconcile_test.go) to include a clear message (e.g., "Walk should not return an
error" or similar) so the test failure explains the context if Walk returns an
error; ensure the message follows existing test-message style and references the
Walk/PayoutTimeoutQueue operation.
---
Duplicate comments:
In `@keeper/reconcile_test.go`:
- Around line 1849-1857: The test calls FeeTimeoutQueue.Walk and asserts no
error with s.Require().NoError(err) but lacks the required descriptive failure
message; update the assertion at the end of the Walk block (the
s.Require().NoError(err) line) to include a clear context string (e.g., "failed
walking FeeTimeoutQueue" or similar) so the message explains what operation
failed when the assertion fails, referencing FeeTimeoutQueue.Walk and the err
variable.
- Around line 2206-2217: The NoError assertion after calling
s.k.FeeTimeoutQueue.Walk (the s.Require().NoError(err) immediately following the
Walk) is missing a descriptive failure message; update that call to include a
clear message (e.g., "Walk over FeeTimeoutQueue returned unexpected error") so
test failures explain the context, keeping the rest of the Walk callback logic
and the subsequent assertions unchanged.
---
Nitpick comments:
In `@keeper/reconcile_test.go`:
- Around line 1510-1522: Remove the debug logging calls that print the vault
balance and events (the s.T().Logf lines around endVault, the loop over events
from normalizeEvents(s.ctx.EventManager().Events()), and their inner s.T().Logf
calls); keep the balance computation and assertion using expectedVaultBalance
and endVault.Amount as-is so the test remains deterministic and noise-free.
Ensure no other logging is left behind in the same block (only remove the debug
Logf calls, not the calls to normalizeEvents, events variable, or the final
s.Require().Equal assertion).
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
keeper/reconcile_test.go (1)
254-255: Add descriptive messages to assertions in composite test cases.Several
NoErrorandTrueassertions in this test function are missing descriptive failure messages, which makes debugging test failures harder. Examples:
- Line 255:
s.Require().NoError(err)- Line 267:
s.Require().True(foundReconcile)- Line 289, 322, 353, 370, 400: Similar missing messages
As per coding guidelines
**/*_test.go: "Every assertion in tests must include a descriptive failure message that provides context".Example fix for a few assertions
err := s.k.TestAccessor_reconcileVault(s.T(), s.ctx, vault) - s.Require().NoError(err) + s.Require().NoError(err, "reconcileVault should not error for full collection case") // ... - s.Require().True(foundReconcile) + s.Require().True(foundReconcile, "expected EventVaultReconcile to be emitted")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/reconcile_test.go` around lines 254 - 255, Update the assertions in the reconcile test function that call s.Require().NoError(err) and s.Require().True(foundReconcile) (and the other NoError/True assertions around those spots) to include descriptive failure messages that provide context about what was being asserted (e.g., which vault or reconcile attempt, expected outcome, or what variable indicates failure). Locate the calls invoked via s.k.TestAccessor_reconcileVault and the surrounding assertions in the test function and replace the bare assertions with versions that pass a clear message string (and optional formatted context) so any failure explains what operation failed and relevant identifiers/state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@keeper/reconcile_test.go`:
- Around line 1849-1857: The NoError assertion after calling
s.k.FeeTimeoutQueue.Walk lacks a descriptive failure message; update the
assertion s.Require().NoError(err) to include a clear message (e.g.,
"FeeTimeoutQueue.Walk returned error" or similar) so failures indicate which
operation failed while still referencing the same call site
(s.k.FeeTimeoutQueue.Walk / s.Require().NoError).
- Around line 2204-2218: The test calls s.k.FeeTimeoutQueue.Walk and asserts no
error with s.Require().NoError(err) but lacks a descriptive failure message;
update the assertion to include a clear message (e.g., "FeeTimeoutQueue.Walk
returned error" or "Walk through FeeTimeoutQueue failed") so replace
s.Require().NoError(err) with s.Require().NoError(err, "<descriptive message>")
to satisfy the test guideline and keep the rest of the Walk logic unchanged.
---
Nitpick comments:
In `@keeper/reconcile_test.go`:
- Around line 254-255: Update the assertions in the reconcile test function that
call s.Require().NoError(err) and s.Require().True(foundReconcile) (and the
other NoError/True assertions around those spots) to include descriptive failure
messages that provide context about what was being asserted (e.g., which vault
or reconcile attempt, expected outcome, or what variable indicates failure).
Locate the calls invoked via s.k.TestAccessor_reconcileVault and the surrounding
assertions in the test function and replace the bare assertions with versions
that pass a clear message string (and optional formatted context) so any failure
explains what operation failed and relevant identifiers/state.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
keeper/reconcile_test.go (1)
1511-1519: Consider removing debug logging statements.These
s.T().Logfstatements appear to be debug artifacts left from development. They add noise to test output without providing value for assertion failures.🧹 Proposed cleanup
endVault := s.simApp.BankKeeper.GetBalance(s.ctx, vaultAddr, underlying.Denom) - s.T().Logf("End vault balance: %s", endVault.String()) events := normalizeEvents(s.ctx.EventManager().Events()) - for _, ev := range events { - s.T().Logf("Event: %s", ev.Type) - for _, attr := range ev.Attributes { - s.T().Logf(" %s: %s", attr.Key, attr.Value) - } - } - expectedVaultBalance := underlying.Amount.Add(smallPrincipal.Amount)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/reconcile_test.go` around lines 1511 - 1519, Remove the leftover debug logging calls (the s.T().Logf lines that print "End vault balance" and iterate events) in the test; instead rely on assertions and use normalizeEvents(s.ctx.EventManager().Events()) only to collect events for assertions, keeping the s.ctx.EventManager().Events() usage and event iteration logic but dropping the s.T().Logf debug prints to avoid noisy test output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@keeper/reconcile_test.go`:
- Around line 1511-1519: Remove the leftover debug logging calls (the s.T().Logf
lines that print "End vault balance" and iterate events) in the test; instead
rely on assertions and use normalizeEvents(s.ctx.EventManager().Events()) only
to collect events for assertions, keeping the s.ctx.EventManager().Events()
usage and event iteration logic but dropping the s.T().Logf debug prints to
avoid noisy test output.
Taztingo
reviewed
Mar 24, 2026
Collaborator
Author
|
@coderabbitai full review. Check PR description and confirm it still matches implementation. |
🧩 Analysis chainI'll check the key implementation details against the PR description before triggering the full review. 🏁 Script executed: Length of output: 8123
✅ Description Matches Implementation
|
| Claim in Description | Actual Implementation | Recommendation |
|---|---|---|
"the reconcileVaultInterest function is called" (Transactional section) |
Function was renamed to reconcileVault — all 8 call sites use the new name |
Update PR description: replace reconcileVaultInterest with reconcileVault |
Issues #16/#17 reference tiered AUM rates (e.g., 7 bps for 0–100M, 5 bps for 100M–1B) |
Implementation uses a flat 15 bps rate with no tier logic | Clarify in the PR body that tiered rates are deferred; this PR implements a flat rate as the first step |
The two items above are the only meaningful gaps. The rest of the description accurately reflects the code. The full review is underway to catch any remaining implementation-level concerns.
🧠 Learnings used
Learnt from: nullpointer0x00
Repo: ProvLabs/vault PR: 178
File: types/keys.go:0-0
Timestamp: 2026-03-19T14:38:12.672Z
Learning: In ProvLabs/vault, the AUM fee collection address (types/keys.go) is managed as a prefix-agnostic raw byte array `AUMFeeAddress = sdk.AccAddress{...}` rather than chain-ID-based Bech32 parsing, to avoid SDK global Bech32 HRP coupling. The address is persisted in module state via `AUMFeeAddressKeyPrefix` (prefix 8). A future PR plans to migrate this to a formal Params structure so the address can be updated via governance proposals. The chain-ID switching function `GetProvLabsFeeAddress` is expected to be removed or scoped to genesis bootstrapping only.
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.
Learnt from: nullpointer0x00
Repo: ProvLabs/vault PR: 116
File: keeper/reconcile_test.go:1057-1101
Timestamp: 2025-12-08T21:45:16.340Z
Learning: In the vault codebase, the price engine has special handling for "uylds.fcc": if either the vault's PaymentDenom or the underlyingAsset equals "uylds.fcc", the price engine assumes a 1:1 equivalence between them without requiring NAV setup. This means tests using "uylds.fcc" as paymentDenom don't need SetNetAssetValue calls. Reference: https://github.com/ProvLabs/vault/issues/73
✅ Actions performed
Full review triggered.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
keeper/vault.go (1)
91-108:⚠️ Potential issue | 🔴 CriticalThis path can still return after partially creating the vault.
SetVaultLookupon Line 91 andSetAccounton Line 104 happen before the duplicate-account checks finish and beforeSafeEnqueueFeeTimeouton Line 106. Any error on Line 98, Line 100, or Line 106 leaves orphan state behind. Run the whole create flow in aCacheContextand commit only after the enqueue succeeds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/vault.go` around lines 91 - 108, The create flow currently writes persistent state early (SetVaultLookup, AuthKeeper.SetAccount) before later checks and SafeEnqueueFeeTimeout, which can leave orphan state on error; wrap the entire sequence in a CacheContext (use ctx.CacheContext()) so you perform SetVaultLookup, the duplicate-account checks, AuthKeeper.NewAccount()/AuthKeeper.SetAccount, and SafeEnqueueFeeTimeout against the cached context, and only on success commit the cached context to the main state (call Commit/Write on the cache) otherwise discard the cache to avoid partial creation.types/keys.go (1)
25-33:⚠️ Potential issue | 🟠 MajorThis constant-based fee policy still cannot satisfy the linked issue scope.
With a single
AUMFeeRateand oneAUMFeeAddress, the module still cannot express the per-vault AUM bands and multi-recipient payouts described in#16/#17. If this PR is meant to close those issues, the fee policy needs to move out of module constants and into vault config or params.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@types/keys.go` around lines 25 - 33, The fee policy is hardcoded as AUMFeeRate and AUMFeeAddress, which prevents per-vault AUM bands and multi-recipient payouts; replace these module-level constants by moving fee configuration into Vault-level config and/or module params: add fee-related fields to your Vault state type (e.g., per-vault AUM bands and recipient slices) and expose module params for defaults, remove or deprecate AUMFeeRate/AUMFeeAddress constants, and update the keeper/handler code that currently references AUMFeeRate/AUMFeeAddress to read the fee policy from the vault config or params instead.
🧹 Nitpick comments (6)
simulation/operations_test.go (1)
39-39: LGTM!Setting a non-zero block time in
SetupTestenables time-dependent features (like AUM fee scheduling) to work correctly in simulation tests.Note: Lines 96 and 178 still call
s.ctx = s.ctx.WithBlockTime(time.Now()), which is now redundant sinceSetupTestalready initializes the context with a block time. Consider removing those lines for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simulation/operations_test.go` at line 39, SetupTest now initializes s.ctx with a non-zero block time, so the subsequent redundant calls that reassign s.ctx using s.ctx = s.ctx.WithBlockTime(time.Now()) should be removed; locate the two occurrences of the exact statement "s.ctx = s.ctx.WithBlockTime(time.Now())" in the test file and delete them to avoid unnecessary context resets while keeping SetupTest's initial context setup (the line using s.app.BaseApp.NewContext(false).WithBlockTime(time.Now())) unchanged.keeper/queue_test.go (1)
118-163: Consider merging the enqueue-timeout tests into one table-driven helper flow.This new test is solid, but it largely duplicates the two neighboring tests’ setup/verification patterns. Consolidating these three into one table-driven test with shared helpers will reduce drift and maintenance cost.
As per coding guidelines: "Use table-driven test patterns for unit and integration tests to ensure exhaustive coverage of edge cases" and "Abstract repeated test operations (e.g., account setup, vault creation, state assertions) into reusable test helper functions".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/queue_test.go` around lines 118 - 163, TestSafeEnqueueFeeTimeout_UpdatesVaultAndQueues duplicates setup/assertions from neighboring tests; refactor into a table-driven test helper that covers the different enqueue-timeout scenarios. Create a helper (e.g., setupVaultAndContext) to initialize the VaultAccount, enqueue preconditions, and set block time, then write a single table-driven test that iterates cases and calls SafeEnqueueFeeTimeout, inspects FeeTimeoutQueue via FeeTimeoutQueue.Walk, and asserts VaultAccount fields (FeePeriodStart, FeePeriodTimeout) using the helper for shared assertions; update TestSafeEnqueueFeeTimeout_UpdatesVaultAndQueues to use that helper or merge it into the new table-driven test to remove duplicated logic.types/vault.go (1)
195-200: Validation logic is acceptable but could benefit from a clarifying comment.The complex condition at line 198 handles the edge case where proto3 unmarshaling can produce a zero-value coin with empty denom. While the logic is correct (allowing empty denom only when amount is nil or zero), a brief inline comment explaining this would improve maintainability.
💡 Optional: Add clarifying comment
if !v.OutstandingAumFee.Amount.IsNil() && v.OutstandingAumFee.IsNegative() { return fmt.Errorf("outstanding AUM fee cannot be negative: %s", v.OutstandingAumFee) } + // Allow empty denom only when amount is nil or zero (proto3 default state); + // otherwise denom must match PaymentDenom. if (v.OutstandingAumFee.Denom != "" || (!v.OutstandingAumFee.Amount.IsNil() && !v.OutstandingAumFee.Amount.IsZero())) && v.OutstandingAumFee.Denom != v.PaymentDenom { return fmt.Errorf("outstanding AUM fee denom %s does not match payment denom %s", v.OutstandingAumFee.Denom, v.PaymentDenom) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@types/vault.go` around lines 195 - 200, Add a brief inline comment above the validation block that explains the proto3 unmarshaling edge case being handled: that a zero-value Coin may be produced with an empty Denom, so the condition using v.OutstandingAumFee.Denom, v.OutstandingAumFee.Amount.IsNil() and v.OutstandingAumFee.Amount.IsZero() intentionally allows an empty denom only when the amount is nil or zero; place this comment near the v.OutstandingAumFee validation (the if checks referencing OutstandingAumFee, OutstandingAumFee.Denom, Amount.IsNil, Amount.IsZero, and IsNegative) so future readers understand the rationale.queue/fee_timeout_test.go (1)
104-110: Assert that the unrelated vault entry survives the removal.This walk only proves
a1disappeared. IfRemoveAllForVaultaccidentally clears the whole queue, the test still passes because the callback never seesa1. Also assert that the(200, a2)entry is still present after the removal.🧪 Suggested test tightening
require.NoError(t, q.RemoveAllForVault(ctx, a1), "remove all fee timeouts for a1 should succeed") + seenA2 := false err := q.Walk(ctx, func(timestamp uint64, address sdk.AccAddress) (bool, error) { require.False(t, address.Equals(a1), "fee timeout queue should not include any entries for a1 after removal") + if address.Equals(a2) && timestamp == 200 { + seenA2 = true + } return false, nil }) require.NoError(t, err, "walking the fee timeout queue after removal should not error") + require.True(t, seenA2, "remove all for a1 should preserve the remaining a2 entry")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@queue/fee_timeout_test.go` around lines 104 - 110, The test currently only verifies that a1 is gone after calling RemoveAllForVault but doesn't assert that other entries remain; update the Walk callback used after q.RemoveAllForVault(ctx, a1) to track whether the expected (200, a2) entry is seen and assert that it is present (e.g., set foundA2 when timestamp == 200 && address.Equals(a2)), while still asserting address does not equal a1, and after Walk assert foundA2 is true and err is nil so the test ensures the unrelated vault entry survives removal; refer to RemoveAllForVault, Walk, a1, a2 and timestamp 200 to locate where to add the check.keeper/reconcile.go (2)
540-552: Reschedule operations are non-atomic and could leave orphaned state.If
Dequeuesucceeds butSetVaultAccountfails, or ifSetVaultAccountsucceeds butEnqueuefails, the vault's timeout state becomes inconsistent with the queue. Consider wrapping these three operations in aCacheContextsimilar toatomicallyReconcileInterest.cacheCtx, write := ctx.CacheContext() if err := k.PayoutTimeoutQueue.Dequeue(cacheCtx, oldTimeout, vault.GetAddress()); err != nil { ctx.Logger().Error("failed to dequeue old interest timeout", ...) return } vault.PeriodTimeout = newTimeout if err := k.SetVaultAccount(cacheCtx, vault); err != nil { ctx.Logger().Error("failed to update vault timeout", ...) return } if err := k.PayoutTimeoutQueue.Enqueue(cacheCtx, newTimeout, vault.GetAddress()); err != nil { ctx.Logger().Error("failed to enqueue new interest timeout", ...) return } write()The same pattern applies to
rescheduleFeeTimeoutat lines 713-725.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/reconcile.go` around lines 540 - 552, The rescheduleInterestTimeout function mutates three pieces of state (PayoutTimeoutQueue.Dequeue, SetVaultAccount, PayoutTimeoutQueue.Enqueue) non-atomically; wrap these operations in a CacheContext to ensure atomicity: create cacheCtx, perform Dequeue on cacheCtx, set vault.PeriodTimeout and call SetVaultAccount on cacheCtx, then Enqueue on cacheCtx, log and return on any error, and call write() only after all succeed; apply the same change to rescheduleFeeTimeout so both rescheduling flows are atomic.
74-76: Wrap error returned fromSafeAddVerification.The error from
SafeAddVerificationis returned without context wrapping, inconsistent with other error paths in this function.🛠️ Suggested fix
if err := k.SafeAddVerification(cacheCtx, v); err != nil { - return err + return fmt.Errorf("safe add verification: %w", err) }As per coding guidelines
**/*.go: "Always wrap errors with context using fmt.Errorf with %w verb".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keeper/reconcile.go` around lines 74 - 76, The call to k.SafeAddVerification(cacheCtx, v) returns an error that is returned directly; update this to wrap the error with context using fmt.Errorf and %w (e.g., return fmt.Errorf("failed to add verification for %v: %w", v, err)) so it matches other error paths—locate the k.SafeAddVerification invocation in reconcile.go and replace the bare return err with a wrapped error that includes identifying context (cacheCtx/v or verification ID).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@keeper/genesis.go`:
- Around line 111-115: The current ExportGenesis code calls
k.AUMFeeAddress.Get(ctx) and swallows all errors; instead, first call
k.AUMFeeAddress.Has(ctx) to differentiate a missing key from a read failure: if
Has returns false, leave aumFeeAddress as an empty string, but if Has returns
true then call k.AUMFeeAddress.Get(ctx) and if that Get returns an error
return/propagate the error (do not ignore it). Update the ExportGenesis flow to
follow this Has->Get->propagate pattern so missing keys and real read errors are
handled distinctly.
In `@keeper/vault.go`:
- Around line 192-193: Update the error message returned when k.reconcileVault
fails to use a neutral wrapper: replace the current fmt.Errorf("failed to
reconcile vault interest: %w", err) with a more general message such as
fmt.Errorf("failed to reconcile vault: %w", err) so the error accurately
reflects that reconcileVault handles both interest and AUM-fee work; locate the
call site where k.reconcileVault(ctx, vault) is invoked and adjust the
fmt.Errorf text accordingly.
---
Duplicate comments:
In `@keeper/vault.go`:
- Around line 91-108: The create flow currently writes persistent state early
(SetVaultLookup, AuthKeeper.SetAccount) before later checks and
SafeEnqueueFeeTimeout, which can leave orphan state on error; wrap the entire
sequence in a CacheContext (use ctx.CacheContext()) so you perform
SetVaultLookup, the duplicate-account checks,
AuthKeeper.NewAccount()/AuthKeeper.SetAccount, and SafeEnqueueFeeTimeout against
the cached context, and only on success commit the cached context to the main
state (call Commit/Write on the cache) otherwise discard the cache to avoid
partial creation.
In `@types/keys.go`:
- Around line 25-33: The fee policy is hardcoded as AUMFeeRate and
AUMFeeAddress, which prevents per-vault AUM bands and multi-recipient payouts;
replace these module-level constants by moving fee configuration into
Vault-level config and/or module params: add fee-related fields to your Vault
state type (e.g., per-vault AUM bands and recipient slices) and expose module
params for defaults, remove or deprecate AUMFeeRate/AUMFeeAddress constants, and
update the keeper/handler code that currently references
AUMFeeRate/AUMFeeAddress to read the fee policy from the vault config or params
instead.
---
Nitpick comments:
In `@keeper/queue_test.go`:
- Around line 118-163: TestSafeEnqueueFeeTimeout_UpdatesVaultAndQueues
duplicates setup/assertions from neighboring tests; refactor into a table-driven
test helper that covers the different enqueue-timeout scenarios. Create a helper
(e.g., setupVaultAndContext) to initialize the VaultAccount, enqueue
preconditions, and set block time, then write a single table-driven test that
iterates cases and calls SafeEnqueueFeeTimeout, inspects FeeTimeoutQueue via
FeeTimeoutQueue.Walk, and asserts VaultAccount fields (FeePeriodStart,
FeePeriodTimeout) using the helper for shared assertions; update
TestSafeEnqueueFeeTimeout_UpdatesVaultAndQueues to use that helper or merge it
into the new table-driven test to remove duplicated logic.
In `@keeper/reconcile.go`:
- Around line 540-552: The rescheduleInterestTimeout function mutates three
pieces of state (PayoutTimeoutQueue.Dequeue, SetVaultAccount,
PayoutTimeoutQueue.Enqueue) non-atomically; wrap these operations in a
CacheContext to ensure atomicity: create cacheCtx, perform Dequeue on cacheCtx,
set vault.PeriodTimeout and call SetVaultAccount on cacheCtx, then Enqueue on
cacheCtx, log and return on any error, and call write() only after all succeed;
apply the same change to rescheduleFeeTimeout so both rescheduling flows are
atomic.
- Around line 74-76: The call to k.SafeAddVerification(cacheCtx, v) returns an
error that is returned directly; update this to wrap the error with context
using fmt.Errorf and %w (e.g., return fmt.Errorf("failed to add verification for
%v: %w", v, err)) so it matches other error paths—locate the
k.SafeAddVerification invocation in reconcile.go and replace the bare return err
with a wrapped error that includes identifying context (cacheCtx/v or
verification ID).
In `@queue/fee_timeout_test.go`:
- Around line 104-110: The test currently only verifies that a1 is gone after
calling RemoveAllForVault but doesn't assert that other entries remain; update
the Walk callback used after q.RemoveAllForVault(ctx, a1) to track whether the
expected (200, a2) entry is seen and assert that it is present (e.g., set
foundA2 when timestamp == 200 && address.Equals(a2)), while still asserting
address does not equal a1, and after Walk assert foundA2 is true and err is nil
so the test ensures the unrelated vault entry survives removal; refer to
RemoveAllForVault, Walk, a1, a2 and timestamp 200 to locate where to add the
check.
In `@simulation/operations_test.go`:
- Line 39: SetupTest now initializes s.ctx with a non-zero block time, so the
subsequent redundant calls that reassign s.ctx using s.ctx =
s.ctx.WithBlockTime(time.Now()) should be removed; locate the two occurrences of
the exact statement "s.ctx = s.ctx.WithBlockTime(time.Now())" in the test file
and delete them to avoid unnecessary context resets while keeping SetupTest's
initial context setup (the line using
s.app.BaseApp.NewContext(false).WithBlockTime(time.Now())) unchanged.
In `@types/vault.go`:
- Around line 195-200: Add a brief inline comment above the validation block
that explains the proto3 unmarshaling edge case being handled: that a zero-value
Coin may be produced with an empty Denom, so the condition using
v.OutstandingAumFee.Denom, v.OutstandingAumFee.Amount.IsNil() and
v.OutstandingAumFee.Amount.IsZero() intentionally allows an empty denom only
when the amount is nil or zero; place this comment near the v.OutstandingAumFee
validation (the if checks referencing OutstandingAumFee,
OutstandingAumFee.Denom, Amount.IsNil, Amount.IsZero, and IsNegative) so future
readers understand the rationale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46fbc91c-4b9b-4af6-a036-42d15800b506
⛔ Files ignored due to path filters (3)
types/events.pb.gois excluded by!**/*.pb.gotypes/genesis.pb.gois excluded by!**/*.pb.gotypes/vault.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (49)
.changelog/unreleased/features/178-aum-fee-schedule.md.changelog/unreleased/improvements/131-withdrawal-atomicity.md.gitignoreCONTRIBUTING.mdGEMINI.mdapi/provlabs/vault/v1/events.pulsar.goapi/provlabs/vault/v1/genesis.pulsar.goapi/provlabs/vault/v1/vault.pulsar.gointerest/interest.gointerest/interest_test.gokeeper/abci.gokeeper/export_test.gokeeper/genesis.gokeeper/genesis_test.gokeeper/keeper.gokeeper/migrations.gokeeper/msg_server.gokeeper/payout.gokeeper/payout_test.gokeeper/query_server.gokeeper/query_server_test.gokeeper/queue.gokeeper/queue_test.gokeeper/reconcile.gokeeper/reconcile_test.gokeeper/state.gokeeper/suite_test.gokeeper/valuation_engine.gokeeper/valuation_engine_test.gokeeper/vault.gokeeper/vault_test.goproto/generate.shproto/provlabs/vault/v1/events.protoproto/provlabs/vault/v1/genesis.protoproto/provlabs/vault/v1/vault.protoqueue/fee_timeout.goqueue/fee_timeout_test.goqueue/payout_timeout.goqueue/payout_timeout_test.gosimapp/test_helpers.gosimulation/operations_test.gospec/01_concepts.mdspec/06_blocker.mdtypes/events.gotypes/genesis.gotypes/genesis_test.gotypes/keys.gotypes/vault.gotypes/vault_test.go
💤 Files with no reviewable changes (1)
- proto/generate.sh
Taztingo
reviewed
Mar 26, 2026
keeper/reconcile.go
Outdated
|
|
||
| if !toCollect.IsZero() { | ||
| if err := k.BankKeeper.SendCoins( | ||
| markertypes.WithBypass(ctx), |
Collaborator
Author
There was a problem hiding this comment.
Any requirements to the tech fee account which will always be ran by us. This might be a question fro @iramiller if this is appropriate.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AUM Fee System Summary
This PR implements a 15 bps (0.15% annual) technology fee for tokenized vaults. The fee is calculated based on the Total Vault Value (TVV) and collected from the vault's principal marker account.
Core Logic
UnderlyingAsset.PaymentDenom.Implementation Mechanics
1. Reconciliation Triggers
The fee system utilizes a "Check-and-Collect" pattern triggered in two ways:
FeeTimeoutQueuemanages periodic fee collection (defaulting to theAutoReconcileTimeout).reconcileVaultInterestfunction is called. This forces an immediate fee calculation and collection for the period elapsed since the last reconciliation.2. Deferred Collection (Outstanding Fees)
If the principal marker account has insufficient liquidity in the
PaymentDenomto cover the calculated fee:OutstandingAumFeefield on theVaultAccount.State Matrix
OutstandingAumFee.Summary by CodeRabbit
closes: #17
closes: #16
Related Issues
2 - Includes fix for #131 (Ensures atomicity in withdrawal processing via CacheContext)
3 - Closes #131