Conversation
|
Thank you! Fixed. |
…:Felt` and remove `miden-core` dependency
next branch commit
c3a8256 to
290fbed
Compare
|
@Keinberger Could you please do another round? I've removed some helpers and used the new mockchain API. |
Keinberger
left a comment
There was a problem hiding this comment.
Hey @greenhat, looking good, but there are two SKILL issue swe should get rid of before merging.
Left inline comments below with specifics.
Nothing else blocking from my side. Minor nits while we're at it:
- Typo "Crate" -> "Create" on
counter_test.rs:23 - Commented-out debug block at
increment_count.rs:92-95
There was a problem hiding this comment.
The test was rewritten to use MockChain::builder() + AccountComponent::from_package + add_account_from_builder + NoteBuilder, but this skill file still references APIs that no longer exist in this PR: create_testing_account_from_package, create_testing_note_from_package, NoteCreationConfig, StorageSlot::with_map / StorageSlot::with_value, StorageSlotName, AccountCreationConfig, and apply_delta().
The stale API references appear on multiple lines between 51 and 162 (and the validation checklist mentions apply_delta() which the new test doesn't use). The counter_test.rs line number references throughout the file are also off since the test was rewritten.
There was a problem hiding this comment.
We have duplicate pitfall numbers from the PR #36 merge. P8 appears twice:
- Line 138:
P8: Recipient::compute Was Removed - Line 183:
P8: NoteType Variants Unavailable in Compiler SDK
Same with P9:
- Line 155:
P9: P2ID Note Root Hardcoding - Line 197:
P9: Note Scripts Cannot Call Native Account Functions
Also the (see P8) cross-reference in the original P9 body is now ambiguous. I think renumbering the PR #36 pitfalls to P10/P11/P12 (since there's already a P10 at line 205, it'd need to be P11/P12/P13) fixes both in one pass.
Refresh the testing guidance to match `InitStorageData`, `AccountComponent::from_package`, `NoteBuilder`, and `RawOutputNote` flows. Also fix the duplicated pitfall numbering and clean up the review nits in the integration example and test.
|
Thank you! Fixed. Please do another round. |
Keinberger
left a comment
There was a problem hiding this comment.
Hey @greenhat, one small thing in .claude/skills/rust-sdk-source-guide/SKILL.md: the clone instruction points to keinberger/miden-bank, but the in-skill links elsewhere all reference 0xMiden/tutorials/blob/main/examples/miden-bank/. We should point the clone to 0xMiden/tutorials as well for consistency. The old repo under my personal github name is outdated.
Otherwise the PR looks good to me, approving!
TODO:
cargo-midenand move git tag;miden-clientv0.14;