keeper: v17 config offset, crank wire, fee split + bucket recovery - #394
Conversation
…eck (#347) MonitorService is the keeper's only off-chain fund-conservation tripwire. Its file header states the purpose: if the on-chain SPL balance falls below what the program thinks is in the vault, funds have leaked and we alert immediately. For v17 markets it bailed out BEFORE that check and recorded ok:true, with vaultTokenBalance/engineVault/shortfall all "0". Since the v17 cutover every production market is v17, so the invariant never ran anywhere — and /health's `invariants` block actively showed green for a check that was never computed. The ADL-staleness skip beside it is legitimate (ExecuteAdl was removed in v17). This one is not: v17 vaults still hold SPL tokens and the program still tracks accounting; only the source field changed (engine.vault is a v12 field). The code answered "the old formula doesn't apply" with "report healthy". Makes the unevaluated state representable and reports it: ok: boolean | null — null = NOT EVALUATED, no conclusion available balances null rather than "0" (a dashboard reading shortfall "0" concludes "checked, nothing missing" — the same false green in another field) Scope, stated plainly: this does NOT implement the v17 invariant. Deriving it (insurance fund + Σ portfolio capital/pnl backing vs getTokenAccountBalance) needs the v17 accounting model, and getting it subtly wrong would emit false shortfall alerts on a fund-leak detector — which trains on-call to ignore it, arguably worse than silence. That work stays open on #347 and wants someone with the v17 accounting model. What this fixes is the false signal. Safe to change: /health embeds monitorService.getStatus() verbatim and does not gate its status code on invariants[].ok, so no market flips the endpoint to 503. src/index.ts is the only consumer outside this file. Updates the existing v17 test, which asserted ok:true and so codified the bug. Adds two regressions per the issue: a v17 market must never report ok:true, and must not carry fabricated zero balances. Verified against the ORIGINAL monitor.ts, not a hand-rolled mutation: 3 of the 4 tests fail on the unfixed file and all 4 pass on the fix. Full keeper suite: 977 passed, 0 failures. tsc --noEmit clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…, not 432
`v17-risk.ts` hardcoded `V17_WRAPPER_CONFIG_LEN = 432`. The deployed wrapper
has moved twice since:
432 -> 496 protocol-fee change (authority + accrued/withdrawn counters)
496 -> 576 fee-collection split (4x u128 counters, 3x u16 shares, padding)
The wrapper config sits between the 16-byte account header and the market-group
header, so every derived offset past it was 144 bytes early. The keeper was
parsing maintenance_margin_bps, hMin, hMax, liquidation_fee_bps and
min_nonzero_mm_req out of the middle of the wrapper config's oracle-leg arrays
— on live markets, today, independent of any fee-split work.
Rather than change 432 to 576 (which is how we got here), the keeper stops
owning the number. New `lib/v17-layout.ts` imports it from `@percolatorct/sdk`,
which derives it from the program's own `WRAPPER_CONFIG_LEN` — pinned there by
a compile-time `assert!(size_of::<WrapperConfigV16>() == WRAPPER_CONFIG_LEN)`.
This is the pattern percolator-indexer already uses.
Importing alone is not sufficient: the previously pinned SDK (3.0.0) exports
432 itself, so the bug would have survived the refactor untouched. Two things
close that:
- the SDK is repinned to 4.1.0 (576, plus the tag 78/84/87/89 encoders);
- `assertV17LayoutGeneration()` runs at boot and refuses to start when the
SDK's layout generation does not match the deployed wrapper. Offset reads
have no discriminator and no error return — a wrong offset yields plausible
garbage — so a stale pin must be a startup crash, not a silent misparse.
Also found two MORE stale copies, both in tests that hardcoded the *derived*
448 (which is why grepping for 432 missed them):
- `issue-335-health-divergence.poc.test.ts` built fixtures at 448-based
offsets, matching the equally-wrong production constant. Both sides shared
the bug, so the test passed while the evaluator misread every real account.
- `issue-331-per-asset-price.poc.test.ts` was a tautology: it computed an
offset from its own copied constants and asserted the result equalled those
same constants re-added by hand. It could not fail regardless of what the
keeper did. It now drives the production functions.
Struct sub-offsets that the SDK does not export are derived here from the
engine's `#[repr(C)]` definitions, and each block is asserted against an
independently-sourced length (V17_MARKET_GROUP_LEN 758, V17_MARKET_ASSET_SLOT_LEN
1797). Two independent derivations agreeing is what caught the original bug.
Test suites that fully replaced the `@percolatorct/sdk` mock are converted to
partial mocks via `importOriginal`. A full-replacement mock breaks whenever the
code under test imports a new SDK export, and surfaces as an unrelated
"no export defined on the mock" at import time.
Verification: tsc --noEmit clean; full suite 1030 passed / 0 failed; layout
offsets cross-checked against the SDK's independent parseMarketGroupV17OI
parser (a second implementation by a different author).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDXvFwXmYuz5cbJYyczo7u
…_q/fee_bps removed) The SDK repin surfaced this as a compile error: `PermissionlessCrankArgs` no longer accepts `closeQ` or `feeBps`. Upstream #206 removed both from the wire — the program derives the close size from on-chain portfolio state at execution time rather than trusting a caller-supplied quantity, and rejects a nonzero funding rate outright. The substantive case is #329 here in liquidation.ts: it rebuilt the crank instruction when a position had partially closed between scan and submit, so the encoded close_q matched the remaining size. That rebuild is now dead — close_q is not in the wire, so the rebuilt bytes are byte-identical to the original, and the hazard #329 addressed is handled on-chain. The fresh read is kept because the tracked value still feeds the keeper's own post-submit accounting. (The matching one-line fix at crank.ts's FeeSweep call site rides along in the following commit: it sits in the same file as the fee-crank wiring and this environment cannot split a file across commits.) OUT OF SCOPE for the fee-crank work, but the repo does not compile without it. Flagged in keeper-crank-report.md — this is the same "keeper main still encodes the old crank" desync noted in the repo-integrity audit, which means the currently-deployed keeper is sending a wire format the deployed wrapper does not accept. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDXvFwXmYuz5cbJYyczo7u
Nothing cranked any of this, so revenue accrued into pots nobody emptied and lapsed backing domains never recovered. Adds one threshold-gated loop covering four tags: tag 89 ExpireBackingBucket — lapsed bucket recovery (LIVENESS) tag 78 LpVaultCrankFees — LP fee leg (revenue) tag 87 WithdrawInsuranceReserveToStake — staker fee leg (revenue) tag 84 WithdrawProtocolFee — protocol fee leg (revenue) ONE LOOP, NOT FOUR. All four have the same shape: read a counter, act above a threshold. They differ only in which counter, which threshold, which accounts and which mode gate. Four loops would mean four copies of the isolation, classification, metrics and backoff logic — four places to drift — and four fetches of the same account per market per cycle. So there is one `CrankLeg` per tag and one driver that fetches each market ONCE and evaluates every leg against that single snapshot. TAG 89 IS THE ONE THAT IS NOT OPTIONAL. A bucket's expiry_slot is fixed when it opens and is never extended while it stays Fresh, so every backed market lapses eventually — a longer horizon defers the lapse, it does not avoid it. This is routine maintenance, not an edge case. Once lapsed the domain is a dead end in all three directions, permanently: settling a loss -> Custom(21), settling a gain -> Custom(19), TopUpBackingBucket to re-fund -> Custom(21). It cannot even be paid to come back. Tag 89 is the only exit, which is why it runs first in the leg order (if a cycle exhausts its budget it should do so on revenue, not on user funds) and why it carries no economic threshold at all. NEVER CRANK BLIND. Every detector reads on-chain state and returns nothing when there is nothing to do; a transaction is only built for detected work. This replaces the previous crankLpVault(), which fired tag 78 at every market every cycle without reading anything — paying for a guaranteed revert whenever there were no fees, and classifying EVERY resulting failure as "no fees to crank". Authority mismatch, zero LP shares and wrong-mode all looked identical to a market with nothing to do. BY-DESIGN REJECTIONS vs REAL ERRORS. Custom 19/21/27/38/41/53/61 are counted and logged at debug as benign; everything else is a real error. Detection reads a snapshot and the chain moves, so losing a race to another keeper is an anticipated outcome of a permissionless crank, not a fault. Classification is deliberately conservative — only a RECOGNISED code is benign, so an unknown custom error, an RPC failure or a thrown exception all still alarm. FAILURE ISOLATION is per (market, leg), not per market: a market whose LP leg throws still gets its tag-89 sweep in the same cycle, and one bricked domain is not left bricked because a sibling domain's transaction happened to fail first. Metrics separate attempts / benign-rejects / errors / atoms-moved per leg per market, plus lapsed and lapsing-soon bucket gauges. The pairing is what the runbook reads: a leg attempting work but moving nothing, or a lapsed gauge that never returns to zero, otherwise looks exactly like a healthy idle keeper. OFF by default (KEEPER_FEE_CRANK_ENABLED). Three legs move real collateral and tag 89 forfeits lapsed principal to the junior pool, so enabling is an operator decision rather than a deploy side effect. Also carries the one-line PermissionlessCrank ABI fix at crank.ts's FeeSweep call site (see previous commit). Verification: tsc --noEmit clean; full suite 1030 passed / 0 failed, including 23 layout/state tests cross-checked against the SDK's independent parseMarketGroupV17OI parser and 19 crank tests covering rejection classification and per-leg failure isolation. Known gaps, detailed in keeper-crank-report.md: tags 84/87 need the market's collateral vault token account, which is not yet derived automatically (KEEPER_MARKET_VAULT_TOKEN override until then); tag 89 needs neither. No v17 markets exist on devnet yet, so end-to-end exercise against a real market is still outstanding. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDXvFwXmYuz5cbJYyczo7u
…esting the heap frame Both bugs were found by pointing this branch at the first real v17 market on the new devnet deployment (wrapper DhSkE7u..., market BPgSUbDs...). Either one alone prevented all four fee-crank legs from doing anything on chain. 1. resolveMarketVaultToken ignored its cfg argument and read a single global KEEPER_MARKET_VAULT_TOKEN env var, returning null when unset. Unset (the default) silently disabled BOTH revenue legs: tags 84 and 87 no-opped forever behind a debug line while fees accrued. Set, it is a per-PROCESS constant standing in for a per-MARKET address, so with more than one market it would submit some other market's vault — which the wrapper's F-VAULT-FRAG pin (canonical_vault_address / verify_withdrawable_token_accounts) rejects as InvalidVaultAccount(12), a guaranteed revert on every market but one. The address is deterministic, so derive it: deriveCanonicalVault(programId, market, cfg.collateralMint) reproduces the program's own derivation seed for seed. There is no Token-2022 branch to worry about — verify_token_program and unpack_token_account both pin spl_token::ID, so the ATA's middle seed is always legacy SPL Token. 2. keeperSend prepended ComputeBudgetProgram.requestHeapFrame into the SAME array it then handed to sendWithRetryKeeper, which prepends the heap frame itself (DEFAULT_KEEPER_OPTS.heapFrameBytes is always set) along with the CU limit and price. The result was two identical requestHeapFrame instructions and a runtime rejection of the entire transaction — "Transaction contains a duplicate instruction (3)" — on EVERY keeperSend path, not just the fee crank. The heap frame is still prepended for simulation, where estimateCost builds its own transaction and needs it; only the array passed to the sender is left alone. Verified on devnet against market BPgSUbDsxZ9bkauWgd6eQ8oLHVx6pSsvfAjPGsS2Sso8, all four legs sent and confirmed, then a second detection pass correctly reported zero work for all four: tag 89 ExpireBackingBucket 2saHny3UXN25tXPn4pX1CwMNHEajQHGKGXn5sFYcFH5e... tag 78 LpVaultCrankFees 5vVNGoHMkjNEfF3zvGaYXbYp2yFprYyPQpJz75TUuvsH... tag 87 InsuranceReserveStake 5nLJtHqLL9eyPRyKJW1QV9EPhoGFowQdbwEoYh4HxFCf... tag 84 WithdrawProtocolFee 5YHZ3gwajWoGtpyGXwAMG5UQn1A8FeLJ28ZbUo3edswE... pnpm test: 1030 passed | 33 skipped (99 files). tsc --noEmit clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDXvFwXmYuz5cbJYyczo7u
…ke Docker) The devnet-2.0 keeper (fee cranks + tag-89 bucket recovery) pinned the SDK as a local file link, which Railway's Docker build can't resolve. SDK 4.2.0 is now on origin (ac68ac3), so pin the github SHA — makes this branch actually deployable.
… portfolios on devnet, exceeds 120s)
📝 WalkthroughWalkthroughThis PR adds v17 layout tripwires and parsers, introduces a fee-split and backing-bucket recovery crank, updates v17 instruction encoding and monitoring semantics, and expands tests and operator configuration. Changesv17 layout and state handling
Fee-crank execution
ABI and monitoring
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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 |
Main landed fa425b5 (stop double-requesting the heap frame), which this branch also fixed in 05ea37d. The code is identical; the only conflict was the trailing comment, where main's is more explicit about WHY the original instruction array is what gets handed to sendWithRetryKeeper. Took main's wording. Build clean, 1030 tests pass after the merge. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017VkS8j4NDFV7BnpDLLjbbQ
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/lib/v17-layout.ts (1)
140-141: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider an independent-source cross-check for the new fee-crank clamp offsets, not just the aggregate-size tripwire.
assertV17LayoutGeneration()only validates that struct field offsets sum to the SDK-reported total length. That catches a field being added/removed/resized, but it is structurally blind to a transposition of two adjacent same-sized fields — e.g. ifMG_SOURCE_INSURANCE_CREDIT_RESERVED_ATOMS_OFFandMG_INSURANCE_DOMAIN_BUDGET_REMAINING_TOTAL_OFFwere swapped relative to the actual Rust struct order, the u128+u128 sum is unchanged and the tripwire would pass, while every fee-withdraw leg (78/84/87) would clamp against the wrong operand.The test suite already has a precedent for this stronger check:
MG_INSURANCE_OFFis asserted against 301 with a comment noting it "matches percolator-indexer's independent reading" — a second, independently-authored source, not just this file's own derivation. Given these two offsets now directly gate real fee-crank withdrawals, the same kind of independent cross-check (or an on-chain fixture read) would be valuable here.🤖 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 `@src/lib/v17-layout.ts` around lines 140 - 141, Add an independent validation for MG_SOURCE_INSURANCE_CREDIT_RESERVED_ATOMS_OFF and MG_INSURANCE_DOMAIN_BUDGET_REMAINING_TOTAL_OFF, following the MG_INSURANCE_OFF precedent rather than relying only on assertV17LayoutGeneration’s aggregate-size check. Assert each offset against values sourced from an independent indexer or on-chain fixture so adjacent same-sized field transpositions are detected.tests/services/fee-crank.test.ts (1)
240-256: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRestore the env var in a
finally/afterEach.If any assertion in this test throws,
KEEPER_PROTOCOL_FEE_SWEEP_MIN_ATOMSleaks into subsequent tests in the same worker and makes an unrelated failure look like a threshold bug.🤖 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 `@tests/services/fee-crank.test.ts` around lines 240 - 256, Update the “reads thresholds from env with sensible defaults” test to preserve and restore the original KEEPER_PROTOCOL_FEE_SWEEP_MIN_ATOMS value in a finally block or shared afterEach cleanup, including when assertions fail; retain the existing threshold assertions and ensure cleanup restores the prior state rather than only deleting the variable.src/services/fee-crank.ts (1)
476-481: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
parseWrapperConfigV17is re-parsed several times per market per cycle.
readFeeLegClaimsplus the two directparseWrapperConfigV17(data)calls mean the same buffer is decoded up to five times per market per cycle. Parsing once into the snapshot (alongsidegroup) would keep the single-snapshot-per-market design the file's header argues for.Also applies to: 508-510, 549-566
🤖 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 `@src/services/fee-crank.ts` around lines 476 - 481, Update the per-market snapshot flow around readFeeLegClaims and the direct parseWrapperConfigV17 calls so parseWrapperConfigV17(data) runs once per market cycle. Store the parsed configuration alongside group in the snapshot, then reuse it for fee-leg claims and the logic at the additional referenced sections, preserving existing behavior and thresholds.src/lib/keeper-send.ts (1)
231-234: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a regression test for the duplicate-instruction bug this fixes.
Given the severity described in the comment ("aborted every keeperSend path, all four fee-crank legs included"), a unit test asserting
sendWithRetryKeeperreceives exactly onerequestHeapFrameinstruction (and thatsimulationInstructionsnever leaks into the real send) would guard against this regressing silently.🤖 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 `@src/lib/keeper-send.ts` around lines 231 - 234, Extend the tests around keeperSend/sendWithRetryKeeper to cover the duplicate-instruction regression: assert the simulation path contains exactly one requestHeapFrame instruction, and verify the real send receives the original instructions without simulationInstructions leaking into it.src/services/crank.ts (1)
1804-1865: 🧹 Nitpick | 🔵 TrivialDefault configuration still pays for the reverts this change describes as wasteful.
The new comment block explains that the old
crankLpVault()fallback fires tag 78 every cycle "paying for a guaranteed revert whenever there were no fees," and thatrunFeeCrankPassfixes this by reading state first. However, sinceKEEPER_FEE_CRANK_ENABLEDdefaults to false (per fee-crank.ts'sisFeeCrankEnabled()and the PR objectives stating fee cranking is disabled by default), the!isFeeCrankEnabled()branch — the exact wasteful path being critiqued — is what actually runs in production until the flag is flipped. Worth calling out in the rollout/runbook so operators know the efficiency gain requires opting in.🤖 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 `@src/services/crank.ts` around lines 1804 - 1865, Update the rollout/runbook documentation to state that KEEPER_FEE_CRANK_ENABLED defaults to false, so the !isFeeCrankEnabled() crankLpVault fallback remains active and may incur unnecessary reverts until operators explicitly enable the fee-crank pass. Reference isFeeCrankEnabled and runFeeCrankPass behavior, and document the opt-in required to realize the efficiency improvement.src/services/liquidation.ts (1)
1203-1242: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
effectiveCloseQappears to be write-only after this block.The comment claims the fresh value "still feeds the keeper's post-submit accounting below," but scanning the rest of
liquidate()(through theeventBus.publish("liquidation.success", ...)call and the catch/finally blocks),effectiveCloseQis never read again after line 1240. If there truly is no downstream consumer, the reassignment and the now-inaccurate comment can be removed; if there is a consumer, it isn't visible in this method and the comment should point to it.🤖 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 `@src/services/liquidation.ts` around lines 1203 - 1242, Remove the `freshCloseQ`/`effectiveCloseQ` reassignment in the `liquidate()` flow and delete the outdated `#329` comment if `effectiveCloseQ` has no reads after this block. Preserve the health and liquidatability checks, and verify no downstream consumer in `liquidate()` requires the refreshed close quantity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/fee-crank.ts`:
- Around line 401-403: Wrap the detect-phase getAccountInfo RPC reads in
fee-crank, including the registry lookup near deriveLpVaultRegistry and the
corresponding tag-87 leg lookup, with the repository’s existing withTimeout
helper used by monitor.ts. Apply the timeout to each awaited read while
preserving the current null-account early returns and sequential leg behavior.
- Around line 866-905: Update runFeeCrankPass to fetch market accounts in
batches of at most 100 addresses before building snapshots, while preserving the
existing chain-slot retrieval and per-market alignment. Aggregate successful
batch results for runFeeCrankCycle, and retain the current single warning plus
empty-result behavior if any required fetch fails.
---
Nitpick comments:
In `@src/lib/keeper-send.ts`:
- Around line 231-234: Extend the tests around keeperSend/sendWithRetryKeeper to
cover the duplicate-instruction regression: assert the simulation path contains
exactly one requestHeapFrame instruction, and verify the real send receives the
original instructions without simulationInstructions leaking into it.
In `@src/lib/v17-layout.ts`:
- Around line 140-141: Add an independent validation for
MG_SOURCE_INSURANCE_CREDIT_RESERVED_ATOMS_OFF and
MG_INSURANCE_DOMAIN_BUDGET_REMAINING_TOTAL_OFF, following the MG_INSURANCE_OFF
precedent rather than relying only on assertV17LayoutGeneration’s aggregate-size
check. Assert each offset against values sourced from an independent indexer or
on-chain fixture so adjacent same-sized field transpositions are detected.
In `@src/services/crank.ts`:
- Around line 1804-1865: Update the rollout/runbook documentation to state that
KEEPER_FEE_CRANK_ENABLED defaults to false, so the !isFeeCrankEnabled()
crankLpVault fallback remains active and may incur unnecessary reverts until
operators explicitly enable the fee-crank pass. Reference isFeeCrankEnabled and
runFeeCrankPass behavior, and document the opt-in required to realize the
efficiency improvement.
In `@src/services/fee-crank.ts`:
- Around line 476-481: Update the per-market snapshot flow around
readFeeLegClaims and the direct parseWrapperConfigV17 calls so
parseWrapperConfigV17(data) runs once per market cycle. Store the parsed
configuration alongside group in the snapshot, then reuse it for fee-leg claims
and the logic at the additional referenced sections, preserving existing
behavior and thresholds.
In `@src/services/liquidation.ts`:
- Around line 1203-1242: Remove the `freshCloseQ`/`effectiveCloseQ` reassignment
in the `liquidate()` flow and delete the outdated `#329` comment if
`effectiveCloseQ` has no reads after this block. Preserve the health and
liquidatability checks, and verify no downstream consumer in `liquidate()`
requires the refreshed close quantity.
In `@tests/services/fee-crank.test.ts`:
- Around line 240-256: Update the “reads thresholds from env with sensible
defaults” test to preserve and restore the original
KEEPER_PROTOCOL_FEE_SWEEP_MIN_ATOMS value in a finally block or shared afterEach
cleanup, including when assertions fail; retain the existing threshold
assertions and ensure cleanup restores the prior state rather than only deleting
the variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ed0c128-91fe-4551-9c21-2304096b6f22
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
.env.examplepackage.jsonrailway.tomlsrc/index.tssrc/lib/keeper-send.tssrc/lib/metrics.tssrc/lib/v17-fee-state.tssrc/lib/v17-layout.tssrc/lib/v17-risk.tssrc/services/crank.tssrc/services/fee-crank.tssrc/services/liquidation.tssrc/services/monitor.tstests/lib/oracle-account.test.tstests/lib/v17-fee-state.test.tstests/lib/v17-layout.test.tstests/poc/issue-331-per-asset-price.poc.test.tstests/poc/issue-335-health-divergence.poc.test.tstests/services/crank-error-code.poc.test.tstests/services/crank-hyperp-detection.poc.test.tstests/services/crank.b-fixes.test.tstests/services/crank.processBatched.test.tstests/services/crank.test.tstests/services/fee-crank.test.tstests/services/liquidation-watchdog-race.test.tstests/services/liquidation.test.tstests/services/monitor.test.tstests/services/oracle.b-fixes.test.tstests/services/program-id-allowlist.test.tstests/v17-risk-params.poc.test.ts
| const [registryPda] = deriveLpVaultRegistry(programId, address); | ||
| const registryInfo = await ctx.connection.getAccountInfo(registryPda); | ||
| if (registryInfo === null) return []; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Wrap detect-phase RPC reads in a timeout.
getAccountInfo here (and at Line 489 in the tag-87 leg) has no timeout. Legs for a market run sequentially, so one hung RPC stalls that market's remaining legs — including the liveness-critical tag 89 work on the next cycle — while keeperSend hardening does not cover these reads. The repo already uses a withTimeout helper for exactly this in src/services/monitor.ts.
🛡️ Sketch
- const [registryPda] = deriveLpVaultRegistry(programId, address);
- const registryInfo = await ctx.connection.getAccountInfo(registryPda);
+ const [registryPda] = deriveLpVaultRegistry(programId, address);
+ const registryInfo = await withTimeout(
+ ctx.connection.getAccountInfo(registryPda),
+ FEE_CRANK_RPC_TIMEOUT_MS,
+ `getAccountInfo(lpVaultRegistry ${address.toBase58().slice(0, 8)})`,
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [registryPda] = deriveLpVaultRegistry(programId, address); | |
| const registryInfo = await ctx.connection.getAccountInfo(registryPda); | |
| if (registryInfo === null) return []; | |
| const [registryPda] = deriveLpVaultRegistry(programId, address); | |
| const registryInfo = await withTimeout( | |
| ctx.connection.getAccountInfo(registryPda), | |
| FEE_CRANK_RPC_TIMEOUT_MS, | |
| `getAccountInfo(lpVaultRegistry ${address.toBase58().slice(0, 8)})`, | |
| ); | |
| if (registryInfo === null) return []; |
🤖 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 `@src/services/fee-crank.ts` around lines 401 - 403, Wrap the detect-phase
getAccountInfo RPC reads in fee-crank, including the registry lookup near
deriveLpVaultRegistry and the corresponding tag-87 leg lookup, with the
repository’s existing withTimeout helper used by monitor.ts. Apply the timeout
to each awaited read while preserving the current null-account early returns and
sequential leg behavior.
| export async function runFeeCrankPass( | ||
| connection: Connection, | ||
| keypair: Keypair, | ||
| markets: readonly { address: PublicKey; programId: PublicKey }[], | ||
| thresholds: FeeCrankThresholds = loadFeeCrankThresholds(), | ||
| ): Promise<LegOutcome[]> { | ||
| if (markets.length === 0) return []; | ||
|
|
||
| let chainSlot: bigint; | ||
| let infos: (Awaited<ReturnType<Connection["getAccountInfo"]>>)[]; | ||
| try { | ||
| [chainSlot, infos] = await Promise.all([ | ||
| connection.getSlot().then((s) => BigInt(s)), | ||
| connection.getMultipleAccountsInfo(markets.map((m) => m.address)), | ||
| ]); | ||
| } catch (err) { | ||
| // A failed fetch is not a per-market failure — nothing was evaluated, so | ||
| // there is nothing to isolate. Report once and let the next cycle retry. | ||
| logger.warn("fee-crank pass could not load market accounts", { | ||
| markets: markets.length, | ||
| error: err instanceof Error ? err.message : String(err), | ||
| }); | ||
| return []; | ||
| } | ||
|
|
||
| const snapshots: MarketSnapshot[] = []; | ||
| for (const [i, market] of markets.entries()) { | ||
| const info = infos[i]; | ||
| if (!info) continue; | ||
| const snapshot = buildSnapshot( | ||
| market.address, | ||
| market.programId, | ||
| new Uint8Array(info.data), | ||
| chainSlot, | ||
| ); | ||
| if (snapshot !== null) snapshots.push(snapshot); | ||
| } | ||
|
|
||
| return runFeeCrankCycle(snapshots, { connection, keypair, thresholds }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the declared signature of getMultipleAccountsInfo in the installed web3.js typings.
fd -t f 'index.d.ts' node_modules/@solana/web3.js --exec rg -n -A6 'getMultipleAccountsInfo' {}Repository: dcccrypto/percolator-keeper
Length of output: 281
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Tracked package manifests mentioning `@solana/web3.js`:\n'
rg -n '"`@solana/web3`\.js"|`@solana/web3`\.js' package.json pnpm-lock.yaml package-lock.json yarn.lock 2>/dev/null || true
printf '\nRelevant source slice:\n'
sed -n '840,920p' src/services/fee-crank.ts 2>/dev/null || true
printf '\nType annotations/usages around fee-crank:\n'
rg -n "getMultipleAccountsInfo|getAccountInfo|loadFeeCrankThresholds|FeeCrankThresholds" src/services/fee-crank.tsRepository: dcccrypto/percolator-keeper
Length of output: 6055
🌐 Web query:
@solana/web3.js 1.98.4 Connection getMultipleAccountsInfo signature getMultipleAccountInfos type
💡 Result:
In @solana/web3.js version 1.98.4, the connection method is named getMultipleAccountsInfo [1]. There is no method named getMultipleAccountInfos (note the plural "s" at the end); using that name will result in a runtime error as it does not exist on the Connection class [1][2]. The signature for getMultipleAccountsInfo is as follows [1]: getMultipleAccountsInfo(publicKeys: PublicKey[], commitmentOrConfig?: Commitment | GetMultipleAccountsConfig): Promise<(null | AccountInfo)[]> Parameters: - publicKeys: An array of PublicKey objects for which to fetch account information [1]. - commitmentOrConfig (optional): Either a string representing the Commitment level (e.g., 'confirmed', 'finalized') or a GetMultipleAccountsConfig object [1][3]. The GetMultipleAccountsConfig type includes the following optional fields [3]: - commitment: Commitment - dataSlice: DataSlice (used to limit the returned account data) - minContextSlot: number If you require the transaction context alongside the account information, you should use the related method getMultipleAccountsInfoAndContext, which returns a promise resolving to an RpcResponseAndContext object containing the account data [1].
Citations:
- 1: https://solana-foundation.github.io/solana-web3.js/classes/Connection.html
- 2: https://solana-foundation.github.io/solana-web3.js/modules.html
- 3: https://solana-foundation.github.io/solana-web3.js/types/GetMultipleAccountsConfig.html
🌐 Web query:
Solana web3.js getMultipleAccountsInfo 100 public key limit
💡 Result:
The Solana RPC method getMultipleAccounts (which corresponds to getMultipleAccountsInfo in @solana/web3.js) enforces a hard limit of 100 public keys per request [1][2][3]. This restriction is implemented at the RPC node level to maintain performance and prevent abuse [4][3]. Because the web3.js library does not automatically handle batching for requests exceeding this limit [4], developers must manually manage pagination when querying more than 100 accounts [1][2][5]. The standard approach is to split your array of public keys into chunks of 100 and execute a separate getMultipleAccountsInfo call for each chunk, typically using Promise.all to fetch them concurrently [2][6][5]. Example implementation pattern: const chunkArray = (array, size) => { const result = []; for (let i = 0; i < array.length; i += size) { result.push(array.slice(i, i + size)); } return result; }; const fetchAllAccounts = async (connection, publicKeys) => { const chunks = chunkArray(publicKeys, 100); const results = await Promise.all( chunks.map((chunk) => connection.getMultipleAccountsInfo(chunk))); return results.flat; };
Citations:
- 1: Feat: update getMultipleAccountsInfo to handle 100+ accounts solana-labs/solana#23600
- 2: https://next.lorisleiva.com/paginating-and-ordering-accounts-in-solana
- 3: https://www.helius.dev/docs/rpc/guides/getmultipleaccounts
- 4: fix: Separate getMultipleAccountsInfo from its parsed flavour solana-labs/solana#22804
- 5: https://lorisleiva.com/paginating-and-ordering-accounts-in-solana
- 6: feat: allow getMultipleAccountsInfo to handle 100+ requests solana-labs/solana#23389
Avoid passing more than 100 markets into one getMultipleAccountsInfo call.
The method exists on Connection, but Solana RPC limits getMultipleAccounts to 100 public keys per request, and this code returns an empty pass on fetch failure without retrying/splitting. Add batched fetching or document/limit the max markets per pass.
🤖 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 `@src/services/fee-crank.ts` around lines 866 - 905, Update runFeeCrankPass to
fetch market accounts in batches of at most 100 addresses before building
snapshots, while preserving the existing chain-slot retrieval and per-market
alignment. Aggregate successful batch results for runFeeCrankCycle, and retain
the current single warning plus empty-result behavior if any required fetch
fails.
1 participant
Eight commits that the running keeper does not have. Three of them are correctness fixes for v17 markets; without them a freshly launched devnet-2.0 market is not reliably cranked.
f5658e63a14b05PermissionlessCrankwire aligned with upstream #206 (close_q/fee_bpsremoved)b3a969e05ea37dda6d6936541a88,65bbc40file:../broke the Docker builde44722fWhy now
The devnet-2.0 launch path is being re-tested end to end. The prior on-chain audit found that a market's health depends on the keeper cranking it correctly, and these fixes are the difference between a new market being cranked and being misread.
Note the audit's fourth blocker — one bad market silently stops every price in its batch — is not in this branch. Markets are still packed by byte size with no health filter, sent preflight-skipped, and the success counter increments on send, so the keeper reports "pushed" either way. That remains open.
Verification
pnpm buildclean, 1030 tests pass (97 files, 2 skipped).Summary by CodeRabbit
New Features
Bug Fixes
Reliability