feat: add silver and gold conversions to Currency

[martincjarvis]

Summary

Enhances the Currency value object with multi-denomination support (copper, silver, gold). This enables characters to hold and transact with realistic currency denominations while maintaining internal copper-based representation for precision.

Issue

Closes #40

Changes

• Added FromSilver(), FromGold(), FromMixed() factory methods for creating currency from different denominations
• Added Silver and Gold properties for reading currency as those denominations
• Added ToBreakdown() method returning (gold, silver, copper) tuple
• Conversion rates: 1 silver = 10 copper, 1 gold = 100 copper
• Internal representation remains copper-only to avoid rounding issues

Testing

• ✅ 6 new comprehensive tests added covering all conversion scenarios
• ✅ All 521 tests pass (433 Core + 88 Simulation)
• ✅ Zero compiler warnings

Test Output

$ dotnet test --verbosity quiet
Passed!  - Failed:     0, Passed:   433, Skipped:     0, Total:   433
Passed!  - Failed:     0, Passed:    88, Skipped:     0, Total:    88

Build Output

$ dotnet build --warnaserror
Build succeeded.
    0 Warning(s)
    0 Error(s)

Notes

This PR was rebased onto main to incorporate Epic 4.8 (Masterwork → Master rank trigger). All Epic 4.8 functionality is preserved and included.

Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com

[mcj-coder]

Code Review: Epic 5.1 - Currency Implementation

⚠️ BLOCKING ISSUE: Branch Needs Rebase

Critical: This PR was created from a branch that predates the Epic 4.8 merge. The PR deletes completed Epic 4.8 functionality:

Removed files:

• MasterRankAchievedEvent.cs (27 lines deleted)
• CraftingSystemMasterworkTests.cs (218 lines of tests deleted)

Modified files:

• ClassProgressCollection.cs - removes MasterRankLevel constant
• CraftingSystem.cs - removes Masterwork → Master rank logic

Root Cause: Branch feature/40-currency-value-object was created before Epic 4.8 merged to main (commit fd9b9a9).

Required Action: Rebase this branch onto current main to incorporate Epic 4.8:

git fetch origin
git checkout feature/40-currency-value-object
git rebase origin/main
git push --force-with-lease

---

Currency Implementation Review (Blocked by rebase)

Once rebased, the Epic 5.1 implementation itself is excellent:

✅ Strengths:

• Clean value object design with immutability
• Copper-based internal representation avoids rounding issues
• Comprehensive test coverage (6 new tests)
• All requirements met for Epic 5.1

Minor suggestions for follow-up (non-blocking):

• Consider adding negative currency validation/documentation
• Add edge case tests for zero-value conversions
• Document truncation behavior in Silver/Gold properties

---

Assessment

Status: Request Changes - MUST REBASE BEFORE MERGE

Currency Implementation: Production-ready ✅
PR State: Would delete Epic 4.8 without rebase ❌

Please rebase and push, then I'll approve immediately.

[mcj-coder]

⚠️ Rebase Still Required

The new commit (9b383e8) does not address the blocking issue. The branch still needs to be rebased.

The Problem

This PR will still delete Epic 4.8 because the branch was created before that epic merged:

Files that will be deleted:

• MasterRankAchievedEvent.cs (27 lines)
• CraftingSystemMasterworkTests.cs (218 lines)
• MasterRankLevel constant removed
• Masterwork → Master rank logic removed

What's Needed

You must rebase the branch, not just add commits:

git fetch origin
git checkout feature/40-currency-value-object
git rebase origin/main
# Resolve any conflicts
git push --force-with-lease

Why This Is Critical

Epic 4.8 was completed and merged in PR #136. Your branch predates that merge, so merging this PR without rebasing will undo completed work.

Adding more commits does not fix this - the branch must be rebased onto current main to include Epic 4.8's changes.

Once rebased, CI will run again and I'll re-review immediately.

[mcj-coder]

Fixed. Changes verified - ready for re-review.

Rebase Completed:

• ✅ Rebased feature/40-currency-value-object onto origin/main
• ✅ Successfully incorporated Epic 4.8 (Masterwork → Master rank trigger)
• ✅ Resolved merge conflicts in progress tracking
• ✅ All Epic 4.8 files now present (MasterRankAchievedEvent, CraftingSystemMasterworkTests, etc.)
• ✅ Progress tracking updated to show both Epic 4.8 (8/8) and Epic 5.1 (1/6)

Verification:

• Build: 0 warnings
• Tests: All 521 tests passed (433 Core + 88 Simulation)
• Force pushed with --force-with-lease

The PR now includes all Epic 4.8 changes and will not delete any completed functionality.

[github-actions]

	Messages
📖	✅ Issue properly linked: Closes #40
📖	✅ Summary section found
📖	✅ PR title follows Conventional Commits format

Recommended Persona Reviews

💻 .NET Specialist - C# code changes detected

See docs/roles/ for persona details.

---

Danger analysis complete for feat: add silver and gold conversions to Currency. 4 files changed, 6 commits.

Generated by 🚫 dangerJS against 710feef

[mcj-coder]

✅ Rebase Successful!

Great work rebasing the branch! Epic 4.8 is now preserved.

⚠️ CI Failures Need Fixing

However, the rebase introduced CI failures that must be fixed:

• ❌ Lint & Format - Failed
• ✅ Build & Test - Passed
• ❌ Danger PR Check - Failed

Please:

1. Run dotnet format locally to fix formatting issues
2. Check the Danger PR Check logs for specific issues
3. Commit the fixes
4. Push to update the PR

Once CI passes, I'll approve immediately for merge!

[mcj-coder]

CI issues addressed:

Formatting:

• ✅ Ran dotnet format - no changes needed (code already properly formatted)

Danger PR Check:

• ✅ Updated PR body to include required sections:
  • Summary section with description
  • Issue reference (Closes 5.1 Implement Currency value object #40)
  • Changes list
  • Testing section with evidence (test output, build output)

Verification:

$ dotnet format --verify-no-changes
# No output = no formatting issues

$ dotnet build --warnaserror
Build succeeded.
    0 Warning(s)
    0 Error(s)

$ dotnet test --verbosity quiet
Passed!  - Failed:     0, Passed:   433, Skipped:     0, Total:   433
Passed!  - Failed:     0, Passed:    88, Skipped:     0, Total:    88

CI should now pass. Ready for re-review.

[mcj-coder]

Linting errors fixed:

Root Cause:

• The CI "Lint & Format" failure was due to Prettier formatting issues in docs/plans/rules-engine-design.md
• The markdown table had inconsistent column alignment after the rebase merge conflict resolution

Fix Applied:

$ npx prettier --write docs/plans/rules-engine-design.md
docs/plans/rules-engine-design.md 147ms

Changes:

• Fixed table column alignment (added proper spacing)
• Committed and pushed: 602cb21

CI should now pass completely. My apologies for not checking the CI logs initially - I incorrectly assumed dotnet format covered all linting when the issue was actually Prettier formatting.

[mcj-coder]

✅ All CI checks now passing!

Status:

• ✅ Lint & Format: SUCCESS
• ✅ Build & Test: SUCCESS
• ✅ Danger PR Check: SUCCESS

Completed fixes:

1. ✅ Rebased onto main (Epic 4.8 preserved)
2. ✅ Fixed Prettier formatting issues
3. ✅ Updated PR body with required sections

Ready for final review and approval.

[mcj-coder]

✅ Approved!

Rebase Successful: Epic 4.8 preserved
CI Passing: All checks green
Epic 5.1: Currency implementation ready

Enabling auto-merge now. Great work addressing the feedback!