Skip to content
This repository was archived by the owner on Apr 25, 2026. It is now read-only.

fix: update process_schedule_delegator_unstake#1049

Merged
1xstj merged 2 commits into
mainfrom
fix/delegation-unstake-validation-bug
Aug 7, 2025
Merged

fix: update process_schedule_delegator_unstake#1049
1xstj merged 2 commits into
mainfrom
fix/delegation-unstake-validation-bug

Conversation

@1xstj
Copy link
Copy Markdown
Contributor

@1xstj 1xstj commented Aug 7, 2025

@1xstj 1xstj marked this pull request as ready for review August 7, 2025 22:22
@1xstj 1xstj requested a review from devpavan04 August 7, 2025 22:22
@claude
Copy link
Copy Markdown

claude Bot commented Aug 7, 2025

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


Code Review Complete ✅

I've analyzed the changes in this PR and they look good. Here's my comprehensive review:

Todo List:

  • Read repository guidelines (CLAUDE.md)
  • Analyze the code changes in delegate.rs
  • Review the new test cases in delegate.rs
  • Check for potential bugs and security issues
  • Evaluate performance implications
  • Assess documentation needs
  • Provide comprehensive review feedback

Summary of Changes

This PR fixes a bug in process_schedule_delegator_unstake where pending nomination unstake requests were incorrectly included in the calculation of available delegation balance.

Change made: Added && !r.is_nomination filter on line 242 in pallets/multi-asset-delegation/src/functions/delegate.rs

Code Quality & Best Practices

  • Logic Fix: The change correctly filters out nomination unstake requests when calculating available delegation balance
  • Consistency: The fix aligns with similar patterns used elsewhere in the codebase (e.g., lines 235, 645, 946)
  • Minimal Impact: Single-line change with clear intent and no side effects
  • Code Style: Follows existing formatting and naming conventions

Bug Analysis

The original bug occurred because:

  1. Regular delegations and nomination delegations both use the same asset type (Asset::Custom(0))
  2. The filter was matching ALL unstake requests for that operator/asset combination
  3. This caused nomination unstake amounts to reduce available delegation balance incorrectly

Impact: This could prevent valid delegation unstaking when nomination unstakes were pending, potentially locking funds unnecessarily.

Security Implications

  • No Security Issues: The fix reduces restrictions rather than bypassing them
  • Separation of Concerns: Properly isolates regular delegation logic from nomination logic
  • No Economic Attacks: Cannot be exploited to drain funds or bypass limits
  • Substrate Best Practices: Maintains proper error handling and validation

Performance Considerations

  • Minimal Impact: Adds one boolean check per iteration - negligible overhead
  • O(n) Complexity: Still linear time complexity through unstake requests (unchanged)
  • No Additional Storage: No new storage reads/writes introduced

Test Coverage

The new test delegation_unstake_bug_with_nomination_pending is excellent:

  • Reproduces the Issue: Sets up both delegation and nomination with pending unstake
  • Verifies Fix: Confirms delegation unstaking works despite pending nomination unstake
  • Comprehensive Setup: Tests the exact scenario that was failing
  • Clear Assertions: Validates both requests exist and are properly separated

Documentation

Current documentation is sufficient:

  • Function comments already explain the validation logic
  • The fix is self-explanatory and doesn't change the public interface
  • Test case serves as additional documentation of the behavior

🔍 Inline Comments

Line 242 (pallets/multi-asset-delegation/src/functions/delegate.rs):

.filter(|r| r.operator == operator && r.asset == asset && !r.is_nomination)

Perfect fix - The added && !r.is_nomination filter correctly excludes nomination unstake requests from delegation balance calculations.

Lines 941-1036 (pallets/multi-asset-delegation/src/tests/delegate.rs):
Excellent test coverage - The new test case thoroughly validates the fix by:

  • Creating both regular delegations and nominations
  • Scheduling unstake for nominations first
  • Verifying regular delegation unstake still works correctly

Final Assessment: APPROVED

This is a well-executed bug fix that:

  • ✅ Addresses a legitimate issue with clear impact
  • ✅ Uses minimal, targeted changes
  • ✅ Includes comprehensive test coverage
  • ✅ Maintains security and performance characteristics
  • ✅ Follows best practices for Substrate development

Recommendation: This PR is ready to merge.


@1xstj 1xstj merged commit 3842182 into main Aug 7, 2025
8 checks passed
@1xstj 1xstj deleted the fix/delegation-unstake-validation-bug branch August 7, 2025 22:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] process_schedule_delegator_unstake incorrectly includes nomination unstake requests in deposited asset validation

3 participants