[Feature] Add SDK guide docs and multi-agent instruction files for AI-assisted development#1249
[Feature] Add SDK guide docs and multi-agent instruction files for AI-assisted development#1249marshacb wants to merge 8 commits into
Conversation
Review: Agent/MCP Skill ArchitectureThe content here is solid — the reference docs cover the right topics, the safety rules in INJECT.md are genuinely useful, and the verification scripts are a clever self-check mechanism. My feedback is about the delivery mechanism, not the substance. Discovery ProblemThe
The Duplication ConcernThe same safety rules appear in three places: Drift RiskThe reference docs are hand-written API docs with code examples. When the SDK changes (e.g., Suggested Approach1. Multi-agent pointer files — Add thin instruction files for each major agent, all pointing to the same canonical content: Each file is 5-10 lines. The actual reference content lives once, in one place. Write once, discover everywhere. 2. Keep the skill structure for Claude Code — It's fine as a power-user feature, just don't treat it as the single source of truth for all agents. Make it a thin pointer to the canonical docs rather than a parallel documentation tree. 3. Inline code documentation — The most drift-proof approach for API-level guidance. JSDoc on 4. MCP resource serving — If building MCP servers that help developers use the SDK, the reference content should be served as MCP resources via the protocol, not discovered via filesystem conventions. TL;DRThe content is valuable. I'd keep the Claude Code skill but make it point to canonical docs rather than maintaining parallel documentation. Add ~5 thin pointer files so Cursor, Copilot, Windsurf, Cline, and Codex users all get the safety rules automatically. This gets you broad coverage for minimal maintenance cost. |
iamalwaysuncomfortable
left a comment
There was a problem hiding this comment.
On the first pass, this is great, operationally let's test it out via the MCP server to see how it does in action.
Additional Findings on Close ReviewA few more issues beyond the previous comment: 1.
|
iamalwaysuncomfortable
left a comment
There was a problem hiding this comment.
Code Accuracy Review
Verified all code examples against the actual SDK source (sdk/src/program-manager.ts, sdk/src/network-client.ts, sdk/src/account.ts, sdk/src/record-scanner.ts, sdk/src/keys/provider/memory.ts). The structure and approach are great — inline comments flag the specific lines where the documented API diverges from the actual implementation.
Must-fix (will generate broken code):
PrivateKeyCiphertextAPI is fabricated — methods don't existestimateDeploymentFee()referenced as user-facing but it's an internal WASM-only static method
Should-fix (inconsistent/misleading):
- Inconsistent
.toString()beforesubmitTransaction()across examples getTransaction()throws on not-found, doesn't return null — polling pattern is misleadingbuildDeploymentTransactionreturn type inconsistency (sometimes.toString(), sometimes not)
| import { PrivateKeyCiphertext } from "@provablehq/sdk/testnet.js"; | ||
|
|
||
| // Encrypt for storage | ||
| const encrypted = PrivateKeyCiphertext.encryptPrivateKey(privateKey, password); |
There was a problem hiding this comment.
Bug: This API does not exist. PrivateKeyCiphertext does not have an encryptPrivateKey static method.
The actual SDK API (from sdk/src/account.ts):
// Encrypt
const encrypted = privateKey.toCiphertext(password);
const encryptedString = encrypted.toString();
// Decrypt
const restored = PrivateKey.fromPrivateKeyCiphertext(
PrivateKeyCiphertext.fromString(encryptedString),
password
);An agent following this doc will generate code that throws TypeError: PrivateKeyCiphertext.encryptPrivateKey is not a function.
|
|
||
| ### Fee Management | ||
|
|
||
| - NEVER hardcode fee amounts — always use `estimateExecutionFee()` or `estimateDeploymentFee()` |
There was a problem hiding this comment.
Bug: estimateDeploymentFee() is not a user-facing method. It only exists as WasmProgramManager.estimateDeploymentFee(programString, imports) — an internal static method on the WASM module. It's not exposed on ProgramManager or AleoNetworkClient.
buildDeploymentTransaction calls it internally to estimate fees automatically. An agent searching for pm.estimateDeploymentFee() or client.estimateDeploymentFee() will find nothing.
Suggested fix: Remove estimateDeploymentFee() from this list, or note that deployment fee estimation is handled internally by buildDeploymentTransaction.
| for (let i = 0; i < 60; i++) { | ||
| try { | ||
| const result = await client.getTransaction(txId); | ||
| if (result) { confirmed = true; break; } |
There was a problem hiding this comment.
Misleading pattern. getTransaction() throws on HTTP error / not-found — it never returns a falsy value on success. The if (result) null-check is dead code; if the call succeeds, result is always truthy.
The retry logic actually works because the catch block swallows the throw, but an agent reading this will think it needs to check for null. Consider:
try {
const result = await client.getTransaction(txId);
confirmed = true; break;
} catch { /* not confirmed yet */ }| feeAuthorization, | ||
| }); | ||
|
|
||
| const txId = await pm.networkClient.submitTransaction(tx.toString()); |
There was a problem hiding this comment.
Inconsistency: .toString() used here but not elsewhere. This line calls tx.toString() before submitting, but deployment.md:27 and transfers.md:57 pass the Transaction object directly.
submitTransaction() accepts Transaction | string, so both work, but the inconsistency across guide files will confuse agents about which pattern is correct. Recommend picking one approach (preferably passing the Transaction directly without .toString()) and using it consistently.
| priorityFee: 0, | ||
| privateFee: false, | ||
| }); | ||
| await pm.networkClient.submitTransaction(execTx.toString()); |
There was a problem hiding this comment.
Same .toString() inconsistency — this line calls execTx.toString() but line 27 above passes tx directly without .toString(). Pick one pattern.
|
|
||
| ```bash | ||
| cargo install leo-lang | ||
| leo devnode start --private-key APrivateKey1zkp8CZNn3yeCseEtxuVPbDCwSyhGW6yZKUYKfgXmcpoGPWH |
There was a problem hiding this comment.
Note: This is a well-known devnet key so it's fine functionally, but the verify-execution.ts script in this same PR will flag this line with "SECURITY: Hardcoded private key detected." Consider adding a comment like # Standard devnet-only key — not used on mainnet so agents (and the verification script) understand it's intentional.
7407f89 to
668dd16
Compare
… AI-assisted SDK development
…rd method signatures and LocalFileKeyStore naming
…lic estimateDeploymentFee, fix polling pattern and submitTransaction consistency
…ion verification, and key/record enhancements
…nError messaging, harden output index validation
2a393ce to
4fea03e
Compare
Motivation
AI coding assistants currently have no guidance for generating Provable SDK code. This adds a generalized builder skill and universal safety rules so they produce correct, secure code by default.
Related PRs
#1245