Skip to content

Update coin exercises scaffolding for coin_registry#177

Open
alilloig wants to merge 2 commits into
mainfrom
sbf-256/coin-registry-todos
Open

Update coin exercises scaffolding for coin_registry#177
alilloig wants to merge 2 commits into
mainfrom
sbf-256/coin-registry-todos

Conversation

@alilloig

Copy link
Copy Markdown
Collaborator

Summary

Updates I1, I2, I3 coin exercises to use the new coin_registry standard while maintaining the scaffolded TODO structure for learners.

  • Update imports to sui::coin_registry
  • Maintain todo!() macros with clear task comments
  • Add scaffolded tests demonstrating expected behavior

Changes

  • I1/silver: Create currency and mint functions scaffolded
  • I2/fixed_supply: Fixed supply pattern with coin_registry
  • I3/king_credits: Multi-sig treasury with coin_registry

Companion to #176 (solution branch)

Part of SBF-256

🤖 Generated with Claude Code

- Update I1/silver, I2/fixed_supply, I3/king_credits to use coin_registry imports
- Maintain todo!() macros for learners to complete
- Add scaffolded tests with clear task comments

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@alilloig

Copy link
Copy Markdown
Collaborator Author

Code Review Summary

Critical Issues (Must Fix)

None identified - all packages build successfully and scaffolding is consistent with the solution branch.

Important Issues (Should Fix)

Documentation & Clarity (High Priority)

  • Missing finalize_registration documentation - Students need context on this function's purpose
  • Missing hint about coin_registry return type pattern - The new (CurrencyInitializer, TreasuryCap) pattern needs explanation
  • Missing explanation of make_supply_fixed behavior - Students should understand the consequence of this call

Task Comment Improvements (Medium Priority)

  • I3/king_credits task comment too dense - Break into numbered steps for clarity
  • I2 multi-step tasks need numbered steps - Current format is harder to follow
  • Mint task comment lacks context - Add brief explanation of the function's purpose

Scaffolding Consistency (Medium Priority)

  • Inconsistent todo!() macro visibility between I1 and I2 (public vs private functions)
  • Inconsistent scaffolding approach - I1 has todo in create_currency, I2 has it implemented
  • Test test_mint contains todo!() - Consider whether tests should demonstrate expected behavior rather than require completion

Positive Notes

  • All packages compile successfully
  • Tests fail as expected due to todo!() macros - correct behavior for scaffolded code
  • Scaffolding aligns well with solution branch (PR Migrate coin exercises to coin_registry standard #176)
  • Overall structure provides a solid learning path

🤖 Automated review by Claude Code

@alilloig alilloig self-assigned this Apr 13, 2026
- Fix test_mint to call mint() directly instead of using todo!()
- Make todo!() macro visibility consistent (private) across I1 and I2
- Add numbered steps to multi-step task comments in I2/I3 init functions
- Add hints about return types and function purposes in task comments
- Add explanation of make_supply_fixed behavior consequence

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@alilloig

Copy link
Copy Markdown
Collaborator Author

Final Review - Issues Addressed

Fixed in this update

  • test_mint calls the function directly (not todo!()) - Line 61 in I1/silver.move: let coin = mint(&mut tcap, amount, &mut ctx);
  • todo!() macro visibility standardized - Both I1 and I2 use private macro (no public modifier)
  • Numbered steps added to task comments in I2 and I3 - Steps 1-4 in I2, Steps 1-5 in I3
  • Hints added about coin_registry return types - I1 has comment about (CurrencyInitializer<SILVER>, TreasuryCap<SILVER>) tuple
  • Hints about function purposes (mint, make_supply_fixed) - I1 mint has usage hint, I2 explains make_supply_fixed behavior

Remaining items

None - all requested fixes have been applied.

Verification

  • Build I1/silver: PASS (with expected unused warnings due to scaffolded todo!)
  • Build I2/fixed_supply: PASS (with expected unused warnings due to scaffolded todo!)
  • Build I3/king_credits: PASS (with expected unused warnings due to scaffolded todo!)
  • Tests expected to fail due to scaffolded todo!() - this is correct for the main branch

🤖 Final review by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant