feat(margin): add asset type info to liquidation and withdraw events

[avernikoz]

Updates LiquidationEvent and WithdrawCollateralEvent to include base_asset_type and quote_asset_type fields. Additionally adds debt_is_base to LiquidationEvent. This provides necessary context for tracking which pair generated the event without needing to query the pool state.

[tonylee08]

@claude

[claude]

Code Review - PR #751

Summary

This PR adds asset type information (base_asset_type, quote_asset_type) to LiquidationEvent and WithdrawCollateralEvent, plus a debt_is_base field to LiquidationEvent. This enhancement improves event observability by allowing off-chain systems to identify the trading pair without additional pool state queries.

✅ Positive Observations

1. Consistent Pattern: The use of type_name::with_defining_ids is consistent with existing patterns throughout the codebase (e.g., in pool_proxy.move, margin_pool.move, and other event emissions).

2. Clear Value Add: The additional fields provide necessary context for event indexers and analytics systems, reducing the need for supplementary queries.

3. Minimal Surface Area: The change is focused and doesn't introduce unnecessary complexity - only 10 additions, no deletions.

4. Proper Event Structure: All events maintain the 'has copy, drop' abilities as required for event structs in Sui Move.

📝 Minor Observations

1. Field Placement in WithdrawCollateralEvent: The new fields are inserted between withdraw_base_asset and remaining_base_asset. While this works, be aware that:

   • Event field ordering changes can affect off-chain indexers that rely on positional parsing
   • Consider documenting this as a breaking change for event consumers if applicable
   • Alternative: placing new fields at the end (before timestamp) would be less disruptive, though the current placement groups related fields logically

2. Test Coverage: I didn't find explicit event validation tests in the test suite. Consider adding tests that:

   • Verify the emitted events contain the correct asset type information
   • Validate that debt_is_base correctly reflects the debt asset in liquidation scenarios

3. Documentation: The struct comments are clear, but consider adding inline comments explaining:

   • That debt_is_base indicates whether the liquidated debt was denominated in the base asset
   • That asset types use with_defining_ids to include package and module information for full type resolution

🔒 Security & Correctness

✅ No security concerns identified:

• The added fields are read-only information derived from type parameters
• type_name::with_defining_ids is a safe, deterministic function
• No changes to access control, state mutations, or critical logic paths

✅ Correctness:

• The debt_is_base field correctly mirrors the existing logic at margin_manager.move:718
• Asset type fields properly use the same type parameters as the function signatures

🎯 Performance

✅ No performance concerns:

• type_name::with_defining_ids is a compile-time/lightweight runtime operation
• Event emissions don't affect transaction gas significantly
• The additional fields add minimal data overhead

📊 Overall Assessment

Recommendation: Approve with minor suggestions

The changes are well-implemented and follow existing patterns. The main consideration is whether the field ordering change in events is acceptable for your off-chain infrastructure. If you have event consumers that depend on field positions, you may want to add a migration notice.

Action Items (Optional)

• Consider adding event validation tests
• Document the event schema change in release notes if you have off-chain consumers
• Add inline comments explaining debt_is_base semantics if helpful for future maintainers

Great work on improving event observability! 🚀