Conversation
Summary of ChangesHello @dimast-x, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust escrow mechanism for channel resizing, enhancing the system's ability to manage funds during state transitions. It adds a dedicated 'resizing' status for channels and refines error handling to provide clearer and more consistent error messages throughout the application. These changes ensure that fund movements during channel resizing are accurately tracked and that channel states are consistently maintained, improving the overall reliability and predictability of channel operations. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
@philanton @nksazonov Lets review and agree on the changes before updating docs and integration tests |
There was a problem hiding this comment.
Code Review
This pull request introduces an escrow mechanism for channel resizing. While the overall direction is good, there are several critical issues in the implementation. A major bug in clearnode/ledger.go causes database errors to be swallowed, which could lead to silent data corruption. The core logic for handling resize withdrawals in clearnode/channel_service.go appears to be inverted, treating withdrawals like deposits. Furthermore, the corresponding event handler in clearnode/custody.go has incomplete logic for withdrawals, leading to funds being 'lost' from the ledger. Several tests have also been updated to assert this incorrect behavior. These issues need to be addressed to ensure the correctness and reliability of the new feature.
| if params.ResizeAmount.Cmp(decimal.Zero) < 0 { | ||
| decimalResizeAmount := rawToDecimal(params.ResizeAmount.BigInt(), asset.Token.Decimals) | ||
| // Lock funds in the channel account. | ||
| if err := ledger.Record(NewAccountID(channel.Wallet), asset.Symbol, decimalResizeAmount, nil); err != nil { | ||
| return err | ||
| } | ||
| if err := ledger.Record(NewAccountID(channel.ChannelID), asset.Symbol, decimalResizeAmount.Neg(), nil); err != nil { | ||
| return err | ||
| } | ||
| _, err := RecordLedgerTransaction(tx, TransactionTypeEscrowLock, NewAccountID(channel.Wallet), NewAccountID(channel.ChannelID), asset.Symbol, decimalResizeAmount) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for handling a negative ResizeAmount (i.e., a channel size decrease or withdrawal) appears to be incorrect. The current implementation debits the user's wallet and credits the channel's account, which is the procedure for a deposit or channel size increase. For a withdrawal, the funds are already in the channel, so the user's wallet should not be debited to lock funds. This logic seems to be inverted.
There was a problem hiding this comment.
probably you dont understand the concept. the specific channel account exists as an escrow for channel funds, like a lock. with most cases it stays 0 as funds go through this but they are not getting locked. however with resize to withdraw we must lock the amount until we receive a confirmation from custody, and then we debit it in the same way as before
| if amount.IsNegative() { | ||
| // Withdraw from the channel escrow account. | ||
| ledger := GetWalletLedger(tx, walletAddress) | ||
| if err := ledger.Record(walletAccountID, asset.Symbol, amount, nil); err != nil { | ||
| return fmt.Errorf("error recording balance update for participant: %w", err) | ||
| } | ||
| if err := ledger.Record(channelAccountID, asset.Symbol, amount.Neg(), nil); err != nil { | ||
| return fmt.Errorf("error recording balance update for wallet: %w", err) | ||
| if err := ledger.Record(channelAccountID, asset.Symbol, amount, nil); err != nil { | ||
| return err | ||
| } | ||
| _, err := RecordLedgerTransaction(tx, TransactionTypeWithdrawal, walletAccountID, channelAccountID, asset.Symbol, amount) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to record transaction: %w", err) | ||
| } | ||
| // 2. Withdraw from the channel account. | ||
| ledger = GetWalletLedger(tx, walletAddress) | ||
| if err := ledger.Record(channelAccountID, asset.Symbol, amount, nil); err != nil { | ||
| return fmt.Errorf("error recording balance update for wallet: %w", err) | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
When handling a negative resize amount (a withdrawal), the new logic debits the channel escrow account but does not credit any other account. This results in funds being "lost" from the ledger's perspective. The previous logic correctly debited the user's wallet account to reflect that funds were withdrawn on-chain from the off-chain unified balance. The new logic is incomplete and breaks the ledger's accounting.
There was a problem hiding this comment.
probably you dont understand the concept. the specific channel account exists as an escrow for channel funds, like a lock. with most cases it stays 0 as funds go through this but they are not getting locked. however with resize to withdraw we must lock the amount until we receive a confirmation from custody, and then we debit it in the same way as before
clearnode/custody_test.go
Outdated
| assert.True(t, initialWalletBalance.Equal(walletBalance), | ||
| "Wallet balance should stay %s after resize, got %s", initialWalletBalance, walletBalance) | ||
|
|
||
| channelBalance, err := ledger.Balance(channelAccountID, asset.Symbol) | ||
| require.NoError(t, err) | ||
| assert.True(t, channelBalance.IsZero(), "Channel balance should be zero after resize") | ||
| assert.True(t, channelBalance.Equal(deltaAmountDecimal), "Channel balance should be zero after resize") |
There was a problem hiding this comment.
This test for a negative resize (withdrawal) seems to be asserting incorrect behavior. It asserts that the wallet balance remains unchanged and the channel balance becomes equal to deltaAmountDecimal (a negative value). This implies that funds are being destroyed from the ledger's perspective. A withdrawal from a channel should be reflected correctly in the user's balance.
Additionally, the assertion message "Channel balance should be zero after resize" contradicts the assertion itself, which checks for equality with deltaAmountDecimal.
There was a problem hiding this comment.
probably you dont understand the concept. the specific channel account exists as an escrow for channel funds, like a lock. with most cases it stays 0 as funds go through this but they are not getting locked. however with resize to withdraw we must lock the amount until we receive a confirmation from custody, and then we debit it in the same way as before
| // Verify ledger balance was debited by the resize withdrawal amount (0.0001 USDC) | ||
| finalBalance, err := ledger.Balance(userAccountID, "usdc") | ||
| require.NoError(t, err) | ||
| require.Equal(t, decimal.NewFromInt(2000), finalBalance) // Should remain unchanged | ||
| require.Equal(t, decimal.NewFromFloat(1999.9999), finalBalance) |
There was a problem hiding this comment.
This test for a resize withdrawal asserts that the user's ledger balance is debited. This confirms the behavior of the RequestResize function, but the underlying logic in RequestResize for negative amounts is incorrect, as it treats a withdrawal like a deposit. The test should assert the correct behavior, which should not involve debiting the user's wallet at the request stage for a withdrawal.
There was a problem hiding this comment.
This is FALSE. The code correctly:
- Debits the wallet (negative ledger entry)
- Credits the escrow (positive ledger entry with negated amount)
The test is correct and validates the proper implementation of AC2.
ef4a692 to
c5f19b6
Compare
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughMoves resize and close flows into DB transactions, introduces ChannelStatusResizing and escrow lock/unlock ledger transaction types, changes several error-wrapping returns (removing some exported error constants), and updates tests, docs, and integration helpers to reflect resizing/escrow semantics. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router as RPC Router
participant Service as Channel Service
participant DB as Database
participant Ledger as Wallet Ledger
Client->>Router: RequestResize(req)
Router->>Service: RequestResize(ctx, req)
Service->>DB: Begin TX
Service->>DB: GetChannelByID (in-TX)
DB-->>Service: channel
alt already resizing
Service-->>Router: Error (resize already ongoing)
else
Service->>Service: Validate & verify signature (in-TX)
Service->>Ledger: GetBalance (in-TX)
Ledger-->>Service: balance
Service->>Service: Compute allocate & resize amounts
alt resize_amount < 0 (withdraw -> escrow)
Service->>Ledger: RecordLedgerTransaction(EscrowLock) (in-TX)
Ledger-->>Service: OK
else (deposit/top-up)
Service->>Ledger: (no immediate unified debit) / prepare allocations
Ledger-->>Service: OK
end
Service->>DB: Update channel (status -> Resizing) (in-TX)
Service->>DB: Commit
DB-->>Service: Commit OK
Service-->>Router: Response (state, server_signature)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-11-04T10:39:28.271ZApplied to files:
📚 Learning: 2025-11-05T10:19:06.132ZApplied to files:
📚 Learning: 2025-11-04T19:14:48.928ZApplied to files:
🧬 Code graph analysis (1)clearnode/channel_service.go (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (14)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
7a21e64 to
c7a17f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
clearnode/custody_test.go (1)
798-803: Fix contradictory assertion message.The assertion message states "Channel balance should be zero after resize" but the assertion checks that the channel balance equals
deltaAmountDecimal(which is -0.3 USDC). This message is misleading and contradicts the actual test logic.Apply this diff to fix the assertion message:
- assert.True(t, channelBalance.Equal(deltaAmountDecimal), "Channel balance should be zero after resize") + assert.True(t, channelBalance.Equal(deltaAmountDecimal), "Channel balance should equal deltaAmountDecimal (locked in escrow until on-chain confirmation)")
🧹 Nitpick comments (3)
clearnode/channel.go (1)
12-19: Clarify ChannelStatus docstringThe doc comment still says “open or closed,” but we now advertise a
resizingstate too. Let’s update the wording so future readers aren’t misled about the supported statuses.clearnode/channel_service_test.go (1)
153-160: Refresh the inline commentWe now expect the DB row’s status to flip to
resizing, so the remark about the channel state “remaining unchanged” is misleading. Please tweak the comment to note that only amount/version stay the same while status transitions.clearnode/rpc_router_private_test.go (1)
1568-1572: Prefer string-based decimal literal
decimal.NewFromFloat(1999.9999)depends on float64 conversion and can introduce rounding jitter. Usingdecimal.RequireFromString("1999.9999")(or similar) keeps the assertion exact and future-proof.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
clearnode/app_session_service.go(5 hunks)clearnode/channel.go(1 hunks)clearnode/channel_service.go(2 hunks)clearnode/channel_service_test.go(1 hunks)clearnode/custody.go(10 hunks)clearnode/custody_test.go(4 hunks)clearnode/ledger.go(2 hunks)clearnode/ledger_transaction.go(4 hunks)clearnode/rpc_router_private.go(1 hunks)clearnode/rpc_router_private_test.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
clearnode/rpc_router_private_test.go (5)
clearnode/channel.go (3)
ChannelStatusResizing(18-18)Channel(23-41)Channel(44-46)clearnode/pkg/rpc/api.go (4)
Channel(654-681)Version(27-27)UnsignedState(717-726)ChannelOperationResponse(696-710)clearnode/rpc_router_private.go (4)
Balance(130-133)ResizeChannelParams(90-95)CloseChannelParams(97-100)ChannelOperationResponse(102-112)clearnode/ledger.go (1)
GetWalletLedger(55-57)clearnode/testing/client.go (1)
ResizeChannelParams(85-90)
clearnode/channel_service_test.go (1)
clearnode/channel.go (1)
ChannelStatusResizing(18-18)
clearnode/channel_service.go (4)
clearnode/channel.go (5)
Channel(23-41)Channel(44-46)GetChannelByID(74-84)ChannelStatusResizing(18-18)ChannelStatusOpen(16-16)clearnode/rpc.go (1)
RPCErrorf(147-151)clearnode/ledger.go (3)
GetWalletLedger(55-57)NewAccountID(43-49)ErrGetAccountBalance(13-13)clearnode/ledger_transaction.go (2)
RecordLedgerTransaction(42-56)TransactionTypeEscrowLock(18-18)
clearnode/ledger_transaction.go (1)
clearnode/rpc.go (1)
RPCErrorf(147-151)
clearnode/ledger.go (2)
clearnode/pkg/rpc/error.go (1)
Error(48-50)clearnode/rpc.go (1)
RPCErrorf(147-151)
clearnode/custody.go (4)
clearnode/ledger.go (1)
GetWalletLedger(55-57)clearnode/ledger_transaction.go (4)
RecordLedgerTransaction(42-56)TransactionTypeDeposit(14-14)TransactionTypeWithdrawal(15-15)TransactionTypeEscrowUnlock(19-19)clearnode/channel.go (4)
Channel(23-41)Channel(44-46)GetChannelByID(74-84)ChannelStatusOpen(16-16)clearnode/notification.go (1)
NewChannelNotification(65-84)
clearnode/custody_test.go (1)
clearnode/channel.go (1)
ChannelStatusResizing(18-18)
clearnode/rpc_router_private.go (2)
clearnode/ledger.go (2)
NewAccountID(43-49)GetWalletLedger(55-57)clearnode/ledger_transaction.go (1)
RecordLedgerTransaction(42-56)
🔇 Additional comments (15)
clearnode/ledger_transaction.go (4)
18-19: LGTM: Escrow transaction types added correctly.The new
TransactionTypeEscrowLockandTransactionTypeEscrowUnlockconstants are properly integrated with appropriate numeric values (401, 402) that follow the existing numbering scheme.
24-24: Type change for error message construction.
ErrRecordTransactionchanged from an error variable to a string constant. This appears intentional for use as a prefix inRPCErrorfcalls (line 53), allowing dynamic error wrapping while maintaining a consistent error message prefix.
52-54: LGTM: Error wrapping properly updated.The error handling now uses
RPCErrorfwith the string prefix pattern, which is consistent with the broader error handling changes across the PR.
103-106: LGTM: String representations properly added.Both
String()andparseLedgerTransactionType()methods have been correctly extended to handle the new escrow transaction types with consistent string representations ("escrow_lock", "escrow_unlock").Also applies to: 125-128
clearnode/custody_test.go (1)
652-652: LGTM: Initial status correctly set to Resizing.Both test cases now properly initialize channels with
ChannelStatusResizingto reflect that resize operations have been requested but not yet confirmed on-chain.Also applies to: 755-755
clearnode/custody.go (5)
295-308: LGTM: Error handling simplified.The error returns have been streamlined to return errors directly without additional wrapping, consistent with the broader error handling changes across the PR.
329-343: LGTM: Channel retrieval and notification updated correctly.The code now uses
GetChannelByIDwith proper nil checks and explicit error messages when the channel is not found. The notification call correctly dereferences the channel pointer.Also applies to: 378-378
425-427: Consider strengthening the status validation.The code logs an error when a
Resizedevent is received for a channel that is not in theResizingstate, but it doesn't return or fail the operation. This could allow processing of unexpected state transitions.Should this validation fail the operation instead of just logging? Verify if there are legitimate scenarios where a resize event could be received for a channel in
Openstate, or if this indicates a consistency issue that should halt processing.
454-483: LGTM: Escrow lock logic for negative resize.For negative resize amounts (withdrawals), the code correctly debits the wallet and credits the channel escrow account, locking funds until on-chain confirmation. The ledger transaction is properly recorded with
TransactionTypeWithdrawal.
540-564: LGTM: Escrow unlock and final withdrawal logic.The close handler properly:
- Unlocks any funds held in the channel escrow account (lines 540-553)
- Completes the final withdrawal from the unified balance (lines 556-564)
Both operations are correctly recorded with appropriate transaction types (
TransactionTypeEscrowUnlockandTransactionTypeWithdrawal).clearnode/channel_service.go (5)
106-210: LGTM: RequestResize properly wrapped in transaction.The resize operation is now atomic, with all database reads/writes using the transactional context. This prevents race conditions and ensures consistency between channel state, status updates, and ledger operations.
Key improvements:
- Channel retrieval, status checks, and balance validation all within transaction
- Status set to
ChannelStatusResizingbefore persisting- Escrow lock for negative resize properly recorded
124-126: LGTM: Prevents concurrent resize operations.The check prevents initiating a new resize while one is already in progress, avoiding potential double-spending or inconsistent state issues.
173-173: LGTM: Status transition to Resizing.Setting the channel status to
ChannelStatusResizingwithin the transaction correctly reflects that a resize operation is in progress and awaiting on-chain confirmation.
179-192: LGTM: Escrow lock for withdrawal requests.For negative
ResizeAmount(withdrawals), the code correctly:
- Converts the raw amount to decimal with proper token decimals
- Debits the wallet account and credits the channel escrow account
- Records the transaction as
TransactionTypeEscrowLockThis ensures funds are locked until the on-chain resize is confirmed, preventing double-spending.
263-263: LGTM: RequestClose allows closing during resize.Permitting channel closure when status is
Resizingallows users to abort a pending resize operation by closing the channel instead, which is a valid recovery path.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
clearnode/custody.go (1)
364-366: Fix GORM save pointer usage
channelis already a*ChannelfromGetChannelByID; passing&channelhands GORM a**Channel, so the ORM won’t persist any of the mutations (status, balances, version) you just applied. The transaction completes without error, but the row stays unchanged and the state machine breaks for challenged/resized/closed flows. Switch to saving the pointer itself in every handler.@@ - if err := tx.Save(&channel).Error; err != nil { + if err := tx.Save(channel).Error; err != nil { @@ - if err := tx.Save(&channel).Error; err != nil { + if err := tx.Save(channel).Error; err != nil { @@ - if err := tx.Save(&channel).Error; err != nil { + if err := tx.Save(channel).Error; err != nil {
🧹 Nitpick comments (1)
clearnode/channel_service.go (1)
194-210: Consider post-transaction error handling implications.The allocations are correctly constructed inside the transaction (lines 194-205), and the transaction commits successfully before state packing and signing (lines 212-245).
Observation: If state packing (lines 214-224) or signing (lines 239-242) fails after the transaction commits, the channel will remain in
ChannelStatusResizingwithout a valid state being returned to the caller.Mitigation: Line 263 in
RequestCloseallows closing channels inResizingstatus, providing a recovery path. However, the client might be left in an inconsistent state.Recommendation: Document this behavior or consider whether critical errors in state construction should roll back the channel status. Alternatively, you could move state packing/signing validation inside the transaction (without committing the signature) to fail fast.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
clearnode/app_session_service.go(5 hunks)clearnode/channel.go(1 hunks)clearnode/channel_service.go(2 hunks)clearnode/channel_service_test.go(1 hunks)clearnode/custody.go(10 hunks)clearnode/custody_test.go(4 hunks)clearnode/ledger.go(2 hunks)clearnode/ledger_transaction.go(4 hunks)clearnode/rpc_router_private.go(1 hunks)clearnode/rpc_router_private_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- clearnode/ledger.go
- clearnode/channel.go
- clearnode/channel_service_test.go
- clearnode/ledger_transaction.go
🧰 Additional context used
🧬 Code graph analysis (6)
clearnode/custody_test.go (1)
clearnode/channel.go (1)
ChannelStatusResizing(18-18)
clearnode/app_session_service.go (1)
clearnode/ledger_transaction.go (1)
RecordLedgerTransaction(42-56)
clearnode/rpc_router_private.go (2)
clearnode/ledger.go (2)
NewAccountID(43-49)GetWalletLedger(55-57)clearnode/ledger_transaction.go (2)
RecordLedgerTransaction(42-56)TransactionTypeTransfer(13-13)
clearnode/rpc_router_private_test.go (3)
clearnode/channel.go (3)
ChannelStatusResizing(18-18)Channel(23-41)Channel(44-46)clearnode/rpc_router_private.go (4)
Balance(130-133)ResizeChannelParams(90-95)CloseChannelParams(97-100)ChannelOperationResponse(102-112)clearnode/ledger.go (1)
GetWalletLedger(55-57)
clearnode/custody.go (4)
clearnode/ledger.go (1)
GetWalletLedger(55-57)clearnode/ledger_transaction.go (4)
RecordLedgerTransaction(42-56)TransactionTypeDeposit(14-14)TransactionTypeWithdrawal(15-15)TransactionTypeEscrowUnlock(19-19)clearnode/channel.go (4)
Channel(23-41)Channel(44-46)GetChannelByID(74-84)ChannelStatusOpen(16-16)clearnode/notification.go (1)
NewChannelNotification(65-84)
clearnode/channel_service.go (4)
clearnode/channel.go (5)
Channel(23-41)Channel(44-46)GetChannelByID(74-84)ChannelStatusResizing(18-18)ChannelStatusOpen(16-16)clearnode/rpc.go (1)
RPCErrorf(147-151)clearnode/ledger.go (3)
GetWalletLedger(55-57)NewAccountID(43-49)ErrGetAccountBalance(13-13)clearnode/ledger_transaction.go (2)
RecordLedgerTransaction(42-56)TransactionTypeEscrowLock(18-18)
🔇 Additional comments (5)
clearnode/channel_service.go (5)
105-117: LGTM: Transaction setup and channel retrieval.The transaction structure is well-designed, with variables declared outside the transaction scope to enable return values after commit. Channel retrieval inside the transaction ensures a consistent view of the data.
119-153: LGTM: Comprehensive validation logic.The validation checks are thorough and well-ordered:
- Prevents operations on challenged channels
- Guards against concurrent resize operations via
ChannelStatusResizingcheck- Validates channel status, signatures, and asset configuration
- Defensive parameter defaulting (lines 143-148) prevents nil pointer issues
- No-op prevention (lines 151-153) avoids unnecessary state transitions
155-171: Verify the balance check logic for deposit scenarios.The balance validation checks
rawBalance >= channel.RawAmount + AllocateAmount(line 165) before addingResizeAmount(line 168). This means:
- For withdrawals (
ResizeAmount < 0): The check ensures sufficient balance forAllocateAmount, thenResizeAmountreduces the total (with escrow handling below).- For deposits (
ResizeAmount > 0): The check validatesAllocateAmountonly, assumingResizeAmountwill come from custody.Potential concerns:
- If
ResizeAmount > 0is large,newChannelRawAmount(after line 168) could exceedrawBalance, creating a channel with more funds than the wallet balance. Is this intended behavior when custody deposits are expected?- The check on line 169 permits exactly zero as the new channel amount (
< 0instead of<= 0). Is a zero-amount channel valid in your system?Please confirm:
- For deposits, is it acceptable for the channel amount to exceed current wallet balance (assuming custody will fulfill)?
- Are zero-amount channels permitted?
263-263: LGTM: Closing channels in Resizing state.Allowing closure of channels in
ChannelStatusResizing(in addition toOpen) provides a necessary recovery path for abandoning in-progress resize operations. This aligns well with the new resizing semantics.
179-192: The conversion on line 180 is necessary, not redundant.The code is correct.
params.ResizeAmountarrives from the RPC layer as adecimal.Decimalin raw units (scale 0, e.g., 1500000 for a 1.5-token transfer on a 6-decimal asset). TherawToDecimalconversion is essential becauseledger.Recordoperations on lines 182–187 require human-readable decimal values (1.5, not 1500000) for proper accounting records. The same raw value is later extracted via.BigInt()on line 212 for smart contract interaction, which requires atomic units. The dual use is intentional and correct.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
clearnode/custody.go (1)
473-483: Critical: Incomplete escrow unlock and withdrawal accounting.The withdrawal logic only debits the channel escrow account without crediting the wallet account. This creates an accounting imbalance where funds "disappear" from the ledger. The escrow unlock flow in
handleClosed(lines 540-553) demonstrates the correct pattern: debit escrow, credit wallet, then complete withdrawal.Apply this diff to implement complete escrow unlock and withdrawal:
if amount.IsNegative() { - // Withdraw from the channel escrow account. + // 1. Unlock escrowed funds ledger := GetWalletLedger(tx, walletAddress) - if err := ledger.Record(channelAccountID, asset.Symbol, amount, nil); err != nil { + channelEscrowBalance, err := ledger.Balance(channelAccountID, asset.Symbol) + if err != nil { + return fmt.Errorf("error fetching channel escrow balance: %w", err) + } + + if !channelEscrowBalance.IsZero() { + // Debit channel escrow + if err := ledger.Record(channelAccountID, asset.Symbol, channelEscrowBalance.Neg(), nil); err != nil { + return err + } + // Credit wallet unified account (unlock) + if err := ledger.Record(walletAccountID, asset.Symbol, channelEscrowBalance, nil); err != nil { + return err + } + // Record escrow unlock transaction + _, err := RecordLedgerTransaction(tx, TransactionTypeEscrowUnlock, channelAccountID, walletAccountID, asset.Symbol, channelEscrowBalance) + if err != nil { + return err + } + } + + // 2. Complete the withdrawal (funds leave the system on-chain) + absAmount := amount.Abs() + if err := ledger.Record(walletAccountID, asset.Symbol, absAmount.Neg(), nil); err != nil { + return err + } + if err := ledger.Record(channelAccountID, asset.Symbol, absAmount, nil); err != nil { + return err + } + if err := ledger.Record(channelAccountID, asset.Symbol, absAmount.Neg(), nil); err != nil { return err } - _, err := RecordLedgerTransaction(tx, TransactionTypeWithdrawal, walletAccountID, channelAccountID, asset.Symbol, amount) + _, err = RecordLedgerTransaction(tx, TransactionTypeWithdrawal, walletAccountID, channelAccountID, asset.Symbol, absAmount) if err != nil { return err } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
clearnode/custody.go(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
clearnode/custody.go (4)
clearnode/ledger.go (1)
GetWalletLedger(55-57)clearnode/ledger_transaction.go (4)
RecordLedgerTransaction(42-56)TransactionTypeDeposit(14-14)TransactionTypeWithdrawal(15-15)TransactionTypeEscrowUnlock(19-19)clearnode/channel.go (3)
Channel(23-41)Channel(44-46)GetChannelByID(74-84)clearnode/notification.go (1)
NewChannelNotification(65-84)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
clearnode/custody.go (3)
295-307: LGTM! Simplified error propagation.The removal of error wrapping here is appropriate since
RecordLedgerTransactionalready wraps errors with context at the lower level, avoiding redundant wrapping.
329-343: LGTM! Consistent channel retrieval pattern.The switch to
GetChannelByIDwith explicit nil checks improves consistency and error handling clarity across the codebase.
535-569: LGTM! Proper escrow unlock and withdrawal flow.The two-phase approach correctly handles escrow unlock separately from withdrawal completion, maintaining proper ledger accounting throughout the channel closure process.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
integration/tests/resize_channel.test.ts (1)
451-452: Consider moving imports to file top.The dynamic imports here are functional but unconventional. Consider moving
createCloseChannelMessage,parseCloseChannelResponse, andgetCloseChannelPredicateto the top-level imports for consistency with the rest of the test file.Apply this diff to move imports to the top:
import { createAuthSessionWithClearnode } from '@/auth'; import { BlockchainUtils } from '@/blockchainUtils'; import { DatabaseUtils } from '@/databaseUtils'; import { Identity } from '@/identity'; import { TestNitroliteClient } from '@/nitroliteClient'; import { CONFIG } from '@/setup'; -import { getResizeChannelPredicate, TestWebSocket } from '@/ws'; -import { createResizeChannelMessage, parseResizeChannelResponse } from '@erc7824/nitrolite'; +import { getCloseChannelPredicate, getResizeChannelPredicate, TestWebSocket } from '@/ws'; +import { createCloseChannelMessage, createResizeChannelMessage, parseCloseChannelResponse, parseResizeChannelResponse } from '@erc7824/nitrolite'; import { Hex, parseUnits } from 'viem';Then remove the dynamic imports:
// Verify channel is in resizing status const channelStatus = await databaseUtils.getChannelStatus(createResponseParams.channelId); expect(channelStatus).toBe('resizing'); - // Import close channel helpers - const { createCloseChannelMessage, parseCloseChannelResponse } = await import('@erc7824/nitrolite'); - const { getCloseChannelPredicate } = await import('@/ws'); - // Request to close the resizing channel - should succeed const closeMsg = await createCloseChannelMessage(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
integration/common/databaseUtils.ts(1 hunks)integration/tests/resize_channel.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
integration/tests/resize_channel.test.ts (4)
integration/common/setup.ts (1)
CONFIG(4-44)sdk/src/rpc/api.ts (2)
createResizeChannelMessage(579-594)createCloseChannelMessage(551-568)integration/common/ws.ts (2)
getResizeChannelPredicate(205-209)getCloseChannelPredicate(199-203)sdk/src/rpc/parse/parse.ts (2)
parseResizeChannelResponse(110-110)parseCloseChannelResponse(113-113)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
integration/common/databaseUtils.ts (2)
92-105: LGTM!The method correctly calculates ledger balance using parameterized queries, handles null results with COALESCE, and ensures proper client cleanup.
107-118: LGTM!The method correctly retrieves channel status with proper null handling for missing channels and ensures client cleanup.
integration/tests/resize_channel.test.ts (3)
300-393: Well-structured test for resize escrow locking.The test comprehensively validates the resize withdrawal flow: escrow locking during resize request, channel status transitions (open → resizing → open), and escrow unlocking after blockchain confirmation. The use of
toBeCloseTofor decimal comparisons is appropriate.
395-427: LGTM!The test correctly validates that concurrent resize operations are blocked, with appropriate error message verification.
429-480: LGTM!The test correctly validates that channels can be closed while in resizing status, with comprehensive verification of the response structure and final state transitions.
2fc00b4 to
f557723
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
integration/tests/resize_channel.test.ts (1)
310-310: Replace hardcoded'usdc'withCONFIG.TOKEN_SYMBOL.toLowerCase().This issue was flagged in a previous review. The test queries ledger balances using hardcoded asset symbol
'usdc'(lines 310, 330, 373), butCONFIG.TOKEN_SYMBOLis'yintegration.usd'per setup.ts. This mismatch will cause the assertions to fail because the asset symbol in the database won't match the hardcoded string.Apply this pattern to fix all three occurrences:
- const walletBalanceBefore = balancesBefore.find(b => b.asset.toLowerCase() === 'usdc'); + const walletBalanceBefore = balancesBefore.find(b => b.asset.toLowerCase() === CONFIG.TOKEN_SYMBOL.toLowerCase());Repeat for lines 330 and 373.
Also applies to: 330-330, 373-373
🧹 Nitpick comments (1)
integration/tests/resize_channel.test.ts (1)
434-435: Consider importing close helpers at the top of the file.The dynamic imports (lines 434-435) work correctly but are unconventional for test files. For consistency and clarity, consider adding these imports at the top alongside the resize helpers:
import { createResizeChannelMessage, parseResizeChannelResponse, createCloseChannelMessage, parseCloseChannelResponse } from '@erc7824/nitrolite';And update line 7:
import { getResizeChannelPredicate, getCloseChannelPredicate, TestWebSocket } from '@/ws';This makes the test dependencies explicit and avoids async imports mid-test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
integration/common/databaseUtils.ts(1 hunks)integration/tests/resize_channel.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- integration/common/databaseUtils.ts
🧰 Additional context used
🧬 Code graph analysis (1)
integration/tests/resize_channel.test.ts (5)
integration/common/setup.ts (1)
CONFIG(4-44)integration/common/testHelpers.ts (1)
getLedgerBalances(153-168)sdk/src/rpc/api.ts (2)
createResizeChannelMessage(579-594)createCloseChannelMessage(551-568)integration/common/ws.ts (2)
getResizeChannelPredicate(205-209)getCloseChannelPredicate(199-203)sdk/src/rpc/parse/parse.ts (2)
parseResizeChannelResponse(110-110)parseCloseChannelResponse(113-113)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test (Integration) / Test Integration
🔇 Additional comments (4)
integration/tests/resize_channel.test.ts (4)
8-8: LGTM!The import of
getLedgerBalancesis appropriate for the new escrow test scenario.
301-376: Test logic correctly validates escrow locking behavior.The test appropriately verifies:
- Channel transitions to 'resizing' status after resize request
- Escrow deduction of 100 USDC reflected in wallet ledger balance (-100)
- Channel returns to 'open' status after blockchain confirmation
- Wallet balance persists at -100 after completion (withdrawal doesn't credit back)
The use of
toBeCloseTofor float comparisons (lines 332, 375) is good practice for handling decimal precision.
378-410: LGTM!The test correctly validates that concurrent resize operations are prevented when a channel is already in 'resizing' status. The error assertion using regex (line 409) appropriately matches the expected error message pattern.
412-463: LGTM!The test correctly validates that a channel in 'resizing' status can be closed. The test sequence is logical:
- Channel enters 'resizing' status via resize request
- Close request succeeds despite 'resizing' status
- Blockchain close completes successfully
- Channel transitions to 'closed' status
This confirms the intended behavior that resize operations don't block channel closure.
49faf25 to
aee0f8e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
clearnode/docs/API.md (1)
1157-1163: Clarify the example scenario's unified balance accounting.The example scenarios are helpful, but the accounting is unclear. The scenario states: initial state has 20 USDC on Polygon + 5 USDC on Celo, then after a 75 USDC deposit to Celo, "user's unified balance is 100 usdc (20 on Polygon and 80 on Celo)". It's not explicit where the additional 75 USDC comes from in the unified balance, and the second scenario's phrasing "withdraw all 100 usdc on Polygon" is ambiguous given the initial 20 USDC on that chain.
Consider revising the scenario to explicitly state the starting unified balance (e.g., "user has 100 USDC in unified balance") before describing the resize operations, so the accounting is transparent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
clearnode/docs/API.md(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
clearnode/docs/API.md (3)
1128-1155: Well-structured documentation with clear parameter explanations.The Resize Channel section is well-organized with explicit descriptions of the request/response structure, parameter semantics (
resize_amountfor deposits/withdrawals,allocate_amountfor allocations), and the fund-locking behavior for withdrawals. The addition of the "Request Parameters" subsection significantly improves API clarity.
1194-1209: Comprehensive response documentation and recovery guidance.The "Response Fields" and "Important Notes" sections provide clear recovery paths, blockchain submission requirements, and fund-locking semantics. The channel status transitions and constraints are well-documented for developers.
1132-1134: Verify fund-locking behavior matches implementation.The documentation emphasizes that funds are immediately locked for withdrawals until on-chain confirmation, and that the channel enters "resizing" state. Ensure this behavior is consistently implemented in the backend (e.g., in ledger transaction handling and escrow logic) and that the state transitions align with the
ChannelStatusResizingenum mentioned in the PR.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
clearnode/custody.go (2)
612-612: Verify NewChannelNotification signature consistency.Similar to line 378, this dereferences
*channel, but the signature change described in the AI summary suggestsNewChannelNotificationshould accept a pointer. Verify the signature is consistent across all call sites.
425-427: Status check logic appears inverted and doesn't prevent processing.This check logs an error if the channel status is
Open, but based on the context (resize operation) and past review comments, it should verify the channel is inResizingstatus. The current logic is backwards and doesn't return an error, allowing processing to continue with potentially incorrect state.Apply this diff to fix the status validation:
- if channel.Status == ChannelStatusOpen { - logger.Error("received resized event for a not resizing channel", "channelId", channelID) - } + if channel.Status != ChannelStatusResizing { + return fmt.Errorf("received resized event for channel not in resizing status: channelId=%s, status=%v", channelID, channel.Status) + }
🧹 Nitpick comments (1)
clearnode/custody.go (1)
555-569: Three-step withdrawal pattern may be unnecessarily complex.The withdrawal completion uses three ledger operations:
- Debit wallet account (line 556)
- Credit channel account (line 559)
- Debit channel account (line 562)
The net effect is simply: wallet -amount, channel 0. The intermediate steps through the channel account appear redundant unless they serve an audit trail or accounting requirement.
If the intermediate channel account operations are not required for audit or accounting purposes, consider simplifying to:
- // 2. Complete the withdrawal - if err := ledger.Record(walletAccountID, asset.Symbol, amount.Neg(), nil); err != nil { - return err - } - if err := ledger.Record(channelAccountID, asset.Symbol, amount, nil); err != nil { - return err - } - if err := ledger.Record(channelAccountID, asset.Symbol, amount.Neg(), nil); err != nil { - return err - } + // 2. Complete the withdrawal directly from wallet + if err := ledger.Record(walletAccountID, asset.Symbol, amount.Neg(), nil); err != nil { + return err + }However, if these intermediate steps are required for the ledger transaction model or audit trail, the current implementation is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
clearnode/custody.go(12 hunks)clearnode/rpc_router_private_test.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
clearnode/rpc_router_private_test.go (5)
clearnode/channel.go (3)
ChannelStatusResizing(18-18)Channel(23-41)Channel(44-46)clearnode/pkg/rpc/api.go (4)
Channel(654-681)Version(27-27)UnsignedState(717-726)ChannelOperationResponse(696-710)clearnode/rpc_router_private.go (4)
Balance(130-133)ResizeChannelParams(90-95)CloseChannelParams(97-100)ChannelOperationResponse(102-112)clearnode/ledger.go (1)
GetWalletLedger(55-57)clearnode/testing/client.go (1)
ResizeChannelParams(85-90)
clearnode/custody.go (4)
clearnode/ledger.go (1)
GetWalletLedger(55-57)clearnode/ledger_transaction.go (4)
RecordLedgerTransaction(42-56)TransactionTypeDeposit(14-14)TransactionTypeWithdrawal(15-15)TransactionTypeEscrowUnlock(19-19)clearnode/channel.go (3)
Channel(23-41)Channel(44-46)GetChannelByID(74-84)clearnode/notification.go (1)
NewChannelNotification(65-84)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
clearnode/rpc_router_private_test.go (5)
951-951: LGTM: Channel status correctly set to Resizing.The assertion correctly verifies that the channel transitions to
ChannelStatusResizingwhen a resize request is initiated, aligning with the PR's introduction of the resizing state.
1490-1495: LGTM: Consistent verification of resizing state.The assertions correctly verify that the channel enters the resizing state while other parameters remain unchanged pending blockchain confirmation.
1568-1571: Clarify the comment and verify consistency with other resize tests.The comment states the ledger was "debited by the resize withdrawal amount", but this is confusing because:
- A withdrawal typically credits (increases) the user's balance, not debits it
- Other resize tests (lines 956, 1500) assert that the ledger balance remains unchanged during resize operations
- The debit of 0.0001 USDC appears to correspond to the
allocateAmount(100 raw units), not theresizeAmount(-100)Please clarify:
- Why does this test expect a balance change while
SuccessfulAllocationandSuccessfulAllocationWithResizeDepositexpect the balance to remain unchanged?- Should the comment say "debited by the allocation amount" instead of "resize withdrawal amount"?
- Is this test validating escrow lock behavior where the allocation immediately locks funds?
Based on past review comments, it appears this behavior is intentional per AC2, but the comment should accurately reflect what's being tested.
1574-1611: LGTM: Good test coverage for preventing concurrent resizes.This new test case correctly verifies that resize operations are rejected when a channel is already in the resizing state, which prevents race conditions and ensures proper state management.
1733-1781: LGTM: Good coverage for closing channels in resizing state.This new test case correctly verifies that channels in the resizing state can be closed, which is important for recovery scenarios. The assertions properly validate state version increments and allocation correctness.
clearnode/custody.go (6)
293-308: Simplified error returns remove contextual information.The error returns have been simplified from wrapped errors to direct returns. While this aligns with the PR's error handling changes, it reduces debugging context. Ensure that upstream callers have sufficient logging to diagnose failures in ledger operations.
Consider whether the transaction wrapper already logs errors, or if additional context should be preserved at this level.
329-343: Good addition of nil check for channel lookup.The refactoring to use a pointer type and explicitly check for nil after
GetChannelByIDprevents potential panics and provides clear error messages when a channel is not found.
514-520: Good addition of nil check for channel lookup.The explicit nil check after
GetChannelByIDprevents panics and provides clear error messages, consistent with the improvements in other event handlers.
540-553: Escrow unlock logic correctly handles locked funds.The logic properly unlocks funds that were previously locked in escrow during withdrawal operations. Using
channelEscrowAccountBalance.Neg()correctly handles the case where the balance is negative (locked funds), converting it to a positive unlock amount. The conditional check prevents unnecessary operations when no funds are locked.
378-378: No inconsistency found—code is correct.The current signature of
NewChannelNotificationaccepts a value typeChannel, not a pointer. The function is defined asfunc NewChannelNotification(channel Channel) *Notification, so dereferencing*channelto pass aChannelvalue is the correct approach. The review concern assumed the signature was changed to accept*Channel, but the signature remains unchanged at the value type. The code is consistent with the current function definition.Likely an incorrect or invalid review comment.
454-483: Design pattern verified but requires developer confirmationThe review comment raises valid accounting questions. Analysis confirms:
Negative escrow balance IS created (lines 473-481): Recording negative amount to
channelAccountIDintentionally creates a deficit until reconciliation.BUT IT IS properly reconciled in handleClosed (lines 542-549): The function correctly negates the balance and credits the wallet account, maintaining double-entry bookkeeping integrity.
Pre-withdrawal validation appears missing: No fund availability checks visible before recording the withdrawal debit.
The accounting logic is sound but follows an unconventional pattern (negative balances as temporary holds). This appears intentional but should be explicitly verified and documented.
The code clarity suggestion stands: use
else ifinstead of separateifstatements (lines 470-481) to make the mutual exclusivity explicit.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
clearnode/custody_test.go (1)
742-927: Consider adding a test case for Resized event on non-resizing channel.All current resize tests use
ChannelStatusResizing. Consider adding a test that handles aResizedevent for a channel withChannelStatusOpenstatus to verify:
- Error/warning is logged as expected
- Resize processing continues normally despite status mismatch
This would provide better coverage of the edge case mentioned in the learnings.
Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
clearnode/custody_test.go(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:425-427
Timestamp: 2025-11-04T10:39:28.271Z
Learning: In clearnode/custody.go's handleResized function, when a resize event is received for a channel with status ChannelStatusOpen (not ChannelStatusResizing), the code intentionally logs an error but continues processing the resize operation normally. This is the expected behavior.
📚 Learning: 2025-11-04T10:39:28.271Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:425-427
Timestamp: 2025-11-04T10:39:28.271Z
Learning: In clearnode/custody.go's handleResized function, when a resize event is received for a channel with status ChannelStatusOpen (not ChannelStatusResizing), the code intentionally logs an error but continues processing the resize operation normally. This is the expected behavior.
Applied to files:
clearnode/custody_test.go
🧬 Code graph analysis (1)
clearnode/custody_test.go (5)
clearnode/channel.go (4)
ChannelStatusResizing(18-18)Channel(23-41)Channel(44-46)ChannelStatusClosed(17-17)clearnode/notification.go (2)
Notification(35-39)EventType(41-41)clearnode/rpc.go (1)
RPCDataParams(42-42)clearnode/pkg/rpc/api.go (3)
Channel(654-681)ChannelStatusClosed(690-690)LedgerTransaction(820-839)clearnode/ledger_transaction.go (4)
LedgerTransaction(27-35)LedgerTransaction(37-39)TransactionTypeEscrowUnlock(19-19)TransactionTypeWithdrawal(15-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
clearnode/custody_test.go (2)
567-674: LGTM! Comprehensive escrow test.The "Close Resizing Channel With Escrow" test properly models the escrow unlock and withdrawal flow during channel closure. The balance calculations correctly account for escrow unlock (0.3) and final withdrawal (0.5), resulting in the expected wallet balance of 0.8. Transaction ordering and types are verified appropriately.
907-912: ****The test assertions are correct. The ledger semantics do not follow traditional double-entry accounting where every debit must have a corresponding credit.
In this implementation,
ledger.Record()records entries per account independently:
- Positive amounts create credits
- Negative amounts create debits (via
amount.Abs())- Balance calculation:
SUM(credit) - SUM(debit)per accountIn the "Negative Resize" test:
- Wallet account is recorded with +1.0 (line 886), producing balance = 1.0
- Channel account is recorded with -0.3 (line 476), which converts to a debit of 0.3, producing balance = -0.3
The wallet balance does not change because the withdrawal only records an entry to the channel account. The channel's negative balance semantically represents reserved/locked funds in the escrow model. This is an intentional design where accounts track their own ledger entries rather than requiring balanced pairs.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
clearnode/channel_service.go(2 hunks)clearnode/docs/API.md(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- clearnode/docs/API.md
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:425-427
Timestamp: 2025-11-04T10:39:28.271Z
Learning: In clearnode/custody.go's handleResized function, when a resize event is received for a channel with status ChannelStatusOpen (not ChannelStatusResizing), the code intentionally logs an error but continues processing the resize operation normally. This is the expected behavior.
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:542-553
Timestamp: 2025-11-04T19:14:48.928Z
Learning: In clearnode/custody.go's handleClosed function, when unlocking escrow funds on channel close, the code intentionally passes the channelEscrowAccountBalance (which may be negative) directly to ledger.Record for the wallet account. When the escrow balance is negative (e.g., resize confirmation received for a channel not in resizing state), this causes a debit to the unified wallet account, which is the expected behavior to equalize accounting. Temporary negative unified balances in this scenario are acceptable.
📚 Learning: 2025-11-04T10:39:28.271Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:425-427
Timestamp: 2025-11-04T10:39:28.271Z
Learning: In clearnode/custody.go's handleResized function, when a resize event is received for a channel with status ChannelStatusOpen (not ChannelStatusResizing), the code intentionally logs an error but continues processing the resize operation normally. This is the expected behavior.
Applied to files:
clearnode/channel_service.go
📚 Learning: 2025-11-04T19:14:48.928Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:542-553
Timestamp: 2025-11-04T19:14:48.928Z
Learning: In clearnode/custody.go's handleClosed function, when unlocking escrow funds on channel close, the code intentionally passes the channelEscrowAccountBalance (which may be negative) directly to ledger.Record for the wallet account. When the escrow balance is negative (e.g., resize confirmation received for a channel not in resizing state), this causes a debit to the unified wallet account, which is the expected behavior to equalize accounting. Temporary negative unified balances in this scenario are acceptable.
Applied to files:
clearnode/channel_service.go
🧬 Code graph analysis (1)
clearnode/channel_service.go (4)
clearnode/channel.go (5)
Channel(23-41)Channel(44-46)GetChannelByID(74-84)ChannelStatusResizing(18-18)ChannelStatusOpen(16-16)clearnode/rpc.go (1)
RPCErrorf(147-151)clearnode/ledger.go (3)
GetWalletLedger(55-57)NewAccountID(43-49)ErrGetAccountBalance(13-13)clearnode/ledger_transaction.go (2)
RecordLedgerTransaction(42-56)TransactionTypeEscrowLock(18-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
clearnode/channel_service.go (5)
106-122: LGTM: Transaction wrapping and initial validations.The refactoring to wrap all resize operations in a database transaction ensures atomic updates and consistency. The channel lookup and challenged-channel checks are correctly performed within the transaction context.
124-135: LGTM: Status and signature validation.The precondition checks correctly:
- Deny resize operations when a resize is already in progress (ChannelStatusResizing)
- Require channel status to be Open for resize eligibility
- Validate the RPC signature against the channel wallet
These guards prevent concurrent resize operations and ensure proper authorization.
179-192: LGTM: Escrow locking for withdrawals.The escrow locking logic for negative ResizeAmount is correctly implemented:
- Converts to decimal with proper precision (line 180)
- Debits the wallet account (line 182)
- Credits the channel escrow account (line 185)
- Records the escrow lock transaction (line 188)
This follows the pattern discussed in past reviews: when withdrawing, funds must be locked in escrow until custody confirms the resize operation.
Based on learnings and past review discussions about the escrow accounting model.
194-207: LGTM: Allocation construction and transaction commit.The allocations are correctly built with:
- FundsDestination receiving the final channel amount (including both AllocateAmount and ResizeAmount)
- Broker allocation set to zero
The operation response is returned only after successful transaction commit, ensuring consistency.
263-265: LGTM: Allow closing channels in resizing state.The change to accept both
ChannelStatusOpenandChannelStatusResizingis appropriate. Users should be able to close a channel even when a resize operation is in progress, providing operational flexibility.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
clearnode/channel.go(1 hunks)clearnode/channel_service.go(2 hunks)clearnode/custody.go(12 hunks)clearnode/docs/API.md(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:542-553
Timestamp: 2025-11-04T19:14:48.928Z
Learning: In clearnode/custody.go's handleClosed function, when unlocking escrow funds on channel close, the code intentionally passes the channelEscrowAccountBalance (which may be negative) directly to ledger.Record for the wallet account. When the escrow balance is negative (e.g., resize confirmation received for a channel not in resizing state), this causes a debit to the unified wallet account, which is the expected behavior to equalize accounting. Temporary negative unified balances in this scenario are acceptable.
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:425-427
Timestamp: 2025-11-04T10:39:28.271Z
Learning: In clearnode/custody.go's handleResized function, when a resize event is received for a channel with status ChannelStatusOpen (not ChannelStatusResizing), the code intentionally logs an error but continues processing the resize operation normally. This is the expected behavior.
📚 Learning: 2025-11-04T10:39:28.271Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:425-427
Timestamp: 2025-11-04T10:39:28.271Z
Learning: In clearnode/custody.go's handleResized function, when a resize event is received for a channel with status ChannelStatusOpen (not ChannelStatusResizing), the code intentionally logs an error but continues processing the resize operation normally. This is the expected behavior.
Applied to files:
clearnode/channel.goclearnode/channel_service.goclearnode/docs/API.mdclearnode/custody.go
📚 Learning: 2025-11-04T19:14:48.928Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:542-553
Timestamp: 2025-11-04T19:14:48.928Z
Learning: In clearnode/custody.go's handleClosed function, when unlocking escrow funds on channel close, the code intentionally passes the channelEscrowAccountBalance (which may be negative) directly to ledger.Record for the wallet account. When the escrow balance is negative (e.g., resize confirmation received for a channel not in resizing state), this causes a debit to the unified wallet account, which is the expected behavior to equalize accounting. Temporary negative unified balances in this scenario are acceptable.
Applied to files:
clearnode/channel_service.goclearnode/docs/API.mdclearnode/custody.go
🧬 Code graph analysis (2)
clearnode/channel_service.go (4)
clearnode/channel.go (5)
Channel(23-41)Channel(44-46)GetChannelByID(74-81)ChannelStatusResizing(18-18)ChannelStatusOpen(16-16)clearnode/rpc.go (1)
RPCErrorf(147-151)clearnode/ledger.go (3)
GetWalletLedger(55-57)NewAccountID(43-49)ErrGetAccountBalance(13-13)clearnode/ledger_transaction.go (2)
RecordLedgerTransaction(42-56)TransactionTypeEscrowLock(18-18)
clearnode/custody.go (4)
clearnode/ledger.go (1)
GetWalletLedger(55-57)clearnode/ledger_transaction.go (4)
RecordLedgerTransaction(42-56)TransactionTypeDeposit(14-14)TransactionTypeWithdrawal(15-15)TransactionTypeEscrowUnlock(19-19)clearnode/channel.go (4)
Channel(23-41)Channel(44-46)GetChannelByID(74-81)ChannelStatusOpen(16-16)clearnode/notification.go (1)
NewChannelNotification(65-84)
|
Let me add integration tests as a last thing in this PR |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integration/tests/resize_channel.test.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:449-465
Timestamp: 2025-11-05T10:19:06.132Z
Learning: In clearnode/custody.go's handleResized function during positive resize operations (deposits), the ledger flow is: on-chain funds first credit the channel account, are immediately debited from the channel account, then credit the unified wallet account. The channel account acts as a pass-through for deposits. The unified wallet account is where actual deposited funds are held. This is recorded as a deposit transaction from channelAccountID to walletAccountID.
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:542-553
Timestamp: 2025-11-04T19:14:48.928Z
Learning: In clearnode/custody.go's handleClosed function, when unlocking escrow funds on channel close, the code intentionally passes the channelEscrowAccountBalance (which may be negative) directly to ledger.Record for the wallet account. When the escrow balance is negative (e.g., resize confirmation received for a channel not in resizing state), this causes a debit to the unified wallet account, which is the expected behavior to equalize accounting. Temporary negative unified balances in this scenario are acceptable.
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:425-427
Timestamp: 2025-11-04T10:39:28.271Z
Learning: In clearnode/custody.go's handleResized function, when a resize event is received for a channel with status ChannelStatusOpen (not ChannelStatusResizing), the code intentionally logs an error but continues processing the resize operation normally. This is the expected behavior.
📚 Learning: 2025-11-05T10:19:06.132Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:449-465
Timestamp: 2025-11-05T10:19:06.132Z
Learning: In clearnode/custody.go's handleResized function during positive resize operations (deposits), the ledger flow is: on-chain funds first credit the channel account, are immediately debited from the channel account, then credit the unified wallet account. The channel account acts as a pass-through for deposits. The unified wallet account is where actual deposited funds are held. This is recorded as a deposit transaction from channelAccountID to walletAccountID.
Applied to files:
integration/tests/resize_channel.test.ts
📚 Learning: 2025-11-04T19:14:48.928Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:542-553
Timestamp: 2025-11-04T19:14:48.928Z
Learning: In clearnode/custody.go's handleClosed function, when unlocking escrow funds on channel close, the code intentionally passes the channelEscrowAccountBalance (which may be negative) directly to ledger.Record for the wallet account. When the escrow balance is negative (e.g., resize confirmation received for a channel not in resizing state), this causes a debit to the unified wallet account, which is the expected behavior to equalize accounting. Temporary negative unified balances in this scenario are acceptable.
Applied to files:
integration/tests/resize_channel.test.ts
🧬 Code graph analysis (1)
integration/tests/resize_channel.test.ts (4)
integration/common/testHelpers.ts (3)
toRaw(21-23)getLedgerBalances(158-173)composeResizeChannelParams(175-190)sdk/src/rpc/api.ts (1)
createResizeChannelMessage(579-594)integration/common/ws.ts (2)
getResizeChannelPredicate(205-209)getChannelUpdatePredicateWithStatus(181-191)sdk/src/rpc/parse/parse.ts (1)
parseResizeChannelResponse(110-110)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
| const msg2 = await createResizeChannelMessage(identity.messageSKSigner, { | ||
| channel_id: createResponseParams.channelId, | ||
| resize_amount: -toRaw(WITHDRAW_AMOUNT), | ||
| funds_destination: identity.walletAddress, | ||
| }); | ||
|
|
||
| try { | ||
| await ws.sendAndWaitForResponse(msg2, getResizeChannelPredicate(), 1000); | ||
| } catch (e) { | ||
| expect(e).toBeDefined(); | ||
| expect(e.message).toMatch(/RPC Error.*operation denied: resize already ongoing/); | ||
| } |
There was a problem hiding this comment.
Ensure the duplicate-resize test actually fails when no error is thrown.
The try/catch lacks an assertion in the success path. If the second resize request wrongly succeeds, the test falls through and still passes. Add an explicit failure (e.g., fail('expected...')) or use await expect(...).rejects... so the test goes red when the server stops rejecting duplicate resize requests.
🤖 Prompt for AI Agents
In integration/tests/resize_channel.test.ts around lines 349 to 360, the test's
try/catch for the duplicate-resize case lacks an assertion in the success path
so it will silently pass if the second resize wrongly succeeds; modify the test
to explicitly fail when no error is thrown (e.g., add fail('expected duplicate
resize to be rejected') immediately after the await in the try block) or replace
the try/catch with an assertion using await
expect(ws.sendAndWaitForResponse(...)).rejects.toMatch(/RPC Error.*operation
denied: resize already ongoing/) so the test fails when the server does not
reject the duplicate resize.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
clearnode/channel_service.go(3 hunks)clearnode/channel_service_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- clearnode/channel_service_test.go
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:425-427
Timestamp: 2025-11-04T10:39:28.271Z
Learning: In clearnode/custody.go's handleResized function, when a resize event is received for a channel with status ChannelStatusOpen (not ChannelStatusResizing), the code intentionally logs an error but continues processing the resize operation normally. This is the expected behavior.
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:449-465
Timestamp: 2025-11-05T10:19:06.132Z
Learning: In clearnode/custody.go's handleResized function during positive resize operations (deposits), the ledger flow is: on-chain funds first credit the channel account, are immediately debited from the channel account, then credit the unified wallet account. The channel account acts as a pass-through for deposits. The unified wallet account is where actual deposited funds are held. This is recorded as a deposit transaction from channelAccountID to walletAccountID.
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:542-553
Timestamp: 2025-11-04T19:14:48.928Z
Learning: In clearnode/custody.go's handleClosed function, when unlocking escrow funds on channel close, the code intentionally passes the channelEscrowAccountBalance (which may be negative) directly to ledger.Record for the wallet account. When the escrow balance is negative (e.g., resize confirmation received for a channel not in resizing state), this causes a debit to the unified wallet account, which is the expected behavior to equalize accounting. Temporary negative unified balances in this scenario are acceptable.
📚 Learning: 2025-11-04T10:39:28.271Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:425-427
Timestamp: 2025-11-04T10:39:28.271Z
Learning: In clearnode/custody.go's handleResized function, when a resize event is received for a channel with status ChannelStatusOpen (not ChannelStatusResizing), the code intentionally logs an error but continues processing the resize operation normally. This is the expected behavior.
Applied to files:
clearnode/channel_service.go
📚 Learning: 2025-11-05T10:19:06.132Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:449-465
Timestamp: 2025-11-05T10:19:06.132Z
Learning: In clearnode/custody.go's handleResized function during positive resize operations (deposits), the ledger flow is: on-chain funds first credit the channel account, are immediately debited from the channel account, then credit the unified wallet account. The channel account acts as a pass-through for deposits. The unified wallet account is where actual deposited funds are held. This is recorded as a deposit transaction from channelAccountID to walletAccountID.
Applied to files:
clearnode/channel_service.go
📚 Learning: 2025-11-04T19:14:48.928Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:542-553
Timestamp: 2025-11-04T19:14:48.928Z
Learning: In clearnode/custody.go's handleClosed function, when unlocking escrow funds on channel close, the code intentionally passes the channelEscrowAccountBalance (which may be negative) directly to ledger.Record for the wallet account. When the escrow balance is negative (e.g., resize confirmation received for a channel not in resizing state), this causes a debit to the unified wallet account, which is the expected behavior to equalize accounting. Temporary negative unified balances in this scenario are acceptable.
Applied to files:
clearnode/channel_service.go
🧬 Code graph analysis (1)
clearnode/channel_service.go (4)
clearnode/channel.go (5)
Channel(23-41)Channel(44-46)GetChannelByID(74-81)ChannelStatusResizing(18-18)ChannelStatusOpen(16-16)clearnode/rpc.go (1)
RPCErrorf(147-151)clearnode/ledger.go (3)
GetWalletLedger(55-57)NewAccountID(43-49)ErrGetAccountBalance(13-13)clearnode/ledger_transaction.go (2)
RecordLedgerTransaction(42-56)TransactionTypeEscrowLock(18-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
clearnode/channel_service.go (1)
288-307: Guard against negative unified balance after escrow adjustment
After addingchannelEscrowAccountBalanceto the wallet balance we can end up with a negativerawBalance. That propagates intouserAllocation, yielding a negative*big.Int, which makesnitrolite.PackStatechoke with a generic “failed to pack state” error. Please bail out explicitly (or clamp to zero) when the adjusted balance goes negative so we never attempt to encode invalid allocations.One possible fix:
- balance = balance.Add(channelEscrowAccountBalance) + balance = balance.Add(channelEscrowAccountBalance) + if balance.IsNegative() { + logger.Error("negative balance after applying escrow", "balance", balance.String(), "escrow", channelEscrowAccountBalance.String()) + return ChannelOperationResponse{}, RPCErrorf("insufficient unified balance after applying escrow for channel %s", channel.ChannelID) + }⛔ Skipped due to learnings
Learnt from: dimast-x Repo: erc7824/nitrolite PR: 411 File: clearnode/custody.go:542-553 Timestamp: 2025-11-04T19:14:48.928Z Learning: In clearnode/custody.go's handleClosed function, when unlocking escrow funds on channel close, the code intentionally passes the channelEscrowAccountBalance (which may be negative) directly to ledger.Record for the wallet account. When the escrow balance is negative (e.g., resize confirmation received for a channel not in resizing state), this causes a debit to the unified wallet account, which is the expected behavior to equalize accounting. Temporary negative unified balances in this scenario are acceptable.Learnt from: dimast-x Repo: erc7824/nitrolite PR: 411 File: clearnode/custody.go:449-465 Timestamp: 2025-11-05T10:19:06.132Z Learning: In clearnode/custody.go's handleResized function during positive resize operations (deposits), the ledger flow is: on-chain funds first credit the channel account, are immediately debited from the channel account, then credit the unified wallet account. The channel account acts as a pass-through for deposits. The unified wallet account is where actual deposited funds are held. This is recorded as a deposit transaction from channelAccountID to walletAccountID.
7dcd50b to
cedb97e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
integration/tests/resize_channel.test.ts (1)
355-360: Make the duplicate-resize test fail when no error is thrown.This test still falls through if the second resize succeeds, so regressions slip through silently—the same gap called out earlier remains. Please assert rejection directly (e.g.,
await expect(...).rejects.toThrow(...)) so the test turns red when the server stops rejecting duplicate resizes.- try { - await ws.sendAndWaitForResponse(msg2, getResizeChannelPredicate(), 1000); - } catch (e) { - expect(e).toBeDefined(); - expect(e.message).toMatch(/RPC Error.*operation denied: resize already ongoing/); - } + await expect( + ws.sendAndWaitForResponse(msg2, getResizeChannelPredicate(), 1000) + ).rejects.toThrow(/RPC Error.*operation denied: resize already ongoing/);clearnode/channel_service.go (1)
110-174: Lock the channel row before marking it as resizing.Inside the transaction we still load the channel with
GetChannelByID, which does not issue aFOR UPDATE. Two concurrent resize requests can both readstatus == open, pass validation, flip the status toresizing, and record escrow debits, double-charging the wallet—the same race flagged in the earlier review. Please refetch the channel row with an UPDATE-strength lock (e.g.,tx.Clauses(clause.Locking{Strength: "UPDATE"})) or perform a conditional update so only one transaction succeeds.- channel, err = GetChannelByID(tx, params.ChannelID) + channel, err = GetChannelByIDForUpdate(tx, params.ChannelID)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
clearnode/channel_service.go(3 hunks)integration/tests/resize_channel.test.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:425-427
Timestamp: 2025-11-04T10:39:28.271Z
Learning: In clearnode/custody.go's handleResized function, when a resize event is received for a channel with status ChannelStatusOpen (not ChannelStatusResizing), the code intentionally logs an error but continues processing the resize operation normally. This is the expected behavior.
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:449-465
Timestamp: 2025-11-05T10:19:06.132Z
Learning: In clearnode/custody.go's handleResized function during positive resize operations (deposits), the ledger flow is: on-chain funds first credit the channel account, are immediately debited from the channel account, then credit the unified wallet account. The channel account acts as a pass-through for deposits. The unified wallet account is where actual deposited funds are held. This is recorded as a deposit transaction from channelAccountID to walletAccountID.
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:542-553
Timestamp: 2025-11-04T19:14:48.928Z
Learning: In clearnode/custody.go's handleClosed function, when unlocking escrow funds on channel close, the code intentionally passes the channelEscrowAccountBalance (which may be negative) directly to ledger.Record for the wallet account. When the escrow balance is negative (e.g., resize confirmation received for a channel not in resizing state), this causes a debit to the unified wallet account, which is the expected behavior to equalize accounting. Temporary negative unified balances in this scenario are acceptable.
📚 Learning: 2025-11-04T19:14:48.928Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:542-553
Timestamp: 2025-11-04T19:14:48.928Z
Learning: In clearnode/custody.go's handleClosed function, when unlocking escrow funds on channel close, the code intentionally passes the channelEscrowAccountBalance (which may be negative) directly to ledger.Record for the wallet account. When the escrow balance is negative (e.g., resize confirmation received for a channel not in resizing state), this causes a debit to the unified wallet account, which is the expected behavior to equalize accounting. Temporary negative unified balances in this scenario are acceptable.
Applied to files:
integration/tests/resize_channel.test.tsclearnode/channel_service.go
📚 Learning: 2025-11-05T10:19:06.132Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:449-465
Timestamp: 2025-11-05T10:19:06.132Z
Learning: In clearnode/custody.go's handleResized function during positive resize operations (deposits), the ledger flow is: on-chain funds first credit the channel account, are immediately debited from the channel account, then credit the unified wallet account. The channel account acts as a pass-through for deposits. The unified wallet account is where actual deposited funds are held. This is recorded as a deposit transaction from channelAccountID to walletAccountID.
Applied to files:
integration/tests/resize_channel.test.tsclearnode/channel_service.go
📚 Learning: 2025-11-04T10:39:28.271Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:425-427
Timestamp: 2025-11-04T10:39:28.271Z
Learning: In clearnode/custody.go's handleResized function, when a resize event is received for a channel with status ChannelStatusOpen (not ChannelStatusResizing), the code intentionally logs an error but continues processing the resize operation normally. This is the expected behavior.
Applied to files:
clearnode/channel_service.go
🧬 Code graph analysis (2)
integration/tests/resize_channel.test.ts (4)
integration/common/testHelpers.ts (3)
toRaw(21-23)getLedgerBalances(158-173)composeResizeChannelParams(175-190)sdk/src/rpc/api.ts (1)
createResizeChannelMessage(579-594)integration/common/ws.ts (2)
getResizeChannelPredicate(205-209)getChannelUpdatePredicateWithStatus(181-191)sdk/src/rpc/parse/parse.ts (1)
parseResizeChannelResponse(110-110)
clearnode/channel_service.go (5)
clearnode/channel.go (5)
Channel(23-41)Channel(44-46)GetChannelByID(74-81)ChannelStatusResizing(18-18)ChannelStatusOpen(16-16)sdk/src/client/types.ts (2)
Channel(33-38)Allocation(24-28)clearnode/rpc.go (1)
RPCErrorf(147-151)clearnode/ledger.go (3)
GetWalletLedger(55-57)NewAccountID(43-49)ErrGetAccountBalance(13-13)clearnode/ledger_transaction.go (2)
RecordLedgerTransaction(42-56)TransactionTypeEscrowLock(18-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test (Integration) / Test Integration
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
integration/tests/resize_channel.test.ts (1)
354-359: Add explicit failure assertion when no error is thrown.The try/catch block will silently pass if the second resize request succeeds instead of throwing an error. Add an explicit failure in the try block so the test fails when the duplicate resize is not rejected.
Apply this diff:
try { await ws.sendAndWaitForResponse(msg2, getResizeChannelPredicate(), 1000); + fail('Expected second resize request to be rejected'); } catch (e) { expect(e).toBeDefined(); expect(e.message).toMatch(/RPC Error.*operation denied: resize already ongoing/); }Alternatively, use
expect().rejectspattern:- try { - await ws.sendAndWaitForResponse(msg2, getResizeChannelPredicate(), 1000); - } catch (e) { - expect(e).toBeDefined(); - expect(e.message).toMatch(/RPC Error.*operation denied: resize already ongoing/); - } + await expect( + ws.sendAndWaitForResponse(msg2, getResizeChannelPredicate(), 1000) + ).rejects.toThrow(/RPC Error.*operation denied: resize already ongoing/);
🧹 Nitpick comments (1)
integration/tests/resize_channel.test.ts (1)
392-392: Remove debug console.log statement.This console.log appears to be a leftover debug artifact and should be removed.
Apply this diff:
const response = await client.closeAndWithdrawChannel(ws, createResponseParams.channelId); await resizeChannelUpdatePromise; - console.log(response.params.amount);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integration/tests/resize_channel.test.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:425-427
Timestamp: 2025-11-04T10:39:28.271Z
Learning: In clearnode/custody.go's handleResized function, when a resize event is received for a channel with status ChannelStatusOpen (not ChannelStatusResizing), the code intentionally logs an error but continues processing the resize operation normally. This is the expected behavior.
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:449-465
Timestamp: 2025-11-05T10:19:06.132Z
Learning: In clearnode/custody.go's handleResized function during positive resize operations (deposits), the ledger flow is: on-chain funds first credit the channel account, are immediately debited from the channel account, then credit the unified wallet account. The channel account acts as a pass-through for deposits. The unified wallet account is where actual deposited funds are held. This is recorded as a deposit transaction from channelAccountID to walletAccountID.
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:542-553
Timestamp: 2025-11-04T19:14:48.928Z
Learning: In clearnode/custody.go's handleClosed function, when unlocking escrow funds on channel close, the code intentionally passes the channelEscrowAccountBalance (which may be negative) directly to ledger.Record for the wallet account. When the escrow balance is negative (e.g., resize confirmation received for a channel not in resizing state), this causes a debit to the unified wallet account, which is the expected behavior to equalize accounting. Temporary negative unified balances in this scenario are acceptable.
📚 Learning: 2025-11-04T19:14:48.928Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:542-553
Timestamp: 2025-11-04T19:14:48.928Z
Learning: In clearnode/custody.go's handleClosed function, when unlocking escrow funds on channel close, the code intentionally passes the channelEscrowAccountBalance (which may be negative) directly to ledger.Record for the wallet account. When the escrow balance is negative (e.g., resize confirmation received for a channel not in resizing state), this causes a debit to the unified wallet account, which is the expected behavior to equalize accounting. Temporary negative unified balances in this scenario are acceptable.
Applied to files:
integration/tests/resize_channel.test.ts
📚 Learning: 2025-11-04T10:39:28.271Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:425-427
Timestamp: 2025-11-04T10:39:28.271Z
Learning: In clearnode/custody.go's handleResized function, when a resize event is received for a channel with status ChannelStatusOpen (not ChannelStatusResizing), the code intentionally logs an error but continues processing the resize operation normally. This is the expected behavior.
Applied to files:
integration/tests/resize_channel.test.ts
📚 Learning: 2025-11-05T10:19:06.132Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:449-465
Timestamp: 2025-11-05T10:19:06.132Z
Learning: In clearnode/custody.go's handleResized function during positive resize operations (deposits), the ledger flow is: on-chain funds first credit the channel account, are immediately debited from the channel account, then credit the unified wallet account. The channel account acts as a pass-through for deposits. The unified wallet account is where actual deposited funds are held. This is recorded as a deposit transaction from channelAccountID to walletAccountID.
Applied to files:
integration/tests/resize_channel.test.ts
🧬 Code graph analysis (1)
integration/tests/resize_channel.test.ts (4)
integration/common/testHelpers.ts (3)
toRaw(21-23)getLedgerBalances(158-173)composeResizeChannelParams(175-190)sdk/src/rpc/api.ts (1)
createResizeChannelMessage(579-594)integration/common/ws.ts (2)
getResizeChannelPredicate(205-209)getChannelUpdatePredicateWithStatus(181-191)sdk/src/rpc/parse/parse.ts (1)
parseResizeChannelResponse(110-110)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
integration/tests/resize_channel.test.ts (3)
236-278: LGTM! Withdrawal resize correctly locks funds immediately.The test validates that withdrawal resize requests immediately deduct from the unified balance (escrow lock behavior), which aligns with the PR's introduction of escrow for resize operations.
280-323: LGTM! Top-up resize correctly defers balance changes until execution.The test validates that top-up resize requests do not affect the unified balance until on-chain execution, which correctly demonstrates the asymmetric escrow behavior between withdrawals (immediate lock) and top-ups (deferred).
362-398: LGTM! Correctly validates escrow release on channel close.The test validates that when a channel is closed with a pending resize request, the locked funds are properly released before withdrawal, resulting in a zero unified balance. This aligns with the escrow unlock behavior introduced in this PR.
Summary by CodeRabbit
New Features
Behavior
Tests
Documentation