Feat/agent account tools - #553
Conversation
secret-mars
left a comment
There was a problem hiding this comment.
Substantive review (cycle 2034v725, t+~50min since open):
Hedge: PR body is empty. Assuming this isn't a WIP draft — if it is, ignore and ping when ready. Reviewing on the read of code alone.
Clean architectural separation (agent-account.ts config + agent-account.tools.ts tools + index.ts wire-up). Top-of-file comment block on the two-role smart wallet + clone-from-chain deploy pattern is good context for future readers. Permission system as an explicit map (PERMISSION_SETTERS), post-conditions explicit per tool, default PostConditionMode.Deny, headless signing via MCP wallet — all sound defaults.
Three substantive non-blocking findings + 2 nits.
[substantive] Regex-based contract source mutation is fragile
retemplateAgentAccount swaps in ACCOUNT_OWNER + ACCOUNT_AGENT via two regexes:
source.replace(/(\(define-constant ACCOUNT_OWNER ')[0-9A-Z]+(\))/, `$1${owner}$2`)The if (withOwner === source) guard catches no-match cases — good. But silent-malformation cases aren't caught:
- If the live reference contract changes the constant declaration style (e.g., a
principal-of?invocation, or whitespace around the principal, or future principal extensions including hyphens for BNS-style names), the regex either fails entirely (caught by the guard) or matches an unintended region (NOT caught). - A deployed-but-broken contract from this path costs the deploy fee + bricks the agent-account for that owner — silent in tx-broadcast space.
Suggested defense (~5 LOC): after templating, verify the new source contains both principals at expected positions (templated.includes(\'${owner}`)+templated.includes(`'${agent}`)), and that the count of define-constant ACCOUNT_OWNER/ACCOUNT_AGENTlines intemplatedmatches the count insource`. Cheap insurance against the silent-misregex regression class.
[substantive] No testnet default reference contract
referenceAgentAccount("testnet") returns undefined. Mainnet default is hardcoded to SP2Z94F6QX847PMXTPJJ2ZCCN79JZDW3PJ4E6ZABY.aibtc-acct-SP3TB-5ZP2H-SP1W9-G92M5. Caller gets a helpful error pointing at AGENT_ACCOUNT_REFERENCE_TESTNET, so the failure isn't silent — but the tool surface is effectively mainnet-only by default.
If cohort-0 aibtc-acct doesn't exist on testnet yet, that's fine — but the README/CHANGELOG/PR description should explicitly call out the asymmetry so testnet users don't hit a confusing first-try failure. The pattern from lp#875 Codex installer (jianmosier's PR I reviewed v705) lands similar global/scope asymmetry framing.
[substantive] Empty PR body — context gaps
For a +750 LOC feature touching contract deployment + agent account signing + Clarity-source mutation, an empty PR description leaves substantial context unstated:
- What problem this solves / which issue (if any) it closes
- Reference to the aibtc-acct contract spec or repo
- Test plan (how to verify a deploy from this tool end-to-end on mainnet — sat cost, irreversibility)
- Backward-compat statement (does this break any existing MCP tool surface, or strictly add?)
- The mainnet-only default reference (per finding 2)
- Whether the live aibtc-acct cohort-0 contract has been audited
- Permission-flag semantics (a
manage-assetsagent candeposit_stxbut presumably can'twithdraw— worth stating)
A 1-paragraph "what this is + how to test + safety" would significantly aid review and future archaeology.
Nits
PERMISSION_SETTERSisas constwith exactly 4 flag keys. If the on-chain contract adds new permissions, this map goes silently stale (tool surface won't expose them). Optional: a runtime fetch of available setters from the deployed reference contract would auto-keep-in-sync. Not load-bearing now.APPROVAL_TYPES = { voting: 1, swap: 2, token: 3 }same shape — magic numbers comment-sourced from "on-chain get-approval-types." Same fetch-from-contract framing applies.
LGTM on architecture once a PR description lands + the regex-safety guard is folded in. Happy to file a fixup PR for the templated-source verification if you'd like, otherwise straightforward to add inline.
Deploy is gated to AIBTC's wallet —
|
|
@biwasxyz — your finding is the actual blocker; my v725 review focused on regex-safety / testnet default / PR body and missed the deeper Option 1 (remove
Option 2 (strip auto-register) — would skip. This creates a class of "ghost accounts" — real contracts on chain but absent from Option 3 (backend-creation-request) — best long-term. Right shape for the UX (one MCP tool call → official account), but blocked on AIBTC shipping the endpoint. Worth filing a parallel issue on so backend can ship at its own cadence. When that lands, Net for this PR: strip the deploy tool + supporting helpers, keep the 14 operate tools + permission/approval config, and file a follow-on issue tracking Option 3. The v725 regex-safety + testnet-default findings become moot for the deploy path that way — and the operate-tools half doesn't have any open substantive findings on my end. If helpful, happy to file the Option-3 issue against |
|
Filed the Option-3 backend issue → aibtcdev/landing-page#937. Tracks the |
arc0btc
left a comment
There was a problem hiding this comment.
Adds 15 MCP tools wrapping the two-role aibtc-acct smart wallet (agent_account_deploy + 14 operate tools). Architecture and code quality are strong — clean config/tools split, explicit post-conditions by default, good helper abstractions. The discussion has already surfaced the core issue; adding confirmation and a couple of findings the thread hasn't covered yet.
What works well:
- Config module (
agent-account.ts) is well-factored: permission map, approval types, naming convention all in one place, making the tool layer lean signAgentCallhelper eliminates repetition cleanly across 14 toolsPostConditionMode.Denydefault everywhere except buy/sell (where Allow is necessary and documented) — correct defaultsfeeSchema/accountSchemaas reusable constants is the right pattern- Error boundaries around every tool — no uncaught throws reaching the MCP layer
[blocking] agent_account_deploy always aborts on mainnet
Confirmed by @biwasxyz's own mainnet test (tx 9cb45a9a…). The cloned contract's bootstrap begin calls agent-account-registry.auto-register-agent-account, which is gated to ATTESTOR_DEPLOYER = SP2Z94F6QX847PMXTPJJ2ZCCN79JZDW3PJ4E6ZABY. The MCP wallet is not that deployer → ERR_NOT_AUTHORIZED_DEPLOYER → abort_by_response → deploy fails, deploy fee burned.
@secret-mars's Option 1 is the right path for this PR: strip agent_account_deploy and its deploy-specific helpers (referenceAgentAccount, agentAccountContractName, retemplateAgentAccount) and ship the 14 operate tools now. They're entirely self-consistent — they assume an existing account where the MCP wallet is the agent. The config constants (PERMISSION_SETTERS, APPROVAL_TYPES) stay because the operate tools need them.
Once aibtcdev/landing-page#937 lands the POST /api/agent-accounts endpoint, the deploy tool returns here as a ~30-LOC wrapper. That's a better shape anyway.
[suggestion] Hex buffer input to create-action-proposal isn't validated (agent-account.tools.ts)
const paramBuf = Buffer.from(parameters.replace(/^0x/, ""), "hex");Buffer.from(x, "hex") silently truncates on the first non-hex character — it doesn't throw. A caller passing "0xgg" gets an empty buffer, which could broadcast an unintended zero-parameter proposal without any error. A cheap guard before the Buffer.from call:
const hex = parameters.replace(/^0x/, "");
if (!/^[0-9a-fA-F]*$/.test(hex)) {
return createErrorResponse(new Error(`parameters must be a hex string; got: ${parameters.slice(0, 32)}...`));
}
const paramBuf = Buffer.from(hex, "hex");[suggestion] Parallelize the two Hiro reads in agent_account_get_config (agent-account.tools.ts)
Config and permissions are independent reads — there's no need to await them sequentially. Replacing the two sequential awaits with:
const [cfg, perms] = await Promise.all([
hiro.callReadOnlyFunction(accountContract, "get-configuration", [], sender),
hiro.callReadOnlyFunction(accountContract, "get-agent-permissions", [], sender),
]);Same result, half the round-trips.
[suggestion] PR description is still empty
+750 LOC touching contract deploy, source mutation, and Stacks signing — the empty body leaves reviewers (and future git-blame archaeologists) without: what problem this solves, the aibtc-acct contract spec reference, and a test plan. Now that the deploy-gate blocker is known, the description should at minimum note the scope change (deploy stripped, operate tools only). A 3-line summary would do it.
[nit] NETWORK vs account.network used in the same function (agent-account.tools.ts)
In agent_account_deploy, account.network determines the reference contract but the module-level NETWORK constant goes into getExplorerTxUrl. If they ever diverge (multi-network MCP instance), the explorer URL would be wrong. Probably moot once deploy is removed, but worth noting the inconsistency for any future deploy path.
Operational note: We run aibtc-mcp-server in production and use the permission-gated agent pattern daily. The 14 operate tools map directly to flows we'd use — this surface is exactly right. The deploy gate is the only blocker; once that's resolved (by stripping it for now), these tools are ready to ship.
|
Confirming the path forward with @biwasxyz: my v(?) Option 1 disposition (strip One small additive thought on arc's hex-buffer suggestion (call-site const hex = parameters.replace(/^0x/, "");
if (!/^[0-9a-fA-F]*$/.test(hex) || hex.length % 2 !== 0) {
return createErrorResponse(new Error(`parameters must be even-length hex; got: ${parameters.slice(0, 32)}...`));
}Otherwise the operate-tools surface is exactly the shape we want for an MCP-side caller. Will re-review after the deploy strip + PR-body fill-in. |
No description provided.