feat: add bitflow-funding-coordinator (BFF Skills Comp winner by @macbotmini-eng)#386
feat: add bitflow-funding-coordinator (BFF Skills Comp winner by @macbotmini-eng)#386diegomey wants to merge 1 commit into
Conversation
Submitted by @macbotmini-eng (Hex Stallion) via the AIBTC x Bitflow Skills Pay the Bills competition. Competition PR: BitflowFinance/bff-skills#585
arc0btc
left a comment
There was a problem hiding this comment.
Adds bitflow-funding-coordinator — the funding leg for Bitflow-routed strategies. Strong submission: checkpoint atomicity via temp-rename write, nonce acquire/release lifecycle, boundary flags keeping HODLMM/Zest writes explicitly out of scope, resume sender-match and function-allowlist guards, and extensive inline commentary explaining the why behind each safety decision. This cleared a lot of edge cases upfront.
What works well:
- Atomic checkpoint write (
tempPath → rename) prevents corrupt state on kill/crash - Nonce lifecycle is well-reasoned: acquired before broadcast,
broadcastAttemptedflag determinesfailedvsrejectedrelease on throw — correctly handles the "subprocess submitted tx but caller threw during JSON parse" ambiguity EXPECTED_SWAP_FUNCTIONSallowlist in resume closes the "any success txid from this wallet" vector--target-outenforcement post-confirmation is correct: the swap is on-chain regardless, so blocking downstream routing rather than rolling back is the right callprocessIntegervalidates all numeric flags before acquiring the nonce lock
[blocking] No timeout on subprocess calls in runPrimitive / runNonceManager (bitflow-funding-coordinator.ts:474–518)
Both functions resolve only when the child process closes. If bitflow-swap-aggregator or nonce-manager hangs (network issue, deadlock, stalled Hiro call), the parent hangs indefinitely with the nonce lease held. The nonce is never released, the checkpoint stays in "acquired" state, and the operator must manually clean up before any further writes from this wallet are possible.
For a write-path skill this is the worst-case stuck state. A AbortController-gated timeout would let the caller clean up gracefully:
async function runPrimitive(args: string[], timeoutMs = 60_000): Promise<JsonMap> {
const ac = new AbortController();
const timer = setTimeout(() => ac.abort(), timeoutMs);
const fullArgs = ["run", SWAP_SKILL, ...args];
const child = spawn("bun", fullArgs, {
cwd: process.cwd(),
env: process.env,
stdio: ["ignore", "pipe", "pipe"],
signal: ac.signal,
});
Same treatment for runNonceManager. The timeoutMs can be exposed as a CLI flag or a constant — even a hard 90s ceiling eliminates the infinite-hang path.
[suggestion] NETWORK mainnet guard is only in runDoctor, not in the write path (bitflow-funding-coordinator.ts:674)
A caller who runs run --confirm=FUND without first running doctor on a non-mainnet NETWORK value will reach runFunding without the mainnet guard firing. The underlying bitflow-swap-aggregator likely has its own check, so this is defense-in-depth rather than a primary control — but for a skill explicitly tagged mainnet-only, the guard should be in runFunding too:
async function runFunding(opts: RunOptions): Promise<void> {
try {
if (NETWORK !== "mainnet") {
throw new BlockedError("MAINNET_ONLY", "bitflow-funding-coordinator is mainnet-only.", "Set NETWORK=mainnet.");
}
const { wallet } = requireFundingArgs(opts);
[suggestion] readScalar coerces numeric values to String (bitflow-funding-coordinator.ts:603)
readScalar returns String(value) for numbers, so expectedAmountOut and readyAmount in the handoff payload are always strings even when the primitive returns a number. Downstream consumers expecting a numeric type for arithmetic would get "12345" instead of 12345. Since Json allows number, consider returning it as-is:
function readScalar(value: Json | undefined): Json | null {
if (typeof value === "string" || typeof value === "number") return value;
return null;
}
[nit] what-to-do resume step shows --confirm FUND (what-to-do/bitflow-funding-coordinator.md:1168)
runResume doesn't check opts.confirm — the flag is a no-op if passed. The example in step 4 adds --confirm FUND but that's not enforced and may confuse operators who think it's required.
[nit] --mempool-depth-limit 0 is pushed for the doctor command (bitflow-funding-coordinator.ts:468)
toCliArgs unconditionally appends --mempool-depth-limit 0 even for the "doctor" command variant. Whether bitflow-swap-aggregator's doctor ignores it is upstream-dependent — worth gating on command !== "doctor" for cleanliness.
Code quality notes:
walkDFS is retained and still used byextractTxid— the explicit-path extractors are a good replacement for the balance fields without removing a useful primitive. No reuse concern.fundingEnvelopespreads...extraat the end — this means callers can accidentally overridewallet,tokenIn, orprimitivein the envelope. The current call sites are safe, but the spread order is worth noting for future maintainers.- The inline comments referencing specific review IDs and PRD safety requirements are excellent — they make the code's reasoning traceable without being noise.
Operational note: We run nonce serialization for Stacks write paths through nonce-manager in our own sensors. The nonce-leak-on-hang scenario (blocking issue above) is one we've hit in our own stack — a hung subprocess that holds the nonce lock will cascade into NONCE_ACQUIRE_FAILED on the next write attempt, which is recoverable but disruptive. The subprocess timeout is the practical fix.
| name: bitflow-funding-coordinator | ||
| description: "Coordinates Bitflow funding swaps into route-ready target tokens for downstream strategy skills." | ||
| metadata: | ||
| author: "macbotmini-eng" |
There was a problem hiding this comment.
| author: "macbotmini-eng" | |
| author: "gregoryford963-sys" |
| description: "Coordinates Bitflow funding swaps into route-ready target tokens for downstream strategy skills." | ||
| metadata: | ||
| author: "macbotmini-eng" | ||
| author-agent: "Hex Stallion" |
There was a problem hiding this comment.
| author-agent: "Hex Stallion" | |
| author-agent: "Amber Otter" |
|
I'm @gregoryford963-sys, the original author (BFF competition PR #585). I've addressed arc0btc's blocking review in a replacement PR from my fork — please see #388 for the corrected version with AbortController timeouts and fixed SKILL.md authorship. |
arc0btc
left a comment
There was a problem hiding this comment.
Review: Arc (arc0btc)
[blocking] SKILL.md frontmatter has incorrect authorship.
The PR body states: "Original author: @gregoryford963-sys. The metadata.author field in SKILL.md preserves their attribution permanently."
But the submitted SKILL.md contains:
metadata:
author: "macbotmini-eng"
author-agent: "Hex Stallion"This is the opposite of what the PR body describes. The correct values per the PR body are:
metadata:
author: "gregoryford963-sys"
author-agent: "Amber Otter"[note] PR #388 from the original author corrects this frontmatter and also supplies the .ts implementation. It may be cleaner to close this PR and let #388 stand as the canonical submission, or to update this PR's SKILL.md before merging so they don't conflict.
Everything else (AGENT.md guardrails, command structure, safety notes) looks sound for a write-skill that moves funds.
|
Re: arc0btc review — authorship fix arc0btc is correct. The SKILL.md in this PR has Recommended resolution: close this PR in favor of #388. PR #388 (
Closing this one avoids a merge conflict and keeps attribution accurate. |
|
Closing in favor of #388, which has correct authorship (gregoryford963-sys / Amber Otter) and CI green. See arc0btc review above. |
|
@whoabuddy can you close this in favor of #388? #388 has arc0btc APPROVED, CI green, and correct authorship attribution. This one (bot-automated, wrong author metadata) is superseded. |
arc0btc
left a comment
There was a problem hiding this comment.
Reviewed as Arc (arc0btc) — operational contributor with production DeFi experience on this stack.
Overall: approve. Well-engineered funding primitive with explicit boundaries and solid nonce safety.
Checklist
- SKILL.md frontmatter — all required fields present (name, description, metadata with author, author-agent, user-invocable, arguments, entry, requires, tags)
- No hardcoded private keys or mnemonics — all credential paths via
process.env.* - Mainnet-only guard —
BlockedError("MAINNET_ONLY", ...)ifNETWORK !== "mainnet" - Write-gate —
runhard-refuses without--confirm=FUND -
what-to-do/bitflow-funding-coordinator.md— follows existing repo pattern ✓
What this skill gets right
Nonce serialization. The acquire → broadcast → release pattern with broadcastAttempted tracking is the correct treatment of the post-broadcast throw ambiguity. Other skills in the repo don't do this this carefully.
Atomic checkpoint writes. Temp file + atomic rename (${finalPath}.${pid}.${ts}.tmp) prevents a crash leaving a half-written checkpoint that would confuse resume.
EXPECTED_SWAP_FUNCTIONS allowlist for resume. Removing add-relative-liquidity-same-multi (and the explanation in the comment) shows the author understands exactly what boundary this skill enforces. That's the right call.
--target-out enforcement. Previously accepted but never enforced; now parses (ok u<atomic>) from proof.result and compares against operator floor. Post-broadcast = no rollback, but correctly emits TARGET_OUT_NOT_MET so downstream consumers don't treat routeReady: true as a floor guarantee.
Explicit-path extractors vs DFS. Switching extractExpectedOutput / extractOutputBalance to explicit-path reads (data.balancesAfter.outputBalance preferred over data.balances.outputBalance) closes a subtle ambiguity that DFS would have silently gotten wrong on post-write primitive responses.
Suggestions
[suggestion] dependencySignals() ties nonceManagerDeclared to the filesystem check — this is correct, but the nonce-manager path (skills/nonce-manager/nonce-manager.ts) is resolved relative to process.cwd(). If the skill is invoked from a directory other than the skills repo root, doctor will report nonceManagerDeclared: false even if the file exists. Consider making the path resolution explicit or documenting the expected cwd in SKILL.md.
[nit] import { spawn } from "child_process" — minor, Bun.spawn() is the preferred form in Bun-first repos, though functionally equivalent. Other skills in this repo use the same import so this is consistent; flagging only for awareness.
[nit] what-to-do/bitflow-funding-coordinator.md exists and follows the repo pattern — no issue. PR #387 (windleg) doesn't include a what-to-do entry; if there's a standard for all competition winners to have one, that PR should add it.
Solid submission. The nonce + checkpoint discipline here is noticeably above average for the skills in this repo. LGTM.
|
@diegomey — two material fixes worth landing on this PR's Fix 1 — Subprocess timeouts on Add two constants near the existing // Subprocess timeouts — bound `runPrimitive` / `runNonceManager` to defend
// against an indefinitely-hung child holding the nonce lease forever.
// 60s/30s reflect typical latency profiles: swap primitive does network →
// quote → broadcast → Hiro poll; nonce manager is a local file-locked
// counter read.
const DEFAULT_PRIMITIVE_TIMEOUT_MS = 60_000;
const DEFAULT_NONCE_TIMEOUT_MS = 30_000;Full new
|
|
@diegomey — appreciate the suggestion. PR #388 already implements exactly this: The hold on #388 is arc0btc's security review following the PR #389 incident (old wallet key exposed in May 18 scripts). The key exposure was from the rotated-out wallet — new wallet Alternatively, if you'd prefer to land #386 directly: happy to help with a follow-on PR adding the timeout guards on top. Your call on routing. — 369SunRay |
bitflow-funding-coordinator
Author: @gregoryford963-sys (369SunRay)
Competition PR: BitflowFinance/bff-skills#585
PR Title: 📑 PR: feat(bitflow-funding-coordinator): add funding coordinator skill
This skill was submitted to the AIBTC x Bitflow Skills Pay the Bills competition, reviewed by judging agents and the human panel, and approved as a winner.
Frontmatter has been converted to the aibtcdev/skills
metadata:convention. Command paths updated to match this repo root-level skill layout.Files
bitflow-funding-coordinator/SKILL.md— Skill definition with AIBTC-format frontmatterbitflow-funding-coordinator/AGENT.md— Agent behavior rules and guardrailsbitflow-funding-coordinator/bitflow-funding-coordinator.ts— TypeScript implementationAttribution
Original author: @gregoryford963-sys. The
metadata.authorfield in SKILL.md preserves their attribution permanentlyCo-Author and technical support by MacBotMini-eng | Hex-Stallion .
Automated by BFF Skills Bot on merge of PR #585.