YNU-82/83: introduce app session deposits and withdrawals#376
YNU-82/83: introduce app session deposits and withdrawals#376
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 intent-aware AppSession state processing via an AppSessionUpdater pattern (operate, deposit, withdraw), per-participant allocation computation and AppSessionUpdate notifications; extends SDK parsing/types for AppSessionUpdate; adds integration test helpers and a unified integration teardown reset. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Service as AppSessionService
participant Quorum as QuorumVerifier
participant Ledger as Ledger/Accounts
participant Notifier
Client->>Service: SubmitAppState(ctx, sessionId, intent, allocations, version, sigs)
Service->>Quorum: verifyQuorum(session, rpcSigners, intent)
alt quorum ok
Note right of Service #bfe6d6: intent-specific processing
alt operate
Service->>Ledger: OperateUpdater.Update (validate zero-sum, apply deltas)
else deposit
Service->>Ledger: DepositUpdater.Update (validate deposits, depositor sigs, record tx)
else withdraw
Service->>Ledger: WithdrawUpdater.Update (validate withdrawals, apply credits)
end
Service->>Ledger: getParticipantAllocations(session)
Service->>Notifier: emit AppSessionUpdate (per-participant allocations)
Notifier-->>Client: AppSessionUpdate notifications
else quorum failed
Service-->>Client: intent-specific quorum error
end
Service-->>Client: SubmitAppState response (status/version)
sequenceDiagram
autonumber
actor Client
participant Service as AppSessionService
participant Ledger as Ledger/Accounts
participant Notifier
Client->>Service: CloseApplication(sessionId, sigs)
Service->>Ledger: finalize allocations, update balances
alt affected participants
Service->>Notifier: notify AppSessionUpdate (affected participants + allocations)
end
Service-->>Client: CloseApplication response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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 @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 the capability for participants to deposit funds into an application session, specifically for the 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to deposit funds into an active application session, a feature supported by the new deposit intent in v0.4 of the protocol. The changes are well-structured, with the core logic refactored out of SubmitAppState into new handler functions for operate and deposit intents. This improves code clarity and maintainability. A comprehensive suite of tests has been added to cover the new deposit functionality and its edge cases. My review includes a couple of suggestions to improve efficiency and error handling.
411efd9 to
d387c29
Compare
|
@philanton have addressed your comments |
nksazonov
left a comment
There was a problem hiding this comment.
Almost LGTM: some questions need answering
56846eb to
1f4360e
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
clearnode/app_session_service.go (4)
218-225: Consider using structured logging for error reporting.The error from
getParticipantAllocationsis logged usingfmt.Printf, which is inconsistent with structured logging patterns typically used in production services. While the error is now logged (addressing a previous concern), consider using the service's logger if available for better observability.If the service has access to a logger (similar to
WSNotifier), apply this pattern:participantAllocations, err := getParticipantAllocations(s.db, updatedAppSession, NewAccountID(params.AppSessionID)) if err != nil { - fmt.Printf("failed to get participant allocations for app session %s, notifications will not be sent: %v\n", updatedAppSession.SessionID, err) + s.logger.Error("failed to get participant allocations, app session notifications will not be sent", + "session_id", updatedAppSession.SessionID, + "error", err) } else {Note: This assumes the service has a logger field. If not, the current approach is acceptable for now.
379-418: Current implementation is clear but could be optimized.The operate intent handler debits the full current balance then credits the new allocation for each participant (lines 402-407). While this "reset and set" approach is straightforward and less error-prone, it performs two database operations per allocation instead of computing and applying the delta once.
The current approach prioritizes clarity over efficiency, which is reasonable for typical app session sizes. Consider optimizing if profiling shows this is a bottleneck.
420-477: Consider stricter validation for deposit intent.The deposit handler only processes allocations that increase (line 437), silently ignoring unchanged or decreased allocations. While this provides flexibility for mixed operations, it may allow unintended behavior.
Consider adding explicit validation to ensure all allocations either increase or remain unchanged:
currentAmount := currentAllocations[walletAddress][alloc.AssetSymbol] if alloc.Amount.GreaterThan(currentAmount) { depositAmount := alloc.Amount.Sub(currentAmount) noDeposits = false // ... deposit processing ... participants[walletAddress] = true + } else if alloc.Amount.LessThan(currentAmount) { + return nil, RPCErrorf("incorrect deposit request: allocation decrease not allowed for participant %s, asset %s", walletAddress, alloc.AssetSymbol) }This makes the intent validation more explicit and prevents confusion about expected behavior.
479-528: Consider stricter validation for withdrawal intent.Similar to the deposit handler, the withdrawal handler only processes allocations that decrease (line 500), silently ignoring unchanged or increased allocations. For consistency and clarity, consider adding explicit validation:
currentAmount := currentAllocations[walletAddress][alloc.AssetSymbol] if alloc.Amount.LessThan(currentAmount) { withdrawalAmount := currentAmount.Sub(alloc.Amount) noWithdrawals = false // ... withdrawal processing ... participants[walletAddress] = true + } else if alloc.Amount.GreaterThan(currentAmount) { + return nil, RPCErrorf("incorrect withdrawal request: allocation increase not allowed for participant %s, asset %s", walletAddress, alloc.AssetSymbol) }This makes the intent validation more explicit and symmetric with the deposit handler improvement suggested above.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
clearnode/app_session_service.go(7 hunks)clearnode/app_session_service_test.go(13 hunks)clearnode/notification.go(3 hunks)clearnode/pkg/rpc/api.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
clearnode/pkg/rpc/api.go (1)
clearnode/app_session.go (2)
AppSession(11-25)AppSession(27-29)
clearnode/app_session_service_test.go (6)
clearnode/ledger.go (3)
NewAccountID(37-43)GetWalletLedger(49-51)AccountID(35-35)clearnode/notification.go (3)
Notification(35-39)EventType(41-41)NewWSNotifier(17-22)clearnode/pkg/rpc/api.go (13)
Version(27-27)AppSession(554-579)ChannelStatusOpen(642-642)LedgerTransaction(774-793)Channel(608-635)ChannelStatusChallenged(646-646)VersionNitroRPCv0_2(31-31)AppAllocation(582-589)VersionNitroRPCv0_4(33-33)ChannelStatusClosed(644-644)AppSessionIntentDeposit(598-598)AppSessionIntentOperate(596-596)AppSessionIntentWithdraw(600-600)clearnode/app_session.go (2)
AppSession(11-25)AppSession(27-29)clearnode/rpc_router_private.go (3)
Balance(124-127)SubmitAppStateParams(42-48)AppAllocation(56-60)clearnode/ledger_transaction.go (4)
LedgerTransaction(24-32)LedgerTransaction(34-36)TransactionTypeAppDeposit(16-16)TransactionTypeAppWithdrawal(17-17)
clearnode/app_session_service.go (8)
clearnode/pkg/rpc/api.go (9)
AppSession(554-579)Version(27-27)VersionNitroRPCv0_4(33-33)AppSessionIntentDeposit(598-598)AppSessionIntentWithdraw(600-600)AppSessionIntentOperate(596-596)VersionNitroRPCv0_2(31-31)Asset(506-515)AppSessionIntent(592-592)clearnode/app_session.go (2)
AppSession(11-25)AppSession(27-29)clearnode/ledger.go (3)
NewAccountID(37-43)AccountID(35-35)GetWalletLedger(49-51)clearnode/rpc.go (1)
RPCErrorf(147-151)clearnode/rpc_router_private.go (3)
AppSessionResponse(62-75)SubmitAppStateParams(42-48)Balance(124-127)clearnode/notification.go (2)
NewBalanceNotification(55-62)NewAppSessionNotification(96-121)clearnode/wallet.go (1)
GetWalletBySigner(35-40)clearnode/ledger_transaction.go (3)
RecordLedgerTransaction(39-51)TransactionTypeAppDeposit(16-16)TransactionTypeAppWithdrawal(17-17)
clearnode/notification.go (2)
clearnode/pkg/rpc/api.go (2)
AppSession(554-579)Version(27-27)clearnode/rpc_router_private.go (1)
AppSessionResponse(62-75)
⏰ 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/pkg/rpc/api.go (2)
129-130: LGTM!The new event type constant follows the established naming pattern and uses the agreed-upon "asu" value for app session updates.
442-448: Design choice: map structure is appropriate for notification payloads.The
ParticipantAllocationsfield uses a nested map structure instead of[]AppAllocationfor efficient lookups by participant and asset. This differs from request parameters but is well-suited for notification payloads where clients need to quickly access their specific allocations without iterating through a slice.The trade-off is acceptable: request parameters benefit from slice consistency and validation, while notification payloads benefit from lookup efficiency.
clearnode/notification.go (2)
8-8: LGTM!The decimal import is necessary for the new notification function signature.
47-47: LGTM!The event type constant correctly uses "asu" as agreed upon in previous review discussions.
clearnode/app_session_service.go (5)
177-179: LGTM!The validation correctly rejects v0.4-specific parameters (intent and version) when using the v0.2 protocol, preventing clients from assuming these features are supported.
357-377: LGTM!The helper function efficiently aggregates participant allocations and filters out zero balances to optimize notification payloads.
530-536: LGTM!The validation helper cleanly verifies participant membership with a clear error message.
538-576: LGTM!The intent-aware error messages in the quorum verification provide clear, context-specific feedback to clients, improving debuggability.
234-325: LGTM!The closure logic correctly validates participants and selectively notifies only those receiving non-zero allocations, with a clear comment explaining the behavior.
clearnode/app_session_service_test.go (2)
17-91: LGTM!The test helpers significantly improve test readability and maintainability by:
- Encapsulating common setup patterns
- Providing clear, reusable building blocks
- Enabling flexible notification capture for verification
The refactoring successfully reduces boilerplate while maintaining clarity.
455-1079: Comprehensive test coverage for deposit and withdrawal flows.The test suites thoroughly validate the new NitroRPC v0.4 deposit and withdrawal functionality:
- Happy paths with single and multiple participants/tokens
- Error cases: insufficient balance, quorum failures, signature validation
- Edge cases: zero deltas, protocol version mismatches, intent validation
- Proper verification of balances, transactions, and notifications
The refactored test structure with helpers significantly improves readability and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
integration/common/databaseUtils.ts (1)
47-52: LGTM! Consider clarifying the placeholder comment.The new
resetClearnodeState()method provides a good abstraction for test cleanup and future-proofs for cache clearing. However, the placeholder comment could be more explicit about whether this is a TODO or just documentation of future capability.Consider rewording to:
// TODO: Add cache-clear API call once Clearnode implements in-memory cachingThis makes it clearer that it's a planned enhancement rather than optional future work.
integration/common/testAppSessionHelpers.ts (3)
53-86: Inconsistent version parameter naming between v02 and v04 helpers.The
submitAppStateUpdate_v02function uses anexpectedVersionparameter (line 24) and validates against it (line 45), whilesubmitAppStateUpdate_v04uses aversionparameter (line 58) and validates against it (line 83). This naming inconsistency could be confusing.For clarity and consistency, consider either:
- Using
expectedVersionin both functions to make it clear that we're validating expectations, or- Using
versionin both if the version is known upfront in the v04 protocol flowApply this diff if you prefer consistent naming with
expectedVersion:export async function submitAppStateUpdate_v04( participantAppWS: TestWebSocket, participantAppIdentity: Identity, appSessionId: string, intent: RPCAppStateIntent, - version: number, + expectedVersion: number, allocations: RPCAppSessionAllocation[], sessionData: object ) { const submitAppStateMsg = await createSubmitAppStateMessage<RPCProtocolVersion.NitroRPC_0_4>( participantAppIdentity.messageSigner, { app_session_id: appSessionId as Hex, intent, - version, + version: expectedVersion, allocations, session_data: JSON.stringify(sessionData), } ); const submitAppStateResponse = await participantAppWS.sendAndWaitForResponse( submitAppStateMsg, getSubmitAppStatePredicate(), 1000 ); const submitAppStateParsedResponse = parseSubmitAppStateResponse(submitAppStateResponse); expect(submitAppStateParsedResponse).toBeDefined(); expect(submitAppStateParsedResponse.params.appSessionId).toBe(appSessionId); expect(submitAppStateParsedResponse.params.status).toBe(RPCChannelStatus.Open); - expect(submitAppStateParsedResponse.params.version).toBe(version); + expect(submitAppStateParsedResponse.params.version).toBe(expectedVersion); return submitAppStateParsedResponse; }
105-114: Remove redundant assertion.Line 111 checks
expect(closeAppSessionResponse).toBeDefined(), but the parsed response is also validated on line 114 withexpect(closeAppSessionParsedResponse).toBeDefined(). If parsing succeeds, the raw response was necessarily defined, making the first check redundant.Apply this diff to remove the redundant check:
const closeAppSessionResponse = await participantAppWS.sendAndWaitForResponse( closeAppSessionMsg, getCloseAppSessionPredicate(), 1000 ); - expect(closeAppSessionResponse).toBeDefined(); - const closeAppSessionParsedResponse = parseCloseAppSessionResponse(closeAppSessionResponse); expect(closeAppSessionParsedResponse).toBeDefined();
18-48: Consider making timeouts configurable.All three helper functions use a hardcoded 1000ms timeout when waiting for responses. In some test environments or under load, this might be too short and cause flaky tests.
Consider adding an optional
timeoutparameter with a default value:export async function submitAppStateUpdate_v02( participantAppWS: TestWebSocket, participantAppIdentity: Identity, appSessionId: string, allocations: RPCAppSessionAllocation[], sessionData: object, - expectedVersion: number + expectedVersion: number, + timeout: number = 1000 ) { // ... message creation ... const submitAppStateResponse = await participantAppWS.sendAndWaitForResponse( submitAppStateMsg, getSubmitAppStatePredicate(), - 1000 + timeout ); // ... rest of function ... }Apply similar changes to
submitAppStateUpdate_v04andcloseAppSessionWithState.Also applies to: 53-86, 91-120
integration/common/testHelpers.ts (1)
60-60: Consider extracting hardcoded test address to a constant.Line 60 contains a hardcoded random address
'0x9965507D1a55bcC2695C58ba16FB37d819B0A4dc'with the comment "random address, no use for now". While acceptable for a test helper, this could be extracted toCONFIGinsetup.tsfor better maintainability and to make it clear this is a placeholder value.For example, in
integration/common/setup.ts:export const CONFIG = { // ... existing config ... ADDRESSES: { CUSTODY_ADDRESS: '0x8658501c98C3738026c4e5c361c6C3fa95DfB255' as Address, DUMMY_ADJUDICATOR_ADDRESS: '0xcbbc03a873c11beeFA8D99477E830be48d8Ae6D7' as Address, GUEST_ADDRESS: '0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266' as Address, USDC_TOKEN_ADDRESS: '0xbD24c53072b9693A35642412227043Ffa5fac382' as Address, + TEST_APP_ADDRESS: '0x9965507D1a55bcC2695C58ba16FB37d819B0A4dc' as Address, // Placeholder for test app sessions }, // ... rest of config ... };Then update line 60 to use
CONFIG.ADDRESSES.TEST_APP_ADDRESS.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
integration/common/databaseUtils.ts(1 hunks)integration/common/testAppSessionHelpers.ts(1 hunks)integration/common/testHelpers.ts(1 hunks)integration/common/testSetup.ts(1 hunks)integration/tests/challenge_channel.test.ts(1 hunks)integration/tests/clearnode_auth.test.ts(1 hunks)integration/tests/close_channel.test.ts(1 hunks)integration/tests/create_channel.test.ts(1 hunks)integration/tests/get_user_tag.test.ts(1 hunks)integration/tests/ledger_transactions.test.ts(1 hunks)integration/tests/lifecycle_nitrorpc_v02.test.ts(8 hunks)integration/tests/lifecycle_nitrorpc_v04.test.ts(9 hunks)integration/tests/resize_channel.test.ts(1 hunks)sdk/src/rpc/parse/app.ts(2 hunks)sdk/src/rpc/parse/parse.ts(1 hunks)sdk/src/rpc/types/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
integration/common/testSetup.ts (7)
integration/common/identity.ts (1)
Identity(7-28)integration/common/ws.ts (2)
TestWebSocket(9-139)getGetAppSessionsPredicate(254-258)integration/common/nitroliteClient.ts (1)
TestNitroliteClient(23-130)integration/common/setup.ts (1)
CONFIG(4-41)integration/common/auth.ts (1)
createAuthSessionWithClearnode(11-47)sdk/src/rpc/api.ts (1)
createGetAppSessionsMessage(350-370)sdk/src/rpc/parse/parse.ts (1)
parseGetAppSessionsResponse(104-104)
integration/tests/lifecycle_nitrorpc_v02.test.ts (5)
integration/common/ws.ts (3)
TestWebSocket(9-139)getCloseChannelPredicate(199-203)getResizeChannelPredicate(205-209)integration/common/identity.ts (1)
Identity(7-28)integration/common/testSetup.ts (2)
setupTestIdentitiesAndConnections(27-60)fetchAndParseAppSessions(66-93)integration/common/testHelpers.ts (5)
createTestChannels(25-44)toRaw(18-20)authenticateAppWithAllowances(49-68)getLedgerBalances(130-145)createTestAppSession(73-123)integration/common/testAppSessionHelpers.ts (2)
submitAppStateUpdate_v02(18-48)closeAppSessionWithState(91-120)
integration/tests/lifecycle_nitrorpc_v04.test.ts (10)
integration/common/ws.ts (3)
TestWebSocket(9-139)getCloseChannelPredicate(199-203)getResizeChannelPredicate(205-209)integration/common/identity.ts (1)
Identity(7-28)integration/common/nitroliteClient.ts (1)
TestNitroliteClient(23-130)integration/common/blockchainUtils.ts (1)
BlockchainUtils(21-187)integration/common/databaseUtils.ts (1)
DatabaseUtils(4-81)integration/common/testSetup.ts (2)
setupTestIdentitiesAndConnections(27-60)fetchAndParseAppSessions(66-93)integration/common/testHelpers.ts (5)
createTestChannels(25-44)toRaw(18-20)authenticateAppWithAllowances(49-68)getLedgerBalances(130-145)createTestAppSession(73-123)integration/common/testAppSessionHelpers.ts (2)
submitAppStateUpdate_v04(53-86)closeAppSessionWithState(91-120)sdk/src/rpc/api.ts (2)
createCloseChannelMessage(526-543)createResizeChannelMessage(554-569)sdk/src/rpc/parse/parse.ts (2)
parseCloseChannelResponse(113-113)parseResizeChannelResponse(110-110)
sdk/src/rpc/parse/app.ts (1)
sdk/src/rpc/parse/common.ts (6)
hexSchema(20-25)statusEnum(46-46)addressSchema(27-32)protocolVersionEnum(48-48)dateSchema(37-37)ParamsParser(7-9)
integration/common/testAppSessionHelpers.ts (5)
integration/common/ws.ts (3)
TestWebSocket(9-139)getSubmitAppStatePredicate(248-252)getCloseAppSessionPredicate(260-264)integration/common/identity.ts (1)
Identity(7-28)sdk/src/rpc/types/common.ts (1)
RPCAppSessionAllocation(247-254)sdk/src/rpc/api.ts (2)
createSubmitAppStateMessage(425-440)createCloseAppSessionMessage(452-467)sdk/src/rpc/parse/parse.ts (2)
parseSubmitAppStateResponse(94-94)parseCloseAppSessionResponse(97-97)
integration/common/testHelpers.ts (8)
integration/common/nitroliteClient.ts (1)
TestNitroliteClient(23-130)integration/common/ws.ts (3)
TestWebSocket(9-139)getCreateAppSessionPredicate(242-246)getGetLedgerBalancesPredicate(218-222)integration/common/setup.ts (1)
CONFIG(4-41)integration/common/identity.ts (1)
Identity(7-28)integration/common/auth.ts (1)
createAuthSessionWithClearnode(11-47)sdk/src/rpc/types/common.ts (2)
RPCAppDefinition(34-47)RPCBalance(99-104)sdk/src/rpc/api.ts (2)
createAppSessionMessage(381-396)createGetLedgerBalancesMessage(220-236)sdk/src/rpc/parse/parse.ts (2)
parseCreateAppSessionResponse(90-91)parseGetLedgerBalancesResponse(75-76)
⏰ 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 (15)
integration/tests/resize_channel.test.ts (1)
35-35: LGTM!The migration to
resetClearnodeState()aligns with the centralized teardown pattern introduced in this PR. This improves maintainability by providing a single point for test cleanup logic.integration/tests/create_channel.test.ts (1)
42-42: LGTM!Consistent with the teardown refactor across the test suite. The change maintains test isolation while centralizing cleanup logic.
integration/tests/get_user_tag.test.ts (1)
34-34: LGTM!The migration to
resetClearnodeState()maintains consistency with other test files in this PR.integration/tests/clearnode_auth.test.ts (1)
22-22: LGTM!Consistent teardown refactor aligning with the centralized state reset pattern.
integration/tests/ledger_transactions.test.ts (1)
39-39: LGTM!The teardown change aligns with the standardized cleanup approach introduced across integration tests.
integration/tests/close_channel.test.ts (1)
35-35: LGTM!Final test file migration to the centralized
resetClearnodeState()teardown pattern. This completes the consistent refactor across the integration test suite.sdk/src/rpc/types/index.ts (1)
212-212: LGTM!The new
AppSessionUpdateenum member follows the established abbreviation pattern for notification methods (e.g.,bufor BalanceUpdate,cufor ChannelUpdate,trfor TransferNotification). The abbreviated valueasuis consistent with the existing convention.integration/tests/lifecycle_nitrorpc_v02.test.ts (4)
19-33: Clean test refactoring with improved modularity.The refactored imports consolidate helper utilities from separate modules (
@/testSetup,@/testHelpers,@/testAppSessionHelpers), making the test structure clearer and more maintainable.
89-98: Well-structured test setup using shared utilities.The use of
setupTestIdentitiesAndConnections()centralizes identity and WebSocket initialization, improving consistency across tests and reducing boilerplate.
104-107: Proper resource cleanup in teardown.The
afterAllhook correctly closes all WebSocket connections and resets the test environment state, preventing resource leaks and test interference.
183-183: Verified: RPCChannelStatus.Closed equals 'closed'. The enum defines Closed = 'closed', so the assertion expect(appSession.status).toBe('closed') is correct.integration/common/testHelpers.ts (4)
18-20: LGTM! Clean unit conversion helper.The
toRawfunction correctly converts decimal amounts to raw units by multiplying by 10^decimals, with a sensible default of 6 decimals for USDC.
25-44: Efficient channel creation helper.The
createTestChannelsfunction cleanly handles multiple channel creations in a loop, collecting and returning channel IDs for use in tests.
81-88: Verify test-specific app definition parameters.The
createTestAppSessionhelper uses specific test values:
weights: [100, 0]- Alice has 100% weight, Bob has 0%quorum: 100- Requires 100% consensus (only Alice's signature needed)challenge: 0- No challenge periodThese values are suitable for simple test scenarios where Alice controls the session, but ensure this matches the intended test behavior. If tests need to verify multi-party consensus or challenge mechanisms, these values would need adjustment.
130-145: Clean ledger balance retrieval helper.The
getLedgerBalancesfunction provides a convenient way to fetch and validate ledger balances, with proper assertions to ensure the response is well-formed.
407fbd6 to
ed19456
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
clearnode/notification.go (1)
96-118: Populate app session timestamps in notifications
AppSessionResponsein the RPC schema carriescreated_at/updated_at, but the notification payload leaves them empty. Clients will now receive blank timestamps, breaking the documented response contract. Please mapappSession.CreatedAtandappSession.UpdatedAtto RFC3339 strings before returning the notification.response := AppSessionResponse{ AppSessionID: appSession.SessionID, Status: string(appSession.Status), ParticipantWallets: appSession.ParticipantWallets, SessionData: appSession.SessionData, Protocol: string(appSession.Protocol), Challenge: appSession.Challenge, Weights: appSession.Weights, Quorum: appSession.Quorum, Version: appSession.Version, Nonce: appSession.Nonce, + CreatedAt: appSession.CreatedAt.Format(time.RFC3339), + UpdatedAt: appSession.UpdatedAt.Format(time.RFC3339), }Based on relevant code snippet
clearnode/app_session_service.go (2)
430-474: Reject decreases inside deposit intentsWhen a deposit request mixes an increase for one participant with a decrease for another, the loop simply skips the decreased allocation. The call completes successfully, but the session ledger retains the old balance for the decreased participant, so the persisted state diverges from the submitted allocations. Add an explicit guard so any decrease triggers a failure.
- currentAmount := currentAllocations[walletAddress][alloc.AssetSymbol] - if alloc.Amount.GreaterThan(currentAmount) { + currentAmount := currentAllocations[walletAddress][alloc.AssetSymbol] + if alloc.Amount.LessThan(currentAmount) { + return nil, RPCErrorf("incorrect deposit request: allocation decreased for %s %s", walletAddress, alloc.AssetSymbol) + } + if alloc.Amount.GreaterThan(currentAmount) { depositAmount := alloc.Amount.Sub(currentAmount) noDeposits = false
499-525: Reject increases inside withdrawal intentsA withdrawal request that reduces one participant but increases another currently succeeds—the increased allocation path is skipped so the ledger keeps the older value, again diverging from the requested final state. Ensure any increase in a withdrawal intent is rejected.
- currentAmount := currentAllocations[walletAddress][alloc.AssetSymbol] - if alloc.Amount.LessThan(currentAmount) { + currentAmount := currentAllocations[walletAddress][alloc.AssetSymbol] + if alloc.Amount.GreaterThan(currentAmount) { + return nil, RPCErrorf("incorrect withdrawal request: allocation increased for %s %s", walletAddress, alloc.AssetSymbol) + } + if alloc.Amount.LessThan(currentAmount) { withdrawalAmount := currentAmount.Sub(alloc.Amount) noWithdrawals = false
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
clearnode/app_session_service.go(7 hunks)clearnode/app_session_service_test.go(9 hunks)clearnode/channel_service.go(2 hunks)clearnode/notification.go(2 hunks)clearnode/pkg/rpc/api.go(2 hunks)clearnode/rpc_router_private.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- clearnode/rpc_router_private.go
- clearnode/pkg/rpc/api.go
🧰 Additional context used
🧬 Code graph analysis (4)
clearnode/app_session_service_test.go (8)
clearnode/ledger.go (3)
NewAccountID(37-43)GetWalletLedger(49-51)AccountID(35-35)clearnode/notification.go (3)
Notification(34-38)EventType(40-40)NewWSNotifier(16-21)clearnode/pkg/rpc/api.go (13)
Version(27-27)AppSession(554-579)ChannelStatusOpen(642-642)LedgerTransaction(774-793)Channel(608-635)ChannelStatusChallenged(646-646)VersionNitroRPCv0_2(31-31)AppAllocation(582-589)VersionNitroRPCv0_4(33-33)AppSessionIntentOperate(596-596)AppSessionIntentDeposit(598-598)AppSessionIntentWithdraw(600-600)ChannelStatusClosed(644-644)clearnode/app_session.go (2)
AppSession(11-25)AppSession(27-29)clearnode/channel.go (5)
ChannelStatusOpen(16-16)Channel(22-40)Channel(43-45)ChannelStatusChallenged(18-18)ChannelStatusClosed(17-17)clearnode/rpc.go (1)
RPCDataParams(42-42)clearnode/rpc_router_private.go (3)
Balance(124-127)SubmitAppStateParams(42-48)AppAllocation(56-60)clearnode/ledger_transaction.go (4)
LedgerTransaction(24-32)LedgerTransaction(34-36)TransactionTypeAppDeposit(16-16)TransactionTypeAppWithdrawal(17-17)
clearnode/app_session_service.go (8)
clearnode/rpc.go (1)
RPCErrorf(147-151)clearnode/pkg/rpc/api.go (10)
AppSession(554-579)Version(27-27)VersionNitroRPCv0_4(33-33)AppSessionIntentDeposit(598-598)AppSessionIntentWithdraw(600-600)AppSessionIntentOperate(596-596)VersionNitroRPCv0_2(31-31)Asset(506-515)AppSessionIntent(592-592)AppAllocation(582-589)clearnode/app_session.go (2)
AppSession(11-25)AppSession(27-29)clearnode/ledger.go (3)
NewAccountID(37-43)AccountID(35-35)GetWalletLedger(49-51)clearnode/rpc_router_private.go (4)
AppSessionResponse(62-75)SubmitAppStateParams(42-48)Balance(124-127)AppAllocation(56-60)clearnode/notification.go (2)
NewBalanceNotification(54-61)NewAppSessionNotification(95-120)clearnode/wallet.go (1)
GetWalletBySigner(35-40)clearnode/ledger_transaction.go (3)
RecordLedgerTransaction(39-51)TransactionTypeAppDeposit(16-16)TransactionTypeAppWithdrawal(17-17)
clearnode/notification.go (2)
clearnode/pkg/rpc/api.go (3)
AppSession(554-579)AppAllocation(582-589)Version(27-27)clearnode/rpc_router_private.go (2)
AppAllocation(56-60)AppSessionResponse(62-75)
clearnode/channel_service.go (2)
clearnode/pkg/rpc/api.go (1)
ChannelOperationResponse(650-664)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). (1)
- GitHub Check: Test (Integration) / Test Integration
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
clearnode/app_session_service.go (1)
35-102: Consider explicit participant validation for clarity.The deposit logic validates participants only when their allocation increases (line 57-59). While the balance check at lines 71-78 implicitly validates participants (non-participants have zero balance), explicit validation for all depositing participants would provide clearer error messages and be more defensive.
However, based on past review discussions, this approach is acceptable to avoid checking participants with unchanged balances.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
clearnode/app_session_service.go(13 hunks)clearnode/app_session_service_test.go(9 hunks)clearnode/rpc_router_private.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
clearnode/rpc_router_private.go (3)
clearnode/rpc.go (1)
RPCErrorf(147-151)clearnode/ledger.go (5)
ErrGetAccountBalance(13-13)ErrDebitSourceAccount(14-14)NewAccountID(43-49)GetWalletLedger(55-57)ErrCreditDestinationAccount(15-15)clearnode/app_session_service.go (1)
AppSessionService(214-217)
clearnode/app_session_service_test.go (7)
clearnode/ledger.go (3)
NewAccountID(43-49)GetWalletLedger(55-57)AccountID(41-41)clearnode/notification.go (3)
Notification(34-38)EventType(40-40)NewWSNotifier(16-21)clearnode/pkg/rpc/api.go (13)
Version(27-27)AppSession(554-579)ChannelStatusOpen(642-642)LedgerTransaction(774-793)Channel(608-635)ChannelStatusChallenged(646-646)VersionNitroRPCv0_2(31-31)AppAllocation(582-589)VersionNitroRPCv0_4(33-33)AppSessionIntentOperate(596-596)AppSessionIntentDeposit(598-598)AppSessionIntentWithdraw(600-600)ChannelStatusClosed(644-644)clearnode/app_session.go (2)
AppSession(11-25)AppSession(27-29)clearnode/app_session_service.go (3)
AppSessionService(214-217)NewAppSessionService(220-222)ErrNegativeAllocation(17-17)clearnode/rpc_router_private.go (3)
Balance(124-127)SubmitAppStateParams(42-48)AppAllocation(56-60)clearnode/ledger_transaction.go (4)
LedgerTransaction(24-32)LedgerTransaction(34-36)TransactionTypeAppDeposit(16-16)TransactionTypeAppWithdrawal(17-17)
clearnode/app_session_service.go (9)
clearnode/pkg/rpc/api.go (11)
AppSession(554-579)Version(27-27)VersionNitroRPCv0_4(33-33)AppSessionIntentDeposit(598-598)AppSessionIntentWithdraw(600-600)AppSessionIntentOperate(596-596)VersionNitroRPCv0_2(31-31)Asset(506-515)AppSessionIntent(592-592)ChannelStatusOpen(642-642)AppAllocation(582-589)clearnode/app_session.go (2)
AppSession(11-25)AppSession(27-29)clearnode/rpc_router_private.go (4)
SubmitAppStateParams(42-48)Balance(124-127)AppSessionResponse(62-75)AppAllocation(56-60)clearnode/ledger.go (6)
AccountID(41-41)NewAccountID(43-49)GetWalletLedger(55-57)ErrGetAccountBalance(13-13)ErrDebitSourceAccount(14-14)ErrCreditDestinationAccount(15-15)clearnode/rpc.go (1)
RPCErrorf(147-151)clearnode/wallet.go (1)
GetWalletBySigner(35-40)clearnode/ledger_transaction.go (3)
RecordLedgerTransaction(39-51)TransactionTypeAppDeposit(16-16)TransactionTypeAppWithdrawal(17-17)clearnode/notification.go (2)
NewBalanceNotification(54-61)NewAppSessionNotification(95-122)clearnode/log.go (1)
LoggerFromContext(95-100)
⏰ 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 (16)
clearnode/app_session_service.go (7)
16-29: LGTM! Clean updater pattern.The error constants, UpdateResult type, and AppSessionUpdater interface provide a clean abstraction for intent-specific handling. The context parameter in Update enables future observability and cancellation support.
107-159: Withdrawal validation follows deposit pattern.Like DepositUpdater, this validates participants implicitly through balance checks (line 128). Non-participants would have zero balance, causing withdrawal attempts to fail at line 149-151. This is acceptable per past review discussions.
164-211: LGTM! Operate intent is properly strict.The operate intent correctly validates all participants (lines 182-184) regardless of balance changes, ensuring allocations only reference session members. The optimization at lines 194-198 (update only non-zero diffs) is good.
329-418: Well-structured updater integration.The refactored SubmitAppState cleanly routes through updater implementations based on protocol and intent. The error handling at lines 403-407 correctly uses LoggerFromContext instead of fmt.Printf, addressing past review feedback.
The notification strategy (balance updates for affected participants at lines 398-401, app session updates for all participants at lines 408-411) is sound.
224-327: LGTM! Consistent refactoring.Both CreateApplication and CloseApplication are properly refactored to:
- Use error constants consistently
- Track only participants with balance changes for notifications
- Validate participants via validateAppParticipant helper
- Use participantsWithUpdatedBalance pattern
The weight validation at lines 237-241 prevents negative weights, which is good.
Also applies to: 420-511
543-625: LGTM! Clean helper functions.The helper functions are well-designed:
getParticipantAllocationsefficiently gathers per-participant balancesvalidateAppParticipantprovides explicit participant validationverifyQuorumreturns participantWeights for validation and provides intent-specific error messagesprepareAppAllocationsconverts to the notification formatThe optimization at lines 556-558 (only include non-zero balances) is good.
329-329: Context parameter is well-positioned.The context parameter is correctly threaded through SubmitAppState (line 329) to all updater implementations (lines 35, 108, 165). While currently used primarily for logging, this enables future enhancements like request cancellation and distributed tracing.
Also applies to: 35-35, 108-108, 165-165
clearnode/rpc_router_private.go (2)
298-298: LGTM! Consistent error constant usage.The changes properly use the error constants
ErrGetAccountBalance,ErrDebitSourceAccount, andErrCreditDestinationAccountfrom clearnode/ledger.go for consistent error handling across the codebase.Also applies to: 304-304, 311-311
415-415: LGTM! Context passing is correct.The context is correctly passed from the RPC handler to
AppSessionService.SubmitAppState, enabling the service to access logging and support future cancellation.clearnode/app_session_service_test.go (7)
18-31: LGTM! Clean test fixtures.The module-level test fixtures (signers, addresses, account IDs) provide a consistent foundation for all tests.
38-92: LGTM! Well-structured test helpers.The test helpers are clean and reusable:
setupWallets: Seeds signer mappings and wallet balancescreateTestAppSession: Creates test session instancescreateTestAppSessionService: Wires service with optional notification capturesetupAppSessionBalances: Seeds per-session balancesrpcSigners: Builds signer mapsThese helpers significantly improve test readability and maintainability.
94-217: LGTM! Comprehensive CreateApplication tests.The tests cover key scenarios:
- Successful creation with multiple participants
- Insufficient funds error
- Negative allocation error (validating ErrNegativeAllocation constant usage)
- Challenged channel prevention
The tests properly use the test helpers and make appropriate assertions.
219-389: LGTM! Thorough operate intent tests.The tests comprehensively cover:
- Both NitroRPC v0.2 and v0.4 protocols
- Version validation for v0.4
- Negative allocation handling
- Non-zero delta validation
- Unsupported intent handling
The test names clearly indicate protocol version and scenario.
391-782: LGTM! Extensive deposit intent tests.The deposit tests provide excellent coverage:
- Basic and complex multi-participant/multi-asset scenarios
- Quorum and signature validation (including depositor signature requirement)
- Balance and allocation validation
- Protocol version restrictions
- Edge cases (zero delta, decreased allocations)
The tests properly verify ledger transactions and notifications at lines 440-444.
784-1003: LGTM! Thorough withdrawal intent tests.The withdrawal tests mirror deposit test coverage:
- Basic and multi-participant/multi-asset scenarios
- Allocation decrease validation (no increases allowed)
- Protocol version restrictions
- Quorum validation
- Ledger transaction verification at lines 833-837
The tests properly validate the inverse of deposit logic (withdrawals must decrease balances).
1006-1117: LGTM! Solid CloseApplication tests.The close tests cover:
- Successful closure with fund redistribution
- Zero allocation edge case (lines 1052-1090)
- Negative allocation error handling
The tests properly verify session status changes to closed and ledger transaction recording.
|
@nksazonov integration tests look good to me. Left a few comments that may help to add more clarity in some parts |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
clearnode/app_session_service.go (2)
21-25: Consider usingmap[string]struct{}for set semantics.
ParticipantsAffectedusesmap[string]boolto represent a set of participants. The more idiomatic Go pattern for sets ismap[string]struct{}, which is slightly more memory-efficient sincestruct{}has zero size.Apply this diff if you prefer the idiomatic approach:
type UpdateResult struct { - ParticipantsAffected map[string]bool + ParticipantsAffected map[string]struct{} UpdatedAppSession AppSession }Then update usages like line 120, 208, 389, 566:
-participantsWithUpdatedBalance[walletAddress] = true +participantsWithUpdatedBalance[walletAddress] = struct{}{}And line 304:
-ParticipantsAffected: make(map[string]bool), +ParticipantsAffected: make(map[string]struct{}),
42-54: Consider extracting common constructor logic.The three
New*Updaterconstructors (lines 42-54, 147-159, 235-247) have nearly identical implementations: they all callverifyQuorumwith the intent parameter and construct the updater struct. This duplication could be reduced with a helper function, though the explicit constructors do provide clarity.Example refactor:
// Helper function to reduce duplication func verifyQuorumAndBuildWeights(ctx context.Context, tx *gorm.DB, params *SubmitAppStateParams, rpcSigners map[string]struct{}) (AppSession, map[string]int64, error) { return verifyQuorum(tx, params.AppSessionID, rpcSigners, params.Intent) } // Then each constructor becomes simpler: func NewDepositUpdater(ctx context.Context, tx *gorm.DB, params *SubmitAppStateParams, rpcSigners map[string]struct{}) (*DepositUpdater, error) { appSession, participantWeights, err := verifyQuorumAndBuildWeights(ctx, tx, params, rpcSigners) if err != nil { return nil, err } return &DepositUpdater{ appSession: appSession, params: params, participantWeights: participantWeights, rpcSigners: rpcSigners, }, nil }Also applies to: 147-159, 235-247
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
clearnode/app_session_service.go(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
clearnode/app_session_service.go (9)
clearnode/pkg/rpc/api.go (11)
AppSession(554-579)Version(27-27)AppSessionIntentDeposit(598-598)AppSessionIntentWithdraw(600-600)AppSessionIntentOperate(596-596)VersionNitroRPCv0_4(33-33)VersionNitroRPCv0_2(31-31)Asset(506-515)AppSessionIntent(592-592)ChannelStatusOpen(642-642)AppAllocation(582-589)clearnode/app_session.go (2)
AppSession(11-25)AppSession(27-29)clearnode/rpc_router_private.go (4)
SubmitAppStateParams(42-48)Balance(124-127)AppSessionResponse(62-75)AppAllocation(56-60)clearnode/ledger.go (6)
NewAccountID(43-49)GetWalletLedger(55-57)ErrGetAccountBalance(13-13)ErrDebitSourceAccount(14-14)ErrCreditDestinationAccount(15-15)AccountID(41-41)clearnode/rpc.go (1)
RPCErrorf(147-151)clearnode/wallet.go (1)
GetWalletBySigner(35-40)clearnode/ledger_transaction.go (3)
RecordLedgerTransaction(39-51)TransactionTypeAppDeposit(16-16)TransactionTypeAppWithdrawal(17-17)clearnode/notification.go (2)
NewBalanceNotification(54-61)NewAppSessionNotification(95-122)clearnode/log.go (1)
LoggerFromContext(95-100)
⏰ 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/app_session_service.go (5)
61-136: LGTM! Deposit logic is sound.The deposit handling correctly:
- Validates participant membership before processing
- Requires depositor signatures
- Checks for sufficient unified balances
- Records ledger transactions properly
- Rejects attempts to decrease allocations during deposits
- Ensures at least one deposit occurs (non-zero delta)
166-224: LGTM! Withdrawal logic is correct.The withdrawal handling properly:
- Checks for negative allocations upfront
- Resolves wallet addresses correctly
- Processes withdrawals with appropriate ledger operations
- Rejects attempts to increase allocations during withdrawals
- Ensures at least one withdrawal occurs (non-zero delta)
Note: Unlike deposit and operate intents, this doesn't explicitly call
validateAppParticipant. However, the balance check provides implicit validation since non-participants have zero allocations incurrentAllocations, making invalid operations fail safely at line 209-211.
254-307: LGTM! Operate logic correctly enforces zero-sum constraint.The operate intent handling properly:
- Validates all participants are session members
- Updates session balances efficiently using diffs
- Enforces zero-sum constraint on allocations (no net deposits or withdrawals)
- Returns empty ParticipantsAffected since operate doesn't change unified balances (only reallocates within the session)
This correctly means balance notifications won't be sent for operate intents, only app session update notifications (lines 497-500).
425-507: LGTM! Intent routing and notification flow are well-structured.The
SubmitAppStatemethod correctly:
- Routes to appropriate updater based on intent (deposit, withdraw, operate)
- Defaults empty intent to operate for backward compatibility
- Validates protocol-specific requirements (v0.4 requires version, v0.2 forbids intent/version)
- Executes the update transactionally
- Sends balance notifications only to affected participants
- Sends app session notifications to all participants with current allocations
- Logs but doesn't fail on notification preparation errors (as per past discussion)
632-714: LGTM! Helper functions are well-implemented.The helper functions correctly:
getParticipantAllocations: Efficiently fetches per-participant balances for all session participantsvalidateAppParticipant: Validates participant membership using the weights mapverifyQuorum: Validates signatures meet quorum threshold and returns intent-specific error messages for better UXprepareAppAllocations: Converts internal allocation map to notification-ready slice, filtering out zero amounts
There was a problem hiding this comment.
Also, I think, if we have for each submit_app_state call at least one shared thing - the state update itself, - maybe it would be good to have it inside the handler instead, and for the interface just to have something like hooks: Validate, Verify, PostUpdate etc.
There was a problem hiding this comment.
Btw, all of this doesn't need to be done in this story as you know, and we can just create a new one for refactor, where we will plan properly the better look
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
integration/tests/nitrorpc_v04.test.ts (2)
67-89: Per-test EVM snapshot isolation looks correctmakeSnapshot in beforeEach + reset in afterEach fixes snapshot-id invalidation and prevents state bleed between tests. LGTM.
268-289: Bob-signed traffic correctly uses Bob’s WebSocketThe deposit/withdraw quorum tests now route Bob-signed requests through bobWS. This will hold once socket-level auth is enforced. LGTM.
To ensure there are no remaining mismatches elsewhere, run:
#!/bin/bash # Find submitAppStateUpdate_v04 calls signed by bobAppIdentity and show the WS arg. rg -nP -C2 'submitAppStateUpdate_v04\(\s*([a-zA-Z0-9_]+)\s*,\s*bobAppIdentity' integration | sed -n '1,200p' # Also check fetchAndParseAppSessions with bobAppIdentity rg -nP -C2 'fetchAndParseAppSessions\(\s*([a-zA-Z0-9_]+)\s*,\s*bobAppIdentity' integration | sed -n '1,200p'Also applies to: 382-389
🧹 Nitpick comments (7)
integration/tests/nitrorpc_v04.test.ts (3)
45-65: Type allocations explicitly to avoid implicit any and tighten assertionsDeclare START_ALLOCATIONS with a concrete type (and prefer const if you never reassign). It improves inference for all structuredClone uses.
- let START_ALLOCATIONS; + let START_ALLOCATIONS: Array<{ + participant: string; + asset: 'USDC'; + amount: string; + }>;
99-117: Prefer rejects.toThrow over try/catch for error assertionsThis shortens tests and removes “Expected error was not thrown” boilerplate.
- try { - await submitAppStateUpdate_v04(aliceAppWS, aliceAppIdentity, appSessionId, RPCAppStateIntent.Operate, currentVersion + 42, allocations, { state: 'blah'}); - } catch (e) { - expect((e as Error).message).toMatch( - `RPC Error: incorrect app state: incorrect version: expected ${ - currentVersion + 1 - }, got ${currentVersion + 42}` - ); - - const { sessionData } = await fetchAndParseAppSessions(aliceAppWS, aliceAppIdentity, appSessionId); - expect(sessionData).toEqual(START_SESSION_DATA); - return; - } - throw new Error('Expected error was not thrown'); + await expect( + submitAppStateUpdate_v04( + aliceAppWS, + aliceAppIdentity, + appSessionId, + RPCAppStateIntent.Operate, + currentVersion + 42, + allocations, + { state: 'blah' } + ) + ).rejects.toThrow( + new RegExp(`RPC Error: incorrect app state: incorrect version: expected ${currentVersion + 1}, got ${currentVersion + 42}`) + ); + const { sessionData } = await fetchAndParseAppSessions(aliceAppWS, aliceAppIdentity, appSessionId); + expect(sessionData).toEqual(START_SESSION_DATA);
15-16: Increase RPC wait time to reduce flakiness (1s is tight in CI)submitAppStateUpdate_v04 waits 1s internally. Consider adding an optional timeout param (default 5000) in the helper and use it here for heavier flows.
Outside this file, update helper:
-export async function submitAppStateUpdate_v04( +export async function submitAppStateUpdate_v04( participantAppWS: TestWebSocket, participantAppIdentity: Identity, appSessionId: string, intent: RPCAppStateIntent, version: number, allocations: RPCAppSessionAllocation[], - sessionData: object + sessionData: object, + timeoutMs: number = 5000 ) { ... - const submitAppStateResponse = await participantAppWS.sendAndWaitForResponse( + const submitAppStateResponse = await participantAppWS.sendAndWaitForResponse( submitAppStateMsg, getSubmitAppStatePredicate(), - 1000 + timeoutMs );Then pass a higher timeout where needed.
integration/tests/lifecycle_nitrorpc_v02.test.ts (2)
112-198: Tests are order-dependent; consider a single E2E test or explicit step groupingEach “it” relies on prior state. That’s fine if the runner preserves order, but it’s brittle. Prefer one end-to-end “it” (or a nested describe with local variables) to keep state scoped and avoid accidental reordering.
If you keep multiple cases, add comments noting dependency and avoid parallelization for this file.
Also applies to: 199-357
113-114: Safer toRaw conversion for future 18‑decimals assetsCurrent toRaw multiplies by BigInt(10 ** decimals), which goes through JS Number and can overflow for larger decimals. Use pure BigInt exponentiation.
Outside this file (integration/common/testHelpers.ts):
-export function toRaw(amount: bigint, decimals: number = 6): bigint { - return amount * BigInt(10 ** decimals); -} +export function toRaw(amount: bigint, decimals: number = 6): bigint { + return amount * (10n ** BigInt(decimals)); +}integration/tests/lifecycle_nitrorpc_v04.test.ts (2)
116-287: Stateful multi-“it” flow; consider a single end‑to‑end testThese cases build on each other. Collapsing into one scenario test (or grouping under a nested describe with clear sequencing) will reduce brittleness.
31-34: Bump helper timeouts to avoid CI flakessubmitAppStateUpdate_v04/closeAppSessionWithState use a 1s wait internally. Consider adding an optional timeout param (default 5000) and pass it from these tests for deposit/withdraw/close steps.
See suggested helper change in the other comment; apply similarly to closeAppSessionWithState.
-export async function closeAppSessionWithState(..., expectedVersion: number) { +export async function closeAppSessionWithState(..., expectedVersion: number, timeoutMs: number = 5000) { ... - const closeAppSessionResponse = await participantAppWS.sendAndWaitForResponse( + const closeAppSessionResponse = await participantAppWS.sendAndWaitForResponse( closeAppSessionMsg, getCloseAppSessionPredicate(), - 1000 + timeoutMs );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
integration/tests/lifecycle_nitrorpc_v02.test.ts(8 hunks)integration/tests/lifecycle_nitrorpc_v04.test.ts(8 hunks)integration/tests/nitrorpc_v04.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
integration/tests/lifecycle_nitrorpc_v04.test.ts (4)
integration/common/ws.ts (3)
TestWebSocket(9-139)getCloseChannelPredicate(199-203)getResizeChannelPredicate(205-209)integration/common/testSetup.ts (2)
setupTestIdentitiesAndConnections(27-60)fetchAndParseAppSessions(66-93)integration/common/testHelpers.ts (5)
createTestChannels(25-44)toRaw(18-20)authenticateAppWithAllowances(49-68)getLedgerBalances(130-145)createTestAppSession(73-123)integration/common/testAppSessionHelpers.ts (2)
submitAppStateUpdate_v04(53-86)closeAppSessionWithState(91-120)
integration/tests/lifecycle_nitrorpc_v02.test.ts (5)
integration/common/blockchainUtils.ts (1)
BlockchainUtils(21-187)integration/common/databaseUtils.ts (1)
DatabaseUtils(4-81)integration/common/testSetup.ts (2)
setupTestIdentitiesAndConnections(27-60)fetchAndParseAppSessions(66-93)integration/common/testHelpers.ts (5)
createTestChannels(25-44)toRaw(18-20)authenticateAppWithAllowances(49-68)getLedgerBalances(130-145)createTestAppSession(73-123)integration/common/testAppSessionHelpers.ts (2)
submitAppStateUpdate_v02(18-48)closeAppSessionWithState(91-120)
integration/tests/nitrorpc_v04.test.ts (9)
integration/common/ws.ts (1)
TestWebSocket(9-139)integration/common/identity.ts (1)
Identity(7-28)integration/common/nitroliteClient.ts (1)
TestNitroliteClient(23-130)integration/common/blockchainUtils.ts (1)
BlockchainUtils(21-187)integration/common/databaseUtils.ts (1)
DatabaseUtils(4-81)integration/common/testSetup.ts (2)
setupTestIdentitiesAndConnections(27-60)fetchAndParseAppSessions(66-93)integration/common/testHelpers.ts (4)
createTestChannels(25-44)toRaw(18-20)authenticateAppWithAllowances(49-68)createTestAppSession(73-123)integration/common/testAppSessionHelpers.ts (1)
submitAppStateUpdate_v04(53-86)integration/common/auth.ts (1)
createAuthSessionWithClearnode(11-47)
⏰ 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 (2)
clearnode/app_session_service.go (2)
40-46: Consider adding validation in the constructor.The constructor currently accepts parameters and returns
(updater, nil)unconditionally. If future requirements introduce pre-conditions (e.g., validating params.Allocations isn't empty, checking specific protocol constraints), performing those checks here would fail fast before entering the transaction.For example, you could validate non-empty allocations:
func NewDepositUpdater(params *SubmitAppStateParams, appSession *AppSession, rpcSigners map[string]struct{}) (*DepositUpdater, error) { + if len(params.Allocations) == 0 { + return nil, RPCErrorf("deposit requires at least one allocation") + } return &DepositUpdater{ appSession: appSession, params: params, rpcSigners: rpcSigners, }, nil }Apply similar validation to
NewWithdrawUpdaterandNewOperateUpdateras needed.
85-87: Consider checking depositor signature earlier.The depositor signature validation (lines 85-87) occurs after fetching the participant's balance (lines 89-96). While functionally correct, checking signatures before querying balances would fail faster on invalid requests, reducing unnecessary database operations.
Move the signature check immediately after resolving the wallet address:
walletAddress := GetWalletBySigner(alloc.ParticipantWallet) if walletAddress == "" { walletAddress = alloc.ParticipantWallet } + + if _, ok := d.rpcSigners[alloc.ParticipantWallet]; !ok { + return UpdateResult{}, RPCErrorf("incorrect deposit request: depositor signature is required") + } currentAmount := currentAllocations[walletAddress][alloc.AssetSymbol] if alloc.Amount.GreaterThan(currentAmount) { if alloc.Amount.IsNegative() { return UpdateResult{}, RPCErrorf(ErrNegativeAllocation+": %s for asset %s", alloc.Amount, alloc.AssetSymbol) } if err := validateAppParticipant(walletAddress, participantWeights); err != nil { return UpdateResult{}, err } depositAmount := alloc.Amount.Sub(currentAmount) noDeposits = false - - if _, ok := d.rpcSigners[alloc.ParticipantWallet]; !ok { - return UpdateResult{}, RPCErrorf("incorrect deposit request: depositor signature is required") - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
clearnode/app_session.go(2 hunks)clearnode/app_session_service.go(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
clearnode/app_session_service.go (9)
clearnode/app_session.go (2)
AppSession(12-26)AppSession(28-30)clearnode/pkg/rpc/api.go (8)
AppSession(554-579)Version(27-27)AppSessionIntentDeposit(598-598)AppSessionIntentWithdraw(600-600)AppSessionIntentOperate(596-596)Asset(506-515)AppSessionIntent(592-592)AppAllocation(582-589)clearnode/rpc_router_private.go (4)
SubmitAppStateParams(42-48)Balance(124-127)AppSessionResponse(62-75)AppAllocation(56-60)clearnode/ledger.go (6)
NewAccountID(43-49)GetWalletLedger(55-57)ErrGetAccountBalance(13-13)ErrDebitSourceAccount(14-14)ErrCreditDestinationAccount(15-15)AccountID(41-41)clearnode/rpc.go (1)
RPCErrorf(147-151)clearnode/wallet.go (1)
GetWalletBySigner(35-40)clearnode/ledger_transaction.go (3)
RecordLedgerTransaction(39-51)TransactionTypeAppDeposit(16-16)TransactionTypeAppWithdrawal(17-17)clearnode/notification.go (2)
NewBalanceNotification(54-61)NewAppSessionNotification(95-122)clearnode/log.go (1)
LoggerFromContext(95-100)
clearnode/app_session.go (1)
clearnode/pkg/rpc/api.go (1)
AppSession(554-579)
⏰ 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/app_session.go (1)
32-39: LGTM!The
getAppSessionhelper cleanly retrieves the latest app session (highest nonce) for a given sessionID and status. Ordering by nonce DESC ensures the most recent state is returned when multiple versions exist.clearnode/app_session_service.go (10)
16-30: Well-structured abstraction for intent-specific logic.The
AppSessionUpdaterinterface with intent-specific implementations (Deposit, Withdraw, Operate) cleanly separates concerns and makes the update flow extensible. TheUpdateResulttype effectively communicates which participants were affected, enabling targeted notifications.
49-128: Deposit logic is correct and comprehensive.The deposit flow properly:
- Validates quorum and participant membership
- Retrieves current allocations
- Processes only increased amounts (deposits)
- Rejects decreased amounts
- Checks depositor signature
- Validates sufficient balance
- Records ledger transactions atomically
- Ensures at least one deposit occurred
- Tracks affected participants for notifications
147-209: Withdrawal logic is sound.The withdrawal flow correctly:
- Validates quorum
- Retrieves current allocations
- Processes only decreased amounts (withdrawals)
- Rejects increased amounts
- Records ledger transactions atomically
- Ensures at least one withdrawal occurred
- Tracks affected participants for notifications
The implicit participant validation via balance checks (non-participants have zero balance) is acceptable, as discussed in previous reviews.
228-285: Operate logic properly enforces zero-sum constraint.The operate flow correctly:
- Validates quorum and all participant memberships explicitly (line 251)
- Fetches app session balances
- Rejects negative allocations
- Updates session balances per participant
- Verifies the allocation sum matches the total session balance (zero-sum)
- Returns no affected participants (operate redistributes, doesn't deposit/withdraw)
311-315: Good addition: non-negative weight validation.Validating participant weights are non-negative during application creation prevents invalid configurations early. This aligns with business logic where zero weight is permitted (allowing non-voting participants), but negative weights are nonsensical.
403-490: Clean refactor using the updater pattern.
SubmitAppStatenow delegates intent-specific logic to updaters, simplifying the main flow:
- Protocol-specific validation (v0.2 vs v0.4)
- Intent-based updater selection
- Update execution within transaction
- Dual notifications (balance updates for affected participants, session updates for all)
The error handling for
getParticipantAllocations(lines 476-478) appropriately logs and continues, ensuring session notifications aren't blocked by transient balance query failures.
620-640: Efficient helper for fetching participant allocations.
getParticipantAllocationsconsolidates per-participant balance retrieval into a structured map, making it easy to populate notification payloads. Filtering out zero balances (line 633) keeps notifications concise.
642-648: Clear participant validation helper.
validateAppParticipantprovides a single point for membership checks, improving code clarity and maintainability. The descriptive error message aids debugging.
650-679: Improved quorum verification with intent-aware errors.Returning
participantWeightsdirectly simplifies callers (no need to rebuild the map), and intent-specific error messages (lines 668-672) improve UX by clearly indicating which operation failed.
681-696: Straightforward allocation formatting helper.
prepareAppAllocationscleanly transforms the internal map representation to the notification format, filtering zero amounts to avoid noise.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
clearnode/app_session_service.go (1)
403-490: Consider propagating context to database operations.The implementation correctly uses the updater pattern and handles notifications appropriately. However, the context is currently only used for logging (line 477). Consider propagating it to database operations for timeout and cancellation support.
For example, you could update the transaction call to:
- err := s.db.Transaction(func(tx *gorm.DB) error { + err := s.db.WithContext(ctx).Transaction(func(tx *gorm.DB) error {This would allow database operations to respect context deadlines and cancellations, improving the service's resilience under load or timeout scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
clearnode/app_session_service.go(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
clearnode/app_session_service.go (9)
clearnode/app_session.go (2)
AppSession(12-26)AppSession(28-30)clearnode/pkg/rpc/api.go (10)
AppSession(554-579)Version(27-27)VersionNitroRPCv0_4(33-33)VersionNitroRPCv0_2(31-31)AppSessionIntentDeposit(598-598)AppSessionIntentWithdraw(600-600)AppSessionIntentOperate(596-596)Asset(506-515)AppSessionIntent(592-592)AppAllocation(582-589)clearnode/rpc_router_private.go (4)
SubmitAppStateParams(42-48)Balance(124-127)AppSessionResponse(62-75)AppAllocation(56-60)clearnode/ledger.go (6)
NewAccountID(43-49)GetWalletLedger(55-57)ErrGetAccountBalance(13-13)ErrDebitSourceAccount(14-14)ErrCreditDestinationAccount(15-15)AccountID(41-41)clearnode/rpc.go (1)
RPCErrorf(147-151)clearnode/wallet.go (1)
GetWalletBySigner(35-40)clearnode/ledger_transaction.go (3)
RecordLedgerTransaction(39-51)TransactionTypeAppDeposit(16-16)TransactionTypeAppWithdrawal(17-17)clearnode/notification.go (2)
NewBalanceNotification(54-61)NewAppSessionNotification(95-122)clearnode/log.go (1)
LoggerFromContext(95-100)
⏰ 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 (7)
clearnode/app_session_service.go (7)
16-30: LGTM! Well-structured updater pattern.The introduction of error constants, UpdateResult type, and AppSessionUpdater interface provides a clean abstraction for handling different intent types. The interface-based design enables extensibility for future intent types.
32-128: LGTM! Deposit logic is correct and well-secured.The DepositUpdater properly validates depositor signatures, checks participant membership, verifies sufficient balances, and performs atomic ledger operations. The requirement that depositors must sign their own deposits (line 72-74) prevents unauthorized deposits.
130-209: LGTM! Withdrawal logic correctly enforces decrease-only allocations.The WithdrawUpdater properly processes withdrawals by ensuring allocations only decrease (line 174) and rejecting any increases (line 194-196). The balance checks implicitly validate participants since
currentAllocationsonly contains actual session participants.
211-285: LGTM! Operate logic correctly maintains zero-sum invariant.The OperateUpdater properly validates participants, efficiently updates balances using diff calculation (line 262-267), and enforces the zero-sum constraint (line 274-276). Returning an empty ParticipantsAffected map (line 282) is correct since operate intent only redistributes within the session without affecting unified balances.
298-401: LGTM! Application creation properly validates and processes initial deposits.The CreateApplication method correctly validates weights (line 311-315), processes initial deposits within a transaction, and notifies only participants with balance changes. The use of error constants ensures consistent error handling throughout.
493-588: LGTM! Application closure properly redistributes funds with validation.The CloseApplication method correctly verifies quorum, validates participants receive allocations, ensures zero-sum redistribution, and notifies only participants receiving funds. The implementation maintains consistency with the updater pattern used in SubmitAppState.
620-696: LGTM! Helper functions are well-designed and focused.The helper functions provide clean abstractions:
getParticipantAllocationsefficiently retrieves all participant balancesvalidateAppParticipantprovides clear error messages for non-participantsverifyQuorumnow returns participant weights for reuse, avoiding redundant map constructionprepareAppAllocationsappropriately filters zero-amount allocations for cleaner notifications
3e4b33d to
0b6dfbf
Compare
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Chores