fix(skills): update Rust SDK skills with Miden Day feedback#36
fix(skills): update Rust SDK skills with Miden Day feedback#36
Conversation
greenhat
left a comment
There was a problem hiding this comment.
Looks good overall. Please check my notes.
|
|
||
| **Constructor**: `Asset::new(word)` creates an Asset from a Word. | ||
|
|
||
| See [miden-bank bank-account](../../../../miden-bank/contracts/bank-account/src/lib.rs) for complete asset handling patterns including deposit, withdrawal, and balance tracking. |
There was a problem hiding this comment.
I don't get the path here. Do we expect an agent to clone miden-bank there?
There was a problem hiding this comment.
The same goes for the same paths introduced below.
There was a problem hiding this comment.
Yeah the idea is to have agents have access to the miden-bank code, working examples turn out to be incredibly helpful for them. I'll update the path to a Github link.
| **Key details:** | ||
|
|
||
| - `Recipient::compute(serial_num: Word, script_digest: Digest, inputs: Vec<Felt>)` — the second parameter is `Digest`, not `Word`. | ||
| - `Digest` does not implement `Copy`. Use `.clone()` when reusing a digest in loops or across calls. |
There was a problem hiding this comment.
Do we really want to do that much hand-holding in our skills? I mean, if some weak model is incapable of figuring this out, I bet it would fail to build any meaningful solution anyway. SOTA models were capable of fixing it a year ago from the compiler build errors.
There was a problem hiding this comment.
Fair enough, removing the padding detail.
| ``` | ||
| **Key details:** | ||
|
|
||
| - `Recipient::compute(serial_num: Word, script_digest: Digest, inputs: Vec<Felt>)` — the second parameter is `Digest`, not `Word`. |
There was a problem hiding this comment.
Agreed, removing the entire Recipient::compute section in favor of #37's note::build_recipient.
|
|
||
| - `Recipient::compute(serial_num: Word, script_digest: Digest, inputs: Vec<Felt>)` — the second parameter is `Digest`, not `Word`. | ||
| - `Digest` does not implement `Copy`. Use `.clone()` when reusing a digest in loops or across calls. | ||
| - P2ID inputs must be padded to 8 elements: `[suffix, prefix, 0, 0, 0, 0, 0, 0]`. |
There was a problem hiding this comment.
That is not true with note::build_recipient.
There was a problem hiding this comment.
Removing, only applies to the old API.
| - `Recipient::compute(serial_num: Word, script_digest: Digest, inputs: Vec<Felt>)` — the second parameter is `Digest`, not `Word`. | ||
| - `Digest` does not implement `Copy`. Use `.clone()` when reusing a digest in loops or across calls. | ||
| - P2ID inputs must be padded to 8 elements: `[suffix, prefix, 0, 0, 0, 0, 0, 0]`. | ||
| - In host/test code, use `NoteRecipient` (from miden-client) instead of `Recipient` for constructing notes. |
There was a problem hiding this comment.
We already have NoteRecipient used in code in #37. I would rather not put in skills what can be deduced from the code.
There was a problem hiding this comment.
Agreed, the code examples show it.
| Notes receive data via inputs (Vec<Felt>), accessed with `active_note::get_inputs()`: | ||
| Notes receive data via inputs (Vec<Felt>), accessed with `active_note::get_inputs()`. | ||
|
|
||
| **Requires alloc**: Since `get_inputs()` returns `Vec<Felt>`, you must have `extern crate alloc;` and `use alloc::vec::Vec;` in your `#![no_std]` contract. See the [no-std setup in any contract](../../../contracts/counter-account/src/lib.rs). |
There was a problem hiding this comment.
As mentioned above, we should not cater to models that cannot figure this out on their own.
There was a problem hiding this comment.
Agreed, removing.
|
|
||
| **NoteType for P2ID**: P2ID output notes created in contract code should use `NoteType::Private` (value 2, see P10). Using `NoteType::Public` triggers an opaque "missing details in advice provider" error at execution time. See [miden-bank withdraw](../../../../miden-bank/contracts/bank-account/src/lib.rs) for the working pattern. | ||
|
|
||
| ## P8: Felt::new() Returns Result in Contract Code |
There was a problem hiding this comment.
Not anymore. Now it's in sync with the off-chain API and wraparound.
There was a problem hiding this comment.
Got it, removing P8.
|
|
||
| **Severity**: Medium -- causes compilation errors | ||
|
|
||
| `Value::read()` is generic over `V: From<Word>`, so an explicit type annotation is mandatory. Omitting it causes a type inference error. |
There was a problem hiding this comment.
No longer relevant for the new account storage API.
|
|
||
| **Severity**: Medium -- causes compilation errors | ||
|
|
||
| In contract code (compiler SDK), only `as_u64()` exists for converting Felt to integer. `as_int()` is available in host/test code only. `as_u32()` does not exist. For construction, `Felt::from_u32()` is available. |
There was a problem hiding this comment.
No longer true with the new Felt API.
|
|
||
| **Severity**: Low -- causes incorrect architecture | ||
|
|
||
| Note inputs (`active_note::get_inputs()`) are baked at note creation time and cannot be modified after creation. Design note input layouts carefully before deployment. |
There was a problem hiding this comment.
| Note inputs (`active_note::get_inputs()`) are baked at note creation time and cannot be modified after creation. Design note input layouts carefully before deployment. | |
| Note inputs (`active_note::note_storage()`) are baked at note creation time and cannot be modified after creation. Design note input layouts carefully before deployment. |
There was a problem hiding this comment.
Okey, keeping P14 with updated active_note::get_storage() wording.
Hey @greenhat, thanks for the review! I'll push an updated commit that strips the outdated content. Here's what I'm planning: Updating:
Keeping (unique content not in #37):
I'll also double-check P10 and P11 against v0.14 and remove them if they're no longer relevant. Thanks! |
Context
During Miden Day (2025-03-17), 11 builders used the agentic template and found incorrect API references, missing patterns, and misleading guidance in the Rust SDK skill files. Issues #28, #29, #30 capture the verified corrections. All changes validated against local source repos on main.
Changes
rust-sdk-pitfalls (#28)
rust-sdk-patterns (#29)
rust-sdk-testing-patterns (#30)
Closes #28, closes #29, closes #30