chore: change credits to on-delegate

[1xstj]

Summary of changes
Changes introduced in this pull request:

• Change credits awarded to on-delegate

Reference issue to close (if applicable)
Closes #1044

[claude]

Claude finished @1xstj's task —— View job

---

PR Review: Change Credits to On-Delegate

Review Checklist:

• Analyze code changes across 12 files
• Review for code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security implications for Substrate blockchain
• Address existing review comment about naming
• Provide specific inline feedback

🔍 Summary

This PR implements a significant architectural change: credits are now awarded when delegation occurs (on-delegate), rather than during deposit. This is a cleaner separation of concerns and better reflects the actual staking behavior.

💡 Key Changes Analysis

Core Logic Changes

• pallets/multi-asset-delegation/src/functions/delegate.rs:171-172: Added credit recording during delegation with lock multiplier support
• pallets/multi-asset-delegation/src/functions/delegate.rs:414-415: Added undelegation credit recording during unstake execution
• pallets/multi-asset-delegation/src/functions/delegate.rs:750-755: Added credit recording for nomination undelegation
• pallets/multi-asset-delegation/src/functions/deposit.rs: Removed credit recording from deposit operations (lines removed from deposit flow)

Test Updates

• Tests now verify credits are recorded during delegation, not deposit
• Mock implementations updated to track delegate/undelegate calls properly
• Comprehensive test coverage for the new behavior

✅ Positive Aspects

1. Better Separation of Concerns: Credits now directly correlate with delegation activity rather than deposits
2. Consistent Architecture: Both regular and nomination delegations follow the same credit recording pattern
3. Comprehensive Testing: All test files properly updated to reflect the new behavior
4. Lock Multiplier Support: Credits correctly account for lock multipliers when available
5. Clean Undelegation Handling: Proper credit cleanup during unstake operations

🐛 Issues Found

Critical Issues

1. Function Naming Inconsistency (addresses existing review comment):
   • In pallets/multi-asset-delegation/src/tests/delegate.rs:74, the function is still called record_delegate_calls() but should be renamed to better reflect its purpose
   • Consider renaming to get_delegate_call_history() or similar for clarity

Minor Issues

2. Code Duplication: Similar credit recording logic appears in multiple places - consider extracting to a helper function
3. Missing Error Handling: Credit recording failures are silently ignored with let _ = T::RewardsManager::record_* - this could mask issues

🔧 Specific Recommendations

Immediate Actions Required

1. Address Naming Issue: As mentioned in drewstone's review comment, consider renaming record_delegate_calls() method for better clarity

Code Quality Improvements

2. Extract Common Logic:

   // Consider creating a helper function like:
   fn record_delegation_credits(
       who: &T::AccountId,
       operator: &T::AccountId,
       asset: Asset<T::AssetId>,
       amount: BalanceOf<T>,
       lock_multiplier: Option<LockMultiplier>,
       is_delegation: bool
   ) -> DispatchResult

3. Improve Error Handling:

   // Instead of:
   let _ = T::RewardsManager::record_delegate(/*...*/);
   
   // Consider:
   T::RewardsManager::record_delegate(/*...*/)
       .map_err(|e| log::warn!("Failed to record credits: {:?}", e))?;

🛡️ Security Assessment

• No security vulnerabilities identified
• Credit recording happens after state validation, maintaining consistency
• No potential for double-spending or manipulation
• Proper access control maintained throughout

🚀 Performance Impact

• Minimal performance impact: Credit recording moved from deposit to delegation (same number of operations)
• Actually improves efficiency by eliminating unnecessary credit operations for non-delegated deposits
• Lock multiplier calculation remains O(1)

📚 Documentation

The changes don't require additional documentation - the behavior is self-explanatory and well-tested.

✨ Overall Assessment

This is a well-executed architectural improvement that makes the codebase more logical and maintainable. The core change aligns credits with actual delegation behavior, which is more intuitive.

Recommendation: APPROVE after addressing the naming consistency issue mentioned in the existing review comment.

---

[drewstone]

This looks good, but I worry there's too few tests to ensure that credits expands as we expect. Do we have enough tests to ensure this is secure? If you think so please merge. If there are more tests to consider please add.