YNU-408: deny outcoming offchain ops for users with non-zero channels#414
YNU-408: deny outcoming offchain ops for users with non-zero channels#414
Conversation
|
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. WalkthroughAdds DB-level aggregation and guards that reject transfers, deposits, and app-session creation when a wallet has non-zero allocations in open or resizing channels; exposes ChannelAmountSum/GetChannelAmountSumByWallet; updates server and integration tests and nitrolite client flow to use allocate_amount semantics. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Handler as RPC / AppSessionService
participant Guard as ensureWalletHasAllAllocationsEmpty
participant Query as GetChannelAmountSumByWallet
participant DB as Database
Client->>Handler: Request (transfer / deposit / create_session)
Handler->>Guard: ensureWalletHasAllAllocationsEmpty(tx, wallet)
Guard->>Query: aggregate COUNT,SUM where wallet & status IN (Open,Resizing)
Query->>DB: execute aggregation
DB-->>Query: {count, sum}
Query-->>Guard: {count, sum}
alt sum == 0
Guard-->>Handler: OK
Handler->>Handler: proceed with ledger / state changes
Handler-->>Client: Success
else sum != 0
Guard-->>Handler: Error (deny)
Handler-->>Client: Error ("operation denied: non-zero allocation in N channel(s)...")
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Summary of ChangesHello @philanton, 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 enhances the system's financial integrity by implementing a crucial validation mechanism. It ensures that users cannot perform specific outgoing off-chain transactions, such as deposits, creating app sessions, or transfers, if they currently have funds allocated within active channels. This measure prevents potential inconsistencies and ensures that channel funds are properly managed, requiring users to un-allocate funds before proceeding with these 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! |
There was a problem hiding this comment.
Code Review
This pull request introduces a check to prevent users with funds allocated in off-chain channels from performing certain operations like transfers and app session deposits. This is a good security and correctness improvement. The implementation adds a new query to sum up channel allocations and applies this check in HandleTransfer, DepositUpdater.Update, and CreateAppSession.
My review includes two main points. First, a TODO comment in the new channel query regarding a ChannelStatusResizing status should be addressed to ensure the check is complete. Second, for better code organization, the new validation function ensureWalletHasZeroChannelAllocation should be moved to a more central location as it's used across different services. Overall, the changes are logical and well-implemented.
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 (3)
clearnode/app_session_service.go(2 hunks)clearnode/channel.go(1 hunks)clearnode/rpc_router_private.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
clearnode/rpc_router_private.go (2)
clearnode/channel.go (1)
GetChannelAmountSumByWallet(123-135)clearnode/rpc.go (1)
RPCErrorf(147-151)
⏰ 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.go (2)
118-121: LGTM!The
ChannelAmountSumstruct is well-defined with appropriate GORM column tags for aggregation results.
123-135: Resolve or remove the TODO referencing a non-existent status.The TODO comment at line 127 mentions filtering by
ChannelStatusResizing, but this status constant does not exist in the codebase. Only three statuses are defined:ChannelStatusOpen,ChannelStatusClosed, andChannelStatusChallenged.The NULL handling concern is not applicable here—
raw_amountis defined with aNOT NULLconstraint in both the database schema and the Channel struct, so SQL'sSUM()will return 0 rather than NULL when no rows match.Clarify whether the TODO reflects an incomplete feature or outdated speculation. If filtering additional statuses is needed, update the comment with the actual status value and implementation plan.
clearnode/rpc_router_private.go (1)
327-329: LGTM!The validation is correctly placed after
checkChallengedChannelsand before any ledger operations, ensuring the check happens within the same transaction.clearnode/app_session_service.go (2)
153-155: LGTM!The validation is correctly placed within the deposit flow, after allocation checks and before balance verification and ledger operations. This ensures deposits are blocked when the wallet has non-zero channel allocations.
436-438: LGTM!The validation is correctly placed within the app session creation flow, after challenged channel checks and before balance verification and ledger operations. This ensures session creation with deposits is blocked when the wallet has non-zero channel allocations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
integration/tests/close_channel.test.ts (1)
76-105: Consider clarifying the resize semantics with better variable names.The test introduces a resize-before-close flow where
allocate_amount: depositAmountandresize_amount: -depositAmountare used. This pattern results in zero channel allocations (lines 89-91 verify both allocations are '0'). However, the variable reuse ofmsgat line 105 makes the flow slightly harder to follow.Consider using more descriptive variable names:
- let msg = await createResizeChannelMessage(identity.messageSKSigner, { + const resizeMsg = await createResizeChannelMessage(identity.messageSKSigner, { channel_id: params.channelId, allocate_amount: depositAmount, resize_amount: -depositAmount, funds_destination: identity.walletAddress, }); - const resizeResponse = await ws.sendAndWaitForResponse(msg, getResizeChannelPredicate(), 1000); + const resizeResponse = await ws.sendAndWaitForResponse(resizeMsg, getResizeChannelPredicate(), 1000); // ... resize flow ... - msg = await createCloseChannelMessage(identity.messageSKSigner, params.channelId, identity.walletAddress); + const closeMsg = await createCloseChannelMessage(identity.messageSKSigner, params.channelId, identity.walletAddress); - const closeResponse = await ws.sendAndWaitForResponse(msg, getCloseChannelPredicate(), 1000); + const closeResponse = await ws.sendAndWaitForResponse(closeMsg, getCloseChannelPredicate(), 1000);integration/tests/lifecycle_nitrorpc_v02.test.ts (1)
36-36: Remove unused import.The import
onfrom 'events' appears to be unused in this file. Please remove it to keep the imports clean.-import { on } from 'events';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
clearnode/app_session_service_test.go(6 hunks)clearnode/rpc_router_private_test.go(2 hunks)integration/common/nitroliteClient.ts(1 hunks)integration/tests/close_channel.test.ts(3 hunks)integration/tests/lifecycle_nitrorpc_v02.test.ts(6 hunks)integration/tests/lifecycle_nitrorpc_v04.test.ts(5 hunks)integration/tests/resize_channel.test.ts(10 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.
📚 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/app_session_service_test.gointegration/tests/resize_channel.test.tsintegration/tests/lifecycle_nitrorpc_v04.test.tsintegration/tests/lifecycle_nitrorpc_v02.test.tsintegration/tests/close_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:
clearnode/app_session_service_test.gointegration/tests/resize_channel.test.tsintegration/tests/lifecycle_nitrorpc_v04.test.tsintegration/common/nitroliteClient.tsintegration/tests/lifecycle_nitrorpc_v02.test.tsintegration/tests/close_channel.test.ts
🧬 Code graph analysis (6)
clearnode/app_session_service_test.go (3)
clearnode/channel.go (3)
Channel(22-40)Channel(43-45)ChannelStatusOpen(16-16)clearnode/rpc_router_private.go (3)
CreateAppSessionParams(42-46)AppAllocation(62-66)SubmitAppStateParams(48-54)clearnode/ledger.go (1)
NewAccountID(44-50)
clearnode/rpc_router_private_test.go (5)
clearnode/channel.go (2)
Channel(22-40)Channel(43-45)clearnode/pkg/rpc/api.go (4)
Channel(652-679)TransferAllocation(566-571)AppDefinition(578-593)AppAllocation(626-633)clearnode/ledger.go (1)
GetWalletLedger(56-58)clearnode/rpc_router_private.go (4)
TransferParams(27-31)TransferAllocation(37-40)CreateAppSessionParams(42-46)AppAllocation(62-66)clearnode/rpc_router_public.go (1)
AppDefinition(36-44)
integration/tests/resize_channel.test.ts (3)
sdk/src/rpc/api.ts (1)
createGetLedgerBalancesMessage(245-261)integration/common/ws.ts (1)
getGetLedgerBalancesPredicate(218-222)sdk/src/rpc/parse/parse.ts (1)
parseGetLedgerBalancesResponse(75-76)
integration/tests/lifecycle_nitrorpc_v04.test.ts (5)
sdk/src/rpc/api.ts (1)
createResizeChannelMessage(579-594)integration/common/testHelpers.ts (2)
toRaw(21-23)composeResizeChannelParams(175-190)integration/common/ws.ts (1)
getResizeChannelPredicate(205-209)sdk/src/rpc/parse/parse.ts (1)
parseResizeChannelResponse(110-110)integration/common/setup.ts (1)
CONFIG(4-44)
integration/tests/lifecycle_nitrorpc_v02.test.ts (5)
sdk/src/rpc/api.ts (1)
createResizeChannelMessage(579-594)integration/common/testHelpers.ts (2)
toRaw(21-23)composeResizeChannelParams(175-190)integration/common/ws.ts (1)
getResizeChannelPredicate(205-209)sdk/src/rpc/parse/parse.ts (1)
parseResizeChannelResponse(110-110)integration/common/setup.ts (1)
CONFIG(4-44)
integration/tests/close_channel.test.ts (4)
sdk/src/rpc/api.ts (2)
createResizeChannelMessage(579-594)createCloseChannelMessage(551-568)integration/common/ws.ts (1)
getResizeChannelPredicate(205-209)sdk/src/rpc/parse/parse.ts (1)
parseResizeChannelResponse(110-110)integration/common/testHelpers.ts (1)
composeResizeChannelParams(175-190)
⏰ 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 (12)
clearnode/app_session_service_test.go (3)
280-316: LGTM! Clear test coverage for zero-allocation guard on app session creation.The test correctly verifies that creating an app session is denied when a participant has non-zero channel allocations. The setup creates a channel with
RawAmount: decimal.NewFromInt(1)andStatus: ChannelStatusOpen, then confirms the expected error message is returned.
393-423: Verify the comment: "Shouldn't return error if operator has non-zero allocation".The comment states that the operate workflow should NOT return an error when a user has non-zero channel allocations. This aligns with the PR objective of denying only "outcoming offchain ops" (deposit, transfer, app-session creation), not operate operations. Please confirm this behavior is intentional and that operate operations are deliberately excluded from the zero-allocation guard.
Based on learnings: The distinction between operations that require zero allocations (deposit, transfer, create) versus those that don't (operate, withdraw, close) should be clearly documented in the code or PR description.
1037-1085: LGTM! Deposit workflow correctly enforces zero-allocation guard.The test properly verifies that deposit operations are denied when a participant has non-zero channel allocations, matching the expected error message format.
clearnode/rpc_router_private_test.go (2)
579-612: LGTM! Transfer operation correctly enforces zero-allocation guard.The test properly verifies that transfer operations are denied when the sender has non-zero channel allocations. The test setup and error message are consistent with other zero-allocation guard tests.
737-780: LGTM! App session creation correctly enforces zero-allocation guard.The test verifies that creating an app session is denied when participants have non-zero channel allocations. The test creates two channels (one for each participant) with non-zero amounts and confirms the expected error is returned.
integration/tests/resize_channel.test.ts (2)
68-75: LGTM! Clear ledger balance verification added.The test now verifies the unified wallet ledger balance after channel creation, showing 500 units are available in the virtual ledger while channel balance is 0. This helps validate the ledger accounting introduced in this PR.
Based on learnings: The unified wallet account holds actual deposited funds, while the channel account acts as a pass-through.
203-271: LGTM! Test clearly demonstrates allocation vs. resize semantics.This test effectively shows the distinction between:
allocate_amount: moves funds between virtual ledger and channel allocation (no on-chain change)resize_amount: changes total channel capacity (on-chain deposit/withdrawal)The test allocates 100 to the channel without changing the resize amount, resulting in a channel balance of 100 (line 263) while the ledger balance remains unchanged at 500 (line 270). This validates the dual accounting model.
integration/tests/lifecycle_nitrorpc_v04.test.ts (2)
299-331: LGTM! Clear resize-before-close flow with proper verification.The test correctly implements the resize-before-close pattern, verifying that allocations are zeroed out (lines 310-315) and balances are properly updated (lines 329-330). The test name accurately reflects the new behavior.
360-392: LGTM! Bob's resize flow mirrors Alice's pattern consistently.The test follows the same resize pattern as Alice's flow, with appropriate balance adjustments (finalBobAmount = 1125). The allocation verification (line 374) and balance check (line 391) are correct.
integration/tests/lifecycle_nitrorpc_v02.test.ts (2)
209-241: LGTM! Alice's resize-before-close flow correctly implemented.The test properly implements the resize-before-close pattern with correct balance assertions (line 240: 900 = 1000 - 100). The allocation verification (lines 220-225) confirms zero allocations before closing.
271-304: LGTM! Bob's resize flow with correct balance calculations.The test correctly handles Bob's case where he receives funds from the app session, resulting in a final balance of 1100 (1000 + 100). The allocation verification (line 286) and balance check (line 303) are accurate.
integration/common/nitroliteClient.ts (1)
109-109: Pattern verified as consistent across all integration tests.The
allocate_amount: -resizeAmountpattern in nitroliteClient.ts:109 is correct and consistently applied throughout the codebase. All 9 usages ofcreateResizeChannelMessagefollow the same inverse relationship:allocate_amount + resize_amount = 0. This ensures that during resize operations, funds move through the channel account in opposite directions—when resizing in (+), allocation moves out (-), and vice versa—maintaining zero net channel allocation as intended by this PR.
nksazonov
left a comment
There was a problem hiding this comment.
Some comments and questions
e6388bd to
ac874c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
clearnode/app_session_service.go (1)
435-437: Participants without allocations bypass the check during app creation.Because the guard runs inside the allocations loop, a participant listed in Definition.ParticipantWallets with zero allocation won’t be checked and can still join with non-zero channels. If that’s unintended, pre-scan all participants and fail early. Otherwise, please confirm the product choice and document it.
Example tweak:
err = s.db.Transaction(func(tx *gorm.DB) error { + // Optional: enforce zero-channel-amount for all participants, not only depositors + for _, p := range params.Definition.ParticipantWallets { + if err := ensureWalletHasZeroChannelAllocation(tx, p); err != nil { + return err + } + } for _, alloc := range params.Allocations { ...clearnode/rpc_router_private_test.go (1)
737-780: Duplicate coverage with app_session_service tests — consider removing or consolidating.This create_app_session non‑zero allocation test duplicates coverage already present in clearnode/app_session_service_test.go. Keeping one reduces maintenance and flakiness surface.
Do you want me to open a follow‑up to consolidate create_app_session guard tests under one suite?
🧹 Nitpick comments (6)
clearnode/app_session_service_test.go (1)
280-316: Good coverage for “non-zero channel allocation” across flows; a couple of small hardening ideas.
- Tests correctly fail create/deposit and permit operate/withdraw/close with non-zero channels. LGTM.
- To make failures more actionable, consider including the offending wallet in the error text and update assertions accordingly.
- Since multiple tests assert the same message, consider a shared test constant to reduce brittleness if wording changes.
Would you like a small test helper (const + assertContains helper) to de-duplicate the error string usage?
Also applies to: 393-404, 1037-1085, 1112-1123, 1340-1350
integration/tests/close_channel.test.ts (1)
7-14: Resize-before-close path is correct; consider sturdier waits.The added resize round-trip + on-chain confirm is spot on for zeroing channel allocation prior to close. To reduce flakiness in CI, bump WS waits (1000ms) to 3000–5000ms or make them configurable.
If desired, I can submit a small helper to centralize default WS timeouts.
Also applies to: 58-58, 64-68, 76-104
integration/tests/lifecycle_nitrorpc_v02.test.ts (2)
36-36: Remove unused import.The import { on } from 'events' isn’t used.
209-241: End-to-end resize-before-close flows LGTM; make waits/configs less brittle.Flows and balance assertions look consistent. Consider:
- Extracting the common error message string to a shared constant.
- Raising WS wait timeouts to avoid CI flakes.
Also applies to: 243-269, 271-304, 306-332
integration/tests/lifecycle_nitrorpc_v04.test.ts (1)
299-331: v0.4 resize/close coverage is consistent; consider test resilience tweaks.
- Keep a single constant for the non‑zero allocation error text to avoid brittle assertions across files.
- Increase WS wait thresholds (or make configurable) to cut flakes.
Also applies to: 333-358, 360-392, 394-419
integration/tests/resize_channel.test.ts (1)
8-14: Ledger-focused assertions are on point; a few test hygiene improvements.
- Prefer computing expected ledger amounts from depositAmount (or helpers) instead of hard-coded "500/600" to avoid drift if decimals change.
- Centralize WS timeout (1000ms) into a single constant and bump to 3000–5000ms for CI stability.
- Nice touch verifying unified balance locks on withdrawal request and stays unchanged on top-up request.
Also applies to: 32-37, 66-74, 78-82, 124-130, 148-159, 168-169, 191-199, 201-269, 263-269, 284-289, 316-359, 374-389, 413-437
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
clearnode/app_session_service.go(2 hunks)clearnode/app_session_service_test.go(6 hunks)clearnode/channel.go(1 hunks)clearnode/rpc_router_private.go(2 hunks)clearnode/rpc_router_private_test.go(2 hunks)integration/common/nitroliteClient.ts(1 hunks)integration/tests/close_channel.test.ts(3 hunks)integration/tests/lifecycle_nitrorpc_v02.test.ts(6 hunks)integration/tests/lifecycle_nitrorpc_v04.test.ts(5 hunks)integration/tests/resize_channel.test.ts(16 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- clearnode/rpc_router_private.go
- integration/common/nitroliteClient.ts
- clearnode/channel.go
🧰 Additional context used
🧠 Learnings (4)
📓 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.
📚 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/app_session_service_test.goclearnode/app_session_service.gointegration/tests/close_channel.test.tsintegration/tests/lifecycle_nitrorpc_v02.test.tsintegration/tests/lifecycle_nitrorpc_v04.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.tsclearnode/app_session_service_test.goclearnode/app_session_service.gointegration/tests/close_channel.test.tsintegration/tests/lifecycle_nitrorpc_v02.test.tsintegration/tests/lifecycle_nitrorpc_v04.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:
clearnode/app_session_service_test.go
🧬 Code graph analysis (6)
integration/tests/resize_channel.test.ts (6)
integration/common/nitroliteClient.ts (1)
TestNitroliteClient(27-187)sdk/src/rpc/api.ts (2)
createGetLedgerBalancesMessage(247-263)createResizeChannelMessage(581-596)integration/common/ws.ts (1)
getGetLedgerBalancesPredicate(218-222)sdk/src/rpc/parse/parse.ts (1)
parseGetLedgerBalancesResponse(75-76)integration/common/setup.ts (1)
CONFIG(4-44)integration/common/testHelpers.ts (1)
toRaw(21-23)
clearnode/app_session_service_test.go (5)
clearnode/channel.go (3)
Channel(23-41)Channel(44-46)ChannelStatusOpen(16-16)clearnode/pkg/rpc/api.go (6)
Channel(652-679)ChannelStatusOpen(686-686)AppDefinition(578-593)AppAllocation(626-633)AppSessionIntentDeposit(642-642)Version(27-27)clearnode/rpc_router_private.go (3)
CreateAppSessionParams(42-46)AppAllocation(62-66)SubmitAppStateParams(48-54)clearnode/ledger.go (1)
NewAccountID(43-49)clearnode/notification.go (1)
Notification(35-39)
clearnode/rpc_router_private_test.go (4)
clearnode/channel.go (3)
Channel(23-41)Channel(44-46)ChannelStatusOpen(16-16)clearnode/pkg/rpc/api.go (5)
Channel(652-679)ChannelStatusOpen(686-686)TransferAllocation(566-571)AppDefinition(578-593)AppAllocation(626-633)clearnode/ledger.go (1)
GetWalletLedger(55-57)clearnode/rpc_router_private.go (4)
TransferParams(27-31)TransferAllocation(37-40)CreateAppSessionParams(42-46)AppAllocation(62-66)
integration/tests/close_channel.test.ts (4)
sdk/src/rpc/api.ts (2)
createResizeChannelMessage(581-596)createCloseChannelMessage(553-570)integration/common/ws.ts (1)
getResizeChannelPredicate(205-209)sdk/src/rpc/parse/parse.ts (1)
parseResizeChannelResponse(110-110)integration/common/testHelpers.ts (1)
composeResizeChannelParams(175-190)
integration/tests/lifecycle_nitrorpc_v02.test.ts (5)
sdk/src/rpc/api.ts (1)
createResizeChannelMessage(581-596)integration/common/testHelpers.ts (2)
toRaw(21-23)composeResizeChannelParams(175-190)integration/common/ws.ts (1)
getResizeChannelPredicate(205-209)sdk/src/rpc/parse/parse.ts (1)
parseResizeChannelResponse(110-110)integration/common/setup.ts (1)
CONFIG(4-44)
integration/tests/lifecycle_nitrorpc_v04.test.ts (5)
sdk/src/rpc/api.ts (1)
createResizeChannelMessage(581-596)integration/common/testHelpers.ts (2)
toRaw(21-23)composeResizeChannelParams(175-190)integration/common/ws.ts (1)
getResizeChannelPredicate(205-209)sdk/src/rpc/parse/parse.ts (1)
parseResizeChannelResponse(110-110)integration/common/setup.ts (1)
CONFIG(4-44)
⏰ 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/app_session_service.go (1)
152-154: Zero-allocation guard placement looks right (early fail before ledger ops).Calling ensureWalletHasZeroChannelAllocation before any debit/credit in both deposit flow and app creation path is the correct control point. Good move.
Please confirm that the error text is intentionally user-facing and stable, since multiple tests assert it verbatim. If it might change, consider funneling through a shared constant to avoid brittle tests.
Also applies to: 435-437
clearnode/rpc_router_private_test.go (1)
579-613: Router transfer guard test looks good.Asserts the new guard trips before transfer moves funds. Solid.
c114825 to
3f5595b
Compare
3f5595b to
70b53d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
integration/tests/resize_channel.test.ts (1)
261-268: Clarify the ledger balance accounting with channel allocations.The test shows that after allocating 100 to the channel (line 261), the ledger balance remains at 500 (line 268). The comment explains "100 allocated to the channel are still in virtual ledger", which is somewhat confusing.
Consider clarifying whether:
- Allocated channel funds are counted in both ledger balance AND channel balance
- Or if the ledger balance represents total off-chain funds (including channel allocations)
This would help future maintainers understand the accounting model.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
clearnode/app_session_service.go(2 hunks)clearnode/app_session_service_test.go(6 hunks)clearnode/channel.go(1 hunks)clearnode/rpc_router_private.go(2 hunks)clearnode/rpc_router_private_test.go(1 hunks)integration/common/nitroliteClient.ts(1 hunks)integration/tests/close_channel.test.ts(3 hunks)integration/tests/lifecycle_nitrorpc_v02.test.ts(6 hunks)integration/tests/lifecycle_nitrorpc_v04.test.ts(5 hunks)integration/tests/resize_channel.test.ts(16 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- clearnode/app_session_service.go
- clearnode/rpc_router_private.go
- clearnode/rpc_router_private_test.go
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:449-465
Timestamp: 2025-11-05T10:19:06.155Z
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.960Z
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-05T10:19:06.155Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:449-465
Timestamp: 2025-11-05T10:19:06.155Z
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/lifecycle_nitrorpc_v04.test.tsclearnode/channel.gointegration/tests/resize_channel.test.tsintegration/tests/lifecycle_nitrorpc_v02.test.tsintegration/common/nitroliteClient.tsclearnode/app_session_service_test.gointegration/tests/close_channel.test.ts
📚 Learning: 2025-11-04T19:14:48.960Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:542-553
Timestamp: 2025-11-04T19:14:48.960Z
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/lifecycle_nitrorpc_v04.test.tsclearnode/channel.gointegration/tests/resize_channel.test.tsintegration/tests/lifecycle_nitrorpc_v02.test.tsclearnode/app_session_service_test.gointegration/tests/close_channel.test.ts
📚 Learning: 2025-11-04T10:39:28.297Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:425-427
Timestamp: 2025-11-04T10:39:28.297Z
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.go
📚 Learning: 2025-10-27T11:25:10.651Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 392
File: clearnode/session_key.go:36-49
Timestamp: 2025-10-27T11:25:10.651Z
Learning: For the erc7824/nitrolite repository, the team uses their own ticket tracking system and prefers not to have GitHub issues created for follow-up items or technical debt tracking.
Applied to files:
clearnode/channel.goclearnode/app_session_service_test.go
🧬 Code graph analysis (5)
integration/tests/lifecycle_nitrorpc_v04.test.ts (5)
sdk/src/rpc/api.ts (1)
createResizeChannelMessage(581-596)integration/common/testHelpers.ts (2)
toRaw(21-23)composeResizeChannelParams(175-190)integration/common/ws.ts (1)
getResizeChannelPredicate(205-209)sdk/src/rpc/parse/parse.ts (1)
parseResizeChannelResponse(110-110)integration/common/setup.ts (1)
CONFIG(4-44)
integration/tests/resize_channel.test.ts (6)
integration/common/nitroliteClient.ts (1)
TestNitroliteClient(27-187)sdk/src/rpc/api.ts (2)
createGetLedgerBalancesMessage(247-263)createResizeChannelMessage(581-596)integration/common/ws.ts (1)
getGetLedgerBalancesPredicate(218-222)sdk/src/rpc/parse/parse.ts (1)
parseGetLedgerBalancesResponse(75-76)integration/common/setup.ts (1)
CONFIG(4-44)integration/common/testHelpers.ts (1)
toRaw(21-23)
integration/tests/lifecycle_nitrorpc_v02.test.ts (5)
sdk/src/rpc/api.ts (1)
createResizeChannelMessage(581-596)integration/common/testHelpers.ts (2)
toRaw(21-23)composeResizeChannelParams(175-190)integration/common/ws.ts (1)
getResizeChannelPredicate(205-209)sdk/src/rpc/parse/parse.ts (1)
parseResizeChannelResponse(110-110)integration/common/setup.ts (1)
CONFIG(4-44)
clearnode/app_session_service_test.go (4)
clearnode/channel.go (3)
Channel(23-41)Channel(44-46)ChannelStatusOpen(16-16)clearnode/pkg/rpc/api.go (6)
Channel(652-679)ChannelStatusOpen(686-686)AppDefinition(578-593)VersionNitroRPCv0_4(33-33)AppAllocation(626-633)Version(27-27)clearnode/rpc_router_private.go (3)
CreateAppSessionParams(42-46)AppAllocation(62-66)SubmitAppStateParams(48-54)clearnode/ledger.go (1)
NewAccountID(43-49)
integration/tests/close_channel.test.ts (4)
sdk/src/rpc/api.ts (1)
createResizeChannelMessage(581-596)integration/common/ws.ts (1)
getResizeChannelPredicate(205-209)sdk/src/rpc/parse/parse.ts (1)
parseResizeChannelResponse(110-110)integration/common/testHelpers.ts (1)
composeResizeChannelParams(175-190)
⏰ 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 (12)
integration/tests/close_channel.test.ts (4)
7-14: LGTM! Imports properly added for resize flow.The imports align with the newly exported resize-related helpers and are correctly organized.
58-67: LGTM! Test structure properly updated for resize-before-close flow.The test name accurately reflects the new multi-step flow, and capturing the
statefrom channel creation is necessary for composing resize parameters.
76-103: LGTM! Resize flow properly implements withdrawal-before-close pattern.The resize operation correctly withdraws on-chain funds (
resize_amount: -depositAmount) while debiting the virtual ledger (allocate_amount: depositAmount) to maintain accounting balance. The validation that allocations are zero (lines 89, 91) aligns with the PR objective to ensure channels have no locked funds before closing.
105-141: LGTM! Close and withdrawal flow properly implemented.The close-channel and withdrawal sequence correctly validates the final state, waits for transaction receipts, and verifies that funds are fully recovered (line 140).
integration/tests/resize_channel.test.ts (8)
8-14: LGTM! Imports properly added for ledger balance validations.The new imports support the enhanced test coverage for ledger-based accounting.
31-36: LGTM! Client setup appropriately moved to beforeAll.Since
TestNitroliteClientis a stateless wrapper around wallet and blockchain clients, and all state is properly reset inafterEachhooks, instantiating once inbeforeAllis more efficient and safe.
55-130: LGTM! Test properly validates ledger-based accounting for deposit resize.The test correctly validates that:
- Initial channel balance is 0 with funds in virtual ledger (lines 66, 73)
- Deposit resize (
resize_amount > 0, allocate_amount < 0) credits the virtual ledger (line 129: 500 → 600)- Allocations remain at 0 since funds aren't locked in channel (line 98)
132-199: LGTM! Test properly validates withdrawal resize accounting.The test correctly validates withdrawal resize (
resize_amount < 0, allocate_amount > 0) debits the virtual ledger (line 198: 500 → 400) while maintaining zero allocations.
271-314: LGTM! Test validates immediate ledger impact of withdrawal resize requests.The test correctly verifies that withdrawal resize requests immediately decrease the unified balance (line 297) before on-chain confirmation, which is essential for preventing double-spending.
316-359: LGTM! Test validates deferred ledger credit for top-up resize.The test correctly demonstrates that top-up resize (without
allocate_amount) only credits the ledger after on-chain confirmation (line 358), not on request (line 342). This asymmetry with withdrawal resizes (which immediately debit) is appropriate for preventing over-withdrawal.
361-398: LGTM! Test properly validates resize lock mechanism.The test correctly validates that concurrent resize requests are prevented with an appropriate error message (line 396).
400-437: LGTM! Test validates proper fund release on close with pending resize.The test correctly validates that closing a channel with a pending (but not executed) resize releases locked funds back to the ledger (line 436). The explanatory comment (lines 434-436) provides valuable context for this edge case.
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)
integration/common/nitroliteClient.ts(2 hunks)integration/tests/nitrorpc_v02.test.ts(1 hunks)integration/tests/nitrorpc_v04.test.ts(4 hunks)integration/tests/transfer.test.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:449-465
Timestamp: 2025-11-05T10:19:06.155Z
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.960Z
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-05T10:19:06.155Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:449-465
Timestamp: 2025-11-05T10:19:06.155Z
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/common/nitroliteClient.ts
🧬 Code graph analysis (4)
integration/tests/transfer.test.ts (6)
integration/common/nitroliteClient.ts (1)
TestNitroliteClient(27-188)integration/common/databaseUtils.ts (1)
DatabaseUtils(4-117)integration/common/blockchainUtils.ts (1)
BlockchainUtils(22-222)integration/common/ws.ts (2)
TestWebSocket(9-139)getTransferPredicate(242-246)sdk/src/rpc/api.ts (1)
createTransferMessage(691-718)sdk/src/rpc/parse/parse.ts (2)
parseTransferResponse(146-146)parseAnyRPCResponse(12-40)
integration/tests/nitrorpc_v04.test.ts (6)
sdk/src/client/types.ts (1)
State(82-84)integration/common/testHelpers.ts (4)
createTestChannels(28-49)toRaw(21-23)createTestAppSession(98-151)authenticateAppWithAllowances(54-74)integration/common/setup.ts (1)
CONFIG(4-44)integration/common/testAppSessionHelpers.ts (1)
submitAppStateUpdate_v04(53-86)integration/common/testSetup.ts (1)
fetchAndParseAppSessions(66-93)integration/common/auth.ts (1)
createAuthSessionWithClearnode(11-45)
integration/common/nitroliteClient.ts (2)
integration/common/ws.ts (1)
TestWebSocket(9-139)sdk/src/rpc/api.ts (1)
createResizeChannelMessage(581-596)
integration/tests/nitrorpc_v02.test.ts (8)
integration/common/setup.ts (1)
CONFIG(4-44)integration/common/ws.ts (1)
TestWebSocket(9-139)integration/common/identity.ts (1)
Identity(7-33)integration/common/nitroliteClient.ts (1)
TestNitroliteClient(27-188)integration/common/blockchainUtils.ts (1)
BlockchainUtils(22-222)integration/common/databaseUtils.ts (1)
DatabaseUtils(4-117)integration/common/testSetup.ts (1)
setupTestIdentitiesAndConnections(27-60)integration/common/testHelpers.ts (4)
createTestChannels(28-49)toRaw(21-23)authenticateAppWithAllowances(54-74)createTestAppSession(98-151)
⏰ 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)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
integration/tests/transfer.test.ts (1)
152-162: Consider clarifying the comment for resize semantics.The test logic correctly sets up a scenario with non-zero on-chain channel funds by resizing with
allocate_amount = 0. The comment on line 158 could be slightly more explicit about the intent.Consider this minor wording improvement:
- BigInt(0) // resize only to the user, so that a channel contains on-chain funds + BigInt(0) // resize without allocating away, so channel retains on-chain funds
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integration/tests/transfer.test.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:449-465
Timestamp: 2025-11-05T10:19:06.155Z
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.960Z
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.
🧬 Code graph analysis (1)
integration/tests/transfer.test.ts (9)
integration/common/nitroliteClient.ts (1)
TestNitroliteClient(27-188)integration/common/databaseUtils.ts (1)
DatabaseUtils(4-117)integration/common/blockchainUtils.ts (1)
BlockchainUtils(22-222)integration/common/identity.ts (1)
Identity(7-33)integration/common/setup.ts (1)
CONFIG(4-44)integration/common/ws.ts (2)
TestWebSocket(9-139)getTransferPredicate(242-246)integration/common/auth.ts (1)
createAuthSessionWithClearnode(11-45)sdk/src/rpc/api.ts (1)
createTransferMessage(691-718)sdk/src/rpc/parse/parse.ts (2)
parseTransferResponse(146-146)parseAnyRPCResponse(12-40)
⏰ 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: Test (Integration) / Test Integration
- GitHub Check: Test (Clearnode) / Test Clearnode
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
integration/tests/transfer.test.ts (4)
40-51: LGTM: Excellent test isolation and lifecycle management.The per-test setup (WebSocket connection, authentication, ledger seeding, blockchain snapshot) and teardown (close connection, reset database state, revert snapshot) provide strong test isolation. This prevents state leakage between tests and ensures each test starts from a clean, known state.
61-91: LGTM: Comprehensive successful transfer test.The assertions thoroughly validate both the response structure and transaction details (accounts, asset, amount, type). This provides good coverage of the happy path.
93-140: LGTM: Proper duplicate detection test with fail-fast error handling.The test correctly reuses the same message (same request ID) to trigger duplicate detection. The explicit throw at line 139 ensures the test fails loudly if the duplicate transfer unexpectedly succeeds, preventing false positives.
180-189: Previous review concern has been addressed.The explicit
throwstatement at line 189 correctly implements the fail-fast pattern requested in the previous review. If the transfer unexpectedly succeeds (no error thrown), the test now fails immediately with a clear message, preventing false positives.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
clearnode/channel.go (1)
121-132: LGTM! Previous issues resolved.The function correctly addresses the critical Postgres compatibility issue by casting
raw_amountto NUMERIC and using COALESCE. The inclusion ofChannelStatusResizingin the status filter is appropriate, as discussed. The implementation handles edge cases well (no matching rows returns Count=0, Sum=0) and uses safe parameterized queries.Consider adding a godoc comment to document the function's purpose and behavior:
+// GetChannelAmountSumByWallet returns the count and sum of raw_amount for all channels +// belonging to the specified wallet that are in Open or Resizing status. func GetChannelAmountSumByWallet(tx *gorm.DB, senderWallet string) (ChannelAmountSum, error) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
clearnode/channel.go(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:449-465
Timestamp: 2025-11-05T10:19:06.155Z
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.960Z
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-05T10:19:06.155Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:449-465
Timestamp: 2025-11-05T10:19:06.155Z
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.go
📚 Learning: 2025-11-04T19:14:48.960Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:542-553
Timestamp: 2025-11-04T19:14:48.960Z
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.go
📚 Learning: 2025-11-04T10:39:28.297Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:425-427
Timestamp: 2025-11-04T10:39:28.297Z
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.go
📚 Learning: 2025-10-27T11:25:10.651Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 392
File: clearnode/session_key.go:36-49
Timestamp: 2025-10-27T11:25:10.651Z
Learning: For the erc7824/nitrolite repository, the team uses their own ticket tracking system and prefers not to have GitHub issues created for follow-up items or technical debt tracking.
Applied to files:
clearnode/channel.go
⏰ 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.go (1)
116-119: LGTM!The struct definition is clean and appropriate for holding aggregated query results. The field types and gorm column tags correctly match the query output.
| } from '@/testHelpers'; | ||
| import { CONFIG } from '@/setup'; | ||
|
|
||
| describe('App session v0.2', () => { |
There was a problem hiding this comment.
In v1 (or v0.6-7) we'll probably remove v0.2 from supported protocols and keep more secure v0.4, or even newer. Anyways, for now its good to have an additional test for that as this one
Summary by CodeRabbit
Bug Fixes
New Features
Tests