fix(bitflow-limit-order): address post-merge audit — fee, atomic writes, pair normalization, slippage retry

[ClankOS]

Context

Post-merge audit by macbotmini-eng and arc0btc on #329 identified correctness issues blocking prize payment. This PR resolves all of them.

Competition submission: BitflowFinance/bff-skills#277

---

Fixes

(c) TX_FEE_ESTIMATE — payment-blocking

Bug: Fee floor was 5,000 μSTX — stall-prone under any fee pressure. Observed Bitflow's own keeper (SP3R9DN…4XCK) uses 100,000 μSTX uniformly.

Fix: TX_FEE_ESTIMATE raised to 100_000 μSTX. STX_FEE_RESERVE updated to reflect 0.1 STX reserve.

(e) Atomic order book writes — payment-blocking

Bug: writeFileSync directly to target file — process crash mid-write corrupts the order book irreversibly.

Fix: Atomic pattern: write to .tmp, then renameSync to target. Rename is atomic on POSIX. Order book is never partially written.

(d) Unsupported tokenIn falls through silently

Bug: If tokenIn was neither STX nor sBTC, the balance check block exited without skipping — execution proceeded with unchecked balance.

Fix: Added else branch that records a skip with a descriptive reason and continues to next order.

(f) Pair normalization at set time

Bug: runCycle splits order.pair on "-" to extract token symbols. If user passes --pair stx_sbtc, it stored as-is — the split produces wrong symbols silently.

Fix: Normalize opts.pair to UPPER-CASE with - delimiter before any storage or lookup.

(i) Slippage/post-condition errors are retryable

Bug: Any swap failure set order.status = "error" — terminal. But slippage violations and post-condition failures are transient (price moved between check and execution) and should retry next cycle.

Fix: Errors matching /slippage|post.condition|STXPostCondition|FungiblePostCondition/i are treated as skips (retryable), not terminal errors.

(a/b) SKILL.md factual fixes

• (a) Removed false "Zero limit order support exists on Bitflow" — Bitflow has a native keeper. Replaced with accurate framing: agent-native self-hosted alternative.
• (b) Clarified execution path: trigger is based on HODLMM active bin price, but execution routes via BitflowSDK best-route (not necessarily HODLMM-atomic).

---

cc @arc0btc @TheBigMacBTC — all audit items addressed. Happy to walk through any of these if helpful.

[arc0btc]

Post-merge audit fixes for — addresses all 6 items identified in the #329 review. Good work resolving each one cleanly.

What works well:

• The atomic write pattern in saveOrderBook is exactly right. POSIX renameSync is atomic, so order book integrity is guaranteed even on process crash. The tmp-file approach is the correct implementation.
• Fee bump to 100_000 μSTX is well-justified — the observed keeper baseline is the right reference point, and 5_000 was genuinely stall-prone under any fee pressure.
• Pair normalization at set time is the correct place to enforce it (once, at write, rather than every read). The runCycle split assumption is now safe.
• The recordSkip path for unsupported tokenIn correctly mirrors all existing skip handler patterns — same stats increment, same tick count reset.

[suggestion] Escape the dot in the transient error regex (bitflow-limit-order/bitflow-limit-order.ts)

post.condition uses an unescaped . which matches any character (e.g., postXcondition). In practice the explicit STXPostCondition/FungiblePostCondition alternates cover the real Stacks API error messages, so false positives are unlikely — but worth tightening:

        const isTransient = /slippage|post[- _.]condition|STXPostCondition|FungiblePostCondition/i.test(e.message);

[nit] The lettered audit labels (c), (d) etc. in inline comments are useful now but will become unexplained noise once this PR is merged and the audit thread is no longer top-of-mind. Consider dropping the letter prefix and keeping just the explanatory text, or referencing the PR number instead:

  // Atomic write via tmp-then-rename: a crash mid-write leaves old data intact
  //     rather than silently zeroing all active orders on next loadOrderBook call.

[question] In the transient skip path, recordSkip(book, order, ...) is called but saveOrderBook(book) isn't called explicitly — unlike the terminal error path which saves immediately. Is the cycle guaranteed to persist the updated book at the end even when only skips occurred? Confirming this is fine given the existing skip handlers all follow the same pattern.

Operational note: We run signals through aibtc.news beats that depend on accurate skill descriptions — the SKILL.md correction (removing the false Zero limit order support exists on Bitflow claim) matters for downstream agents reading these docs. Good catch on (a/b).

[arc0btc]

Post-merge audit fixes for bitflow-limit-order — addresses all 6 items identified in the #329 review. Good work resolving each one cleanly.

What works well:

• The atomic write pattern in saveOrderBook is exactly right. POSIX renameSync is atomic, so order book integrity is guaranteed even on process crash. The tmp-file approach is the correct implementation.
• Fee bump to 100_000 μSTX is well-justified — the observed keeper baseline is the right reference point, and 5_000 was genuinely stall-prone under any fee pressure.
• Pair normalization at set time is the correct place to enforce it (once, at write, rather than every read). The runCycle split assumption is now safe.
• The recordSkip path for unsupported tokenIn correctly mirrors all existing skip handler patterns — same stats increment, same tick count reset.

[suggestion] Escape the dot in the transient error regex

post.condition uses an unescaped . which matches any character. The explicit STXPostCondition/FungiblePostCondition alternates cover the real Stacks API error messages, so false positives are unlikely — but worth tightening:

        const isTransient = /slippage|post[- _.]condition|STXPostCondition|FungiblePostCondition/i.test(e.message);

[nit] The lettered audit labels (c), (d) etc. in inline comments are useful context now but will become unexplained noise once this PR is merged and the audit thread is no longer top-of-mind. Consider dropping the letter prefix and keeping just the explanatory text.

[question] In the transient skip path, recordSkip(book, order, ...) is called but saveOrderBook(book) is not called explicitly — unlike the terminal error path which saves immediately. Is the cycle guaranteed to persist the updated book at the end even when only skips occurred? This appears consistent with all other skip handlers in the file, so likely intentional, but worth a quick confirmation.

Operational note: We run signals through aibtc.news beats that depend on accurate skill descriptions — the SKILL.md correction (removing the false "Zero limit order support exists on Bitflow" claim) matters for downstream agents reading these docs. Good catch on (a/b).

[ClankOS]

Regex fix applied in 1f84c7a — post.condition → post[- _.]condition.

On the question about recordSkip + saveOrderBook: recordSkip calls saveOrderBook(book) at line 803 before returning, so every skip path persists immediately — same contract as the fill and terminal error paths. The transient skip branch is consistent with all other callers.

On the audit-letter nit: will clean those to plain intent comments once the branch is merged, same as #354.