Skip to content

Conversation

@ben-kaufman
Copy link
Contributor

Track channels once closed and show them in the list of channels.

Screenshot_1763502935 Screenshot_1763502943

@ben-kaufman ben-kaufman self-assigned this Nov 18, 2025
@claude
Copy link

claude bot commented Nov 18, 2025

Pull Request Review: Add Closed Channels

Summary

This PR adds functionality to track and display closed Lightning channels in the UI. The implementation introduces a channel cache mechanism, persists closed channel data, and displays closed channels with proper visual differentiation.

Code Quality & Best Practices ✅

Strengths:

  • Good use of Kotlin coroutines and proper dispatcher usage (bgDispatcher, ServiceQueue)
  • Appropriate use of @Suppress annotations for complexity warnings with justification
  • Consistent error handling with try-catch blocks and proper logging
  • Clean separation of concerns between service, repository, and UI layers
  • Proper use of StateFlow for reactive UI updates

Suggestions:

  1. Constants naming - Consider extracting magic values:

    // In LightningService.kt:36
    private const val CHANNEL_ID_PREVIEW_LENGTH = 10
    // Usage: channel.channelId.take(CHANNEL_ID_PREVIEW_LENGTH) + "…"
  2. Null safety improvement in LightningService.kt:265:

    // Current:
    val reason = event.reason?.toString() ?: ""
    
    // Consider:
    val reason = event.reason?.toString().orEmpty()

Potential Bugs & Issues ⚠️

Critical Issues:

  1. Race condition in channel cache (LightningService.kt:234-249):

    • The channelCache is a mutable map accessed from multiple coroutines without synchronization
    • Potential for concurrent modification if refreshChannelCache() runs while registerClosedChannel() is removing entries
    • Recommendation: Use ConcurrentHashMap or add synchronization:
    private val channelCache = ConcurrentHashMap<String, ChannelDetails>()
  2. Missing null check (LightningService.kt:262-266):

    • If fundingTxo is null, the function returns early but the channel has already been removed from the cache
    • This creates an inconsistent state where the channel is neither cached nor persisted
    • Recommendation: Check before removing from cache:
    val fundingTxo = channel.fundingTxo
    if (fundingTxo == null) {
        Logger.error("Channel has no funding transaction", context = TAG)
        // Re-add to cache since we couldn't persist it
        channelCache[channelId] = channel
        return
    }
  3. Silent failure in closed channel filtering (LightningConnectionsViewModel.kt:144-147):

    • The check if (currentSelectedChannel.details.channelId in closedChannelIds) silently returns without updating the UI
    • User might see stale data if a channel closes while viewing details
    • Recommendation: Consider emitting a state update or navigation event

Minor Issues:

  1. Potential memory leak: The channelCache grows unbounded. Consider:

    • Adding a cleanup mechanism for very old entries
    • Or clearing the cache when the node stops
  2. Error message in LightningService.kt:254: Uses string interpolation but logs as an error - the actual channel object isn't logged. Consider logging more context.

Performance Considerations 🚀

Strengths:

  • Efficient use of parallel operations with map { async { ... } }.awaitAll()
  • Proper use of background dispatchers to avoid blocking UI

Concerns:

  1. Triple data load on initialization (LightningConnectionsViewModel.kt:69-71):

    • Both observeState() and loadClosedChannels() are called in init
    • Then observeState() triggers state collection which calls refreshObservedState()
    • This could lead to multiple redundant loads on startup
    • Recommendation: Audit the initialization flow to reduce redundant operations
  2. Reversed list operation (LightningConnectionsScreen.kt:201):

    channels = uiState.closedChannels.reversed()
    • Creates a new list every composition
    • Recommendation: Reverse once in the ViewModel when loading:
    closedChannels = closedChannels.mapIndexed { ... }.reversed()
  3. N+1 query pattern (LightningConnectionsViewModel.kt:224-245):

    • Each closed channel mapping creates individual ChannelUi objects
    • Recommendation: This is acceptable for the current use case, but monitor if the number of closed channels grows large

Security Concerns 🔒

Overall: No significant security issues identified

Minor observations:

  1. Channel IDs and node IDs are properly logged (good for debugging)
  2. Sensitive data like funding transactions are handled appropriately
  3. No injection vulnerabilities detected in the string operations
  4. Proper use of typed parameters prevents SQL/NoSQL injection in the data layer

Test Coverage 📊

Concerns:

  1. No tests found for new functionality:

    • No tests for LightningService.registerClosedChannel()
    • No tests for LightningService.refreshChannelCache()
    • No tests for ActivityRepo.getClosedChannels()
    • No UI tests for closed channel display
  2. Testing recommendations:

    // Suggested test cases:
    - Test channel cache synchronization with multiple concurrent operations
    - Test closed channel registration with null fundingTxo
    - Test closed channel filtering and display order
    - Test state updates when a channel closes while viewing details
    - Test edge cases: empty closure reason, missing channel details
  3. Integration testing needed:

    • Test the full flow: channel close event → cache update → persistence → UI display
    • Test the interaction between refreshChannelCache() and registerClosedChannel()

Additional Recommendations

  1. Documentation: Add KDoc comments to registerClosedChannel() and refreshChannelCache() explaining their threading model and error handling

  2. Metrics: Consider adding telemetry for:

    • How many channels users have closed
    • Common closure reasons
    • Time between channel open and close
  3. User Experience:

    • The closure reason display is great for transparency
    • Consider adding a "filter" to show/hide closed channels (current implementation shows them when toggling failed orders)

Conclusion

This is a solid implementation that adds valuable functionality. The main concerns are:

  1. Thread safety of the channel cache (critical)
  2. Inconsistent state when fundingTxo is null (important)
  3. Test coverage for the new functionality (important)

After addressing these issues, this PR will be ready to merge.

Approval Status: ⚠️ Conditional Approval - Please address the critical thread safety and null handling issues before merging.


🤖 Generated with Claude Code

@claude
Copy link

claude bot commented Nov 18, 2025

PR Review: Add Closed Channels

Overview

This PR adds functionality to track and display closed Lightning channels. The implementation includes persistence of closed channel data, UI updates to show closed channels in the connections list, and proper state management.

Code Quality & Best Practices

Strengths:

  • Clean separation of concerns with proper use of repositories and services
  • Good use of Kotlin coroutines and structured concurrency
  • Appropriate suppression annotations for detekt warnings (@Suppress("LargeClass"), @Suppress("CyclomaticComplexMethod"))
  • Consistent logging throughout the implementation
  • Good error handling with try-catch blocks and logging

Areas for Improvement:

  1. Race Condition in Channel Cache (LightningService.kt:196-211)

    • The channelCache is accessed from multiple coroutines without proper synchronization beyond ConcurrentHashMap
    • In registerClosedChannel(), the channel is retrieved from cache, but it could be removed by another thread before the data is used
    • Recommendation: Consider capturing the channel data atomically or using a read-write lock pattern
  2. Error Recovery (LightningService.kt:214-275)

    • If registerClosedChannel() fails, the channel closure is logged but not persisted
    • The cache entry is removed even if persistence fails
    • Recommendation: Only remove from cache after successful persistence, or implement a retry mechanism
  3. Nullable Handling (LightningService.kt:237-242)

    • The fallback to channelId.take(10) + "…" for channel name is good, but the Unicode ellipsis might cause display issues
    • Recommendation: Consider using ASCII "..." or ensure UI properly handles Unicode

Potential Bugs

  1. Missing Null Safety (LightningService.kt:218)

    val channel = ServiceQueue.LDK.background {
        channelCache[channelId]
    } ?: run {
        Logger.error(...)
        return@registerClosedChannel
    }

    This early return is inside a suspend function but uses a labeled return. While valid, it could be clearer with explicit error handling.

  2. Data Consistency (LightningConnectionsViewModel.kt:141-151)

    val closedChannelIds = _uiState.value.closedChannels.map { it.details.channelId }.toSet()
    val activeChannels = channels.filterNot { it.channelId in closedChannelIds }

    There's a potential race where a channel could be in both active and closed lists briefly during the transition. The current implementation filters correctly, but the logic could be more explicit.

  3. Index Calculation (LightningConnectionsViewModel.kt:219)

    baseIndex = openChannels.size + pendingConnections.size + index

    The baseIndex is calculated but only used for display naming. If channels are reordered, names could change. Consider if this is the desired behavior.

Performance Considerations

  1. Multiple Sequential Loads (LightningConnectionsViewModel.kt:138)

    • refreshObservedState() calls loadClosedChannels() which already loads data
    • On every channel event, closed channels are reloaded
    • Recommendation: Consider debouncing or caching to avoid excessive reloads
  2. Reverse Operation (LightningConnectionsViewModel.kt:89)

    }.reversed()

    The list is reversed after mapping. Since mapIndexed is used, reversing changes the semantic meaning of the index.

    • Recommendation: Either reverse before mapping or adjust the index calculation
  3. String Operations in Hot Path (LightningService.kt:237-238)

    • String concatenation and substring operations in the event handler
    • Recommendation: This is minor, but could be optimized if channel closures become frequent

Security Concerns

No significant security issues found. The code properly:

  • Uses parameterized database operations (through bitkit-core)
  • Doesn't expose sensitive data inappropriately
  • Handles user input safely

Minor consideration:

  • The channelClosureReason is stored directly from the event without sanitization. Ensure the upstream LDK library provides safe strings.

Test Coverage

Missing Tests:

  • No unit tests found for registerClosedChannel() functionality
  • No UI tests for the closed channels display
  • No tests for the channel cache refresh logic

Recommendations:

  1. Add unit tests for LightningService.registerClosedChannel() covering:

    • Successful channel closure registration
    • Channel not found in cache
    • Null funding transaction
    • Database persistence failure
  2. Add tests for LightningConnectionsViewModel.loadClosedChannels():

    • Successful load
    • Empty results
    • Error handling
  3. Add UI tests for:

    • Closed channels appearing in the list
    • Show/hide closed channels toggle
    • Closed channel alpha transparency
    • Navigation to closed channel details

Architecture & Design

Positive:

  • Good separation between UI state and business logic
  • Proper use of StateFlow for reactive UI updates
  • Clean event handling through the LDK event system

Suggestions:

  1. Consider extracting closed channel persistence logic into ActivityRepo for better separation
  2. The ChannelUi.closureReason field is nullable and only used for closed channels - consider a sealed class hierarchy for better type safety:
    sealed class ChannelUi {
        data class Open(...) : ChannelUi()
        data class Pending(...) : ChannelUi()
        data class Closed(..., val closureReason: String) : ChannelUi()
    }

Additional Notes

  1. String Resources - Ensure R.string.lightning__closure_reason and R.string.lightning__conn_closed are properly localized

  2. Accessibility - The alpha transparency on closed channels (CLOSED_CHANNEL_ALPHA = 0.64f) might make them hard to read for some users. Consider adding a visual indicator beyond just opacity.

  3. Data Migration - If users upgrade from a previous version, existing closed channels won't be tracked. Consider if historical channel closures should be preserved during the upgrade.

Summary

This is a well-implemented feature with good code quality. The main concerns are:

  • High Priority: Race condition in channel cache access
  • Medium Priority: Error recovery in channel persistence
  • Low Priority: Missing test coverage
  • Low Priority: Performance optimization opportunities

The code follows the project's patterns and conventions well. With the suggested improvements, this will be a solid addition to the codebase.

Overall Assessment: Approve with minor suggestions for improvement.

🤖 Generated with Claude Code

Copy link
Member

@jvsena42 jvsena42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested:

  1. Close channel
  2. Open channel
  3. Reset and restore
  4. Check channel list

Copy link
Collaborator

@ovitrif ovitrif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK.

Code is ok with the exception that it violates the otherwise esablished pattern about LightingService, in that LightningService acts more like a wrapper for ldk-node bindings.

Now we're introducing state to it, and we are inter-connecting it with more code components, a practice that was avoided until now.

My suggestion is to move the specific logic which orchestrates multiple code components and manages state to LightningRepo; where we did most if such work.

So the data flow up would be:
ViewModel (UI) → Repository (Domain) → LightningService (bridge to ldk-node bindings)

@ovitrif
Copy link
Collaborator

ovitrif commented Nov 19, 2025

Tested:

  • Close channel 🟢

@ovitrif ovitrif merged commit 23f55e0 into master Nov 19, 2025
15 checks passed
@ovitrif ovitrif deleted the feat/closed-channels branch November 19, 2025 18:33
@ovitrif
Copy link
Collaborator

ovitrif commented Nov 19, 2025

Architectural change referenced here can be done later (it's not critical to implementation):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants