Refactor: Redirect surplus ICP to reward pool for stakers

[evanmcfarland]

Summary

Refactors surplus ICP handling to redirect deposits directly to stakers via REWARD_POOL instead of transferring to platform canister.

Key Changes

• Renamed: sweep_surplus_to_revshare() → process_surplus()
• Removed: External transfer logic to lbry_fun canister
• Added: Direct addition to REWARD_POOL with checked arithmetic
• Lowered: Threshold from 1 ICP (100M E8S) to 0.01 ICP (1M E8S)
• Removed: Operational buffer (not needed for internal accounting)
• Reordered: Call process_surplus() BEFORE distribute_reward() in timer
• Deleted: transfer_surplus_to_revshare() helper function
• Updated: All logging messages and comments

Architecture Benefits

1. Direct Staker Rewards: External ICP sent to icp_swap automatically flows to stakers
2. No Tokenomics Interference: No token buying/burning logic
3. Platform Still Gets Share: 1% flows through normal distribution mechanism
4. Cleaner Design: Internal accounting only, no external transfers
5. Better UX: Parent companies can fund rewards by sending to icp_swap

Safety Guarantees

• Checked arithmetic prevents overflow
• Lower threshold safe since no transfer fees for internal ops
• Sweep before distribution ensures deposits included in same cycle
• Comprehensive logging maintained

Test Plan

• Build verification: ./scripts/build.sh
• Manual testing: Send ICP to icp_swap canister
• Verify: ICP appears in REWARD_POOL via get_icp_pool_balance() query
• Verify: Next distribution includes deposited ICP
• Verify: Platform gets 1% of distribution as expected

🤖 Generated with Claude Code

[claude]

PR Review: Surplus ICP Refactoring

Summary

This PR refactors surplus ICP handling to redirect external deposits directly to stakers via the REWARD_POOL instead of transferring to the platform canister. The architectural change is clean and well-executed, but there are several critical issues that must be addressed before merging.

---

🔴 Critical Issues (P0)

1. Arithmetic Overflow in Expected Balance Calculation

Location: src/icp_swap/src/update.rs:1870

let expected_balance = reward_pool + uncollected_alex + total_staked + archived_balance;

Problem: This uses unchecked addition with four u64 values. If the sum exceeds u64::MAX, it will overflow and wrap around, causing actual_balance > expected_balance to incorrectly trigger, sweeping funds that shouldn't be swept.

Impact: Financial loss - could incorrectly identify legitimate funds as "surplus" and redirect them to rewards.

Fix: Use checked arithmetic:

let expected_balance = reward_pool
    .checked_add(uncollected_alex)
    .and_then(|sum| sum.checked_add(total_staked))
    .and_then(|sum| sum.checked_add(archived_balance))
    .ok_or_else(|| ExecutionError::AdditionOverflow {
        operation: "Calculating expected balance".to_string(),
        details: format\!("reward_pool: {}, uncollected_alex: {}, total_staked: {}, archived_balance: {}",
                        reward_pool, uncollected_alex, total_staked, archived_balance)
    })?;

2. Race Condition: Double Processing Risk

Location: Timer execution order in src/icp_swap/src/script.rs:347-384

Problem: The new order processes surplus BEFORE distribution:

1. process_surplus() - Adds to REWARD_POOL
2. distribute_reward() - Distributes from REWARD_POOL

However, both functions can fail independently. If process_surplus() succeeds but distribute_reward() fails, the next timer tick will:

• Re-check for surplus (1-hour rate limit prevents immediate reprocessing ✓)
• But there's no atomic transaction guarantee

Additional concern: What happens if the canister is upgraded between process_surplus() and distribute_reward()? The REWARD_POOL state is persisted, but the distribution won't happen until the next timer tick.

Recommendation: Document this behavior explicitly, or consider adding a flag to prevent double-processing within the same logical distribution cycle.

3. Inconsistent Threshold Constants

Location: src/icp_swap/src/storage.rs:15-17

Problem: The constants still reference the old 1 ICP threshold:

pub const SURPLUS_SWEEP_THRESHOLD_E8S: u64 = 100_000_000;  // 1 ICP (unused - see process_surplus)
pub const MIN_SWEEP_AMOUNT_E8S: u64 = 1_000_000;           // 0.01 ICP (minimum surplus to process)

But process_surplus() hardcodes a different value:

const SURPLUS_THRESHOLD_E8S: u64 = 1_000_000; // 0.01 ICP (was 1 ICP)

Impact: Confusion, potential bugs if someone uses the public constant expecting it to match actual behavior.

Fix: Either:

• Use the constant from storage: MIN_SWEEP_AMOUNT_E8S
• Or update SURPLUS_SWEEP_THRESHOLD_E8S to 1M and add deprecation notice

---

⚠️ High Priority Issues (P1)

4. Missing Error Context in Balance Fetch

Location: src/icp_swap/src/update.rs:1857-1860

The error handling loses context:

let actual_balance = fetch_canister_icp_balance().await
    .map_err(|e| ExecutionError::StateError(
        format\!("Failed to fetch balance for surplus processing: {:?}", e)
    ))?;

Recommendation: Include the canister principal being queried for better debugging.

5. Potential Time Manipulation via Upgrades

Location: src/icp_swap/src/update.rs:1897-1911

The 1-hour rate limit uses ic_cdk::api::time(). If a canister is upgraded and the system time is manipulated (edge case, but possible), this could allow bypassing the rate limit.

Recommendation: Store both timestamp AND a "processing generation counter" to ensure even with time manipulation, processing can only happen once per distribution cycle.

6. SweepRecord Field Semantics Changed

Location: src/icp_swap/src/update.rs:1941-1949

let process_record = SweepRecord {
    timestamp: now,
    amount_swept: surplus,
    surplus_before: surplus,
    operational_buffer_kept: 0, // No buffer needed for internal accounting
    transfer_block_index: 0, // No transfer, internal state update
    success: true,
    error_message: None,
};

Problem: transfer_block_index: 0 is ambiguous - does 0 mean "no transfer" or "block index 0"? Future code reading this data might misinterpret.

Recommendation: Consider using Option<u64> for transfer_block_index, or use u64::MAX as a sentinel value with clear documentation.

---

💡 Code Quality Improvements

7. Redundant Planning Document in Repo

The file SURPLUS_TO_STAKERS_PLAN.md (526 lines) contains implementation instructions and appears to be a development artifact. This should either be:

• Moved to documentation if it provides value
• Removed from the PR (use PR description instead)
• Moved to a docs/architecture folder if it serves as architectural documentation

8. Improved Logging Granularity

Consider adding more detailed logs for debugging:

• Log the individual components of expected_balance calculation
• Log the actual_balance vs expected_balance diff even when below threshold
• Log when rate limiting prevents processing (with time until next eligible processing)

9. Comment Clarity

Line 1897: "Reuse existing timestamp tracker" - the comment could be clearer about what get_last_sweep_timestamp() tracks now (both sweeps AND internal processing).

---

✅ Positive Aspects

1. Clean Architecture: Removing external transfers simplifies the system significantly
2. Good Use of Checked Arithmetic: The REWARD_POOL update uses checked_add() correctly
3. Consistent Error Handling: Proper use of ExecutionError enum
4. Backward Compatible Logging: Reusing SweepRecord maintains audit trail
5. Timer Ordering Logic: Processing surplus before distribution is the right choice
6. Comprehensive PR Description: Very clear explanation of changes and rationale

---

🧪 Test Coverage Concerns

Issue: No tests found for sweep_surplus_to_revshare or process_surplus in the test suite.

Required Tests:

1. Unit test: Surplus detection with various balance states
2. Unit test: Overflow protection in expected_balance calculation
3. Unit test: Rate limiting (1-hour minimum between processing)
4. Integration test: Full flow: deposit ICP → process_surplus → distribute_reward → verify staker balances
5. Edge case test: Processing when REWARD_POOL is near u64::MAX
6. Edge case test: Multiple deposits within 1-hour window

Recommendation: Add at least basic unit tests in tests/simulation/icp_swap_tests.rs before merging.

---

🔒 Security Assessment

Overall: The changes reduce external attack surface by removing cross-canister transfers, which is positive.

Specific Concerns:

• ✅ No new external calls introduced
• ✅ Checked arithmetic prevents overflow in critical path (REWARD_POOL update)
• ⚠️ Unchecked arithmetic in expected_balance calculation (P0 issue Swap integration #1)
• ✅ Rate limiting prevents spam processing
• ✅ No new authorization bypasses

---

📊 Performance Implications

Positive:

• Removes one cross-canister call (transfer to lbry_fun)
• Reduces cycle costs per distribution
• Simplifies state machine (fewer failure modes)

Neutral:

• Additional REWARD_POOL read/write (minimal overhead)
• Same number of timer ticks

---

Recommended Actions Before Merge

Must Fix (P0):

1. ✅ Add checked arithmetic to expected_balance calculation
2. ⚠️ Fix or document the constant inconsistency (SURPLUS_THRESHOLD_E8S vs SURPLUS_SWEEP_THRESHOLD_E8S)
3. ⚠️ Clarify transfer_block_index: 0 semantics in SweepRecord

Should Fix (P1):

4. Add basic unit tests for process_surplus()
5. Remove or relocate SURPLUS_TO_STAKERS_PLAN.md
6. Improve error context in balance fetch

Nice to Have:

7. Enhanced logging for debugging
8. Document canister upgrade behavior between process_surplus and distribute_reward

---

Final Recommendation

Status: ❌ Request Changes

The core architectural change is sound and well-implemented, but the arithmetic overflow risk in expected_balance calculation is a critical financial safety issue that must be fixed before merging. The constant inconsistency should also be resolved to prevent future bugs.

Once P0 issues are addressed and basic tests are added, this will be a solid improvement to the codebase.

---

Great work on the refactoring! The new flow is cleaner and more aligned with the project's goals. Just needs the safety checks tightened up. 🚀

[claude]

Pull Request Review: Surplus ICP to Reward Pool Refactoring

Overview

This PR refactors the surplus ICP handling mechanism to redirect external deposits directly to the staker reward pool instead of transferring to the platform canister. The architectural change is sound and aligns well with the project's goal of simplicity and directness.

---

✅ Strengths

1. Cleaner Architecture

• Eliminates external transfer complexity
• Direct internal accounting is simpler and more efficient
• Better separation of concerns (no tokenomics interference)

2. Improved Safety

• Excellent: Added checked arithmetic for overflow prevention (lines 1871-1879, 1930-1935)
• Good: Maintains comprehensive logging throughout
• Good: Proper CEI pattern adherence (Check-Effect-Interact)

3. Better Ordering Logic

The reordering in script.rs (process_surplus BEFORE distribute_reward) is correct:

• External deposits are captured first
• Then included in the same distribution cycle
• This prevents one-cycle delay in reward distribution

4. Code Quality

• Clear documentation with safety guarantees
• Consistent naming conventions
• Proper error handling maintained

---

🔍 Issues & Recommendations

P1 - High Priority (Should Fix)

1. Inconsistent Constant Usage (update.rs:1895)

Issue: Function uses MIN_SWEEP_AMOUNT_E8S but the comment mentions it should be 0.01 ICP (1M E8S), while the constant is defined correctly in storage.rs:18. However, there's confusion with SURPLUS_SWEEP_THRESHOLD_E8S being marked as unused.

Current Code:

// storage.rs
pub const SURPLUS_SWEEP_THRESHOLD_E8S: u64 = 100_000_000;  // 1 ICP (unused - see process_surplus)
pub const MIN_SWEEP_AMOUNT_E8S: u64 = 1_000_000;           // 0.01 ICP (minimum surplus to process)

Recommendation:

• The code is actually correct as-is ✓
• Consider removing or deprecating SURPLUS_SWEEP_THRESHOLD_E8S and OPERATIONAL_BUFFER_E8S entirely if they're truly unused to reduce confusion
• Add a comment explaining why these were kept (backwards compatibility? historical records?)

2. Semantic Mismatch in SweepRecord (update.rs:1954)

Issue: Using u64::MAX as a sentinel value for "no external transfer" is clever, but the field name transfer_block_index implies there was a transfer.

Recommendation:

// Option 1: Make it Option<u64>
pub struct SweepRecord {
    // ...
    pub transfer_block_index: Option<u64>, // None for internal ops, Some(block) for external
    // ...
}

// Option 2: Add explicit field
pub struct SweepRecord {
    // ...
    pub transfer_block_index: u64,
    pub is_internal_only: bool, // true when no external transfer
    // ...
}

// Option 3: Keep as-is but add clear documentation
/// Block index for external transfers, or u64::MAX if internal-only operation
pub transfer_block_index: u64,

Since this project doesn't worry about backwards compatibility (per CLAUDE.md), Option 1 is cleanest.

---

P2 - Medium Priority (Nice to Have)

3. Field Naming Inconsistency (update.rs:1949-1957)

Issue: In the new context, amount_swept equals surplus_before, and operational_buffer_kept is always 0. This creates redundant/confusing data.

Recommendation:

pub struct SweepRecord {
    pub timestamp: u64,
    pub surplus_detected: u64,        // Renamed for clarity
    pub amount_added_to_pool: u64,    // More descriptive than amount_swept
    pub transfer_block_index: Option<u64>, // None for internal ops
    pub success: bool,
    pub error_message: Option<String>,
}

4. Timestamp Update Logic

Good: The code correctly updates the last sweep timestamp via record_sweep() (storage.rs:226-227).

Minor Enhancement: Consider making the timestamp update atomic with the pool update to prevent edge cases:

// After REWARD_POOL update in process_surplus()
LAST_SWEEP_TIMESTAMP.with(|t| {
    t.borrow_mut().insert((), now);
});
record_sweep(process_record);

This ensures timestamp is updated even if record_sweep were to fail (though it shouldn't).

5. Error Message Clarity

The error messages are good, but could specify the impact:

// Before
"Failed to fetch balance for surplus processing: {:?}"

// Better
"Failed to fetch balance for surplus processing (external deposits won't be included in this cycle): {:?}"

---

P3 - Low Priority (Optional)

6. Magic Number for Rate Limiting

let one_hour_nanos = 3_600_000_000_000u64; // 1 hour in nanoseconds

Recommendation: Extract to constant in storage.rs:

pub const SURPLUS_PROCESS_COOLDOWN_NANOS: u64 = 3_600_000_000_000; // 1 hour

7. Test Coverage

Missing: No tests found for process_surplus() in the test suite.

Recommendation: Add integration tests covering:

• Normal surplus processing flow
• Overflow scenarios (checked arithmetic)
• Rate limiting (1-hour cooldown)
• Surplus below threshold
• Interaction with distribute_reward()

---

🔒 Security Analysis

✅ Safe Operations

1. Overflow Protection: Checked arithmetic prevents financial loss ✓
2. Rate Limiting: 1-hour cooldown prevents DoS ✓
3. State Consistency: Proper ordering (surplus → distribution) ✓
4. Logging: Comprehensive audit trail ✓

⚠️ Potential Concerns

1. No cap on surplus: Could REWARD_POOL grow unbounded?

   • Assessment: Acceptable - pool naturally drains via hourly 1% distribution
   • Edge case: If no stakers exist, pool keeps growing (but this is by design per distribute_reward.rs:913-917)

2. Integer division loses precision:

   let total_distribution = reward_pool / 100; // Loses up to 99 E8S per distribution

   • Assessment: Acceptable for this use case (negligible at scale)

---

📊 Performance Considerations

✅ Improvements

• Reduced inter-canister calls: No more external transfer
• Lower gas costs: Internal accounting is much cheaper
• Simpler error handling: Fewer failure modes

🔍 Observations

• Async overhead: Still fetches canister balance (necessary)
• Storage growth: SweepRecord history grows unbounded (consider periodic archival)

---

📝 Documentation & Code Style

✅ Good

• Clear function documentation with safety guarantees
• Inline comments explain rationale
• Git commit messages follow good practices

🔧 Suggestions

1. Update any external documentation mentioning the old flow
2. Consider adding a migration note explaining the behavioral change
3. The PR description is excellent - ensure it's reflected in code comments

---

🧪 Testing Recommendations

Since the project uses pocket-ic for testing, add:

#[test]
fn test_process_surplus_adds_to_reward_pool() {
    // Setup: Send ICP to icp_swap canister
    // Act: Call process_surplus()
    // Assert: REWARD_POOL increased by surplus amount
    // Assert: SweepRecord created with u64::MAX block index
}

#[test]
fn test_surplus_processing_before_distribution() {
    // Setup: Send external ICP to canister
    // Act: Wait for timer (or call wrapper)
    // Assert: Distribution includes the external deposit in same cycle
}

#[test]
fn test_overflow_prevention() {
    // Setup: Set REWARD_POOL near u64::MAX
    // Act: Try to process large surplus
    // Assert: Returns overflow error, state unchanged
}

#[test]
fn test_rate_limiting() {
    // Setup: Process surplus successfully
    // Act: Try to process again immediately
    // Assert: Returns "Too soon" error
}

---

✅ Final Verdict

Approval Recommendation: ✅ APPROVE with Minor Suggestions

This is a solid refactoring that achieves its goals:

• Simpler architecture ✓
• Better safety guarantees ✓
• Cleaner code ✓
• Proper ordering logic ✓

Must Fix Before Merge:

• None (code is functionally correct)

Should Consider:

1. Making transfer_block_index an Option<u64> for clarity
2. Adding integration tests
3. Removing deprecated constants

Nice to Have:

1. Extract magic numbers to constants
2. Consider renaming SweepRecord fields for new semantics
3. More descriptive error messages

---

📋 Checklist for Author

• Consider changing transfer_block_index to Option<u64>
• Add integration tests for process_surplus()
• Remove or clearly deprecate unused constants (SURPLUS_SWEEP_THRESHOLD_E8S, OPERATIONAL_BUFFER_E8S)
• Verify ./scripts/build.sh completes successfully (marked as done in test plan)
• Manual testing with actual ICP deposits
• Confirm platform still receives 1% through distribution mechanism

---

Great work on this refactoring! The code quality is high, and the architectural decision makes sense for the project's goals. The checked arithmetic additions are especially valuable for preventing financial bugs. 🚀

---

Review generated by Claude Code

[claude]

PR Review: Refactor Surplus ICP to Reward Pool

Overview

This PR successfully simplifies the surplus ICP handling by eliminating external transfers to the platform canister and instead adding surplus directly to the REWARD_POOL. This is a clean architectural improvement that reduces complexity and external dependencies.

✅ Strengths

1. Architectural Simplification

• Eliminates unnecessary external transfer to lbry_fun canister
• Reduces inter-canister dependencies
• More straightforward flow: external deposits → reward pool → stakers
• Platform still gets its 1% through normal distribution mechanism

2. Code Quality

• Proper use of checked arithmetic (checked_add) to prevent overflow
• Maintains comprehensive logging
• Good documentation in function comments
• Clean separation of concerns (CHECK → EFFECT → RECORD phases)

3. Safety Improvements

• Lowered threshold (1 ICP → 0.01 ICP) is appropriate for internal accounting since no transfer fees
• Proper error handling with ExecutionError
• Rate limiting (1 hour minimum between processing)

4. Backward Compatibility

• Reuses existing SweepRecord structure with Option<u64> for transfer_block_index
• Preserves historical tracking mechanism
• Query functions like get_sweep_history() will continue to work

⚠️ Issues & Concerns

1. CRITICAL: Missing Timestamp Update Bug

The process_surplus() function calls record_sweep() which updates the timestamp via LAST_SWEEP_TIMESTAMP, BUT there's a subtle race condition in the timer logic:

In script.rs:347-367:

match process_surplus().await {
    Ok(msg) => { /* logs success */ }
    Err(e) => { /* logs error but continues */ }
}

Problem: If process_surplus() returns Ok("No surplus to process") or Ok("Surplus below threshold"), it does NOT call record_sweep(), so the timestamp is NOT updated. However, the 1-hour check happens INSIDE process_surplus(). This means:

• If there's no surplus, it checks the timestamp and returns early
• But it never updates the timestamp
• This could lead to repeated checks even when there's nothing to do

Recommendation: Either:

• Option A: Always update timestamp when function is called (even if no surplus)
• Option B: Remove the timestamp check from process_surplus() and handle it in the wrapper
• Option C: Document that this is intentional (only update timestamp on actual processing)

2. Race Condition with Expected Balance Calculation

In update.rs:1862-1868:

let reward_pool = REWARD_POOL.with(|p| p.borrow().get(&()).unwrap_or(0));
let uncollected_alex = UNCOLLECTED_ALEX_FEES.with(|f| f.borrow().get(&()).unwrap_or(0));
let total_staked = STAKES.with(|s| {
    s.borrow().iter().map(|(_, stake)| stake.reward_icp as u64).sum::<u64>()
});
let archived_balance = crate::queries::get_total_archived_balance();

These reads are NOT atomic. Between reading these values and updating the pool, another operation could modify them (e.g., a user claiming rewards, staking, etc.).

Recommendation: Either:

• Accept this as a known limitation (surplus processing is "opportunistic")
• Add documentation that race conditions may cause temporary discrepancies
• Consider adding a lock/mutex if precise accounting is critical

3. Integer Division Precision Loss

In the context of distribute_reward() (update.rs:898):

let total_distribution = reward_pool / 100;  // 1% of pool

This uses integer division which truncates. For example:

• Pool = 99 E8S → distribution = 0 E8S (nothing distributed!)
• Pool = 199 E8S → distribution = 1 E8S

While this is acceptable for large pools, it means small deposits won't distribute until the pool is ≥ 100 E8S (0.000001 ICP). This is likely fine but should be documented.

Note: This issue exists in the original code, not introduced by this PR.

4. Deprecated Constants Still Referenced

In storage.rs:18-20:

// Deprecated constants - kept for historical record reading but no longer used
// pub const SURPLUS_SWEEP_THRESHOLD_E8S: u64 = 100_000_000;
// pub const OPERATIONAL_BUFFER_E8S: u64 = 10_000_000;

These are commented out but still in the codebase. Since you stated "do not worry about backwards compatibility," consider removing them entirely in a follow-up.

5. operational_buffer_kept Field Always 0

In update.rs:1953:

operational_buffer_kept: 0, // No buffer needed for internal accounting

While technically correct, this makes the SweepRecord structure misleading for new internal records vs old external transfer records. Consider:

• Adding a discriminator field to distinguish record types
• OR documenting that operational_buffer_kept == 0 && transfer_block_index == None means internal processing

6. No Test Coverage

The PR description shows test plan checkboxes but grep found no tests for process_surplus. Given this is a financial operation, comprehensive tests should be added:

Suggested test cases:

• ✅ Normal surplus processing (surplus > threshold)
• ✅ Surplus below threshold
• ✅ No surplus (actual ≤ expected)
• ✅ Rate limiting (repeated calls within 1 hour)
• ✅ Overflow prevention (checked arithmetic)
• ✅ Integration with distribute_reward() (end-to-end flow)

🔒 Security Considerations

✅ Good:

• Checked arithmetic prevents overflow
• No unchecked unwraps on critical paths
• Proper error propagation
• Rate limiting prevents spam

⚠️ Consider:

• Griefing vector: Anyone can send ICP to the icp_swap canister to inflate the reward pool. While this benefits stakers, it could be used to manipulate APY calculations or create unexpected behavior.
  • Mitigation: Document this as a feature (anyone can fund rewards)
  • OR: Add a whitelist of allowed senders if needed

📊 Performance Considerations

✅ Efficient:

• Internal state update (no external calls except ledger balance check)
• O(n) iteration over stakes for expected balance calculation (acceptable)
• Proper use of saturating_sub to prevent panics

⚠️ Potential issue:

• The expected balance calculation iterates over ALL stakes every time. If there are thousands of stakers, this could be slow. Consider caching or tracking total staked separately.

🎯 Recommendations Summary

Must Fix (Before Merge):

1. Clarify timestamp update logic - Decide on Option A/B/C above and implement
2. Add test coverage - At minimum, integration test for happy path

Should Fix (Before Merge):

3. Document race condition behavior - Add comments about non-atomic reads
4. Add end-to-end test - Verify surplus → distribution → staker rewards flow

Nice to Have (Follow-up PR):

5. Remove deprecated constants entirely
6. Consider refactoring SweepRecord to better distinguish internal vs external operations
7. Add performance optimization for large stake sets
8. Add security documentation about griefing considerations

✅ Approval Pending Fixes

Overall, this is a solid refactor that simplifies the architecture. The core logic is sound, but the timestamp management needs clarification and tests should be added before merging.

Verdict: Approve with changes requested (fix timestamp logic + add tests)

---

🤖 Review generated with Claude Code