Cap abci loop#236
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThis PR enforces per-block visit budgets across ABCI queue processors and pending swap-out processing. Paused-vault swap-outs are now dequeued and refunded with a new refund reason. Tests, specifications, and the changelog document the updated behavior. ChangesPer-block budgets and paused-vault swap-out refunds
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant BeginBlocker
participant EndBlocker
participant ReconcileHandlers
participant processPendingSwapOuts
participant processSwapOutJobs
participant refundPausedVaultSwapOut
BeginBlocker->>ReconcileHandlers: process bounded timeout queues
EndBlocker->>ReconcileHandlers: process bounded verification set
EndBlocker->>processPendingSwapOuts: process MaxSwapOutBatchSize visited entries
processPendingSwapOuts->>processSwapOutJobs: pass collected jobs
processSwapOutJobs->>refundPausedVaultSwapOut: refund paused-vault job
refundPausedVaultSwapOut-->>processSwapOutJobs: dequeue and emit vault_paused refund event
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| keeper/abci.go | Adds three budget constants and threads them as arguments to the four queue processors; straightforward and correct. |
| keeper/payout.go | Removes the inline paused-vault skip in the collection walk; adds refundPausedVaultSwapOut with correct single-cache-context atomicity (dequeue + refund commit together or not at all). |
| keeper/reconcile.go | Adds per-block visit budgets to handleVaultInterestTimeouts, handleVaultFeeTimeouts, and handleReconciledVaults; paused entries consume budget slots but remain enqueued (intentional per design). |
| keeper/payout_test.go | Adds comprehensive tests: happy-path paused-vault dequeue+refund, refund-failure path, and batch-budget starvation scenario; all match the implemented behavior. |
| keeper/reconcile_test.go | Adds per-block visit-budget tests for all three reconcile processors (zero/under/over-budget) and the paused-front starvation boundary; correctly documents the accepted asymmetry. |
| types/events.go | Adds RefundReasonVaultPaused constant; consistent with the existing refund-reason pattern. |
| keeper/suite_test.go | Adds four queue-counting helpers used across new tests. |
| keeper/export_test.go | Updates three test accessors to forward the new limit parameter; mechanical change, no issues. |
| spec/06_blocker.md | Documentation accurately reflects the new per-block budget semantics and paused-vault refund behavior for all processors. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant EB as EndBlocker
participant PSO as processPendingSwapOuts
participant PSJ as processSwapOutJobs
participant RPV as refundPausedVaultSwapOut
participant Q as PendingSwapOutQueue
participant Bank as BankKeeper
EB->>PSO: "processPendingSwapOuts(ctx, batchSize=100)"
PSO->>Q: WalkDue(now)
note over PSO,Q: All entries count against budget (paused or not)
PSO->>PSJ: processSwapOutJobs(ctx, jobsToProcess)
alt vault not found
PSJ->>Q: Dequeue and skip
else vault.Paused
PSJ->>RPV: refundPausedVaultSwapOut(ctx, job)
RPV->>RPV: "cacheCtx, write = ctx.CacheContext()"
RPV->>Q: Dequeue(cacheCtx)
alt Dequeue fails
RPV-->>PSJ: log error, entry stays queued
else Dequeue succeeds
RPV->>Bank: refundWithdrawal(cacheCtx, vault_paused)
alt refund fails
RPV-->>PSJ: log error, cache discarded, entry stays queued
else refund succeeds
RPV->>RPV: write() commit atomically
note over RPV: EventSwapOutRefunded emitted
end
end
else vault active
PSJ->>PSJ: processSingleWithdrawal normal path
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant EB as EndBlocker
participant PSO as processPendingSwapOuts
participant PSJ as processSwapOutJobs
participant RPV as refundPausedVaultSwapOut
participant Q as PendingSwapOutQueue
participant Bank as BankKeeper
EB->>PSO: "processPendingSwapOuts(ctx, batchSize=100)"
PSO->>Q: WalkDue(now)
note over PSO,Q: All entries count against budget (paused or not)
PSO->>PSJ: processSwapOutJobs(ctx, jobsToProcess)
alt vault not found
PSJ->>Q: Dequeue and skip
else vault.Paused
PSJ->>RPV: refundPausedVaultSwapOut(ctx, job)
RPV->>RPV: "cacheCtx, write = ctx.CacheContext()"
RPV->>Q: Dequeue(cacheCtx)
alt Dequeue fails
RPV-->>PSJ: log error, entry stays queued
else Dequeue succeeds
RPV->>Bank: refundWithdrawal(cacheCtx, vault_paused)
alt refund fails
RPV-->>PSJ: log error, cache discarded, entry stays queued
else refund succeeds
RPV->>RPV: write() commit atomically
note over RPV: EventSwapOutRefunded emitted
end
end
else vault active
PSJ->>PSJ: processSingleWithdrawal normal path
end
Reviews (3): Last reviewed commit: "fix conflicts" | Re-trigger Greptile
There was a problem hiding this comment.
🧹 Nitpick comments (1)
keeper/payout_test.go (1)
356-398: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffSetup block duplicates existing swap-in/escrow boilerplate.
This new case repeats the same
CreateAndFundAccount/setupBaseVault/SwapIn/escrow/enqueue sequence already present in several sibling table entries in this test. As per path instructions,**/*_test.go: "If a setup block (marker creation, vault setup, funding) appears in three or more tests, it MUST be extracted into a helper insuite_test.go." This pattern already recurs well beyond 3 times in this table; consider extracting a sharedsetupPendingSwapOut(shareDenom, assets)helper as a follow-up cleanup (not necessarily scoped to this PR).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@keeper/payout_test.go` around lines 356 - 398, The setup block in this table-driven case duplicates the same swap-in, escrow, and enqueue boilerplate used by several sibling tests. Refactor the repeated logic in this `payout_test.go` case by extracting the shared account funding, `setupBaseVault`, `SwapIn`, escrow, and `PendingSwapOutQueue.Enqueue` flow into a helper in `suite_test.go` (for example, a shared pending-swap-out setup helper). Keep the test case focused on the paused-vault behavior and reuse the helper from this case and the other matching table entries.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@keeper/payout_test.go`:
- Around line 356-398: The setup block in this table-driven case duplicates the
same swap-in, escrow, and enqueue boilerplate used by several sibling tests.
Refactor the repeated logic in this `payout_test.go` case by extracting the
shared account funding, `setupBaseVault`, `SwapIn`, escrow, and
`PendingSwapOutQueue.Enqueue` flow into a helper in `suite_test.go` (for
example, a shared pending-swap-out setup helper). Keep the test case focused on
the paused-vault behavior and reuse the helper from this case and the other
matching table entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 21583ec3-e8f4-4b46-975d-754498fc1102
📒 Files selected for processing (12)
.changelog/unreleased/bug-fixes/225-cap-abci-loop.mdkeeper/abci.gokeeper/export_test.gokeeper/payout.gokeeper/payout_test.gokeeper/reconcile.gokeeper/reconcile_test.gokeeper/suite_test.gospec/03_messages.mdspec/04_events.mdspec/06_blocker.mdtypes/events.go
Taztingo
left a comment
There was a problem hiding this comment.
LGTM. Great improvement.
closes: #225
Summary by CodeRabbit
vault_pausedreason.