feat: get_channels RPC, GetAppSessions fix, compat SDK & tests#573
Merged
feat: get_channels RPC, GetAppSessions fix, compat SDK & tests#573
Conversation
Add full-stack support for listing user channels with optional filtering and pagination, enabling apps to retrieve all channels including closed ones. Clearnode (Go): - Add GetUserChannels to database interface with status/asset filters and pagination - Implement GetUserChannels query using GORM with dynamic WHERE clauses - Add RPC handler for channels.v1.get_channels with request validation - Register the new method in rpc_router - Add mock implementation for testing SDK (TypeScript): - Add getChannels() method to Client, wrapping channelsV1GetChannels RPC - Transform RPC response into typed Channel[] with PaginationMetadata Tested end-to-end with local clearnode Docker setup against Sepolia.
- channelStatusFromString now returns error on unknown status instead of silently defaulting to Void - Centralise default limit=100 in handler before store call, removing the duplicate default in the store layer - Remove redundant limit>0 guards (limit is guaranteed non-zero after the handler default is applied) - Remove unsupported sort field from SDK PaginationParams and getChannels request mapping (backend does not implement sort)
The GetAppSessions query used SELECT DISTINCT app_sessions_v1.id with ORDER BY created_at, which PostgreSQL rejects because created_at is not in the SELECT list. Replace with a subquery (WHERE id IN (SELECT id ...)) to filter by participant without conflicting with ORDER BY.
Compatibility layer bridging v0.5.3 API to v1.0.0 TS SDK, enabling minimal-diff migrations for existing apps. Includes client, auth, signers, RPC helpers, event polling, config, and comprehensive documentation with migration guides. WIP: beatwav and nitrolite example app integrations pending testing.
Handler tests (6): success, status filter, pagination, empty result, missing wallet validation, and store error propagation. Database tests (6): multi-channel retrieval, status filtering, asset filtering, pagination limiting, empty result for unknown user, and invalid status filter rejection.
6 tasks
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Channels GetChannels API with optional status/asset/channel_type filters and offset pagination, store and DB support (GetUserChannels), RPC wiring and SDK client method, unit and DB tests, and a large Nitrolite compat SDK providing a compat client, types, signers, stubs, docs, and examples. Changes
Sequence Diagram(s)sequenceDiagram
participant SDK as SDK Client (TS)
participant RPC as RPC Router
participant Handler as channel_v1.Handler
participant Store as Store (DBStore)
participant DB as Database
SDK->>RPC: channelsV1GetChannels(request)
RPC->>Handler: invoke GetChannels
Handler->>Store: GetUserChannels(wallet, status, asset, type, limit, offset)
Store->>DB: COUNT(*) and SELECT rows with WHERE, ORDER BY created_at DESC LIMIT/OFFSET
DB-->>Store: rows, totalCount
Store-->>Handler: []core.Channel, totalCount
Handler-->>RPC: ChannelsV1GetChannelsResponse (channels + pagination)
RPC-->>SDK: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Summary of ChangesHello @ihsraham, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the clearnode's API by introducing a robust method for fetching user channels, which was a missing feature in the v1.0.0 migration. It also addresses a crucial database query issue affecting app session retrieval in PostgreSQL, ensuring data consistency and application stability. Furthermore, a new compatibility SDK is provided to streamline the transition for dApps upgrading from the previous SDK version, minimizing the development overhead associated with protocol changes. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
The pull request introduces a new get_channels RPC endpoint and a compatibility layer for the TypeScript SDK to facilitate migration from v0.5.3 to v1.0.0. The Go implementation for the new endpoint is well-structured and includes thorough tests. The PostgreSQL fix for GetAppSessions correctly resolves a known issue with DISTINCT and ORDER BY. However, the compatibility SDK contains several critical logic errors, particularly regarding the handling of asset amounts (mixing raw units and human-readable decimals) and a hardcoded chain ID in the EIP-712 domain. Additionally, there are some pagination and efficiency concerns in both the Go and TypeScript code that should be addressed to ensure production readiness.
High priority: - Use wallet chain ID instead of hardcoded chainId=1 in EIP-712 domain - Convert raw-unit amounts to human-readable decimals before passing to v1 SDK in submitAppState, closeAppSession, and transfer - Convert v1 human-readable amounts back to raw units in getAppSessionsList allocations for v0.5.3 compatibility Medium priority: - Cap get_channels pagination limit to 1000 to prevent abuse - Use uint64 for pageCount calculation to prevent uint32 overflow - Add lazy-load retry for asset map (ensureAssets on every resolve) - Replace cross-decimal balance sum with per-asset balances in AccountInfo - Optimize closeChannel to query open channels instead of all assets
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (16)
clearnode/store/database/app_session.go (2)
101-105: PostgreSQL DISTINCT/ORDER BY fix looks correct — minor simplification available.GORM v2 automatically generates subqueries when a
*gorm.DBis passed as a parameter, and theWhere("... > (?)", db.Table(...).Select(...))form with parenthesised?is the documented idiomatic pattern — soWhere("id IN (?)", subQuery)is correct here.The fix correctly removes
SELECT DISTINCT … ORDER BYfrom within a single SELECT (the PostgreSQL SQLSTATE 42P10 trigger) and pushes deduplication into the SQL engine viaIN, which handles duplicates implicitly.That said, the subquery joins back through
app_sessions_v1purely to project itsidcolumn — an extra table hop that isn't needed.AppParticipantV1already holdsapp_session_id, so the join can be eliminated:♻️ Simplified subquery (drop the unnecessary JOIN)
- subQuery := s.db.Model(&AppSessionV1{}). - Select("app_sessions_v1.id"). - Joins("JOIN app_session_participants_v1 ON app_sessions_v1.id = app_session_participants_v1.app_session_id"). - Where("app_session_participants_v1.wallet_address = ?", strings.ToLower(*participant)) + subQuery := s.db.Model(&AppParticipantV1{}). + Select("app_session_id"). + Where("wallet_address = ?", strings.ToLower(*participant)) query = query.Where("id IN (?)", subQuery)This generates
WHERE id IN (SELECT app_session_id FROM app_session_participants_v1 WHERE wallet_address = ?), which is equivalent and avoids the join entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/store/database/app_session.go` around lines 101 - 105, The subquery currently builds from AppSessionV1 and joins to app_session_participants_v1 unnecessarily; instead construct subQuery from the AppParticipantV1 model selecting app_session_id with the wallet_address filter (use strings.ToLower(*participant)) and keep the outer filter query = query.Where("id IN (?)", subQuery); update references to subQuery, AppSessionV1, AppParticipantV1 and participant accordingly to remove the JOIN and select the app_session_id directly.
101-105: PostgreSQL DISTINCT/ORDER BY fix looks correct.The subquery approach sidesteps the
SELECT DISTINCT … ORDER BYincompatibility (SQLSTATE 42P10) entirely. SQLINimplicitly deduplicates, so no explicitDISTINCTis needed, and the outer query'sORDER BY created_at DESCremains unaffected.That said, the subquery joins
app_sessions_v1toapp_session_participants_v1purely to project backapp_sessions_v1.id—a JOIN that can be eliminated by queryingAppParticipantV1directly:♻️ Simplified subquery (avoids the unnecessary JOIN)
- subQuery := s.db.Model(&AppSessionV1{}). - Select("app_sessions_v1.id"). - Joins("JOIN app_session_participants_v1 ON app_sessions_v1.id = app_session_participants_v1.app_session_id"). - Where("app_session_participants_v1.wallet_address = ?", strings.ToLower(*participant)) + subQuery := s.db.Model(&AppParticipantV1{}). + Select("app_session_id"). + Where("wallet_address = ?", strings.ToLower(*participant)) query = query.Where("id IN (?)", subQuery)Note: GORM v2's
Where("id IN (?)", subQuery)correctly renders a*gorm.DBvalue as an inline subquery—this is the idiomatic v2 pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/store/database/app_session.go` around lines 101 - 105, The subquery currently joins AppSessionV1 to app_session_participants_v1 just to return session IDs; replace that JOIN by building the subquery from AppParticipantV1 directly (e.g. s.db.Model(&AppParticipantV1{}).Select("app_session_id").Where("wallet_address = ?", strings.ToLower(*participant))) and then use query = query.Where("id IN (?)", subQuery) so you avoid the unnecessary JOIN while keeping the outer ORDER BY and leveraging GORM v2's inline subquery rendering.sdk/compat/tsconfig.prod.json (1)
4-4:declaration: trueis redundant — the base config already sets it.Removing it from
tsconfig.prod.jsonreduces noise without changing behavior.♻️ Proposed cleanup
{ "extends": "./tsconfig.json", "compilerOptions": { - "declaration": true, "declarationMap": false, "sourceMap": false, "removeComments": true, "inlineSources": false } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/compat/tsconfig.prod.json` at line 4, Remove the redundant "declaration": true setting from tsconfig.prod.json: locate the JSON key "declaration" in tsconfig.prod.json (the explicit declaration property) and delete that entry since the base config already provides it, leaving the prod config to inherit the declaration setting.clearnode/api/channel_v1/interface.go (1)
61-62: Same pagination convention inconsistency asclearnode/store/database/interface.goline 30.For consistency with how pagination is expressed elsewhere in the
Storeinterface (and the underlyingDatabaseStore), consider using*core.PaginationParamsand returningcore.PaginationMetadatahere as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/api/channel_v1/interface.go` around lines 61 - 62, The GetUserChannels method signature in the channel API interface uses raw limit/offset which is inconsistent with the Store/DatabaseStore pagination convention; update the GetUserChannels declaration to accept a *core.PaginationParams instead of limit and offset and change its return to include core.PaginationMetadata (i.e., GetUserChannels(wallet string, status *string, asset *string, params *core.PaginationParams) ([]core.Channel, core.PaginationMetadata, error)) so it matches the Store interface and DatabaseStore implementations (update callers and implementations of GetUserChannels accordingly).sdk/ts/src/client.ts (1)
1075-1095: Pagination API inconsistency withgetTransactions/getAppSessions.
getTransactions(line 1044) andgetAppSessions(line 1194) both accept{ page, pageSize }and computeoffset/limitinternally.getChannelsexposes the lower-levelcore.PaginationParamsdirectly. SDK consumers now face two different pagination shapes for equivalent operations.♻️ Proposed alignment
async getChannels( wallet: Address, - options?: { status?: string; asset?: string; pagination?: core.PaginationParams } + options?: { status?: string; asset?: string; page?: number; pageSize?: number } ): Promise<{ channels: core.Channel[]; metadata: core.PaginationMetadata }> { const req: API.ChannelsV1GetChannelsRequest = { wallet, status: options?.status, asset: options?.asset, - pagination: options?.pagination - ? { - offset: options.pagination.offset, - limit: options.pagination.limit, - } - : undefined, + pagination: options?.page && options?.pageSize ? { + offset: (options.page - 1) * options.pageSize, + limit: options.pageSize, + } : undefined, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts/src/client.ts` around lines 1075 - 1095, getChannels currently accepts core.PaginationParams (offset/limit) while getTransactions and getAppSessions accept {page, pageSize}; update getChannels signature and request construction to match the higher-level pagination shape for consistency: change getChannels(wallet, options?: { status?: string; asset?: string; pagination?: { page?: number; pageSize?: number } }) and inside getChannels convert page/pageSize to offset = (page-1)*pageSize and limit = pageSize when building the API.ChannelsV1GetChannelsRequest; keep transformChannel and transformPaginationMetadata usage unchanged and ensure any callers/tests are updated to the new pagination shape (or add a thin adapter if you must preserve backward compatibility with core.PaginationParams).clearnode/store/database/interface.go (1)
53-54:GetUserChannelspagination style diverges fromGetUserTransactions.
GetUserTransactions(line 30) takes*core.PaginationParamsand returns the richercore.PaginationMetadata, whileGetUserChannelsuses flatlimit, offset uint32and returns a bareuint32total count. The calling handler must reconstruct pagination metadata manually. Consider aligning signatures for a consistent persistence interface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/store/database/interface.go` around lines 53 - 54, The GetUserChannels method uses flat limit/offset and a bare uint32 total which diverges from GetUserTransactions; update the GetUserChannels signature to accept *core.PaginationParams (like GetUserTransactions) and return the richer pagination result (e.g., *core.PaginationMetadata or the same type returned by GetUserTransactions) along with []core.Channel and error, then update all implementations and call sites to construct/consume core.PaginationParams and the metadata instead of separate limit/offset and total count.sdk/compat/package.json (1)
43-48:@types/node@^25.3.0overreaches the declared engine minimum (>=20.0.0).
@types/nodemajor and minor version conventions dictate that they must match your Node.js environment's major and minor version. Withengines.node >= 20.0.0, developers running on Node.js 20 or 22 will have type definitions for Node.js 25 APIs that are not available in their runtime, risking false-confidence type errors.Consider pinning to the minimum supported major:
diff
- "@types/node": "^25.3.0", + "@types/node": "^20.0.0",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/compat/package.json` around lines 43 - 48, The devDependency "@types/node" is set to "^25.3.0" which is incompatible with the declared engines.node >=20.0.0; change the entry in package.json devDependencies ("@types/node") to a version that matches the minimum supported Node major (e.g., "^20.0.0" or a 20.x.x pin) so the type definitions align with Node 20, then run your package manager to update the lockfile; edit the package.json devDependencies block where "@types/node" appears to make this change.clearnode/store/database/channel_test.go (1)
765-784: Consider adding an offset/page-2 sub-test.The pagination test verifies that
limit=2returns 2 rows out of 5 with the correct total, but no sub-test exercisesoffset > 0to confirm the result window is correctly shifted. A case likelimit=2, offset=2should return channels 3–4 andtotal=5, which would also validate the page-number metadata computed in the handler.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/store/database/channel_test.go` around lines 765 - 784, Add a sub-test to the existing "Success - Pagination limits results" test that calls store.GetUserChannels with limit=2 and offset=2 to verify pagination shifting; after creating the five channels via NewDBStore and CreateChannel, call GetUserChannels("0xuser_pg", nil, nil, 2, 2), assert no error, assert the returned slice length is 2, assert total equals uint32(5), and optionally assert the returned ChannelIDs match the expected "0xch_pg_2" and "0xch_pg_3" to ensure the window is correctly shifted.clearnode/api/channel_v1/get_channels.go (1)
39-51: Read-only handler wrapped in a write transaction.
GetChannelsonly reads data, butuseStoreInTxexecutes it insideExecuteInTransaction(a write transaction). For a high-frequency query endpoint this adds unnecessary lock acquisition overhead. Consider either adding a read-only store accessor or usingdbStoredirectly without a transaction for read-only handlers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/api/channel_v1/get_channels.go` around lines 39 - 51, The GetChannels handler is performing a read-only operation inside useStoreInTx (which invokes ExecuteInTransaction / a write transaction); change it to avoid write transactions by either calling the store read method directly (replace the useStoreInTx block and call h.dbStore.GetUserChannels(req.Wallet, req.Status, req.Asset, limit, offset) and handle the returned channels/totalCount/error) or add a dedicated read-only helper (e.g., useStoreReadOnly that wraps an ExecuteReadOnlyTransaction / read-only accessor) and call Store.GetUserChannels inside that helper; update error handling to behave exactly like before but without acquiring a write transaction.sdk/compat/examples/lifecycle.ts (1)
52-56: Hardcodedsepoliachain may conflict withCHAIN_IDenv var.The wallet client is always created with the
sepoliachain config, butCHAIN_IDcan be overridden to a different chain. For an example file this is understandable, but a brief comment noting this limitation would help users who copy-paste the example for other networks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/compat/examples/lifecycle.ts` around lines 52 - 56, The example hardcodes the sepolia chain when creating walletClient via createWalletClient({ chain: sepolia, ... }), which can conflict if callers set a different CHAIN_ID; update the example to either derive the chain from the CHAIN_ID env var (use a simple mapping or helper to convert CHAIN_ID to the correct chain constant) or add a one-line comment above the createWalletClient call explicitly stating that sepolia is hardcoded for the example and should be replaced when using a different CHAIN_ID so users who copy-paste don’t accidentally target the wrong network.clearnode/api/channel_v1/get_channels_test.go (1)
16-34: Ignored error fromNewChannelDefaultSignerin test helper.Line 19 discards the error from
core.NewChannelDefaultSigner. If the mock setup ever changes and this starts returning an error, all tests usingnewGetChannelsHandlerwill silently get a nilnodeSignerand panic with an unhelpful stack trace. Consider acceptingt *testing.Tand usingrequire.NoError.🔧 Proposed fix
-func newGetChannelsHandler(mockTxStore *MockStore) *Handler { +func newGetChannelsHandler(t *testing.T, mockTxStore *MockStore) *Handler { + t.Helper() mockAssetStore := new(MockAssetStore) mockSigner := NewMockSigner() - nodeSigner, _ := core.NewChannelDefaultSigner(mockSigner) + nodeSigner, err := core.NewChannelDefaultSigner(mockSigner) + require.NoError(t, err) nodeAddress := mockSigner.PublicKey().Address().String()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/api/channel_v1/get_channels_test.go` around lines 16 - 34, The helper newGetChannelsHandler ignores the error returned by core.NewChannelDefaultSigner causing silent nil nodeSigner; change newGetChannelsHandler to accept t *testing.T, call nodeSigner, err := core.NewChannelDefaultSigner(mockSigner) and assert require.NoError(t, err) (or t.Fatalf) before using nodeSigner, and update all test callers to pass the testing.T from their tests so the helper fails loudly if signer creation errors; refer to newGetChannelsHandler and core.NewChannelDefaultSigner when making the change.sdk/compat/src/rpc.ts (1)
61-64: Parse stubs will throw on invalid JSON, unlikeparseAnyRPCResponse.
parseAnyRPCResponsewrapsJSON.parsein try/catch, but all theparse*Responsestubs (e.g.,parseGetChannelsResponse, etc.) do not. If any remaining callsite passes non-JSON input, these will throw an unhandled error. Since these are documented no-op stubs and callers should migrate toNitroliteClientmethods, this is low risk — but worth noting for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/compat/src/rpc.ts` around lines 61 - 64, The parseGetChannelsResponse stub currently calls JSON.parse(raw) directly and will throw on invalid JSON; update parseGetChannelsResponse to mirror parseAnyRPCResponse by wrapping JSON.parse in a try/catch and returning the safe default shape ({ params: { channels: [] } }) when parsing fails or the expected path is missing; locate the function parseGetChannelsResponse and adjust the parsing block to catch any JSON errors (optionally log or ignore) and return the no-op default instead of allowing an exception to propagate.sdk/compat/src/types.ts (1)
95-104:RPCLedgerEntryandLedgerEntryhave identical shapes.Both interfaces define the same fields (
id,account_id,account_type,asset,participant,credit,debit,created_at). If they're intentionally kept separate for wire-format vs. client-response clarity, consider a brief comment noting the relationship to prevent accidental divergence during future changes.Also applies to: 306-315
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/compat/src/types.ts` around lines 95 - 104, RPCLedgerEntry and LedgerEntry currently have identical shapes; locate the RPCLedgerEntry and LedgerEntry interfaces and either replace one with a type alias (e.g., "export type RPCLedgerEntry = LedgerEntry" or vice versa) to prevent accidental divergence, or add a short comment above both definitions explaining they intentionally mirror each other (wire-format vs client-response) so future maintainers don't change one without the other; update both occurrences (the one at RPCLedgerEntry and the duplicate at lines referenced around LedgerEntry) accordingly.sdk/compat/src/client.ts (3)
569-591: Magic number3for close intent — useINTENT_MAPor a named constant.Line 579 uses
intent: 3directly, while aINTENT_MAPis already defined at lines 605-610 withclose: 3. Consider reusing that to keep intent values consistent and readable.Proposed fix
const appUpdate = { appSessionId, - intent: 3, // Close + intent: NitroliteClient.INTENT_MAP['close'], version: session.version + 1n,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/compat/src/client.ts` around lines 569 - 591, Replace the magic literal intent: 3 in closeAppSession with the named intent from the existing map; reference INTENT_MAP.close (or introduce a local constant like INTENT_CLOSE = INTENT_MAP.close) and use that instead in the appUpdate object so intent values are readable and consistent with the rest of the file (change usage in closeAppSession to use INTENT_MAP.close or the new INTENT_CLOSE identifier).
262-271: Double classification of errors is redundant and obscures the logic.Line 266 checks
!(err instanceof AllowanceError)and then!(NitroliteClient.classifyError(err) instanceof AllowanceError). SinceclassifyErroralready handlesAllowanceErrorinstances (they pass through at line 259), the firstinstanceofcheck is redundant. Additionally, iferris not anAllowanceErrorand classification also doesn't produce one, the code throws the classified error — which is correct. Simplify to a single classification call.Simplified version
private async checkpointWithApproval(symbol: string, chainId: bigint, tokenAddress: string): Promise<any> { try { return await this.innerClient.checkpoint(symbol); } catch (err) { - if (!(err instanceof AllowanceError) && !(NitroliteClient.classifyError(err) instanceof AllowanceError)) throw NitroliteClient.classifyError(err); + const classified = NitroliteClient.classifyError(err); + if (!(classified instanceof AllowanceError)) throw classified; console.log('[compat] Allowance insufficient, requesting token approval…'); await this.innerClient.approveToken(chainId, tokenAddress, NitroliteClient.MAX_UINT256); return await this.innerClient.checkpoint(symbol); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/compat/src/client.ts` around lines 262 - 271, In checkpointWithApproval, the error handling redundantly checks both err instanceof AllowanceError and NitroliteClient.classifyError(err) instanceof AllowanceError; replace this with a single classification: call NitroliteClient.classifyError(err) once, store the result, and if that classified error is not an AllowanceError re-throw it; otherwise proceed to log the allowance message, call this.innerClient.approveToken(chainId, tokenAddress, NitroliteClient.MAX_UINT256) and retry this.innerClient.checkpoint(symbol). Ensure references to AllowanceError, NitroliteClient.classifyError, checkpointWithApproval, innerClient.checkpoint, innerClient.approveToken, and NitroliteClient.MAX_UINT256 are used to locate the change.
277-283:setHomeBlockchainerrors are silently swallowed.Line 279 uses
.catch(() => {})to suppress all errors fromsetHomeBlockchain. If this call fails for a legitimate reason (e.g., network error, invalid symbol), the subsequentdepositwill likely fail with a confusing error. The same pattern appears at line 333. Consider at least logging the error.Proposed fix
- await this.innerClient.setHomeBlockchain(symbol, chainId).catch(() => {}); + await this.innerClient.setHomeBlockchain(symbol, chainId).catch((err) => { + console.warn('[compat] setHomeBlockchain failed, continuing:', (err as Error).message); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/compat/src/client.ts` around lines 277 - 283, In deposit (and the other occurrence) do not silently swallow errors from this.innerClient.setHomeBlockchain; instead catch the error, log a descriptive message including symbol and chainId and the error object (e.g., via this.logger.error or equivalent) so failures are visible, and decide whether to rethrow or continue based on expected behavior — update the setHomeBlockchain call in deposit and the similar call at the other site to use a .catch(err => { this.logger.error(`setHomeBlockchain failed for ${symbol}@${chainId}`, err); /* optionally: throw err; */ }) pattern so errors are recorded with context before proceeding to innerClient.deposit and checkpointWithApproval.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clearnode/api/channel_v1/get_channels.go`:
- Around line 21-34: Clamp the requested pagination limit to an enforced maximum
to prevent DoS via huge LIMIT values: introduce a maxLimit constant (e.g.,
maxLimit uint32 = 1000) in get_channels.go and when reading req.Pagination.Limit
(referencing variables limit, defaultLimit, and req.Pagination.Limit) replace
the current assignment with logic that sets limit to defaultLimit if zero or
nil, then if limit > maxLimit set limit = maxLimit (and optionally ensure limit
>= 1). Update any downstream code that uses limit accordingly so GORM never
receives an unbounded LIMIT.
In `@sdk/compat/package.json`:
- Around line 13-18: The "clean" npm script currently calls "rimraf dist" but
rimraf is not declared in devDependencies; fix by adding rimraf (e.g., "rimraf":
"^5.0.0") to devDependencies in package.json or replace the "clean" script value
with a Node-native command like "node -e \"fs.rmSync('dist', { recursive: true,
force: true })\"" (or an equivalent cross-platform JS snippet) so the "clean"
script works without an undeclared dependency; update the "clean" script or
devDependencies accordingly and ensure package.json's "scripts" block still
contains "clean": "..." after the change.
In `@sdk/compat/README.md`:
- Line 7: The README contains bare fenced code blocks triggering markdownlint
MD040; update the two raw triple-backtick blocks that contain the ASCII diagram
and the env-var example to use a language specifier "text" (i.e., change the
fences to ```text) so the diagram and the NEXT_PUBLIC_BLOCKCHAIN_RPCS
environment line are marked as plain text.
- Line 53: The README heading currently reads "Initialise the client" (British
spelling) but code identifiers and badges use American spelling (e.g.,
NotInitializedError); change the heading text to "Initialize the client" and
scan the README for any other "initialise" occurrences to replace with
"initialize" so spelling is consistent with NotInitializedError and the rest of
the codebase.
In `@sdk/compat/src/client.ts`:
- Around line 651-655: The transfer method currently performs sequential
innerClient.transfer calls and will stop on the first failure, causing partial
execution; update the async transfer(destination: Address, allocations:
TransferAllocation[]) in client.ts to (1) track which allocations completed
(e.g., collect completed indexes or allocations) while calling
innerClient.transfer, (2) wrap each await in a try/catch so on error you throw a
new error that includes the original error plus structured metadata about which
allocations succeeded and which failed, and (3) add/update the method’s
docstring to explicitly document that transfers are not atomic and callers may
receive partial success information in the thrown error; reference the transfer
method, innerClient.transfer calls and the TransferAllocation shape so reviewers
can find the changes.
- Around line 197-201: getTokenDecimals currently returns a silent default of 6
when a token key is missing, which can produce incorrect amounts; change
getTokenDecimals (and its usage points like formatAmount and parseAmount) to
surface the error instead of silently defaulting: look up the token in
assetsByToken inside getTokenDecimals, and if not found either throw an Error
(consistent with resolveToken) or at minimum emit a warning via the logger and
then fallback; prefer throwing to force callers to handle unknown tokens—update
any callers/tests that assumed the 6-decimal fallback accordingly and reference
getTokenDecimals, assetsByToken, formatAmount, parseAmount, and resolveToken
when making the change.
- Around line 252-260: The allowance check in classifyError is too broad and
matches positive messages; update the condition in classifyError (the branch
that currently checks lower.includes('allowance') &&
lower.includes('sufficient')) to detect negative allowance phrases instead —
e.g. match patterns like "insufficient allowance", "allowance insufficient", or
"allowance not sufficient" (or generally the presence of 'allowance' together
with 'insufficient'/'not sufficient') and only then return new
AllowanceError(msg); leave other branches (UserRejectedError,
InsufficientFundsError, NotInitializedError) unchanged.
- Around line 521-535: In getAssetsList(), the symbol field is incorrectly
populated from asset.name while refreshAssets() expects asset.symbol; update
getAssetsList() to use asset.symbol for the symbol property (and optionally fall
back to asset.name if asset.symbol is undefined) so ClearNodeAsset.symbol
consistently contains the ticker across both getAssetsList and refreshAssets.
- Around line 60-82: WalletTxSigner currently hardcodes chainId: 1 in
signMessage which breaks EIP-712 signatures on non-mainnet chains; modify the
WalletTxSigner class to accept a chainId (number) in its constructor, store it
as a private property, and use that property in the domain object inside
signMessage instead of the literal 1; finally, update whatever
factory/constructor in this module that instantiates WalletTxSigner to pass the
appropriate chainId (from config or WalletClient context) and adjust types for
the new constructor parameter.
In `@sdk/compat/src/events.ts`:
- Around line 48-68: The poll() method uses Promise.allSettled so the outer
try/catch never catches per-request failures and rejected results are ignored;
update poll() to inspect each settled result (channels, balances, assets) for
status === 'rejected' and forward the rejection reason(s) to
this.callbacks.onError (e.g., call this.callbacks.onError with the Error or
converted Error for each rejected result, or aggregate multiple reasons into one
Error) while still invoking onChannelUpdate/onBalanceUpdate/onAssetsUpdate for
fulfilled results; reference the settled values from Promise.allSettled of
this.client.getChannels(), this.client.getBalances(), and
this.client.getAssetsList() and ensure you convert non-Error reasons into Error
objects before calling this.callbacks.onError.
---
Nitpick comments:
In `@clearnode/api/channel_v1/get_channels_test.go`:
- Around line 16-34: The helper newGetChannelsHandler ignores the error returned
by core.NewChannelDefaultSigner causing silent nil nodeSigner; change
newGetChannelsHandler to accept t *testing.T, call nodeSigner, err :=
core.NewChannelDefaultSigner(mockSigner) and assert require.NoError(t, err) (or
t.Fatalf) before using nodeSigner, and update all test callers to pass the
testing.T from their tests so the helper fails loudly if signer creation errors;
refer to newGetChannelsHandler and core.NewChannelDefaultSigner when making the
change.
In `@clearnode/api/channel_v1/get_channels.go`:
- Around line 39-51: The GetChannels handler is performing a read-only operation
inside useStoreInTx (which invokes ExecuteInTransaction / a write transaction);
change it to avoid write transactions by either calling the store read method
directly (replace the useStoreInTx block and call
h.dbStore.GetUserChannels(req.Wallet, req.Status, req.Asset, limit, offset) and
handle the returned channels/totalCount/error) or add a dedicated read-only
helper (e.g., useStoreReadOnly that wraps an ExecuteReadOnlyTransaction /
read-only accessor) and call Store.GetUserChannels inside that helper; update
error handling to behave exactly like before but without acquiring a write
transaction.
In `@clearnode/api/channel_v1/interface.go`:
- Around line 61-62: The GetUserChannels method signature in the channel API
interface uses raw limit/offset which is inconsistent with the
Store/DatabaseStore pagination convention; update the GetUserChannels
declaration to accept a *core.PaginationParams instead of limit and offset and
change its return to include core.PaginationMetadata (i.e.,
GetUserChannels(wallet string, status *string, asset *string, params
*core.PaginationParams) ([]core.Channel, core.PaginationMetadata, error)) so it
matches the Store interface and DatabaseStore implementations (update callers
and implementations of GetUserChannels accordingly).
In `@clearnode/store/database/app_session.go`:
- Around line 101-105: The subquery currently builds from AppSessionV1 and joins
to app_session_participants_v1 unnecessarily; instead construct subQuery from
the AppParticipantV1 model selecting app_session_id with the wallet_address
filter (use strings.ToLower(*participant)) and keep the outer filter query =
query.Where("id IN (?)", subQuery); update references to subQuery, AppSessionV1,
AppParticipantV1 and participant accordingly to remove the JOIN and select the
app_session_id directly.
- Around line 101-105: The subquery currently joins AppSessionV1 to
app_session_participants_v1 just to return session IDs; replace that JOIN by
building the subquery from AppParticipantV1 directly (e.g.
s.db.Model(&AppParticipantV1{}).Select("app_session_id").Where("wallet_address =
?", strings.ToLower(*participant))) and then use query = query.Where("id IN
(?)", subQuery) so you avoid the unnecessary JOIN while keeping the outer ORDER
BY and leveraging GORM v2's inline subquery rendering.
In `@clearnode/store/database/channel_test.go`:
- Around line 765-784: Add a sub-test to the existing "Success - Pagination
limits results" test that calls store.GetUserChannels with limit=2 and offset=2
to verify pagination shifting; after creating the five channels via NewDBStore
and CreateChannel, call GetUserChannels("0xuser_pg", nil, nil, 2, 2), assert no
error, assert the returned slice length is 2, assert total equals uint32(5), and
optionally assert the returned ChannelIDs match the expected "0xch_pg_2" and
"0xch_pg_3" to ensure the window is correctly shifted.
In `@clearnode/store/database/interface.go`:
- Around line 53-54: The GetUserChannels method uses flat limit/offset and a
bare uint32 total which diverges from GetUserTransactions; update the
GetUserChannels signature to accept *core.PaginationParams (like
GetUserTransactions) and return the richer pagination result (e.g.,
*core.PaginationMetadata or the same type returned by GetUserTransactions) along
with []core.Channel and error, then update all implementations and call sites to
construct/consume core.PaginationParams and the metadata instead of separate
limit/offset and total count.
In `@sdk/compat/examples/lifecycle.ts`:
- Around line 52-56: The example hardcodes the sepolia chain when creating
walletClient via createWalletClient({ chain: sepolia, ... }), which can conflict
if callers set a different CHAIN_ID; update the example to either derive the
chain from the CHAIN_ID env var (use a simple mapping or helper to convert
CHAIN_ID to the correct chain constant) or add a one-line comment above the
createWalletClient call explicitly stating that sepolia is hardcoded for the
example and should be replaced when using a different CHAIN_ID so users who
copy-paste don’t accidentally target the wrong network.
In `@sdk/compat/package.json`:
- Around line 43-48: The devDependency "@types/node" is set to "^25.3.0" which
is incompatible with the declared engines.node >=20.0.0; change the entry in
package.json devDependencies ("@types/node") to a version that matches the
minimum supported Node major (e.g., "^20.0.0" or a 20.x.x pin) so the type
definitions align with Node 20, then run your package manager to update the
lockfile; edit the package.json devDependencies block where "@types/node"
appears to make this change.
In `@sdk/compat/src/client.ts`:
- Around line 569-591: Replace the magic literal intent: 3 in closeAppSession
with the named intent from the existing map; reference INTENT_MAP.close (or
introduce a local constant like INTENT_CLOSE = INTENT_MAP.close) and use that
instead in the appUpdate object so intent values are readable and consistent
with the rest of the file (change usage in closeAppSession to use
INTENT_MAP.close or the new INTENT_CLOSE identifier).
- Around line 262-271: In checkpointWithApproval, the error handling redundantly
checks both err instanceof AllowanceError and NitroliteClient.classifyError(err)
instanceof AllowanceError; replace this with a single classification: call
NitroliteClient.classifyError(err) once, store the result, and if that
classified error is not an AllowanceError re-throw it; otherwise proceed to log
the allowance message, call this.innerClient.approveToken(chainId, tokenAddress,
NitroliteClient.MAX_UINT256) and retry this.innerClient.checkpoint(symbol).
Ensure references to AllowanceError, NitroliteClient.classifyError,
checkpointWithApproval, innerClient.checkpoint, innerClient.approveToken, and
NitroliteClient.MAX_UINT256 are used to locate the change.
- Around line 277-283: In deposit (and the other occurrence) do not silently
swallow errors from this.innerClient.setHomeBlockchain; instead catch the error,
log a descriptive message including symbol and chainId and the error object
(e.g., via this.logger.error or equivalent) so failures are visible, and decide
whether to rethrow or continue based on expected behavior — update the
setHomeBlockchain call in deposit and the similar call at the other site to use
a .catch(err => { this.logger.error(`setHomeBlockchain failed for
${symbol}@${chainId}`, err); /* optionally: throw err; */ }) pattern so errors
are recorded with context before proceeding to innerClient.deposit and
checkpointWithApproval.
In `@sdk/compat/src/rpc.ts`:
- Around line 61-64: The parseGetChannelsResponse stub currently calls
JSON.parse(raw) directly and will throw on invalid JSON; update
parseGetChannelsResponse to mirror parseAnyRPCResponse by wrapping JSON.parse in
a try/catch and returning the safe default shape ({ params: { channels: [] } })
when parsing fails or the expected path is missing; locate the function
parseGetChannelsResponse and adjust the parsing block to catch any JSON errors
(optionally log or ignore) and return the no-op default instead of allowing an
exception to propagate.
In `@sdk/compat/src/types.ts`:
- Around line 95-104: RPCLedgerEntry and LedgerEntry currently have identical
shapes; locate the RPCLedgerEntry and LedgerEntry interfaces and either replace
one with a type alias (e.g., "export type RPCLedgerEntry = LedgerEntry" or vice
versa) to prevent accidental divergence, or add a short comment above both
definitions explaining they intentionally mirror each other (wire-format vs
client-response) so future maintainers don't change one without the other;
update both occurrences (the one at RPCLedgerEntry and the duplicate at lines
referenced around LedgerEntry) accordingly.
In `@sdk/compat/tsconfig.prod.json`:
- Line 4: Remove the redundant "declaration": true setting from
tsconfig.prod.json: locate the JSON key "declaration" in tsconfig.prod.json (the
explicit declaration property) and delete that entry since the base config
already provides it, leaving the prod config to inherit the declaration setting.
In `@sdk/ts/src/client.ts`:
- Around line 1075-1095: getChannels currently accepts core.PaginationParams
(offset/limit) while getTransactions and getAppSessions accept {page, pageSize};
update getChannels signature and request construction to match the higher-level
pagination shape for consistency: change getChannels(wallet, options?: {
status?: string; asset?: string; pagination?: { page?: number; pageSize?: number
} }) and inside getChannels convert page/pageSize to offset = (page-1)*pageSize
and limit = pageSize when building the API.ChannelsV1GetChannelsRequest; keep
transformChannel and transformPaginationMetadata usage unchanged and ensure any
callers/tests are updated to the new pagination shape (or add a thin adapter if
you must preserve backward compatibility with core.PaginationParams).
Comment on lines
651
to
655
| async transfer(destination: Address, allocations: TransferAllocation[]): Promise<void> { | ||
| for (const alloc of allocations) { | ||
| await this.innerClient.transfer(destination, alloc.asset, new Decimal(alloc.amount)); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Sequential transfers fail partway with no rollback.
If one of the allocations fails mid-loop, prior transfers have already been submitted and subsequent ones are skipped. This partial-execution behavior should at least be documented so callers know they may need to handle partial failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/compat/src/client.ts` around lines 651 - 655, The transfer method
currently performs sequential innerClient.transfer calls and will stop on the
first failure, causing partial execution; update the async transfer(destination:
Address, allocations: TransferAllocation[]) in client.ts to (1) track which
allocations completed (e.g., collect completed indexes or allocations) while
calling innerClient.transfer, (2) wrap each await in a try/catch so on error you
throw a new error that includes the original error plus structured metadata
about which allocations succeeded and which failed, and (3) add/update the
method’s docstring to explicitly document that transfers are not atomic and
callers may receive partial success information in the thrown error; reference
the transfer method, innerClient.transfer calls and the TransferAllocation shape
so reviewers can find the changes.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
- Fix onError never firing in EventPoller (Promise.allSettled never rejects, so try/catch was dead code; now forwards rejected reasons) - Fix getAssetsList using asset.name instead of asset.symbol - Add missing await on async resolveAssetDisplay in lifecycle example - Replace rimraf with Node-native fs.rmSync in clean script - Add language specifiers to bare markdown code blocks - Normalize "Initialise" to "Initialize" for consistency
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
sdk/compat/src/client.ts (1)
612-620: UseNitroliteClient.INTENT_MAP['close']instead of magic number3.
INTENT_MAPis defined at line 638 precisely for this purpose and is already used insubmitAppState. Using the named constant here avoids the unexplained literal.♻️ Proposed fix
const appUpdate = { appSessionId, - intent: 3, // Close + intent: NitroliteClient.INTENT_MAP['close'], version: session.version + 1n,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/compat/src/client.ts` around lines 612 - 620, The appUpdate object uses a magic number for intent (intent: 3)—replace that literal with the named constant from the class map: use NitroliteClient.INTENT_MAP['close'] for intent so it matches the existing pattern used in submitAppState; locate the appUpdate construction and set intent to NitroliteClient.INTENT_MAP['close'] instead of 3 to improve clarity and consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clearnode/api/channel_v1/get_channels.go`:
- Around line 62-67: The page/pageCount mismatch occurs when totalCount == 0:
pageCount becomes 0 but page defaults to 1; fix by clamping the computed page
against pageCount after calculating both. After computing pageCount (uint32) and
deriving page from offset/limit, if pageCount == 0 set page = 0, otherwise set
page = min(page, pageCount) to ensure page never exceeds pageCount; update the
logic around the variables pageCount, page, offset and limit in get_channels.go
accordingly.
In `@sdk/compat/examples/lifecycle.ts`:
- Around line 121-124: The example uses info.available which doesn't exist on
AccountInfo; update the success message after client.getAccountInfo() to
reference valid fields on AccountInfo (e.g., info.balances.length to show number
of balances or info.balances[0] to display the first balance entry) and keep
channelCount (info.channelCount) as-is so ok('getAccountInfo()', ...) prints
meaningful data; locate the call to client.getAccountInfo() and the
ok('getAccountInfo()', `available=...`) invocation and replace the undefined
field with either balances.length or a formatted first balance value.
- Around line 166-173: The three calls to the async client methods need to be
awaited: change the calls to client.resolveToken(token.token),
client.resolveAsset(resolved.symbol), and client.getTokenDecimals(token.token)
to use await so you get actual results instead of Promises; specifically update
the variables resolved, bySymbol, and decimals to await
client.resolveToken(...), await client.resolveAsset(...), and await
client.getTokenDecimals(...) respectively (keeping the existing
ok('resolveToken()', ...), ok('resolveAsset()', ...) and
ok('getTokenDecimals()', ...) usage).
- Line 26: The CHAIN_ID assignment uses Number(process.env.CHAIN_ID ||
'11155111') which lets Number('abc') produce NaN and silently bypass the
default; change it to parse and validate the env var first and only use it if
numeric (e.g. parse with Number() or parseInt and check Number.isFinite /
!Number.isNaN), otherwise fall back to the Sepolia literal 11155111; update the
variable referenced as CHAIN_ID and ensure the validated numeric value is what
gets passed into NitroliteClient.create (or any other use sites) so NaN cannot
propagate.
In `@sdk/compat/src/client.ts`:
- Around line 227-239: resolveAssetDisplay currently looks up assetsByToken
(keyed by lowercase token address) but refreshAssets overwrites same-address
entries across chains, so the alternate-chain find is unreachable; change
assetsByToken from Map<string, AssetInfo> to Map<string, Map<number, AssetInfo>>
(or Map<string, AssetInfo[]>) and update refreshAssets to append/store AssetInfo
under the token address keyed by chainId instead of overwriting, then change
resolveAssetDisplay to first get the per-address map/array and select the entry
whose chainId matches the provided chainId (or pick a sensible default when
chainId is undefined); also update related helpers like resolveToken to use the
new nested structure when choosing the best-matching AssetInfo.
- Around line 181-183: ensureAssets currently uses assetsBySymbol.size as the
"loaded" sentinel which stays zero for chains without registered symbols and
causes repeated refreshAssets network calls; update ensureAssets (or create a
boolean like this._assetsLoaded) to check assetsByToken.size (or set the boolean
inside refreshAssets) instead so a single successful refreshAssets() marks
assets as loaded; update refreshAssets() to set the flag (or populate
assetsByToken) reliably and have getDecimalsForAsset / closeChannel rely on the
new sentinel to avoid refetching on every call.
---
Duplicate comments:
In `@clearnode/api/channel_v1/get_channels.go`:
- Around line 21-38: The pagination clamp logic ensures limit is constrained to
[1,1000] before reaching the store: keep the current logic around defaultLimit,
maxLimit, and the req.Pagination handling (variables limit, offset), and add a
unit test that exercises req.Pagination.Limit == nil, == 0, and > maxLimit to
assert limit becomes defaultLimit (100) for zero/nil and maxLimit (1000) when
over the limit; no functional code change required other than adding the test.
In `@sdk/compat/src/client.ts`:
- Around line 266-274: In classifyError, the allowance branch currently checks
lower.includes('sufficient') which also matches phrases like "allowance
insufficient"; change that condition to check for 'insufficient' (e.g., use
lower.includes('allowance') && lower.includes('insufficient')) so only true
insufficient messages map to AllowanceError; update the static method
classifyError (and keep existing AllowanceError, UserRejectedError,
InsufficientFundsError, NotInitializedError references) accordingly.
- Around line 210-215: getTokenDecimals currently falls back to 6 for unknown
tokens causing large amount errors; change it to throw for unrecognized
addresses like resolveToken does. In the getTokenDecimals function (use
tokenAddress param, ensureAssets call, assetsByToken map lookup), if info is
undefined/null do not return 6 — instead throw a descriptive Error including the
tokenAddress (or its lowercase key) so callers must handle unknown tokens; keep
the existing await this.ensureAssets() behavior and use the same lookup key
generation (tokenAddress.toString().toLowerCase()) as currently implemented.
- Around line 691-697: The transfer method currently performs sequential
innerClient.transfer calls inside transfer(...) and will stop mid-loop on an
error leaving prior transfers committed; update transfer to either 1) use a
single batch API if innerClient exposes one (e.g., innerClient.batchTransfer) to
make the whole allocations set atomic, or 2) if no batch API exists, run all
transfers concurrently using Promise.all over allocations (mapping each alloc ->
innerClient.transfer(destination, alloc.asset, humanAmount)) so failures
propagate instead of being silently skipped, and wrap the Promise.all in
try/catch to log and rethrow the error; also add a brief comment in transfer
mentioning the chosen behavior so callers know whether partial commits are
possible. Ensure you reference getDecimalsForAsset, TransferAllocation,
innerClient.transfer and the transfer(...) method when making the change.
---
Nitpick comments:
In `@sdk/compat/src/client.ts`:
- Around line 612-620: The appUpdate object uses a magic number for intent
(intent: 3)—replace that literal with the named constant from the class map: use
NitroliteClient.INTENT_MAP['close'] for intent so it matches the existing
pattern used in submitAppState; locate the appUpdate construction and set intent
to NitroliteClient.INTENT_MAP['close'] instead of 3 to improve clarity and
consistency.
sdk/compat/src/client.ts
Outdated
Comment on lines
227
to
239
| async resolveAssetDisplay(tokenAddress: Address | string, chainId?: number): Promise<{ symbol: string; decimals: number } | null> { | ||
| await this.ensureAssets(); | ||
| const key = tokenAddress.toString().toLowerCase(); | ||
| const info = this.assetsByToken.get(key); | ||
| if (!info) return null; | ||
| if (chainId !== undefined && Number(info.chainId) !== chainId) { | ||
| const alt = Array.from(this.assetsByToken.values()).find( | ||
| (a) => a.tokenAddress === key && Number(a.chainId) === chainId, | ||
| ); | ||
| return alt ? { symbol: alt.symbol, decimals: alt.decimals } : { symbol: info.symbol, decimals: info.decimals }; | ||
| } | ||
| return { symbol: info.symbol, decimals: info.decimals }; | ||
| } |
Contributor
There was a problem hiding this comment.
resolveAssetDisplay multi-chain alternative lookup is a no-op.
assetsByToken is a Map<string, AssetInfo> keyed by lowercase token address (line 173). If the same token contract exists on multiple chains, refreshAssets() overwrites the entry with the last-iterated chain — only one AssetInfo per address survives. The find at line 233 therefore can never locate an entry with the same tokenAddress but a different chainId than the one already in info, making the entire alt branch unreachable.
To support per-chain resolution for the same token address, the map would need to be Map<string, Map<bigint, AssetInfo>> (or Map<string, AssetInfo[]>), with refreshAssets appending rather than overwriting.
♻️ Minimal structural fix
- private assetsByToken = new Map<string, AssetInfo>();
+ private assetsByToken = new Map<string, AssetInfo[]>();In refreshAssets():
- this.assetsByToken.set(info.tokenAddress, info);
+ const existing = this.assetsByToken.get(info.tokenAddress) ?? [];
+ existing.push(info);
+ this.assetsByToken.set(info.tokenAddress, existing);Then resolveToken and resolveAssetDisplay pick the best-matching entry by chainId.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/compat/src/client.ts` around lines 227 - 239, resolveAssetDisplay
currently looks up assetsByToken (keyed by lowercase token address) but
refreshAssets overwrites same-address entries across chains, so the
alternate-chain find is unreachable; change assetsByToken from Map<string,
AssetInfo> to Map<string, Map<number, AssetInfo>> (or Map<string, AssetInfo[]>)
and update refreshAssets to append/store AssetInfo under the token address keyed
by chainId instead of overwriting, then change resolveAssetDisplay to first get
the per-address map/array and select the entry whose chainId matches the
provided chainId (or pick a sensible default when chainId is undefined); also
update related helpers like resolveToken to use the new nested structure when
choosing the best-matching AssetInfo.
- Warn on unknown token fallback to 6 decimals instead of silent default - Tighten classifyError to match 'insufficient' not just 'sufficient' - Use assetsByToken.size in ensureAssets to avoid repeated refetch when configured chain has no assets - Remove dead multi-chain alt lookup in resolveAssetDisplay - Fix page=0/pageCount=0 for empty results in get_channels - Add missing await on resolveToken/resolveAsset/getTokenDecimals in lifecycle example - Fix info.available -> info.balances in lifecycle example - Validate CHAIN_ID env var for NaN in lifecycle example - Add JSDoc noting non-atomic transfer behavior
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
sdk/compat/src/client.ts (5)
273-281: First condition incheckpointWithApprovalis alwaysfalse— dead code.
err instanceof AllowanceErrorcan never betruebecause the inner v1 client has no knowledge of the compatAllowanceErrortype and will never throw it. The whole expression reduces to!(NitroliteClient.classifyError(err) instanceof AllowanceError).♻️ Proposed fix — drop the unreachable condition
- if (!(err instanceof AllowanceError) && !(NitroliteClient.classifyError(err) instanceof AllowanceError)) throw NitroliteClient.classifyError(err); + if (!(NitroliteClient.classifyError(err) instanceof AllowanceError)) throw NitroliteClient.classifyError(err);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/compat/src/client.ts` around lines 273 - 281, In checkpointWithApproval, remove the unreachable check for err instanceof AllowanceError and rely solely on NitroliteClient.classifyError(err) to detect allowance errors; update the catch in the checkpointWithApproval method (which calls this.innerClient.checkpoint and this.innerClient.approveToken) so it throws NitroliteClient.classifyError(err) unless the classified error is an instance of AllowanceError, and only then log the message, call approveToken(chainId, tokenAddress, NitroliteClient.MAX_UINT256) and retry this.innerClient.checkpoint(symbol).
185-189:getDecimalsForAssetsilently falls back to 6 decimals without any warning.
getTokenDecimals(lines 214–216) logs aconsole.warnfor unknown tokens;getDecimalsForAssettakes the same silent-fallback path but is used in arguably higher-stakes paths:closeAppSession,submitAppState, andtransfer. An unknown symbol here would silently corrupt amounts with no diagnostic signal.♻️ Proposed fix — add warning parity with `getTokenDecimals`
private async getDecimalsForAsset(assetSymbol: string): Promise<number> { await this.ensureAssets(); const info = this.assetsBySymbol.get(assetSymbol.toLowerCase()); + if (!info) { + console.warn(`[compat] Unknown asset symbol ${assetSymbol}, falling back to 6 decimals`); + } return info?.decimals ?? 6; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/compat/src/client.ts` around lines 185 - 189, getDecimalsForAsset currently silently returns a default of 6 for unknown symbols; update it to mirror getTokenDecimals by emitting a console.warn when assetsBySymbol.lookup of assetSymbol.toLowerCase() yields no info so callers (closeAppSession, submitAppState, transfer) get a diagnostic signal; keep the existing ensureAssets() call, include the assetSymbol and context in the warning, and then return the existing fallback of 6.
511-514:status ?? undefinedis a no-op.
statusis already typed asstring | undefined, sostatus ?? undefinedis identical tostatusalone.- status: status ?? undefined, + status,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/compat/src/client.ts` around lines 511 - 514, The call to this.innerClient.getAppSessions uses "status ?? undefined", which is redundant because status is already string | undefined; change the argument to pass status directly (i.e., replace status ?? undefined with status) and leave wallet as wallet ?? this.userAddress so the call becomes this.innerClient.getAppSessions({ wallet: wallet ?? this.userAddress, status });—update the invocation around getAppSessions to remove the unnecessary nullish coalescing on status.
355-371:getChannelDatabypasses the populated asset cache and issues a fresh network call on every invocation.Line 356 calls
this.innerClient.getAssets()directly, ignoring theassetsByToken/assetsBySymbolmaps thatensureAssets()already maintains. For an operation that iterates all assets to find a single channel, this is an unnecessary network round-trip.♻️ Proposed fix — use `ensureAssets()` + cached maps
async getChannelData(_channelId: string): Promise<any> { - const assets = await this.innerClient.getAssets(); - for (const asset of assets) { + await this.ensureAssets(); + for (const [, info] of this.assetsBySymbol) { + const asset = { symbol: info.symbol }; try { const ch = await this.innerClient.getHomeChannel(this.userAddress, asset.symbol);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/compat/src/client.ts` around lines 355 - 371, getChannelData currently calls this.innerClient.getAssets() causing an unnecessary network call; change it to await this.ensureAssets() and iterate the cached maps (this.assetsBySymbol or this.assetsByToken) to find the asset matching the channelId, then call this.innerClient.getHomeChannel and getLatestState only for the matched asset; specifically, inside getChannelData replace the direct getAssets() usage with await this.ensureAssets(), iterate this.assetsBySymbol/this.assetsByToken to locate the asset whose channelId matches _channelId (handling try/catch as before), and return the same object structure using the cached asset symbol when calling getHomeChannel/getLatestState.
320-336: Deadelsebranch in the bulk-close loop —infois always truthy here.The
if (!symbol) continueguard on line 325 ensures the loop skips any iteration whereinfois falsy. By line 328infois guaranteed to be defined, making theelseat line 330 unreachable.♻️ Proposed fix — remove dead branch
await this.innerClient.closeHomeChannel(symbol); - if (info) { - await this.checkpointWithApproval(symbol, info.chainId, info.tokenAddress); - } else { - await this.innerClient.checkpoint(symbol); - } + await this.checkpointWithApproval(symbol, info.chainId, info.tokenAddress);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/compat/src/client.ts` around lines 320 - 336, The loop's else branch is dead because the earlier `if (!symbol) continue` guarantees `info` is defined; remove the unreachable `else` and call the approved checkpoint path unconditionally for the open-channel case. In the for-loop that calls `this.ensureAssets()` and reads `const info = this.assetsByToken.get(ch.token.toLowerCase())`, replace the `if (info) { await this.checkpointWithApproval(symbol, info.chainId, info.tokenAddress); } else { await this.innerClient.checkpoint(symbol); }` with a single call to `await this.checkpointWithApproval(symbol, info.chainId, info.tokenAddress);` (keeping the surrounding `try/catch` and the `if (!symbol) continue` guard).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clearnode/api/channel_v1/get_channels.go`:
- Around line 16-19: The handler currently trusts req.Wallet directly (see
req.Wallet and c.Fail) which allows IDOR; ensure the authenticated principal
from the RPC context is asserted before using req.Wallet by retrieving the
caller identity from rpc.Context (or your auth middleware) and comparing it to
req.Wallet, returning c.Fail on mismatch; if no context binding exists,
add/authenticate middleware that binds the authenticated wallet ID into the
request context (or into the handler input) and update this handler to validate
req.Wallet against that bound identity (apply the same pattern to all handlers
that accept a wallet).
---
Duplicate comments:
In `@clearnode/api/channel_v1/get_channels.go`:
- Around line 62-69: The page calculation can exceed pageCount when offset
overshoots results; after computing pageCount and page in the get_channels
handler (variables pageCount, page, totalCount, limit, offset), add a clamp so
that if page > pageCount then page = pageCount (preserving the existing
totalCount > 0 guard and zero-case behavior). This ensures page is bounded to
the last available page when offset is beyond the result set.
---
Nitpick comments:
In `@sdk/compat/src/client.ts`:
- Around line 273-281: In checkpointWithApproval, remove the unreachable check
for err instanceof AllowanceError and rely solely on
NitroliteClient.classifyError(err) to detect allowance errors; update the catch
in the checkpointWithApproval method (which calls this.innerClient.checkpoint
and this.innerClient.approveToken) so it throws
NitroliteClient.classifyError(err) unless the classified error is an instance of
AllowanceError, and only then log the message, call approveToken(chainId,
tokenAddress, NitroliteClient.MAX_UINT256) and retry
this.innerClient.checkpoint(symbol).
- Around line 185-189: getDecimalsForAsset currently silently returns a default
of 6 for unknown symbols; update it to mirror getTokenDecimals by emitting a
console.warn when assetsBySymbol.lookup of assetSymbol.toLowerCase() yields no
info so callers (closeAppSession, submitAppState, transfer) get a diagnostic
signal; keep the existing ensureAssets() call, include the assetSymbol and
context in the warning, and then return the existing fallback of 6.
- Around line 511-514: The call to this.innerClient.getAppSessions uses "status
?? undefined", which is redundant because status is already string | undefined;
change the argument to pass status directly (i.e., replace status ?? undefined
with status) and leave wallet as wallet ?? this.userAddress so the call becomes
this.innerClient.getAppSessions({ wallet: wallet ?? this.userAddress, status
});—update the invocation around getAppSessions to remove the unnecessary
nullish coalescing on status.
- Around line 355-371: getChannelData currently calls
this.innerClient.getAssets() causing an unnecessary network call; change it to
await this.ensureAssets() and iterate the cached maps (this.assetsBySymbol or
this.assetsByToken) to find the asset matching the channelId, then call
this.innerClient.getHomeChannel and getLatestState only for the matched asset;
specifically, inside getChannelData replace the direct getAssets() usage with
await this.ensureAssets(), iterate this.assetsBySymbol/this.assetsByToken to
locate the asset whose channelId matches _channelId (handling try/catch as
before), and return the same object structure using the cached asset symbol when
calling getHomeChannel/getLatestState.
- Around line 320-336: The loop's else branch is dead because the earlier `if
(!symbol) continue` guarantees `info` is defined; remove the unreachable `else`
and call the approved checkpoint path unconditionally for the open-channel case.
In the for-loop that calls `this.ensureAssets()` and reads `const info =
this.assetsByToken.get(ch.token.toLowerCase())`, replace the `if (info) { await
this.checkpointWithApproval(symbol, info.chainId, info.tokenAddress); } else {
await this.innerClient.checkpoint(symbol); }` with a single call to `await
this.checkpointWithApproval(symbol, info.chainId, info.tokenAddress);` (keeping
the surrounding `try/catch` and the `if (!symbol) continue` guard).
Comment on lines
+16
to
+19
| if req.Wallet == "" { | ||
| c.Fail(rpc.Errorf("wallet is required"), "missing wallet") | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check wallet validation patterns in channel_v1 handlers
echo "=== Files in channel_v1 ==="
fd -e go . clearnode/api/channel_v1 2>/dev/null || find clearnode/api/channel_v1 -name "*.go" 2>/dev/null | head -20
echo -e "\n=== Wallet usage in get_channels.go and nearby handlers ==="
fd -e go . clearnode/api/channel_v1 --exec grep -l "req\.Wallet\|Wallet" {} \; 2>/dev/null | head -10
echo -e "\n=== rpc.Context structure (check for auth/identity fields) ==="
find . -path ./node_modules -prune -o -type f -name "*.go" -print | xargs grep -l "type.*Context" | xargs grep -B5 -A10 "type.*Context" | grep -E "(type|Wallet|Identity|Caller|Principal|Auth)" | head -40
echo -e "\n=== Check if get_channels.go compares wallet with context identity ==="
cat clearnode/api/channel_v1/get_channels.go 2>/dev/null | head -100Repository: erc7824/nitrolite
Length of output: 5541
🏁 Script executed:
#!/bin/bash
# More targeted search for authentication patterns
echo "=== All .go files in channel_v1 ==="
ls -la clearnode/api/channel_v1/*.go 2>/dev/null || echo "Directory listing failed, trying find:"
find clearnode/api/channel_v1 -name "*.go" 2>/dev/null
echo -e "\n=== Check request type definition ==="
rg "type.*Request" clearnode/api/channel_v1/ --type=go -A5 2>/dev/null | head -50
echo -e "\n=== Look for wallet field usage across multiple handlers ==="
rg "req\.Wallet" clearnode/api/ --type=go -B3 -A3 2>/dev/null | head -80Repository: erc7824/nitrolite
Length of output: 5728
🏁 Script executed:
#!/bin/bash
# Check for rpc package structure and context
echo "=== rpc package files ==="
find . -path "*/pkg/rpc*" -o -path "*/rpc/*.go" 2>/dev/null | head -20
echo -e "\n=== Search rpc.Context definition ==="
rg "type Context" . --type=go -A10 2>/dev/null | head -50
echo -e "\n=== Check how wallet param is used with store ==="
rg "GetUserChannels\|StoreGetUserChannels" clearnode/ --type=go -B2 -A5 2>/dev/null | head -60Repository: erc7824/nitrolite
Length of output: 1257
🏁 Script executed:
#!/bin/bash
echo "=== Full rpc.Context definition ==="
cat pkg/rpc/context.go | head -100
echo -e "\n=== Check for auth/wallet methods on Context ==="
rg "func.*Context.*Wallet\|func.*Context.*Auth\|func.*Context.*Identity\|func.*Context.*Caller" pkg/rpc/ --type=go -A3
echo -e "\n=== Check Connection type for auth info ==="
rg "type Connection" pkg/rpc/ --type=go -A20 | head -60
echo -e "\n=== Look for middleware or handlers that validate wallet ==="
rg "Storage\|wallet\|Wallet" pkg/rpc/ --type=go | grep -i "auth\|valid\|check\|identity\|principal" | head -20Repository: erc7824/nitrolite
Length of output: 7011
🏁 Script executed:
#!/bin/bash
echo "=== Check handler setup and middleware ==="
cat clearnode/api/channel_v1/handler.go | head -80
echo -e "\n=== Look for any existing wallet validation in handler setup ==="
rg "Wallet" clearnode/api/channel_v1/handler.go --type=go -B3 -A3
echo -e "\n=== Check if there's auth wrapper or middleware in other files ==="
rg "GetWallet\|AuthWallet\|ValidateWallet\|IsAuthorized" clearnode/ --type=go | head -20Repository: erc7824/nitrolite
Length of output: 4761
🏁 Script executed:
#!/bin/bash
echo "=== Check for wallet/auth extraction middleware ==="
rg "authMapping\|UserID\|authenticated\|middleware" pkg/rpc/ --type=go -B2 -A5 | head -80
echo -e "\n=== Check how handlers are registered/called ==="
rg "api.go\|methods.go" pkg/rpc/ --type=go | head -5
cat pkg/rpc/api.go | head -100
echo -e "\n=== Check if node setup passes auth to handlers ==="
rg "newHandler\|NewHandler" clearnode/api/ --type=go -B3 -A5 | grep -A5 "Handler" | head -40
echo -e "\n=== Check user_v1 handlers for wallet validation ==="
cat clearnode/api/user_v1/get_balances.go | head -50Repository: erc7824/nitrolite
Length of output: 4868
🏁 Script executed:
#!/bin/bash
echo "=== Check how requests are authenticated and dispatched ==="
rg "Handle\|RegisterMethod\|dispatch" pkg/rpc/ --type=go -B2 -A8 | head -100
echo -e "\n=== Check if there's any auth context setup before handler invocation ==="
cat pkg/rpc/node.go | head -120
echo -e "\n=== Check test files for how wallet validation should work ==="
head -100 clearnode/api/channel_v1/get_channels_test.go
echo -e "\n=== Check if SafeStorage has auth-related methods ==="
rg "SafeStorage\|type SafeStorage" pkg/rpc/ --type=go -A20 | head -60Repository: erc7824/nitrolite
Length of output: 7637
🏁 Script executed:
#!/bin/bash
echo "=== Search for userID/wallet extraction from connections ==="
rg "connHub\|connectionID\|userID" clearnode/api/ --type=go -B2 -A3 | head -80
echo -e "\n=== Check how node.Notify uses userID and if handlers have access to it ==="
cat pkg/rpc/node.go | grep -A 30 "Notify\|userID" | head -60
echo -e "\n=== Look for connection authentication or identity storage ==="
rg "authMapping\|GetUser\|Identity\|Principal" pkg/rpc/connection_hub.go --type=go -B2 -A5 | head -100
echo -e "\n=== Check if there's any middleware that sets auth context ==="
rg "middleware\|Use\|auth" clearnode/api/ --type=go -i | grep -E "handler|middleware|auth" | head -30
echo -e "\n=== Directly search for wallet validation patterns across all handlers ==="
rg "req\.Wallet.*==\|ValidateWallet\|c\.Request\..*Wallet\|authenticated" clearnode/api/ --type=go -B3 -A3 | head -80Repository: erc7824/nitrolite
Length of output: 2790
🏁 Script executed:
#!/bin/bash
echo "=== Check connection_hub authMapping implementation ==="
cat pkg/rpc/connection_hub.go | head -150
echo -e "\n=== Check if there's any auth setup in clearnode main/init ==="
find clearnode -name "main.go" -o -name "init.go" -o -name "*setup*" | xargs cat 2>/dev/null | head -150
echo -e "\n=== Final check: does rpc.Context expose connection/userID? ==="
cat pkg/rpc/context.go
echo -e "\n=== Verify: how is GetChannels actually called (trace the flow) ==="
rg "GetChannels" clearnode/ pkg/rpc/ --type=go -B2 -A2 | head -60Repository: erc7824/nitrolite
Length of output: 19480
🏁 Script executed:
#!/bin/bash
echo "=== Check rpc_router setup for middleware/auth ==="
cat clearnode/api/rpc_router.go
echo -e "\n=== Check if there's any authentication decorator or wrapper on handlers ==="
rg "Handle.*Wallet\|middleware.*auth\|func.*auth\|AuthMiddleware" clearnode/api/ --type=go -B3 -A5 | head -100
echo -e "\n=== Final check: trace NewRPCRouter to see if auth is wired ==="
rg "NewRPCRouter" clearnode/api/ --type=go -A50 | head -150Repository: erc7824/nitrolite
Length of output: 9602
🏁 Script executed:
#!/bin/bash
echo "=== Final verification: check if there's any client-level wallet binding ==="
rg "authWallet\|authenticatedWallet\|connectionWallet\|sessionWallet" . --type=go | head -20
echo -e "\n=== Check if Connection interface carries any auth info ==="
grep -A 50 "type Connection interface" pkg/rpc/connection.go
echo -e "\n=== Confirm: no auth validation in any handler ==="
rg "req\.Wallet.*==" clearnode/api/ --type=go -B2 -A2 | head -40Repository: erc7824/nitrolite
Length of output: 3117
Verify wallet authorization against authenticated session identity to prevent IDOR.
req.Wallet is accepted directly from the request payload without validation against the authenticated caller's identity. This allows any authenticated user to query channels, balances, or states for arbitrary wallets.
Check whether the RPC layer enforces that the wallet in the request matches the authenticated principal (via rpc.Context or middleware). If not, add authentication middleware to bind the authenticated wallet to the context and validate req.Wallet against it before handler execution. This pattern should be applied consistently to all handlers that accept a wallet parameter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clearnode/api/channel_v1/get_channels.go` around lines 16 - 19, The handler
currently trusts req.Wallet directly (see req.Wallet and c.Fail) which allows
IDOR; ensure the authenticated principal from the RPC context is asserted before
using req.Wallet by retrieving the caller identity from rpc.Context (or your
auth middleware) and comparing it to req.Wallet, returning c.Fail on mismatch;
if no context binding exists, add/authenticate middleware that binds the
authenticated wallet ID into the request context (or into the handler input) and
update this handler to validate req.Wallet against that bound identity (apply
the same pattern to all handlers that accept a wallet).
alessio
approved these changes
Feb 23, 2026
philanton
reviewed
Feb 23, 2026
| } | ||
|
|
||
| const defaultLimit uint32 = 100 | ||
| const maxLimit uint32 = 1000 |
Contributor
There was a problem hiding this comment.
This setting should be configurable and have an adequate value
Collaborator
Author
There was a problem hiding this comment.
The current constants (defaultLimit=100, maxLimit=1000) follow the same pattern as store/database/list_options.go (DefaultLimit=10, MaxLimit=100). Making these configurable via handler config is a valid improvement. happy to address in a follow-up if the we later want to standardize configurable pagination across all endpoints.
Contributor
There was a problem hiding this comment.
Would be good to have an optional channel_type parameter accepting either core.ChannelTypeHome or core.ChannelTypeEscrow value
Collaborator
Author
There was a problem hiding this comment.
Added in 05f5c3b: channel_type is now an optional filter accepting "home" or "escrow". Wired through the RPC request type, handler, store interface, mock, DB layer, and TS SDK.
Contributor
There was a problem hiding this comment.
Didn't the current implementation work?
Collaborator
Author
There was a problem hiding this comment.
Old query failed because it asked PostgreSQL to DISTINCT by id while sorting by created_at, and Postgres refuses to sort by a column that isn’t included in the distinct result set since it can be ambiguous after deduplication
While SQLite allows it and that's why the tests passed.
clearnode/store/database/channel.go
Outdated
| return channels, uint32(totalCount), nil | ||
| } | ||
|
|
||
| func channelStatusFromString(s string) (core.ChannelStatus, error) { |
Contributor
There was a problem hiding this comment.
It would be better to have this function on api level and expect status as core.ChannelStatus here above
Collaborator
Author
There was a problem hiding this comment.
Addressed in 05f5c3b: moved channelStatusFromString from the database layer to the API handler. The store now accepts typed *core.ChannelStatus and *core.ChannelType instead of raw strings.
…atus parsing to API layer
- Add optional channel_type filter ("home"/"escrow") to get_channels
endpoint across RPC types, handler, store, mock, and TS SDK
- Move channelStatusFromString from database layer to API handler;
store now accepts typed *core.ChannelStatus and *core.ChannelType
- Add channelTypeFromString parser at API level
- Add DB test for channel type filtering
- Remove invalid-status DB test (validation now in handler)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
clearnode/api/channel_v1/get_channels.go (1)
45-48: Outstanding:req.Walletis still not validated against the authenticated caller's identity (IDOR).Any authenticated user can query channels for an arbitrary wallet by supplying a different address in the payload. This was flagged in the previous review cycle and remains unaddressed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/api/channel_v1/get_channels.go` around lines 45 - 48, The handler currently only checks req.Wallet for emptiness but does not verify it matches the authenticated caller (causing IDOR); fetch the caller's authenticated wallet/identity from the request context or auth claims (e.g., the auth principal in the handler/context or session used by this endpoint) and compare it to req.Wallet, and if they differ return a forbidden error (use c.Fail with a proper rpc.Errorf("permission denied" or "forbidden")) instead of proceeding; update the logic in get_channels.go around the req.Wallet check to perform this equality/ownership check and log/return immediately on mismatch.
🧹 Nitpick comments (2)
clearnode/api/channel_v1/get_channels_test.go (2)
204-236:TestGetChannels_EmptyResultomitsPageandPageCountmetadata assertions.When
totalCount=0, the page computation yieldsPage=1(floor(0/limit)+1) andPageCount=0(ceil(0/limit)), an asymmetric state (Page=1 of 0 pages) that is worth pinning explicitly.assert.Equal(t, uint32(0), response.Metadata.TotalCount) +assert.Equal(t, uint32(1), response.Metadata.Page) +assert.Equal(t, uint32(0), response.Metadata.PageCount)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/api/channel_v1/get_channels_test.go` around lines 204 - 236, Update the test TestGetChannels_EmptyResult to assert the pagination metadata: after translating the response in the test, add assertions that response.Metadata.Page equals uint32(1) and response.Metadata.PageCount equals uint32(0). This ensures the handler's computed pagination for totalCount=0 (Page=1 and PageCount=0) is explicitly verified; locate these additions in the TestGetChannels_EmptyResult function immediately after err = ctx.Response.Payload.Translate(&response).
36-202: Handler-level tests forchannel_typeandassetfilters are absent.The
GetUserChannelsmock signature accepts both a*core.ChannelTypeand a*string(asset) filter, and the PR addschannelTypeFromStringparsing at the handler level. None of the six tests exercise either filter, leaving the handler-side parsing logic uncovered.Add at minimum one test for each filter, analogous to
TestGetChannels_WithStatusFilter:func TestGetChannels_WithChannelTypeFilter(t *testing.T) { mockTxStore := new(MockStore) handler := newGetChannelsHandler(mockTxStore) userWallet := "0x1234567890123456789012345678901234567890" chType := core.ChannelTypeHome mockTxStore.On("GetUserChannels", userWallet, (*core.ChannelStatus)(nil), (*string)(nil), &chType, uint32(100), uint32(0)). Return([]core.Channel{{ChannelID: "0xCh", Type: core.ChannelTypeHome, Status: core.ChannelStatusOpen}}, uint32(1), nil) typeStr := "home" reqPayload := rpc.ChannelsV1GetChannelsRequest{ Wallet: userWallet, ChannelType: &typeStr, } // ... build ctx, invoke handler, assert response ... } func TestGetChannels_InvalidChannelType(t *testing.T) { // ensures handler rejects unknown channel_type strings }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/api/channel_v1/get_channels_test.go` around lines 36 - 202, Tests for GetChannels are missing coverage for the channel_type and asset filters; add handler-level tests that exercise channelTypeFromString parsing and asset forwarding to the store. Create a TestGetChannels_WithChannelTypeFilter that sets ChannelType = "home", expects handler.GetChannels to call GetUserChannels with &core.ChannelTypeHome (reference chType) and returns a matching channel, then assert the response contains the filtered channel; also add a TestGetChannels_InvalidChannelType that passes an invalid type string and asserts the handler returns an error (no store call). Ensure the mock expectations reference GetUserChannels with the appropriate (*core.ChannelStatus)(nil), (*string)(nil) or &assetString, &chType, and pagination params so the parsing logic is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clearnode/api/channel_v1/get_channels_test.go`:
- Around line 238-259: Add a new unit test that verifies handler-level
validation rejects invalid status strings: create TestGetChannels_InvalidStatus
using newGetChannelsHandler(mockTxStore) and rpc.ChannelsV1GetChannelsRequest
with a valid Wallet and Status set to "garbage", wrap it in rpc.NewPayload and
rpc.Context, call handler.GetChannels(ctx), then assert ctx.Response.Error() is
non-nil and its message contains "status", and finally call
mockTxStore.AssertExpectations(t) to ensure the MockStore (MockStore /
GetChannels interaction) was not invoked.
---
Duplicate comments:
In `@clearnode/api/channel_v1/get_channels.go`:
- Around line 45-48: The handler currently only checks req.Wallet for emptiness
but does not verify it matches the authenticated caller (causing IDOR); fetch
the caller's authenticated wallet/identity from the request context or auth
claims (e.g., the auth principal in the handler/context or session used by this
endpoint) and compare it to req.Wallet, and if they differ return a forbidden
error (use c.Fail with a proper rpc.Errorf("permission denied" or "forbidden"))
instead of proceeding; update the logic in get_channels.go around the req.Wallet
check to perform this equality/ownership check and log/return immediately on
mismatch.
---
Nitpick comments:
In `@clearnode/api/channel_v1/get_channels_test.go`:
- Around line 204-236: Update the test TestGetChannels_EmptyResult to assert the
pagination metadata: after translating the response in the test, add assertions
that response.Metadata.Page equals uint32(1) and response.Metadata.PageCount
equals uint32(0). This ensures the handler's computed pagination for
totalCount=0 (Page=1 and PageCount=0) is explicitly verified; locate these
additions in the TestGetChannels_EmptyResult function immediately after err =
ctx.Response.Payload.Translate(&response).
- Around line 36-202: Tests for GetChannels are missing coverage for the
channel_type and asset filters; add handler-level tests that exercise
channelTypeFromString parsing and asset forwarding to the store. Create a
TestGetChannels_WithChannelTypeFilter that sets ChannelType = "home", expects
handler.GetChannels to call GetUserChannels with &core.ChannelTypeHome
(reference chType) and returns a matching channel, then assert the response
contains the filtered channel; also add a TestGetChannels_InvalidChannelType
that passes an invalid type string and asserts the handler returns an error (no
store call). Ensure the mock expectations reference GetUserChannels with the
appropriate (*core.ChannelStatus)(nil), (*string)(nil) or &assetString, &chType,
and pagination params so the parsing logic is exercised.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
clearnode/api/channel_v1/get_channels.goclearnode/api/channel_v1/get_channels_test.goclearnode/api/channel_v1/interface.goclearnode/api/channel_v1/testing.goclearnode/store/database/channel.goclearnode/store/database/channel_test.goclearnode/store/database/interface.gopkg/rpc/api.gosdk/ts/src/client.tssdk/ts/src/rpc/api.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- sdk/ts/src/client.ts
Verifies that get_channels rejects unrecognized status strings at the API layer (before hitting the store) now that channelStatusFromString was moved from the database layer to the handler.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
clearnode/api/channel_v1/get_channels_test.go (1)
280-282: Weak error-content assertion is an acknowledged trade-off.The assertion only checks
ctx.Response.Type == rpc.MsgTypeRespErrwithout verifying the error message. Per the prior review thread this is intentional becauseNewErrorPayloadcausesPayload.Error()to returnnilwhen the message contains quoted strings. No further action needed here, but the discrepancy versusTestGetChannels_MissingWalletandTestGetChannels_StoreErroris worth documenting inline so future readers understand why the pattern differs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/api/channel_v1/get_channels_test.go` around lines 280 - 282, The test only asserts ctx.Response.Type == rpc.MsgTypeRespErr without checking the error message; add an inline comment above that assertion in get_channels_test.go (near the failing assertion in the TestGetChannels_* tests) explaining that this weaker check is intentional because NewErrorPayload causes Payload.Error() to return nil when the message contains quoted strings, and note that this differs from TestGetChannels_MissingWallet and TestGetChannels_StoreError so future readers understand the discrepancy.
🧹 Nitpick comments (2)
clearnode/api/channel_v1/get_channels_test.go (2)
19-19: Consider usingrequire.NoErrorforcore.NewChannelDefaultSigner.Silently discarding the error (
_) means a misconfiguredMockSignerwill produce a nilnodeSignerand cause cryptic downstream panics instead of a clear setup failure.🔧 Proposed fix
- nodeSigner, _ := core.NewChannelDefaultSigner(mockSigner) + nodeSigner, err := core.NewChannelDefaultSigner(mockSigner) + require.NoError(t, err)(This requires
t *testing.Tto be passed into the helper or usetesting.TB.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/api/channel_v1/get_channels_test.go` at line 19, The test currently ignores the error returned by core.NewChannelDefaultSigner, which can hide setup failures; update the test helper to accept t (testing.T or testing.TB) and call core.NewChannelDefaultSigner(mockSigner) capturing the error, then assert require.NoError(t, err) before using nodeSigner so any signer creation failure fails clearly; reference the nodeSigner variable and core.NewChannelDefaultSigner in your changes.
208-208: Use a valid Ethereum address format for consistency.
"0xNoChannelsUser"is not a valid address. If address-format validation is added to the handler later, this test will break without any obvious connection to the change.🔧 Proposed fix
- userWallet := "0xNoChannelsUser" + userWallet := "0x0000000000000000000000000000000000000001"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/api/channel_v1/get_channels_test.go` at line 208, The test uses an invalid Ethereum address string in the userWallet variable ("0xNoChannelsUser"); update userWallet in get_channels_test.go to a syntactically valid Ethereum address (for example a checksummed or zero-prefixed 40-hex char address like "0x0000000000000000000000000000000000000001") so future address-format validation in the handler won't break the test; locate and replace the userWallet declaration in the test function that sets userWallet to ensure it uses a proper 0x-prefixed 40-hex character address.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clearnode/api/channel_v1/get_channels_test.go`:
- Around line 255-256: Replace the non-fatal assertion before dereferencing the
error with a fatal one: in TestGetChannels_MissingWallet and
TestGetChannels_StoreError, use require.NotNil(t, ctx.Response.Error()) instead
of assert.NotNil(t, ctx.Response.Error()) so the test stops if the error is nil
before calling ctx.Response.Error().Error(), then keep the subsequent
assert.Contains checks as-is to verify the "wallet" message.
---
Duplicate comments:
In `@clearnode/api/channel_v1/get_channels_test.go`:
- Around line 280-282: The test only asserts ctx.Response.Type ==
rpc.MsgTypeRespErr without checking the error message; add an inline comment
above that assertion in get_channels_test.go (near the failing assertion in the
TestGetChannels_* tests) explaining that this weaker check is intentional
because NewErrorPayload causes Payload.Error() to return nil when the message
contains quoted strings, and note that this differs from
TestGetChannels_MissingWallet and TestGetChannels_StoreError so future readers
understand the discrepancy.
---
Nitpick comments:
In `@clearnode/api/channel_v1/get_channels_test.go`:
- Line 19: The test currently ignores the error returned by
core.NewChannelDefaultSigner, which can hide setup failures; update the test
helper to accept t (testing.T or testing.TB) and call
core.NewChannelDefaultSigner(mockSigner) capturing the error, then assert
require.NoError(t, err) before using nodeSigner so any signer creation failure
fails clearly; reference the nodeSigner variable and
core.NewChannelDefaultSigner in your changes.
- Line 208: The test uses an invalid Ethereum address string in the userWallet
variable ("0xNoChannelsUser"); update userWallet in get_channels_test.go to a
syntactically valid Ethereum address (for example a checksummed or zero-prefixed
40-hex char address like "0x0000000000000000000000000000000000000001") so future
address-format validation in the handler won't break the test; locate and
replace the userWallet declaration in the test function that sets userWallet to
ensure it uses a proper 0x-prefixed 40-hex character address.
Comment on lines
+255
to
+256
| assert.NotNil(t, ctx.Response.Error()) | ||
| assert.Contains(t, ctx.Response.Error().Error(), "wallet") |
Contributor
There was a problem hiding this comment.
Use require.NotNil before dereferencing the error to avoid a nil-pointer panic.
assert.NotNil is non-fatal — if it fails, execution continues and the immediate .Error() call on a nil value panics, masking the real failure. This applies to both TestGetChannels_MissingWallet (lines 255–256) and TestGetChannels_StoreError (lines 307–308).
🐛 Proposed fix
- assert.NotNil(t, ctx.Response.Error())
- assert.Contains(t, ctx.Response.Error().Error(), "wallet")
+ require.NotNil(t, ctx.Response.Error())
+ assert.Contains(t, ctx.Response.Error().Error(), "wallet")- assert.NotNil(t, ctx.Response.Error())
- assert.Contains(t, ctx.Response.Error().Error(), "failed to get channels")
+ require.NotNil(t, ctx.Response.Error())
+ assert.Contains(t, ctx.Response.Error().Error(), "failed to get channels")Also applies to: 307-308
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clearnode/api/channel_v1/get_channels_test.go` around lines 255 - 256,
Replace the non-fatal assertion before dereferencing the error with a fatal one:
in TestGetChannels_MissingWallet and TestGetChannels_StoreError, use
require.NotNil(t, ctx.Response.Error()) instead of assert.NotNil(t,
ctx.Response.Error()) so the test stops if the error is nil before calling
ctx.Response.Error().Error(), then keep the subsequent assert.Contains checks
as-is to verify the "wallet" message.
Contributor
|
This PR is growing too much. It risks to become unreadable. Hence, I am merging. Great job, team! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Three related changes from the v0.5.3 → v1.0.0 migration effort (YNU-788):
channels.v1.get_channelsendpoint — New RPC method to list all channels (open, closed, challenged) for a wallet, with status/asset filtering and pagination. The v0.5.3 appstore displayed closed channels; v1.0.0 had no equivalent endpoint.GetAppSessionsPostgreSQL fix — The existing query combinedDISTINCTwithORDER BY created_at, which PostgreSQL rejects (SQLSTATE 42P10). Replaced with a subquery approach. This bug was hidden in tests because SQLite (the test DB) is lenient with this pattern.@erc7824/nitrolite-compatSDK (WIP) — Compatibility layer that wraps the v1.0.0 TS SDK behind the v0.5.3 API surface, reducing migration diffs from ~400+ lines to ~15 lines for existing apps.Changes by area
Clearnode
api/channel_v1/get_channels.goapi/channel_v1/get_channels_test.gostore/database/channel.goGetUserChannels()— GORM query with optional status/asset filters + countstore/database/channel_test.gostore/database/app_session.goDISTINCT→ subquery to resolve PostgreSQL ORDER BY conflictapi/channel_v1/interface.goGetUserChannelsadded to Store interfaceapi/channel_v1/testing.gostore/database/interface.goGetUserChannelsadded to DatabaseStore interfaceapi/rpc_router.goTypeScript SDK
sdk/ts/src/client.tsClient.getChannels(wallet, options?)wrapping the new RPC methodCompat SDK (
sdk/compat/) — WIPclient.ts,auth.ts,signers.ts,rpc.ts,events.ts,config.ts,errors.ts,types.ts,index.tsREADME.md,docs/migration-overview.md,docs/migration-onchain.md,docs/migration-offchain.mdexamples/lifecycle.tsGetAppSessions fix — detail
Before:
GetAppSessionsusedDistinct("app_sessions_v1.id")+Order("created_at DESC"). PostgreSQL requires ORDER BY columns to appear in the DISTINCT select list, so this failed with:All
get_app_sessionsRPC calls were broken on production PostgreSQL.After: Deduplication moved to a subquery (
WHERE id IN (SELECT DISTINCT id ... JOIN participants WHERE wallet = ?)), keeping the main query free to ORDER BY without conflict. Existing tests pass (SQLite). Verified manually against local PostgreSQL clearnode.get_channelsAPITest coverage
get_channelshandlerGetUserChannelsDBchannel_v1suitestore/databasesuiteSummary by CodeRabbit
New Features
Documentation
Tests
Chores