chore(skills): migrate agent-tools skills to Miden v0.16 - #17
Conversation
…t-A, FPI model, citation fixes
Keinberger
changed the title
0xrouss
left a comment
0xrouss
left a comment
There was a problem hiding this comment.
Reviewed commit 13f754adb5e48d9888087d97eb2e82e4619c52b2 against the pinned v0.16 release candidates and the final v0.16 releases. These 20 comments cover migration defects and examples that need correction before merging.
Validation included the pinned rc.7 client against devnet, final v0.16 local mock execution, React hook reproductions, published signer TypeScript declarations, the actual IndexedDB schema, and tagged release sources. Testnet was not used. The final v0.16 Node client was rejected during devnet initialization, so its executable examples were checked locally. No full Rust contract build, external signer login, browser performance measurement, or transaction proof/submission was performed; individual comments distinguish reproduced failures from source-based findings.
| miden = "0.14.0" | ||
| ``` | ||
|
|
||
| Contracts build on the pinned nightly toolchain (`channel = "nightly-2026-04-30"`, `targets = ["wasm32-wasip2"]`); the compiler's MSRV is 1.97. |
There was a problem hiding this comment.
[P2] Use a toolchain that meets the published SDK's Rust 1.99 requirement
The recommended nightly-2026-04-30 is Rust 1.97, but the published miden 0.14.0 and cargo-miden 0.10.0 both declare rust-version 1.99. A project following this setup is rejected by Cargo before compilation. Use the release's nightly-2026-09-01 toolchain and update the duplicated MSRV/toolchain matrices in rust-sdk-source-guide and rust-sdk-pitfalls. The same matrices still describe alpha.4 protocol/VM 0.25 dependencies although the pinned compiler release declares protocol 0.16.0-rc.4 and VM 0.29.
Validation: Checked the installed April nightly version, published crates.io metadata, compiler Cargo.toml, and release example rust-toolchain.toml. No complete Rust build was run.
Sources: Compiler release manifest; Release example toolchain.
| let request = TransactionRequestBuilder::new() | ||
| .fee_conversion_info(conversion_info, salt) // salt: Word, mandatory | ||
| .build()?; |
There was a problem hiding this comment.
[P2] Document the released fee builder APIs
TransactionRequestBuilder has no fee_conversion_info(conversion_info, salt) method in either the cited rc.5 or final v0.16. Its method is fee_conversion_salt(salt); the client supplies native-asset conversion info at rate 1/1 during transaction preparation. The companion claim in web-client-usage:893–896 that no web fee API exists is also false: feeAwareTransactionRequestBuilder(account) is exported in rc.7 and final v0.16. Replace the Rust example and align the web/React fee guidance with these actual APIs so custom transaction builders compile and prepare auth arguments correctly.
Validation: Verified Rust builder/client source in rc.5 and final v0.16; verified the JS method exists at runtime in both npm versions.
Sources: Rust request builder; Web fee-aware builder.
| # Good: a missing required entry is an error | ||
| adv.has_mapkey | ||
| assert.err=ERR_MISSING_REQUIRED_ADVICE |
There was a problem hiding this comment.
[P2] Move the membership flag from advice to the operand stack
adv.has_mapkey places its result on the advice stack. The following assert therefore checks an existing operand-stack element instead of checking whether the key exists. With a missing key whose top limb is 1, the documented check succeeds. Insert adv_push before assert and retain the key's actual stack shape. Also remove the claim that adv.push_mapval is a no-op for an absent key: the VM raises a missing-advice-map-key error.
Validation: Reproduced the false success with rc.7/devnet and final v0.16/mock. The control with adv_push fails as expected; an absent adv.push_mapval also fails.
Sources: Advice event implementation.
| # remove the asset from the account | ||
| exec.native_account::remove_asset | ||
| dropw | ||
| # => [ASSET_KEY, ASSET_VALUE, note_idx, pad(7)] | ||
| # => [ASSET, note_idx, pad(11)] | ||
|
|
||
| dupw dup.8 movdn.4 |
There was a problem hiding this comment.
[P2] Update the remove-then-add example to the two-word asset ABI
The example treats remove_asset's return as the asset being transferred and passes only one asset word to add_asset. In v0.16 remove_asset consumes ASSET_ID and ASSET_VALUE and returns FINAL_ASSET_VALUE, the balance remaining in the vault; output_note::add_asset requires the original ASSET_ID, ASSET_VALUE and note index. The shown duplication and comments consequently describe the wrong data and stack shape. Preserve the original two-word asset before removal, discard the returned remainder as appropriate, and rebuild the add_asset arguments.
Validation: Checked both protocol rc.9 and final v0.16 procedure signatures and implementations. No transaction was submitted for this example.
Sources: remove_asset ABI; add_asset ABI.
| const chainAnchor = await captureAnchor({ request: buildRequest }); | ||
| // ALWAYS preview/execute against `anchoredRequest`, never the value you passed in: | ||
| // re-resolving a request factory draws a fresh serial number from the client RNG | ||
| // and builds a different transaction than the anchor pins. | ||
| const txSummary = await preview({ accountId, request: anchoredRequest!, anchor: chainAnchor }); |
There was a problem hiding this comment.
[P2] Preview the same local request that was anchored
Awaiting captureAnchor does not update the anchoredRequest binding captured by the current React render. On first use this example passes null to preview despite the non-null assertion; on a subsequent capture it passes the previous request together with the new anchor. Resolve the factory once into a local request, pass that request to captureAnchor, and pass the same value to preview/execution, or wait for the new hook state in a later render. The release hook documentation explicitly demonstrates the local-request approach.
Validation: Ran the actual rc.7 and final useChainAnchor hook implementations with React and a fake client/provider. The documented flow anchored request 1 but previewed null, then anchored 2 but previewed 1; the corrected flow used request 3 for both.
| **2. You must bring up the rayon thread pool yourself.** Every MT entry | ||
| re-exports `initThreadPool(n)` from `wasm-bindgen-rayon`. **The React SDK does | ||
| NOT call it for you.** Skip it and rayon spawns zero workers, every | ||
| `par_iter(...)` falls through to a sequential loop, and you have paid the full | ||
| COOP/COEP deployment cost to prove single-threaded anyway. |
There was a problem hiding this comment.
[P2] Account for the SDK's automatic worker thread-pool initialization
The default worker-backed MT client already passes the hardware thread count to its worker, which calls wasm.initThreadPool before creating the client. Consequently the claim that React requires this main-thread initialization or proves sequentially is false. The proposed startup/effect creates a pool in a different WASM instance and does not configure the instance performing proofs. Remove the mandatory boot component for the default worker path and distinguish any explicitly non-worker usage that needs separate setup; align frontend-pitfalls with wasm-bridge's existing correct guidance.
Validation: Verified the rc.7 and final client initialization/worker implementations. This is a source-based finding; browser performance and memory usage were not measured.
Sources: Thread count passed to worker; Automatic pool initialization.
| | `active_account::has_non_fungible_asset(asset)` | `active_account::has_asset(asset_id: Word) -> bool` | | ||
| | `faucet::create_fungible_asset` / `create_non_fungible_asset` / `has_callbacks`, and the whole `asset` module | build the `Asset` outside the transaction; only `faucet::mint(Asset)` and `faucet::burn(Asset)` remain | | ||
| | `AttachmentLocation` | `Option<u32>` from `find_attachment` | | ||
| | `output_note::set_attachment` | shape-specific setters (`set_word_attachment`, `set_array_attachment`) | |
There was a problem hiding this comment.
[P2] Use the append-based output-note attachment API
The migration table replaces set_attachment with set_word_attachment/set_array_attachment, but those setters are absent from the pinned miden 0.14.0 guest bindings. The replacement APIs are add_word_attachment, add_attachment and add_attachment_from_memory. Update the table to those signatures and explain append semantics; a developer following the current advice gets an unresolved function instead of a migrated attachment call.
Validation: Checked the tagged guest bindings and SDK migration document.
Sources: Attachment bindings.
| Cross-component dependencies go in `miden-project.toml`'s `[dependencies]` — never in `Cargo.toml`, | ||
| which the macros read only for `[package] name` / `description`. The | ||
| `[package.metadata.miden.dependencies].<name>.wit` key is an **optional override**; without it the | ||
| macro searches `<dep-root>`, `<dep-root>/wit` and `<dep-root>/target/generated-wit`. It becomes | ||
| mandatory only when the dependency points at a `.masp` file rather than a directory. |
There was a problem hiding this comment.
[P2] Align dependency resolution with embedded WIT and the package cache
This section still says to search generated-wit directories and requires an explicit wit path for .masp dependencies. The pinned macro instead uses embedded package WIT and rejects an explicit wit override when embedded WIT exists, including for .masp files. Apply the correction already made in rust-sdk-patterns consistently here. Also qualify that file's 'entry is all you need' claim: plain cargo check/build and IDE analysis with source dependencies require the release's build.rs calling prepare_package_cache and matching miden-sdk-build-script-support dependency, because the macro resolves source dependencies through MIDENC_PACKAGE_CACHE.
Validation: Verified macro resolution/error paths, the missing-cache regression test, the SDK migration document, and the release example build.rs. Did not run a full Rust contract build.
Sources: Dependency package resolver; Plain-Cargo migration requirements; Example build script.
| Run with the `--debug` flag to see output: | ||
|
|
||
| ```bash | ||
| miden-vm run program.masm --release | ||
| miden-vm run program.masm --debug |
There was a problem hiding this comment.
[P2] Replace the removed debug instruction and unsupported CLI flag
The new run command recommends --debug, but the pinned miden-vm run command does not expose that flag. The section's debug.stack/debug.stack.N instructions are also rejected by the current assembler. Use the release's miden::core::debug procedures, such as exec.debug::print_stack, and a command supported by the CLI. Remove the assertion that omitting --debug silently strips these calls; an ordinary procedure call has different semantics.
Validation: Confirmed the v0.29.1 RunCmd arguments and debug library source; both rc.7 and final v0.16 reject debug.stack during compilation.
Sources: VM run arguments; Debug library.
| A reusable account component must not bake a storage-slot index into its procedure bodies. The same component can be installed into many accounts, each mapping it to a different slot, so a hard-coded slot index only works for one layout. | ||
|
|
||
| This is the pattern the standard `array` / `double_word_array` data-structure utilities use: `get(slot_id_suffix, slot_id_prefix, index)` takes the slot id as input so one utility can drive many different maps. | ||
|
|
||
| This rule does **not** apply to a component referencing its **own dedicated named slot**. That is already correct and portable — see "When NOT to parameterize" below. | ||
| Instead, take the storage slot as a parameter — the slot id, split into its `slot_id_prefix` / `slot_id_suffix` felts — and pass it into the storage-access procedure (`active_account::get_item` / `get_map_item`, `native_account::set_item` / `set_map_item`). The account-level glue procedure that knows the real layout supplies the slot id. |
There was a problem hiding this comment.
[P2] Preserve stable named slots owned by a component
The rule assumes installation remaps each component's storage slots to account-specific numeric positions. v0.16 slot IDs are hashes of slot names and remain stable across accounts. A component accessing its own named AUTHORITY_SLOT is therefore valid, as the release's authority component does; the 'bad' example should not be forbidden. Restore the distinction between component-owned named slots and generic helpers that intentionally accept a caller-selected slot, so this skill does not require unnecessary interface changes when migrating working components.
Validation: Checked StorageSlotId derivation and the standard authority component in protocol rc.9/final. This finding concerns incorrect migration guidance, not a demonstrated transaction failure.
Sources: Stable named-slot IDs; Authority component named slot.
2 participants
Review-only draft — migrate the agent-tools skills to Miden v0.16. For independent audit; not for merge as-is.
What this does
Brings all 33
SKILL.mdinskills/onto Miden v0.16, with every version-sensitive claim proven against real pinned source and nothing carried over that v0.16 invalidated. The prior state was the v0.15 sync (PR #12).Classification (per the task's decided rules):
protocolorigin/next(the MASM family + shared skills), then spot-verified at the pins.masm-proc-type-signatures(the only protocol skill in scope; the ~17 generic-Rust/repo-hygiene ones were deliberately skipped).Pins (all resolve)
protocol v0.16.0-rc.6·miden-client v0.16.0-rc.2·web-sdk v0.16.0-rc.3·miden-vm v0.29.1·compiler sdk/v0.14.0-rc.1(contract SDK — note thesdk/tag prefix) ·midenc v0.10.0-rc.1.Verification
rg -n "0\.15" skills/→ no matches ·ls skills | wc -l→ 33.<repo>:<tag>:<path>a skill cites exists (the anti-fabrication gate).felt-constructionupstream is stale,u32assert4doesn't exist →u32assertw,dyncall→dynexec).Framing scrub (extra pass beyond the task)
Swept for redundant historical narration and rewrote it as current-state, without over-scrubbing (version pins and "there is no X" footguns are kept). Removed e.g. "the mock shapes that changed in v0.16", "what used to be called
AssetIdis nowAssetClass", "the crate formerly published as …", and "the oldAssetId/four-word/link_*_library" phrasings — the technical facts and the trap warnings stay, only the "old code" framing a v0.16-native reader can't use is gone.Scope
skills/**+README.md.AGENTS.mdis a harness-injected file and is git-excluded (not in this diff). No source repo touched — skills only.Known non-blocking follow-ups (from the audit)
rust-sdk-pitfallswide-call diagnostics conflate two separate compiler checks (precision).Base
Off
main(1283946). Nothing else on the branch.