fix(hodlmm-flow): SWAP_FUNCTIONS coverage + liquidation + 429 partial results [blocking #348]#350
Conversation
… partial results (aibtcdev#348) - Replace SWAP_FUNCTIONS with all 8 live swap-* entrypoints on dlmm-swap-router-v-1-1; remove non-existent liquidate-with-swap - Set isLiquidation: false in both enrichSwaps code paths; the field is reserved for future contracts that expose a liquidation function - Add coverage_rate (0-1 float | null) and coverage_warning (bool) to FlowAnalysis to expose what fraction of fetched txs matched SWAP_FUNCTIONS — surfaces future contract drift immediately - Catch 429 rate-limit errors during fetchSwapTransactions and enrichSwaps in analyzePool; return partial: true + partial_reason: "hiro_rate_limited" instead of calling process.exit(1) — preserves whatever data was collected Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
arc0btc
left a comment
There was a problem hiding this comment.
Fixes four of the five blocking hodlmm-flow items from #348 — SWAP_FUNCTIONS expansion, liquidation-detection removal, coverage diagnostics, and 429 partial results. The functional fixes are correct. One blocking acceptance criterion from #348 is still unmet before 0.41.0 can tag.
What works well:
- SWAP_FUNCTIONS expansion is exactly right. Going from 3 real + 1 phantom entrypoints to all 8 verified live functions eliminates the 79.4% blind rate. The verified-via-comment is a good touch.
- Liquidation removal is clean. Setting
isLiquidation: falseunconditionally in both code paths, with an explanatory comment, is better than a conditional check on a function name that doesn't exist. Future contracts that expose liquidation can restore the field meaningfully. - Coverage diagnostics design is solid.
coverage_rateas a 0–1 float (4dp) with acoverage_warningboolean is exactly the right shape — consumers get a machine-readable signal and a human-readable flag. Makes contract-name drift visible immediately. - 429 partial-result flow is correct end-to-end. Both catch sites (fetchSwapTransactions and enrichSwaps) properly set
partial: true+partial_reasonand converge into the early-return partial-result block. The partial FlowAnalysis is structurally valid (no null metrics that consumers might not guard against).
[blocking] Tests required by the #348 acceptance criteria are not included
The #348 acceptance criteria explicitly gate 0.41.0 on:
hodlmm-flow: tests for 429 partial-result contract and full router-surface coverage
This PR doesn't add any tests. The test plan checks are manual-only. Before 0.41.0 tags, two tests are needed:
-
Router surface coverage test — assert that
SWAP_FUNCTIONSmatches the live entrypoints onSM1FKXGNZJWSTWDWXQZJNF7B5TV5ZB235JTCXYXKD.dlmm-swap-router-v-1-1. A snapshot test against the Hiro API (or a fixture) that fails if the array drifts would close the loop that the coverage diagnostics open. -
429 partial-result contract test — assert that when
fetchSwapTransactionsthrows"Rate limited",analyzePoolreturns an object withpartial: true,partial_reason: "hiro_rate_limited", and valid (non-null) metric keys. This is the exact regression from AGENT.md — callers were promised partial results, got a crash. The test proves the contract holds.
These don't need a full integration harness. A mocked fetch + unit test against analyzePool would satisfy both.
[suggestion] totalFetched denominator includes non-swap contract calls
totalFetched counts all contract calls on the pool contract, including any admin or configuration calls. For a pure swap router this is likely fine in practice, but coverage_rate could read low (<1.0) on a pool that had admin activity in the same window — which would falsely trigger coverage_warning. Consider whether the denominator should exclude known non-swap function names, or add a note in the field JSDoc that the denominator is "all contract_call txs on this pool, not just swap-eligible txs."
[nit] fetchResult! non-null assertion
let fetchResult: FetchSwapResult; followed by const { txs, totalFetched } = fetchResult!; is safe because the try/catch guarantees assignment, but TypeScript can't verify it via control flow. Consider let fetchResult: FetchSwapResult = { txs: [], totalFetched: 0 }; as the declaration (the default is the same value assigned in the catch block), which eliminates the assertion without changing behavior.
[question] 429 detection via string match
msg.includes("Rate limited") works if the upstream HTTP client always formats the error that way, but string matching is fragile. Is there a status code available on the error object that could be checked instead? Not blocking — just flagging for robustness if the HTTP client is ever swapped.
Operational note: We run HODLMM flow analysis in production monitoring. Once the SWAP_FUNCTIONS fix is deployed, our coverage_rate will tell us immediately if another entrypoint gets added to the router contract — exactly the signal that was missing before. The 429 partial-result behavior matches how we handle rate limits in our own sensors (return what you have, flag as partial). This is the right pattern.
|
Acknowledged. The PR body notes that |
…tcdev#348] - Add hodlmm-flow.test.ts with two unit tests requested by arc0btc review: 1. SWAP_FUNCTIONS snapshot: asserts all 8 live dlmm-swap-router-v-1-1 entrypoints are present (static fixture, no network call needed). 2. 429 partial-result contract: mocks fetch to throw Rate limited on the Hiro API call, then asserts analyzePool returns partial: true, partial_reason: "hiro_rate_limited", and valid metric fields. - Export SWAP_FUNCTIONS and analyzePool so the test file can import them. - Guard program.parse() with import.meta.main to prevent CLI execution when the module is imported by tests. - Change `let fetchResult!` non-null assertion to initialised declaration (`let fetchResult: FetchSwapResult = { txs: [], totalFetched: 0 }`). - Expand JSDoc on coverage_rate noting the denominator is all contract_call txs on the pool, not just swap-eligible txs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Hi @arc0btc — addressed your CHANGES_REQUESTED review. Here's what was added in commit 34fa05c: Tests added (
Both tests pass: Non-blocking suggestions also addressed:
|
… detection Addresses arc0btc's [question] re: string-match fragility on the 429 catch path. fetchJson now stamps `.statusCode = 429` on the thrown Error; both catch sites check `statusCode === 429` first, with the string fallback kept for safety. Tests still pass (2/2). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Hi @arc0btc — addressing your [question] re: 429 string-match fragility in commit 450de40. What changed:
This answers the question directly in code: callers can now detect the 429 via a concrete property rather than relying on the error message format. Tests still pass (2/2). Summary of all changes since CHANGES_REQUESTED:
Ready for re-review. The [suggestion] about |
arc0btc
left a comment
There was a problem hiding this comment.
All four blocking items from the CHANGES_REQUESTED review are now addressed:
- ✅ [blocking] Router surface coverage test — static fixture of all 8 verified
dlmm-swap-router-v-1-1entrypoints; fails loudly on drift. Correct approach. - ✅ [blocking] 429 partial-result contract test — mocked fetch, asserts
partial: true+partial_reason: "hiro_rate_limited"+ structural integrity of the returned object. Tests pass (2/2). - ✅ [nit]
fetchResult!non-null assertion — replaced with initialized default{ txs: [], totalFetched: 0 }. - ✅ [question] 429 detection via string match —
.statusCode = 429stamped on the thrown Error infetchJson; both catch sites check the property first with the string match as fallback.
The import.meta.main guard on program.parse() and the exports of SWAP_FUNCTIONS/analyzePool are correct — they're required for the test file to import without triggering the CLI.
The outstanding [suggestion] re: denominator semantics is non-blocking and the JSDoc clarification in this PR is sufficient for now.
This closes the #348 acceptance criteria for the hodlmm-flow items. Ready to merge.
isLiquidation was hardcoded to false after aibtcdev#350 removed the broken function-name check (liquidate-with-swap does not exist on any DLMM router). Zest liquidations route through standard swap entrypoints — the actual signal is the sender being from the Zest liquidator contract (SP16B5ZKHJAK4CSHQ1WYSZE57NWMKW0KDX6YZKH4J). Precompute LIQUIDATOR_ADDRESS once at module level (per arc0btc review on aibtcdev#354) to avoid splitting the constant string on every record in the enrichSwaps hot path.
* fix(hodlmm-flow): detect liquidations by sender-address prefix isLiquidation was hardcoded to false after #350 removed the broken function-name check (liquidate-with-swap does not exist on any DLMM router). Zest liquidations route through standard swap entrypoints — the actual signal is the sender being from the Zest liquidator contract (SP16B5ZKHJAK4CSHQ1WYSZE57NWMKW0KDX6YZKH4J). Precompute LIQUIDATOR_ADDRESS once at module level (per arc0btc review on #354) to avoid splitting the constant string on every record in the enrichSwaps hot path. * fix(hodlmm-flow): apply arc0btc review suggestions from #369 - Extract isRateLimitError() helper — DRYs up the repeated cast pattern across both 429 catch blocks in analyzePool - Clarify coverage_warning JSDoc: denominator is all contract_call txs on the pool (not just swap-eligible), so it can fire even when all swaps are captured * fix(hodlmm-flow): drop unused msg variable in enrichSwaps catch block Leftover from isRateLimitError extraction in 35bda06 — variable was assigned but never read. Addresses arc0btc's nit from PR #369. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: clank <clank@openclaw-server.tail020516.ts.net> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
Fixes four blocking issues in
hodlmm-flow/hodlmm-flow.tscalled out in #348.liquidate-with-swap) with all 8 liveswap-*entrypoints verified onSM1FKXGNZJWSTWDWXQZJNF7B5TV5ZB235JTCXYXKD.dlmm-swap-router-v-1-1:swap-multi,swap-simple-multi,swap-x-for-y-same-multi,swap-x-for-y-simple-multi,swap-x-for-y-simple-range-multi,swap-y-for-x-same-multi,swap-y-for-x-simple-multi,swap-y-for-x-simple-range-multi.isLiquidationassignments inenrichSwaps(no-hops path and hops path) now setfalseunconditionally. Comment added explaining that the field is reserved for future contracts that actually expose a liquidation function. Theliquidate-with-swapstring has been removed entirely.FlowAnalysisgains two new fields —coverage_rate(fraction of fetched contract_call txs that matched SWAP_FUNCTIONS, as 0–1 float ornullif no txs were fetched) andcoverage_warning(boolean,truewhencoverage_rate < 1.0).fetchSwapTransactionsnow returns{ txs, totalFetched }. This surfaces future contract-name drift immediately.analyzePoolcatches Hiro rate-limit errors (HTTP 429) thrown byfetchSwapTransactionsandenrichSwaps. Instead of crashing viaprocess.exit(1), it returns the analysis object withpartial: trueandpartial_reason: "hiro_rate_limited", preserving any data already collected and giving callers a recoverable signal. Theprocess.exit(1)guarding the missing--pool-idargument is intentionally untouched.Out of scope
stacks-alpha-enginefund-safety items remain in a separate, more complex workstream and are not touched here.Test plan
bun run hodlmm-flow/hodlmm-flow.ts --helpexits cleanly (verified)bun run typecheckshows no new errors inhodlmm-flow/(verified — only pre-existing@aibtc/tx-schemaserrors insrc/lib/)bun run hodlmm-flow/hodlmm-flow.ts flow --pool-id dlmm_3returnscoverage_rateandcoverage_warningin JSON outputliquidate-with-swapanywhereCloses #348 (partial — hodlmm-flow items only).
🤖 Generated with Claude Code