YNU-324/337: add app session keys and allowances and expiration#392
YNU-324/337: add app session keys and allowances and expiration#392
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (68)
Tip You can get early access to new features in CodeRabbit.Enable the 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 SessionKey model, DB migration and cache; threads optional session_key through ledger entries; enforces per-app session-key spending caps; switches quorum checks from signer-based to wallet-based across app-session flows; exposes get_session_keys RPC and SDK helper; normalizes asset symbols to lowercase. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(230,245,255,0.5)
participant Client
participant RPC as RPC Handler
participant SK as SessionKeySvc
participant Ledger as WalletLedger
participant DB as Database
end
Client->>RPC: Submit/Transfer (signed)
RPC->>SK: GetWalletBySessionKey(signer)
SK->>DB: SELECT session_keys WHERE address=...
DB-->>SK: SessionKey or nil
alt signer is session-key
RPC->>SK: ValidateSessionKeySpending(sessionKey, asset, amount)
SK->>Ledger: GetSessionKeySpending(sessionKey, asset)
Ledger-->>SK: current_spent
alt within cap
RPC->>Ledger: Record(accountID, asset, amount, sessionKey=sessionKey)
Ledger->>DB: INSERT ledger (session_key = sessionKey)
RPC->>SK: UpdateSessionKeyUsage(sessionKey)
SK->>DB: UPDATE used_allowance
RPC-->>Client: Success
else exceeds cap
RPC-->>Client: Error (exceeds cap)
end
else wallet-signed
RPC->>Ledger: Record(accountID, asset, amount, sessionKey=nil)
Ledger->>DB: INSERT ledger (session_key = NULL)
RPC-->>Client: Success
end
sequenceDiagram
participant Client
participant RPC as RPC Handler
participant AppSvc as AppSessionService
participant Quorum as verifyQuorum
participant DB as Database
Client->>RPC: SubmitAppState(params, rpcWallets, rpcSigners)
RPC->>AppSvc: SubmitAppState(...)
AppSvc->>Quorum: verifyQuorum(session, rpcWallets, intent)
Quorum->>DB: Load participants, dedupe wallets, sum weights
alt quorum satisfied
AppSvc->>AppSvc: Process deposits/ops (session-key checks)
AppSvc->>Ledger: Record(..., sessionKey?)
AppSvc->>SK: UpdateSessionKeyUsage(if used)
AppSvc-->>RPC: Success
else
AppSvc-->>RPC: Error (insufficient quorum)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 significantly enhances the security and control mechanisms within the system by introducing a robust session key management framework. It centralizes the handling of all signers, differentiating between primary wallet signers and application-specific session keys. The new system enforces spending limits and expiration times for delegated session keys, providing granular control over their capabilities during financial transactions like deposits and transfers. This change improves the overall integrity and auditability of delegated 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 support for session key spending caps, a significant feature for security and usability. The changes are extensive, touching database migrations, core services, and RPC handlers. The implementation correctly integrates session key validation in various flows like app session creation and transfers. However, I've identified a few issues: a critical bug in quorum verification that could lead to double-counting votes, a bug in adding new signers to wallets, and incorrect logic for tracking session key usage. The new tests are a great addition and cover many scenarios for the new functionality.
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
clearnode/rpc_router_private_test.go (1)
879-882: Fix incorrect require.Empty argument order (test is vacuously true).You’re asserting an empty string, not appResp.SessionData.
- require.Empty(t, "", appResp.SessionData, "session data should not be returned in response") + require.Empty(t, appResp.SessionData, "session data should not be returned in response")clearnode/rpc_router_private.go (1)
417-425: Spending-cap bypass risk: router normalizes signers before servicegetWallets maps session keys to wallets; service then can’t detect session-key usage and may skip ValidateSessionKeySpending in app-session flows. Pass raw signers to the service.
- rpcSigners, err := getWallets(&c.Message) + rpcSigners, err := c.Message.GetRequestSignersMap()Apply the same change in HandleSubmitAppState and HandleCloseApplication. verifyQuorum already maps session keys to wallets internally. Based on relevant code.
Also applies to: 455-463, 490-498
clearnode/app_session_service.go (1)
66-101: Spending caps can be skipped due to signer normalizationd.rpcSigners are wallet addresses when routed via getWallets; the session-key branch won’t trigger, so ValidateSessionKeySpending is never called. Pair this with the router fix to pass raw signers; here, keep detecting session keys by iterating raw signers.
If you accept the router change, this logic works as-is. Otherwise, change the method signature to accept both walletSigners and rawSigners, and iterate rawSigners in the session-key detection block.
Also applies to: 116-133
🧹 Nitpick comments (10)
clearnode/rpc_router_auth.go (1)
261-285: LGTM! Clean validation logic with good error messages.The helper function properly validates allowances against supported assets. The inline comment acknowledges that an in-memory asset cache would improve performance - this is a reasonable optimization to consider for future iterations, especially if authentication frequency becomes a bottleneck.
clearnode/ledger.go (2)
60-68: API change acknowledged; consider options pattern next time.Record gained a sessionKey parameter and propagates to Entry. Signature churn is fine here, but an options struct would avoid future breaking changes.
79-79: Avoid stdout in library code.Replace fmt.Println with structured logging (or remove). Printing on hot paths pollutes stdout and complicates tests.
- fmt.Println("recording entry for: ", l.wallet, " in account ", accountID, " ", assetSymbol, " ", amount) + // TODO: inject logger; for now, remove noisy stdout + // logger.Debug("recording entry", "wallet", l.wallet.Hex(), "account", accountID.String(), "asset", assetSymbol, "amount", amount)clearnode/session_keys_test.go (1)
362-365: Gitleaks false positive: public Ethereum addresses, not secrets.These constants are known token contract addresses; safe to keep in tests. Consider allowlisting EVM address patterns in gitleaks.
# .gitleaks.toml [[rules.allowlist.regexes]] description = "EVM address" regex = '''0x[0-9a-fA-F]{40}'''clearnode/rpc_router_private.go (1)
241-247: Avoid mutating request signature bytesRecoverAddress mutates v in-place; you pass c.Message.Sig[0] directly. Copy before recovery or make RecoverAddress copy internally to prevent side effects across verifications.
Minimal local fix here:
- recovered, err := RecoverAddress(c.Message.Req.rawBytes, c.Message.Sig[0]) + sigCopy := append([]byte(nil), c.Message.Sig[0]...) + recovered, err := RecoverAddress(c.Message.Req.rawBytes, sigCopy)Or update RecoverAddress to copy the slice first (see clearnode/signer.go). Would you like a patch there?
clearnode/app_session_service_test.go (2)
737-793: Great coverage for session-key deposit pathValidates quorum+session-key authorization and spending accounting. Consider adding an end-to-end router test to ensure raw signer propagation (no wallet normalization) so caps are enforced in production paths too.
1240-1355: CreateAppSession/Deposit spending-cap tests use raw signersTests pass sessionKeyAddr directly to the service. In the real router flow, signers were being normalized to wallets; ensure router passes raw signers (see router comment) or add a router-level test that would fail without that fix.
clearnode/session_key.go (3)
176-181: IsSessionKey should consider key activity (expiry)Callers often need to know if a session key is valid “now,” not merely present. Either add IsActiveSessionKey or extend this to check expiration_time > now.
-func IsSessionKey(db *gorm.DB, signerAddress string) bool { - var count int64 - db.Model(&SessionKey{}).Where("signer_address = ? AND signer_type = ?", signerAddress, "session").Count(&count) - return count > 0 -} +func IsSessionKey(db *gorm.DB, signerAddress string) bool { + var count int64 + db.Model(&SessionKey{}). + Where("signer_address = ? AND signer_type = ? AND (expiration_time IS NULL OR expiration_time > ?)", + signerAddress, "session", time.Now()). + Count(&count) + return count > 0 +}
39-49: Cache includes expired keysloadSessionKeyCache loads all signers; consider skipping expired session keys or validating expiration at use sites (router/service). Since you now validate in ValidateSessionKeySpending/IsSessionKey, either approach works; prefer validation at use sites to avoid stale cache.
13-31: Add DB constraints to enforce uniqueness and invariantsTo prevent drift under concurrency/restarts: unique index on signer_address; and a composite unique index on (wallet_address, application_name, signer_type='session') to enforce single session key per app per wallet.
Example GORM tags (plus a migration):
- SignerAddress string `gorm:"column:signer_address;index;not null"` + SignerAddress string `gorm:"column:signer_address;uniqueIndex;not null"` - WalletAddress string `gorm:"column:wallet_address;index;not null"` + WalletAddress string `gorm:"column:wallet_address;index;not null"` + // Ensure at most one session key per wallet/app + // gorm: unique index across (wallet_address, application_name, signer_type) + // Implement with a manual migration if GORM tag is cumbersome.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
clearnode/app_session_service.go(13 hunks)clearnode/app_session_service_test.go(9 hunks)clearnode/channel_service_test.go(4 hunks)clearnode/config/migrations/postgres/20251015000001_add_session_keys.sql(1 hunks)clearnode/custody.go(6 hunks)clearnode/custody_test.go(5 hunks)clearnode/database.go(1 hunks)clearnode/ledger.go(2 hunks)clearnode/main.go(1 hunks)clearnode/rpc_router.go(1 hunks)clearnode/rpc_router_auth.go(3 hunks)clearnode/rpc_router_private.go(9 hunks)clearnode/rpc_router_private_test.go(27 hunks)clearnode/rpc_router_public_test.go(3 hunks)clearnode/rpc_router_test.go(2 hunks)clearnode/session_key.go(1 hunks)clearnode/session_keys_test.go(1 hunks)clearnode/wallet.go(0 hunks)clearnode/wallet_test.go(0 hunks)
💤 Files with no reviewable changes (2)
- clearnode/wallet_test.go
- clearnode/wallet.go
🧰 Additional context used
🧬 Code graph analysis (11)
clearnode/rpc_router_test.go (2)
clearnode/ledger.go (2)
Entry(19-29)Entry(31-33)clearnode/session_key.go (2)
SessionKey(14-31)SessionKey(33-35)
clearnode/ledger.go (1)
clearnode/session_key.go (2)
SessionKey(14-31)SessionKey(33-35)
clearnode/database.go (2)
clearnode/ledger.go (2)
Entry(19-29)Entry(31-33)clearnode/session_key.go (2)
SessionKey(14-31)SessionKey(33-35)
clearnode/session_key.go (1)
clearnode/ledger.go (2)
Entry(19-29)Entry(31-33)
clearnode/rpc_router_auth.go (3)
clearnode/session_key.go (3)
AddSessionKey(53-103)SessionKey(14-31)SessionKey(33-35)clearnode/signer.go (1)
Allowance(18-21)clearnode/asset.go (1)
GetAllAssets(30-40)
clearnode/session_keys_test.go (4)
clearnode/session_key.go (10)
AddSessionKey(53-103)GetWalletBySigner(169-174)GetSessionKeysByWallet(184-196)UpdateSessionKeyUsage(215-253)AddSigner(106-166)GetAllSignersForWallet(286-290)GetActiveSessionKeysByWallet(199-212)ValidateSessionKeySpending(312-360)GetSessionKeySpending(293-309)GetSessionKeyByKey(256-263)clearnode/ledger.go (2)
NewAccountID(44-50)GetWalletLedger(56-58)clearnode/rpc_router_private.go (3)
TransferParams(23-27)TransferAllocation(33-36)TransferResponse(135-137)clearnode/pkg/rpc/payload.go (1)
Params(177-177)
clearnode/custody.go (1)
clearnode/ledger.go (1)
GetWalletLedger(56-58)
clearnode/app_session_service.go (2)
clearnode/session_key.go (4)
GetWalletBySigner(169-174)IsSessionKey(177-181)ValidateSessionKeySpending(312-360)UpdateSessionKeyUsage(215-253)clearnode/rpc.go (1)
RPCErrorf(147-151)
clearnode/rpc_router_private_test.go (2)
clearnode/session_key.go (1)
AddSigner(106-166)clearnode/ledger.go (1)
GetWalletLedger(56-58)
clearnode/rpc_router_private.go (3)
clearnode/session_key.go (6)
SessionKey(14-31)SessionKey(33-35)GetWalletBySigner(169-174)ValidateSessionKeySpending(312-360)UpdateSessionKeyUsage(215-253)GetSessionKeysByWallet(184-196)clearnode/pkg/rpc/api.go (1)
Allowance(520-525)clearnode/signer.go (2)
Allowance(18-21)RecoverAddress(63-81)
clearnode/app_session_service_test.go (4)
clearnode/session_key.go (3)
AddSigner(106-166)AddSessionKey(53-103)GetSessionKeySpending(293-309)clearnode/ledger.go (2)
NewAccountID(44-50)GetWalletLedger(56-58)clearnode/rpc_router_private.go (3)
CreateAppSessionParams(38-42)AppAllocation(58-62)SubmitAppStateParams(44-50)clearnode/pkg/rpc/api.go (8)
AppDefinition(540-553)VersionNitroRPCv0_2(31-31)AppAllocation(584-591)Version(27-27)ChannelStatusOpen(644-644)Allowance(520-525)Asset(508-517)VersionNitroRPCv0_4(33-33)
🪛 Gitleaks (8.28.0)
clearnode/session_keys_test.go
[high] 234-234: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 363-363: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 364-364: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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 (14)
clearnode/main.go (1)
36-39: LGTM! Clean refactoring to session key management.The initialization flow correctly loads the session key cache instead of the wallet cache, aligning with the PR's objective of introducing session key management.
clearnode/rpc_router.go (1)
82-82: LGTM! New session keys endpoint properly secured.The new
get_session_keysroute is registered within the private group with authentication middleware, ensuring only authenticated users can access session key data.clearnode/database.go (1)
258-258: LGTM! SessionKey model added to migrations.The SessionKey model is now included in SQLite auto-migration, ensuring schema consistency with the new session key management feature.
clearnode/rpc_router_test.go (2)
29-29: LGTM! Test migrations updated for SessionKey.SessionKey model is now included in SQLite test migrations, enabling tests to exercise session key functionality.
66-66: LGTM! PostgreSQL test migrations updated.SessionKey model is included in PostgreSQL test setup, ensuring consistency across database drivers in tests.
clearnode/rpc_router_public_test.go (3)
626-627: LGTM! Test updated for extended ledger.Record signature.The ledger.Record call now passes
nilfor the optional sessionKey parameter, which is appropriate for test scenarios without session key context.
639-640: LGTM! Consistent with extended signature.
779-780: LGTM! Consistent with extended signature.clearnode/custody_test.go (1)
406-407: LGTM! Custody tests updated for extended ledger.Record signature.All ledger.Record calls in custody tests now pass
nilfor the optional sessionKey parameter, which is appropriate since custody operations (deposits, withdrawals, resizes) occur at the blockchain level without session key context.clearnode/custody.go (1)
298-307: LGTM! Custody operations correctly pass nil for session key.Custody contract event handlers (handleCreated, handleResized, handleClosed) appropriately pass
nilfor the sessionKey parameter since these are blockchain-level operations without session key context. Session keys are used for application-level operations, not for custody contract interactions.clearnode/rpc_router_auth.go (1)
231-240: Excellent addition of allowance validation before session key storage.The validation ensures that only session keys with supported asset allowances are stored, preventing potential issues downstream. The error handling and logging are appropriate.
clearnode/channel_service_test.go (1)
99-100: LGTM: tests updated for new Record signature.Nil sessionKey makes sense for non-session-key paths. No further issues spotted.
Also applies to: 143-144, 218-219, 260-261
clearnode/session_keys_test.go (1)
196-204: LGTM: end-to-end session key spending coverage.Good assertions for within-cap, over-cap, unauthorized asset, and ledger-backed spending.
Also applies to: 206-212, 293-301, 309-319, 320-324, 333-341, 349-355
clearnode/rpc_router_private_test.go (1)
192-196: Verification confirmed: all Record calls adopt the 4-argument pattern with nil.Scan results show no lingering 3-argument Record calls across the codebase. All 27 calls in
rpc_router_private_test.goand all calls codebase-wide consistently use the new 4-argument signature (accountID, asset, amount, sessionKeyOrNil). The target lines 192–196 correctly pass nil as the sessionKey parameter. The migration is complete and consistent.
clearnode/config/migrations/postgres/20251015000001_add_session_keys.sql
Outdated
Show resolved
Hide resolved
clearnode/config/migrations/postgres/20251015000001_add_session_keys.sql
Outdated
Show resolved
Hide resolved
clearnode/config/migrations/postgres/20251015000001_add_session_keys.sql
Outdated
Show resolved
Hide resolved
c8f3ae3 to
5fcea45
Compare
5fcea45 to
7ee7bc6
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
clearnode/main.go (1)
93-100: Variable shadowing breaks worker startup (empty client map).Inner custodyClients shadows outer; range iterates over empty map; worker gets no clients.
- if len(custodyClients) > 0 { - custodyClients := make(map[uint32]CustodyInterface, len(custodyClients)) - for chainID, client := range custodyClients { - custodyClients[chainID] = client - } - worker := NewBlockchainWorker(db, custodyClients, logger) - go worker.Start(context.Background()) - } + if len(custodyClients) > 0 { + workerClients := make(map[uint32]CustodyInterface, len(custodyClients)) + for chainID, client := range custodyClients { + workerClients[chainID] = client + } + worker := NewBlockchainWorker(db, workerClients, logger) + go worker.Start(context.Background()) + }
🧹 Nitpick comments (2)
clearnode/rpc_router_test.go (1)
66-67: Postgres test migration missing UserActionLog (parity gap with SQLite).SQLite path migrates UserActionLog; Postgres path does not. Align both to avoid test-only divergences.
-err = db.AutoMigrate(&Entry{}, &Channel{}, &AppSession{}, &RPCRecord{}, &Asset{}, &ContractEvent{}, &LedgerTransaction{}, &UserTagModel{}, &BlockchainAction{}, &SessionKey{}, &SignerWallet{}) +err = db.AutoMigrate(&Entry{}, &Channel{}, &AppSession{}, &RPCRecord{}, &Asset{}, &ContractEvent{}, &LedgerTransaction{}, &UserTagModel{}, &UserActionLog{}, &BlockchainAction{}, &SessionKey{}, &SignerWallet{})clearnode/custody.go (1)
297-306: Record signature adaptation is correct; remove redundant ledger lookups.Multiple consecutive GetWalletLedger(tx, walletAddress) calls can be reused to cut allocations and noise.
Example for handleCreated:
- ledger := GetWalletLedger(tx, walletAddress) - if err := ledger.Record(channelAccountID, asset.Symbol, amount, nil); err != nil { + ledger := GetWalletLedger(tx, walletAddress) + if err := ledger.Record(channelAccountID, asset.Symbol, amount, nil); err != nil { return fmt.Errorf("error recording balance update for wallet: %w", err) } - if err := ledger.Record(channelAccountID, asset.Symbol, amount.Neg(), nil); err != nil { + if err := ledger.Record(channelAccountID, asset.Symbol, amount.Neg(), nil); err != nil { return fmt.Errorf("error recording balance update for wallet: %w", err) } - ledger = GetWalletLedger(tx, walletAddress) if err := ledger.Record(walletAccountID, asset.Symbol, amount, nil); err != nil { return fmt.Errorf("error recording balance update for wallet: %w", err) }Apply same reuse in handleResized deposit/withdraw branches.
Also applies to: 301-306, 447-457, 466-479, 541-546, 554-555
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (21)
clearnode/app_session_service.go(20 hunks)clearnode/app_session_service_test.go(30 hunks)clearnode/channel_service_test.go(4 hunks)clearnode/config/migrations/postgres/20251015000001_add_session_keys.sql(1 hunks)clearnode/custody.go(6 hunks)clearnode/custody_test.go(5 hunks)clearnode/database.go(1 hunks)clearnode/docs/API.md(4 hunks)clearnode/ledger.go(2 hunks)clearnode/main.go(1 hunks)clearnode/rpc_router.go(1 hunks)clearnode/rpc_router_auth.go(3 hunks)clearnode/rpc_router_private.go(12 hunks)clearnode/rpc_router_private_test.go(27 hunks)clearnode/rpc_router_public_test.go(3 hunks)clearnode/rpc_router_test.go(2 hunks)clearnode/session_key.go(1 hunks)clearnode/session_keys_test.go(1 hunks)clearnode/wallet.go(4 hunks)clearnode/wallet_test.go(2 hunks)integration/common/databaseUtils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- clearnode/channel_service_test.go
- clearnode/rpc_router.go
- clearnode/config/migrations/postgres/20251015000001_add_session_keys.sql
- clearnode/wallet_test.go
- clearnode/database.go
- clearnode/ledger.go
🧰 Additional context used
🧬 Code graph analysis (10)
clearnode/app_session_service_test.go (6)
clearnode/wallet.go (1)
AddSigner(40-114)clearnode/ledger.go (2)
NewAccountID(44-50)GetWalletLedger(56-58)clearnode/rpc_router_private.go (3)
CreateAppSessionParams(42-46)AppAllocation(62-66)SubmitAppStateParams(48-54)clearnode/pkg/rpc/api.go (5)
AppDefinition(540-553)AppAllocation(584-591)Version(27-27)Allowance(520-525)Asset(508-517)clearnode/rpc_router_public.go (1)
AppDefinition(36-43)clearnode/session_key.go (2)
AddSessionKey(47-109)GetSessionKeySpending(223-239)
clearnode/rpc_router_private.go (5)
clearnode/pkg/rpc/api.go (2)
ListOptions(457-464)Allowance(520-525)clearnode/session_key.go (7)
SessionKey(13-27)SessionKey(29-31)GetWalletBySigner(112-122)IsSessionKey(125-128)ValidateSessionKeySpending(242-295)UpdateSessionKeyUsage(172-210)GetActiveSessionKeysByWallet(146-169)clearnode/signer.go (2)
Allowance(18-21)RecoverAddress(63-81)clearnode/rpc_node.go (1)
RPCContext(252-266)clearnode/pkg/rpc/payload.go (1)
Params(177-177)
clearnode/rpc_router_auth.go (3)
clearnode/session_key.go (3)
AddSessionKey(47-109)SessionKey(13-27)SessionKey(29-31)clearnode/signer.go (1)
Allowance(18-21)clearnode/asset.go (1)
GetAllAssets(30-40)
clearnode/rpc_router_test.go (3)
clearnode/ledger.go (2)
Entry(19-29)Entry(31-33)clearnode/session_key.go (2)
SessionKey(13-27)SessionKey(29-31)clearnode/wallet.go (2)
SignerWallet(12-15)SignerWallet(17-19)
clearnode/session_keys_test.go (4)
clearnode/session_key.go (10)
AddSessionKey(47-109)GetWalletBySigner(112-122)GetSessionKeysByWallet(131-143)UpdateSessionKeyUsage(172-210)SessionKey(13-27)SessionKey(29-31)GetActiveSessionKeysByWallet(146-169)ValidateSessionKeySpending(242-295)GetSessionKeySpending(223-239)GetSessionKeyBySigner(213-220)clearnode/wallet.go (1)
AddSigner(40-114)clearnode/ledger.go (2)
NewAccountID(44-50)GetWalletLedger(56-58)clearnode/rpc_router_private.go (3)
TransferParams(27-31)TransferAllocation(37-40)TransferResponse(139-141)
clearnode/app_session_service.go (3)
clearnode/rpc_router_private.go (2)
SubmitAppStateParams(48-54)CreateAppSessionParams(42-46)clearnode/app_session.go (2)
AppSession(12-26)AppSession(28-30)clearnode/session_key.go (4)
GetWalletBySigner(112-122)IsSessionKey(125-128)ValidateSessionKeySpending(242-295)UpdateSessionKeyUsage(172-210)
clearnode/custody.go (1)
clearnode/ledger.go (1)
GetWalletLedger(56-58)
clearnode/wallet.go (2)
clearnode/pkg/rpc/error.go (2)
Error(48-50)Errorf(71-75)clearnode/session_key.go (2)
SessionKey(13-27)SessionKey(29-31)
clearnode/session_key.go (1)
clearnode/ledger.go (2)
Entry(19-29)Entry(31-33)
clearnode/rpc_router_private_test.go (2)
clearnode/wallet.go (1)
AddSigner(40-114)clearnode/ledger.go (1)
GetWalletLedger(56-58)
🪛 Gitleaks (8.28.0)
clearnode/session_keys_test.go
[high] 237-237: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 366-366: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 367-367: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
clearnode/docs/API.md
[high] 437-437: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 465-465: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test (Integration) / Test Integration
🔇 Additional comments (23)
clearnode/rpc_router_test.go (1)
29-31: SQLite migration addition looks good.Including SessionKey in AutoMigrate keeps the in-memory schema aligned with new models.
clearnode/main.go (1)
41-44: Session key cache loader added — good placement.Loaded before services start; failure is fatal as expected.
clearnode/rpc_router_public_test.go (1)
626-627: Tests updated for new Record signature — OK.Passing nil for sessionKey keeps existing semantics.
Also applies to: 639-640, 779-780
integration/common/databaseUtils.ts (1)
19-19: Primary concern is incorrect: table name is "signers" (verified), not "signer_wallets".The Go model SignerWallet.TableName() returns "signers", confirming the table name in line 19 is correct. All nine tables in the list align with their respective GORM model mappings. Adding error handling for 42P01 is a defensive coding practice and optional, but not necessary if the schema is guaranteed to have all tables during test cleanup.
Likely an incorrect or invalid review comment.
clearnode/rpc_router_private.go (7)
151-160: LGTM! Session key response structure is well-defined.The
SessionKeyResponsetype properly usesomitemptyfor optional fields and includes all necessary spending cap and allowance data.
245-251: LGTM! Signer address recovery is correct.The recovered address is properly stored for session key detection later in the transfer flow.
308-316: LGTM! Session key detection properly differentiates from custody signers.The logic correctly checks
IsSessionKeyto ensure only session keys (not custody signers) are treated as such, as addressed in the previous review.
328-352: LGTM! Session key validation and ledger tracking are correctly implemented.The spending cap is validated before the transfer, and only the sender's session key usage is tracked (line 345) while the receiver's credit correctly passes
nil(line 352).
372-378: LGTM! Session key usage tracking after transfer is correct.The usage update occurs after the transaction succeeds and includes proper error handling.
702-759: LGTM! Session keys retrieval endpoint correctly returns only active keys.Uses
GetActiveSessionKeysByWalletto filter expired keys and properly unmarshals the JSON allowance data with error handling, as addressed in the previous review.
459-473: LGTM! App session handlers correctly updated for wallet and signer maps.Both
rpcWalletsandrpcSignersare properly extracted and passed to the service methods for session-key-aware processing.clearnode/session_key.go (6)
12-31: LGTM! SessionKey model is well-structured.The model includes all necessary fields with appropriate GORM tags and indexes. JSON serialization for allowances is a reasonable choice.
45-109: LGTM! AddSessionKey properly validates and enforces single key per wallet+app.The function correctly validates expiration, sets defaults, invalidates existing keys for the same wallet+app, and atomically updates both database and cache within a transaction.
111-128: LGTM! Helper functions correctly resolve signers to wallets.
GetWalletBySignerproperly checks session keys before custody signers, andIsSessionKeyprovides clear session key identification.
145-169: LGTM! Active session key retrieval correctly filters expired keys.The function properly filters for non-expired keys and includes pagination support via
ListOptions.
171-210: LGTM! Session key usage update correctly recalculates from ledger.The function properly queries actual spending from ledger entries and updates the
used_allowancefield.
241-295: LGTM! Spending validation includes expiration check.The function properly validates expiration (line 249-251) as addressed in the previous review, and correctly checks spending caps against current usage.
clearnode/app_session_service.go (6)
36-47: LGTM! DepositUpdater correctly tracks both wallets and signers.The updater now maintains both
rpcWalletsandrpcSignersto enable session key detection and validation.
363-370: LGTM! Quorum validation correctly enforces that target quorum doesn't exceed total weights.This validation prevents invalid app session configurations.
390-410: LGTM! Session key detection correctly prefers direct wallet signature.The if-else structure ensures that if the wallet signs directly, session key detection is skipped. This is the correct approach and should be used consistently in
DepositUpdater(lines 90-106) as well.
746-784: LGTM! Quorum verification correctly counts each participant once.By operating on
rpcWallets(which is deduplicated bygetWallets()), the function avoids double-counting when both a wallet and its session key sign. TheparticipantsSignedmap provides an additional safety layer, as addressed in the previous review.
180-189: LGTM! Updaters correctly transitioned to wallet-based quorum checks.Both
WithdrawUpdaterandOperateUpdaternow userpcWalletsfor quorum verification and appropriately passnilfor session key tracking in ledger operations.Also applies to: 261-270
499-499: LGTM! Public method signatures correctly updated for session key support.
SubmitAppStateandCloseApplicationnow accept the necessary wallet and signer maps to support session-key-aware operations.Also applies to: 589-589
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
clearnode/wallet.go (1)
10-11: Clarify deprecation timeline or remove the comment.The comment suggests this might be deprecated "with a new version of smart contracts" but provides no timeline or migration path. If this is actually planned for deprecation, consider adding a TODO with a tracking issue number or expected version. If not actively planned, remove the comment to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
clearnode/wallet.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
clearnode/wallet.go (2)
clearnode/pkg/rpc/error.go (2)
Error(48-50)Errorf(71-75)clearnode/session_key.go (2)
SessionKey(13-27)SessionKey(29-31)
⏰ 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 (4)
clearnode/wallet.go (4)
21-37: LGTM!The cache structure and loader implementation are clean. Using
sync.Mapis appropriate for the concurrent access pattern, and the loader correctly populates the cache from the database.
40-52: Good use of cache for early validation.The pre-transaction cache checks provide fast-path returns for common cases (idempotent re-registration and obvious conflicts). The subsequent database checks inside the transaction provide consistency guarantees, making the cache checks safe optimizations rather than sources of race conditions.
54-79: Validation logic correctly handles self-custody case.The
if wallet != signerguard at line 62 properly exempts self-custody scenarios from the wallet-as-signer validation checks. This addresses the concern raised in the past review comment about self-signer re-registration failing when the cache is cold.When
wallet == signer, the code skips lines 63-79 and proceeds directly to the signer existence check at line 83, which correctly handles idempotent re-registration by checking the database (lines 86-88).
119-131: LGTM!The deletion order is correct: database first, then cache. If the database deletion fails, the cache remains consistent. If the database deletion succeeds, the cache is cleaned up appropriately.
701eea7 to
866129f
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
clearnode/rpc_router_auth.go (1)
172-187: Add session key persistence and asset validation to JWT authentication path.The review comment is accurate. The signature authentication path (lines 232-240) calls both
validateAllowanceAssets()andAddSessionKey(), but the JWT authentication path (lines 172-187) skips both. This creates an architectural inconsistency where:
- Signature-authenticated sessions persist to the database with validated allowances
- JWT-authenticated sessions are only verified in-memory without database persistence
The suggested diff correctly applies the same validation and persistence logic to align both authentication paths. Implement the recommended changes to ensure session keys are consistently managed across both auth methods.
clearnode/rpc_router_private.go (1)
583-590: Fix nil-pointer panic in logging Resize/Allocate amounts.params.ResizeAmount and params.AllocateAmount are optional pointers (
*decimal.Decimal) marked withomitemptyvalidation tags. The struct definition and RequestResize function both contain nil checks for these fields (channel_service.go:143, 146), confirming they can be nil. However, lines 588-589 unconditionally call.String()on both, which will panic if either is nil.Apply this diff:
- logger.Info("channel resize requested", - "userID", c.UserID, - "channelID", resp.ChannelID, - "fundsDestination", params.FundsDestination, - "resizeAmount", params.ResizeAmount.String(), - "allocateAmount", params.AllocateAmount.String(), - ) + resizeStr := "" + if params.ResizeAmount != nil { + resizeStr = params.ResizeAmount.String() + } + allocateStr := "" + if params.AllocateAmount != nil { + allocateStr = params.AllocateAmount.String() + } + logger.Info("channel resize requested", + "userID", c.UserID, + "channelID", resp.ChannelID, + "fundsDestination", params.FundsDestination, + "resizeAmount", resizeStr, + "allocateAmount", allocateStr, + )
♻️ Duplicate comments (1)
clearnode/rpc_router_private.go (1)
306-352: Session-key validation in transfers is correctly gated.Detecting session keys only when IsSessionKey(signer) and mapping matches fromWallet prevents forcing custody signers through caps.
Also applies to: 372-379
🧹 Nitpick comments (10)
clearnode/session_keys_test.go (2)
236-243: Add an explicit expired-key validation test.We exercise active/expired filtering, but not the ValidateSessionKeySpending "session key expired" path. Add a subtest that inserts an expired session key and asserts ValidateSessionKeySpending returns "session key expired".
237-243: Optional: Add gitleaks allowlist to reduce scanner noise for test fixtures.No active gitleaks scanning is currently configured in this repository. The hex strings in question (lines 237–243, 366–373) are test fixtures and public blockchain addresses (USDC, WETH mainnet contracts), not credentials. If gitleaks scanning is introduced in the future, consider adding a
.gitleaks.tomlallowlist to suppress false positives for Ethereum-style addresses in test code, or optionally replace synthetic keys with descriptive placeholders (e.g.,"0x<sessionkey_fixture>") for clarity.clearnode/channel_service_test.go (1)
79-87: Avoid embedding real-looking API keys in URLs.Replace "https://polygon-mainnet.infura.io/v3/test" with a placeholder like "https://polygon-mainnet.infura.io/v3/<INFURA_KEY>" to avoid gitleaks noise.
- BlockchainRPC: "https://polygon-mainnet.infura.io/v3/test", + BlockchainRPC: "https://polygon-mainnet.infura.io/v3/<INFURA_KEY>",clearnode/docs/API.md (2)
1076-1082: Use placeholders for signatures to prevent secret scanners from flagging examples.Replace long hex server_signature with "0x" in examples.
- "server_signature": "0x1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1c" + "server_signature": "0x<signature>"
1131-1136: Apply the same placeholder change to other samples.- "server_signature": "0x1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef1c" + "server_signature": "0x<signature>"Also applies to: 1191-1194
clearnode/app_session_service_test.go (1)
737-799: Add an expired session-key path test for deposits/creation.We validate spending caps with a live key. Add a case where the same flows use an expired session key and assert the service returns the wrapped "session key expired" error from ValidateSessionKeySpending.
Also applies to: 1246-1362
clearnode/rpc_router_private.go (2)
742-751: Optional: refresh used_allowance on read or document staleness.Consider calling UpdateSessionKeyUsage before responding so used_allowance reflects latest ledger debits, or document that values may lag until post-transaction updates run.
27-66: Casing consistency for asset symbols (future-proofing).If the broader system standardizes symbols to lower case (docs mostly use "usdc"), consider normalizing asset keys at boundaries (e.g., in validation and when recording ledger entries) to avoid surprises.
Also applies to: 147-160
clearnode/session_key.go (2)
112-123: Type assertions could panic if cache contains non-string values.Lines 116 and 120 perform unsafe type assertions (
w.(string)) without checking the type. If the cache ever contains a non-string value due to a bug, these will panic at runtime.Apply this diff to use safe type assertions:
func GetWalletBySigner(signer string) string { // Check session key cache first if w, ok := sessionKeyCache.Load(signer); ok { - return w.(string) + if wallet, isString := w.(string); isString { + return wallet + } } // Check custody signer cache if w, ok := custodySignerCache.Load(signer); ok { - return w.(string) + if wallet, isString := w.(string); isString { + return wallet + } } return "" }
70-76: Consider optimizing deletion of existing session keys.Lines 71-76 delete existing session keys one by one in a loop. For efficiency, consider using a single
DELETEquery.- // Remove existing session keys for this app (invalidate them) - for _, existingKey := range existingKeys { - if err := tx.Delete(&existingKey).Error; err != nil { - return fmt.Errorf("failed to remove existing session key: %w", err) - } + // Remove existing session keys for this app (invalidate them) + if len(existingKeys) > 0 { + if err := tx.Where("wallet_address = ? AND application_name = ?", + walletAddress, applicationName).Delete(&SessionKey{}).Error; err != nil { + return fmt.Errorf("failed to remove existing session keys: %w", err) + } + for _, existingKey := range existingKeys { - sessionKeyCache.Delete(existingKey.SignerAddress) - } + sessionKeyCache.Delete(existingKey.SignerAddress) + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (21)
clearnode/app_session_service.go(20 hunks)clearnode/app_session_service_test.go(30 hunks)clearnode/channel_service_test.go(4 hunks)clearnode/config/migrations/postgres/20251015000001_add_session_keys.sql(1 hunks)clearnode/custody.go(6 hunks)clearnode/custody_test.go(5 hunks)clearnode/database.go(1 hunks)clearnode/docs/API.md(4 hunks)clearnode/ledger.go(2 hunks)clearnode/main.go(1 hunks)clearnode/rpc_router.go(1 hunks)clearnode/rpc_router_auth.go(3 hunks)clearnode/rpc_router_private.go(12 hunks)clearnode/rpc_router_private_test.go(27 hunks)clearnode/rpc_router_public_test.go(3 hunks)clearnode/rpc_router_test.go(2 hunks)clearnode/session_key.go(1 hunks)clearnode/session_keys_test.go(1 hunks)clearnode/wallet.go(3 hunks)clearnode/wallet_test.go(2 hunks)integration/common/databaseUtils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- clearnode/database.go
- clearnode/wallet_test.go
- clearnode/config/migrations/postgres/20251015000001_add_session_keys.sql
- clearnode/ledger.go
- clearnode/rpc_router_test.go
- clearnode/main.go
🧰 Additional context used
🧬 Code graph analysis (9)
clearnode/rpc_router_private_test.go (2)
clearnode/wallet.go (1)
AddSigner(40-116)clearnode/ledger.go (1)
GetWalletLedger(56-58)
clearnode/custody.go (1)
clearnode/ledger.go (1)
GetWalletLedger(56-58)
clearnode/app_session_service_test.go (5)
clearnode/wallet.go (1)
AddSigner(40-116)clearnode/ledger.go (2)
NewAccountID(44-50)GetWalletLedger(56-58)clearnode/rpc_router_private.go (3)
CreateAppSessionParams(42-46)AppAllocation(62-66)SubmitAppStateParams(48-54)clearnode/pkg/rpc/api.go (9)
AppDefinition(540-553)VersionNitroRPCv0_2(31-31)AppAllocation(584-591)Version(27-27)ChannelStatusOpen(644-644)Allowance(520-525)Asset(508-517)VersionNitroRPCv0_4(33-33)AppSessionIntentDeposit(600-600)clearnode/session_key.go (2)
AddSessionKey(47-110)GetSessionKeySpending(224-240)
clearnode/wallet.go (2)
clearnode/pkg/rpc/error.go (2)
Error(48-50)Errorf(71-75)clearnode/session_key.go (2)
SessionKey(13-27)SessionKey(29-31)
clearnode/rpc_router_auth.go (4)
clearnode/session_key.go (3)
AddSessionKey(47-110)SessionKey(13-27)SessionKey(29-31)clearnode/auth.go (1)
Policy(48-55)clearnode/signer.go (1)
Allowance(18-21)clearnode/asset.go (1)
GetAllAssets(30-40)
clearnode/session_key.go (1)
clearnode/ledger.go (2)
Entry(19-29)Entry(31-33)
clearnode/app_session_service.go (6)
clearnode/rpc_router_private.go (3)
SubmitAppStateParams(48-54)CreateAppSessionParams(42-46)CloseAppSessionParams(56-60)clearnode/pkg/rpc/api.go (2)
AppSession(556-581)AppSessionIntent(594-594)clearnode/app_session.go (2)
AppSession(12-26)AppSession(28-30)clearnode/ledger.go (2)
NewAccountID(44-50)ErrDebitSourceAccount(14-14)clearnode/rpc.go (1)
RPCErrorf(147-151)clearnode/session_key.go (4)
GetWalletBySigner(113-123)IsSessionKey(126-129)ValidateSessionKeySpending(243-296)UpdateSessionKeyUsage(173-211)
clearnode/session_keys_test.go (5)
clearnode/session_key.go (10)
AddSessionKey(47-110)GetWalletBySigner(113-123)GetSessionKeysByWallet(132-144)UpdateSessionKeyUsage(173-211)SessionKey(13-27)SessionKey(29-31)GetActiveSessionKeysByWallet(147-170)ValidateSessionKeySpending(243-296)GetSessionKeySpending(224-240)GetSessionKeyBySigner(214-221)clearnode/wallet.go (1)
AddSigner(40-116)clearnode/ledger.go (2)
NewAccountID(44-50)GetWalletLedger(56-58)clearnode/rpc_router_private.go (3)
TransferParams(27-31)TransferAllocation(37-40)TransferResponse(139-141)clearnode/pkg/rpc/payload.go (1)
Params(177-177)
clearnode/rpc_router_private.go (4)
clearnode/pkg/rpc/api.go (2)
ListOptions(457-464)Allowance(520-525)clearnode/session_key.go (7)
SessionKey(13-27)SessionKey(29-31)GetWalletBySigner(113-123)IsSessionKey(126-129)ValidateSessionKeySpending(243-296)UpdateSessionKeyUsage(173-211)GetActiveSessionKeysByWallet(147-170)clearnode/signer.go (2)
Allowance(18-21)RecoverAddress(63-81)clearnode/pkg/rpc/payload.go (1)
Params(177-177)
🪛 Gitleaks (8.28.0)
clearnode/docs/API.md
[high] 437-437: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 465-465: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
clearnode/session_keys_test.go
[high] 237-237: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 366-366: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 367-367: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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 (23)
integration/common/databaseUtils.ts (1)
19-19: LGTM! Session keys table added to cleanup.The addition of
session_keysto the cleanup routine correctly aligns with the new session key infrastructure introduced in this PR.clearnode/rpc_router_auth.go (2)
231-240: LGTM! Proper validation and persistence of session keys.The allowance validation is performed before persisting the session key, ensuring only supported assets are accepted. Error logging includes helpful context.
261-285: LGTM! Asset validation implementation is correct.The
validateAllowanceAssetshelper:
- Correctly queries all supported assets
- Builds an efficient lookup map
- Validates each allowance symbol
- Provides clear error messages
The comment about in-memory assets (line 267) is a useful optimization note for future improvement.
clearnode/wallet.go (1)
39-116: LGTM! Cache management and validation logic improved.The updated
AddSignerimplementation correctly addresses the previous review concerns:
Cache updated after transaction commits (lines 110-113): The cache is now updated only after successful transaction completion, preventing inconsistency if the transaction fails.
Self-signer registration works when cache is cold: Lines 79-87 handle idempotent re-registration by checking the database directly when the cache misses, returning
nilif the signer already exists for the same wallet.Cross-validation with session keys: The function properly prevents address conflicts between custody signers and session keys (lines 50-52, 91-95).
The validation flow is robust and maintains consistency between cache and database state.
clearnode/rpc_router.go (1)
82-82: LGTM! Session keys RPC handler registered correctly.The new
get_session_keysmethod is properly registered in the authenticated private group, consistent with other sensitive operations.clearnode/custody.go (1)
298-554: LGTM! All ledger.Record calls updated correctly.All calls to
ledger.Recordhave been updated to passnilfor the newsessionKeyparameter (lines 298, 301, 305, 448, 452, 456, 466, 469, 478, 541, 544, 554). This is appropriate for custody operations, which don't track session keys.clearnode/rpc_router_public_test.go (1)
626-779: LGTM! Test ledger.Record calls updated correctly.All test calls to
ledger.Record(lines 626, 639, 779) have been updated to passnilfor the newsessionKeyparameter, maintaining consistency with the API change.clearnode/custody_test.go (1)
406-1552: LGTM! All custody test ledger.Record calls updated.All test calls to
ledger.Recordthroughout the file have been consistently updated to passnilfor the newsessionKeyparameter, maintaining test compatibility with the updated API.clearnode/rpc_router_private_test.go (2)
216-234: LGTM! Test setup using AddSigner helper.The tests now use the
AddSignerhelper function instead of direct database insertions. This is better because:
- It validates the signer-wallet relationship through production code
- It ensures cache consistency
- Self-signer registration (wallet == signer) is properly handled by the updated AddSigner logic
192-1702: LGTM! All test ledger.Record calls updated correctly.All test calls to
ledger.Recordthroughout the file have been consistently updated to passnilfor the newsessionKeyparameter. The tests maintain their original behavior while complying with the updated API signature.clearnode/rpc_router_private.go (1)
714-751: Good: return only active session keys and hide empty fields.Using GetActiveSessionKeysByWallet and omitempty on optional fields matches the intended API.
clearnode/channel_service_test.go (1)
98-101: LGTM: adapted to WalletLedger.Record(..., nil).All updated call sites pass the new sessionKey argument. Good alignment with the new signature.
clearnode/app_session_service.go (11)
32-48: LGTM: Separation of wallets and signers.The addition of both
rpcWalletsandrpcSignerstoDepositUpdatercorrectly addresses the need to distinguish between participant wallets (for quorum) and actual signers (for session key detection).
91-109: LGTM: Session key detection correctly prioritizes direct wallet signatures.The if-else-if structure (lines 96-109) ensures that when a participant wallet signs directly (line 96), the session key detection in lines 98-109 is skipped. This correctly addresses the previous concern about non-deterministic behavior when both a wallet and its session key sign the same message.
127-134: LGTM: Session key spending validation applied correctly.Session key spending validation is only applied when
sessionKeyAddress != nil, which occurs only when the wallet did not sign directly. This ensures that session key limits are not enforced when the wallet (with full authority) signs.
159-165: LGTM: Session key usage tracked only when actually used.The usage tracking loop only processes session keys that were actually used (populated in
sessionKeysUsedat line 133), and only when they were the authorizing signature (not when wallet signed directly). This correctly addresses previous concerns about misleading usage tracking.
172-251: LGTM: Withdrawal updater correctly uses wallet-based quorum.
WithdrawUpdaternow usesrpcWalletsfor quorum verification (line 190) and does not track session keys, which is correct since session keys control spending (deposits), not withdrawals.
253-327: LGTM: Operate updater correctly uses wallet-based quorum.
OperateUpdaternow usesrpcWalletsfor quorum verification (line 271) and does not track session keys for balance redistributions, which is appropriate.
379-382: Verify intended behavior for zero allocations.Line 380 skips processing for zero allocations (
if alloc.Amount.IsZero() { continue }). This means:
- No signature validation for zero-amount participants
- No session key detection or validation
- Zero-amount participants still appear in
params.AllocationsIs this the intended behavior? If zero allocations are semantically meaningful (e.g., indicating participation without initial deposit), the signature check might still be necessary.
If signature validation is required for all participants (including zero amounts), apply this diff:
- if alloc.Amount.IsZero() { - continue - } if alloc.Amount.IsNegative() { return RPCErrorf(ErrNegativeAllocation+": %s for asset %s", alloc.Amount, alloc.AssetSymbol) } // Check if participant has signed directly or via session key var sessionKeyAddress *string signatureProvided := false // Check direct signature from participant if _, ok := rpcSigners[alloc.ParticipantWallet]; ok { signatureProvided = true } else { // Check if any signer is a session key for this participant for signer := range rpcSigners { if walletForSigner := GetWalletBySigner(signer); walletForSigner == alloc.ParticipantWallet && IsSessionKey(signer) { signatureProvided = true sessionKeyAddress = &signer break } } } - if alloc.Amount.IsPositive() && !signatureProvided { + if !alloc.Amount.IsZero() && !signatureProvided { return RPCErrorf("missing signature for participant %s", alloc.ParticipantWallet) } + if alloc.Amount.IsZero() { + continue + } + // Validate session key spending if applicableThis ensures signature validation happens before skipping zero amounts.
387-416: LGTM: Session key detection in CreateAppSession mirrors DepositUpdater pattern.The session key detection logic (lines 391-403) correctly prioritizes direct wallet signatures over session keys, consistent with the pattern in
DepositUpdater. Validation and tracking (lines 409-415) are correctly applied only when a session key is used.
489-576: LGTM: SubmitAppState correctly routes wallets and signers to updaters.The updated signature (line 489) accepts both
rpcWalletsandrpcSigners, and correctly passes them to updaters. OnlyDepositUpdaterreceives both (line 518), while others receive onlyrpcWallets(lines 520, 522, 524), which is appropriate for their validation needs.
736-774: LGTM: Quorum verification correctly deduplicates participant weights.The
verifyQuorumfunction now correctly usesrpcWallets(line 736) and tracksparticipantsSigned(line 743) to ensure each participant's weight is counted only once (line 754). This addresses the previous critical issue where both a wallet and its session key could inflate the total weight.
578-674: LGTM: CloseApplication correctly uses wallet-based quorum.
CloseApplicationnow acceptsrpcWallets(line 579) and uses it for quorum verification (line 592), ensuring consistent behavior with other operations.
210fb3f to
fead99d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
clearnode/pkg/rpc/client_test.go (1)
215-227: Inconsistent asset symbol casing in test data.Line 219 now uses lowercase "eth", but line 218 uses
testSymbolwhich is "USDC" (uppercase, defined on line 25). If the codebase is normalizing asset symbols to lowercase, consider updating all test asset symbols for consistency.Apply this diff to normalize all asset symbols in this test to lowercase:
t.Run("balances", func(t *testing.T) { balances := rpc.GetLedgerBalancesResponse{ LedgerBalances: []rpc.LedgerBalance{ - {Asset: testSymbol, Amount: decimal.NewFromInt(1000)}, + {Asset: "usdc", Amount: decimal.NewFromInt(1000)}, {Asset: "eth", Amount: decimal.NewFromInt(5)}, }, }Alternatively, update the
testSymbolconstant on line 25 to lowercase "usdc" if lowercase is now the standard across the codebase.clearnode/wallet_test.go (1)
16-29: Critical: Tests reference removed function.The tests call
GetWalletBySigneron lines 26-28 (and also lines 47, 69, 93, 114), but this function has been removed fromwallet.goaccording to the AI summary. The tests will fail with undefined function errors.You need to either:
- Restore the
GetWalletBySignerfunction inwallet.go, or- Update all tests to use the new cache-based lookup pattern
Example of option 2:
- assert.Equal(t, "w1", GetWalletBySigner("alice")) - assert.Equal(t, "w2", GetWalletBySigner("bob")) - assert.Empty(t, GetWalletBySigner("carol")) + w1, ok1 := custodySignerCache.Load("alice") + assert.True(t, ok1) + assert.Equal(t, "w1", w1.(string)) + + w2, ok2 := custodySignerCache.Load("bob") + assert.True(t, ok2) + assert.Equal(t, "w2", w2.(string)) + + _, ok3 := custodySignerCache.Load("carol") + assert.False(t, ok3)clearnode/app_session_service.go (1)
340-466: Session key limits are silently bypassed when called through the RPC router.
CreateAppSessionrelies on therpcSignersmap to spot session-key signatures and runValidateSessionKeySpending, butHandleCreateApplicationnow providesrpcSigners := getWallets(&c.Message), which normalizes every signer to the wallet address. As a result, when a request is signed only with a session key, this function sees the wallet address, never the session key address, sosessionKeyAddressstays nil, the allowance check never runs, and usage is never updated. Tests pass because they callCreateAppSessiondirectly with the actual signer map, but the production RPC path skips caps entirely—letting delegated keys spend without limits.Please propagate the real signer map (e.g., call
c.Message.GetRequestSignersMap()in the router and pass both wallets and signers, similar toHandleSubmitAppState), and adjust this function to use the signer map when looking up session keys. Otherwise a malicious session key can create sessions with unlimited allocations.
🧹 Nitpick comments (1)
clearnode/pkg/rpc/client.go (1)
532-532: Documentation example updated correctly.The change from "USDC" to "usdc" aligns with the lowercase convention adopted in tests and fixtures across the codebase.
For consistency, you may want to update other asset symbol examples in this file (lines 431, 473, 976, 977, 1034, 1104, 1172) to use lowercase "usdc" as well, though this can be deferred given the PR is still WIP.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (28)
clearnode/app_session_service.go(20 hunks)clearnode/app_session_service_test.go(30 hunks)clearnode/channel_service_test.go(4 hunks)clearnode/config/migrations/postgres/20251015000001_add_session_keys.sql(1 hunks)clearnode/custody.go(6 hunks)clearnode/custody_test.go(5 hunks)clearnode/database.go(1 hunks)clearnode/docs/API.md(4 hunks)clearnode/ledger.go(2 hunks)clearnode/main.go(1 hunks)clearnode/pkg/rpc/client.go(1 hunks)clearnode/pkg/rpc/client_test.go(1 hunks)clearnode/rpc_router.go(1 hunks)clearnode/rpc_router_auth.go(3 hunks)clearnode/rpc_router_private.go(12 hunks)clearnode/rpc_router_private_test.go(27 hunks)clearnode/rpc_router_public_test.go(3 hunks)clearnode/rpc_router_test.go(2 hunks)clearnode/session_key.go(1 hunks)clearnode/session_keys_test.go(1 hunks)clearnode/wallet.go(3 hunks)clearnode/wallet_test.go(2 hunks)docker-compose.yml(1 hunks)integration/common/databaseUtils.ts(1 hunks)integration/common/testHelpers.ts(1 hunks)integration/tests/lifecycle_nitrorpc_v02.test.ts(4 hunks)integration/tests/lifecycle_nitrorpc_v04.test.ts(7 hunks)integration/tests/nitrorpc_v04.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- integration/tests/nitrorpc_v04.test.ts
- docker-compose.yml
🚧 Files skipped from review as they are similar to previous changes (7)
- integration/common/databaseUtils.ts
- clearnode/database.go
- clearnode/rpc_router.go
- clearnode/rpc_router_auth.go
- clearnode/config/migrations/postgres/20251015000001_add_session_keys.sql
- clearnode/session_key.go
- clearnode/ledger.go
🧰 Additional context used
🧬 Code graph analysis (11)
integration/tests/lifecycle_nitrorpc_v04.test.ts (1)
integration/common/testHelpers.ts (1)
getLedgerBalances(130-145)
clearnode/pkg/rpc/client_test.go (1)
clearnode/pkg/rpc/api.go (1)
Asset(508-517)
clearnode/rpc_router_private_test.go (2)
clearnode/wallet.go (1)
AddSigner(40-116)clearnode/ledger.go (1)
GetWalletLedger(56-58)
integration/tests/lifecycle_nitrorpc_v02.test.ts (1)
integration/common/testHelpers.ts (1)
getLedgerBalances(130-145)
clearnode/app_session_service_test.go (5)
clearnode/wallet.go (1)
AddSigner(40-116)clearnode/ledger.go (2)
NewAccountID(44-50)GetWalletLedger(56-58)clearnode/rpc_router_private.go (3)
CreateAppSessionParams(42-46)AppAllocation(62-66)SubmitAppStateParams(48-54)clearnode/pkg/rpc/api.go (8)
AppDefinition(540-553)VersionNitroRPCv0_2(31-31)AppAllocation(584-591)Version(27-27)Allowance(520-525)Asset(508-517)VersionNitroRPCv0_4(33-33)AppSessionIntentDeposit(600-600)clearnode/session_key.go (2)
AddSessionKey(47-110)GetSessionKeySpending(224-240)
clearnode/custody.go (1)
clearnode/ledger.go (1)
GetWalletLedger(56-58)
clearnode/rpc_router_test.go (3)
clearnode/ledger.go (2)
Entry(19-29)Entry(31-33)clearnode/session_key.go (2)
SessionKey(13-27)SessionKey(29-31)clearnode/wallet.go (2)
SignerWallet(12-15)SignerWallet(17-19)
clearnode/app_session_service.go (1)
clearnode/session_key.go (4)
GetWalletBySigner(113-123)IsSessionKey(126-129)ValidateSessionKeySpending(243-296)UpdateSessionKeyUsage(173-211)
clearnode/wallet.go (2)
clearnode/pkg/rpc/error.go (2)
Error(48-50)Errorf(71-75)clearnode/session_key.go (2)
SessionKey(13-27)SessionKey(29-31)
clearnode/rpc_router_private.go (5)
clearnode/pkg/rpc/api.go (2)
ListOptions(457-464)Allowance(520-525)clearnode/session_key.go (7)
SessionKey(13-27)SessionKey(29-31)GetWalletBySigner(113-123)IsSessionKey(126-129)ValidateSessionKeySpending(243-296)UpdateSessionKeyUsage(173-211)GetActiveSessionKeysByWallet(147-170)clearnode/signer.go (2)
Allowance(18-21)RecoverAddress(63-81)clearnode/rpc_node.go (1)
RPCContext(252-266)clearnode/pkg/rpc/payload.go (1)
Params(177-177)
clearnode/session_keys_test.go (5)
clearnode/session_key.go (10)
AddSessionKey(47-110)GetWalletBySigner(113-123)GetSessionKeysByWallet(132-144)UpdateSessionKeyUsage(173-211)SessionKey(13-27)SessionKey(29-31)GetActiveSessionKeysByWallet(147-170)ValidateSessionKeySpending(243-296)GetSessionKeySpending(224-240)GetSessionKeyBySigner(214-221)clearnode/wallet.go (1)
AddSigner(40-116)clearnode/ledger.go (2)
NewAccountID(44-50)GetWalletLedger(56-58)clearnode/rpc_router_private.go (3)
TransferParams(27-31)TransferAllocation(37-40)TransferResponse(139-141)clearnode/pkg/rpc/payload.go (1)
Params(177-177)
🪛 Gitleaks (8.28.0)
clearnode/docs/API.md
[high] 437-437: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 465-465: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
clearnode/session_keys_test.go
[high] 237-237: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 366-366: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 367-367: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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 (22)
integration/tests/lifecycle_nitrorpc_v04.test.ts (9)
128-128: LGTM! Asset identifier normalized to lowercase.The change from 'USDC' to 'usdc' aligns with the broader test standardization effort across the integration suite.
143-154: LGTM! Allocations use normalized asset identifiers.Consistent lowercase 'usdc' identifiers in allocation objects.
166-177: LGTM! Allocations use normalized asset identifiers.Consistent with the test-wide asset identifier standardization.
191-202: LGTM! Allocations use normalized asset identifiers.Top-up allocations correctly use lowercase 'usdc'.
206-218: LGTM! Ledger balance assertions updated.Both Alice's and Bob's ledger balance checks now expect lowercase 'usdc'.
221-235: LGTM! Withdrawal allocations use normalized identifiers.Partial withdrawal allocations consistently use lowercase 'usdc'.
237-249: LGTM! Post-withdrawal assertions updated.Ledger balance assertions for both participants use normalized 'usdc'.
252-266: LGTM! Close session allocations normalized.Final allocations for app session closure use lowercase 'usdc'.
275-287: LGTM! Final balance assertions updated.Both participants' final ledger balance checks expect lowercase 'usdc'.
integration/tests/lifecycle_nitrorpc_v02.test.ts (4)
120-125: LGTM! Initial ledger balance assertion normalized.Asset identifier correctly updated to lowercase 'usdc'.
139-153: LGTM! State update allocations normalized.Allocation objects consistently use lowercase 'usdc'.
162-176: LGTM! Close session allocations normalized.Final allocations use consistent lowercase 'usdc' identifiers.
185-197: LGTM! Final ledger balance assertions updated.Both Alice's and Bob's final balance checks expect lowercase 'usdc'.
integration/common/testHelpers.ts (1)
90-101: Lowercase asset identifiers have been consistently applied throughout the codebase.The verification confirms that all asset identifier references in test helpers and integration tests use lowercase 'usdc' without any uppercase variants in functional code. However, the backend API compatibility cannot be definitively verified through static code analysis alone, as backend code and API specifications are not visible in this codebase.
The changes are consistent within the client-side codebase, but manual verification is needed to confirm that the backend API and database queries correctly handle and accept lowercase asset identifiers.
clearnode/wallet.go (1)
39-116: LGTM! Cache consistency issue resolved.The refactored
AddSignerimplementation properly addresses the previous review comments:
- Cache updates now occur only after successful transaction commit (lines 110-115)
- Early validation checks reduce unnecessary database queries
- Transaction boundaries are well-defined
The logic correctly prevents:
- Wallet addresses from being used as signers
- Signers from being associated with multiple wallets
- Conflicts between custody signers and session keys
clearnode/main.go (1)
36-44: LGTM! Cache initialization properly updated.The initialization sequence correctly loads both custody signers and session keys with appropriate error handling. The error messages accurately reflect the operations being performed.
clearnode/rpc_router_test.go (1)
29-29: LGTM! SessionKey model added to test migrations.The SessionKey model is correctly added to AutoMigrate for both SQLite and PostgreSQL test setups, ensuring the session_keys table is created during tests.
clearnode/custody.go (1)
298-307: LGTM! Ledger.Record calls updated for new signature.All
ledger.Recordcalls throughout the custody event handlers have been consistently updated to passnilfor the newsessionKeyparameter. This is appropriate since custody operations (deposits/withdrawals through blockchain events) don't involve session keys.clearnode/rpc_router_public_test.go (1)
626-627: LGTM! Test updated for new Record signature.The test correctly passes
nilfor the session key parameter in allledger.Recordcalls, maintaining consistency with the updated API throughout the test file.clearnode/rpc_router_private_test.go (2)
216-216: LGTM! Tests properly use AddSigner helper.The test now correctly uses the
AddSignerhelper function instead of direct database operations. This ensures signer setup goes through proper validation and cache management, making tests more robust and aligned with production code paths.
192-192: LGTM! Test updated for new Record signature.Consistent with the broader changes, all
ledger.Recordcalls throughout this test file now correctly passnilfor the session key parameter.clearnode/custody_test.go (1)
406-407: LGTM! Test updated for new Record signature.All custody test cases have been systematically updated to pass
nilfor the session key parameter inledger.Recordcalls, maintaining consistency with the API changes.
4b43329 to
cfc17fc
Compare
30545c9 to
9ded309
Compare
|
The PR is rebased on master |
|
@nksazonov waiting for your review |
nksazonov
left a comment
There was a problem hiding this comment.
Resolved all the previous comments - well done!
Posting some final thoughts.
I still think we should NOT make any session key response fields optional.
clearnode/config/migrations/postgres/20251015000001_add_session_keys.sql
Outdated
Show resolved
Hide resolved
nksazonov
left a comment
There was a problem hiding this comment.
One more question regarding the migration
clearnode/config/migrations/postgres/20251015000001_add_session_keys.sql
Outdated
Show resolved
Hide resolved
nksazonov
left a comment
There was a problem hiding this comment.
LGTM!
Thanks for determination to complete this enormous chunk of work!
Well done!
Summary by CodeRabbit
Release Notes
New Features
get_session_keysRPC endpoint to retrieve active session keys and usage metricsUpdates
app_nameparameter renamed toapplicationAppAllocationfield renamed fromParticipantWallettoParticipantAppSessionandAppDefinitionnow include application name fieldImprovements