YNU-915: return success on absent-resource GET endpoints#748
Conversation
Four single-resource lookups (get_latest_state, get_home_channel,
get_escrow_channel, get_app_definition) used to respond with RespErr
when the record was absent. That counted absence as a failure in
rpc_requests_total{result="failed"}, skewing success-rate dashboards,
and forced clients to parse error strings to distinguish "not found"
from a real failure.
Make absence a successful response: the relevant payload field is now
optional/nullable. Handlers drop the nil→Fail branch. Go and TS SDK
methods return nil/null on absence; callers explicitly check for nil
instead of treating the error path as the "no channel yet" branch.
- pkg/rpc/api.go: pointerize Channel/State/Definition fields
- nitronode/api: drop nil→Fail, return success with empty payload
- sdk/go, cerebro, examples: handle nil from lookup methods
- sdk/ts, sdk/ts-compat: return T | null, drop redundant try/catch
- docs/api.yaml: mark fields optional, remove *_not_found errors
- tests: NotFound paths assert success + nil payload
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughHandlers, RPC types, SDKs, clients, CLI, examples, tests, and docs were changed so absent channels/states/app-definitions are returned as nil/null in successful RPC responses and callers handle null vs RPC errors. ChangesNullable "not found" semantics for API queries
Sequence Diagram(s)sequenceDiagram
participant Client
participant RPC_Server
participant Store
Client->>RPC_Server: channels.v1.get_home_channel(request)
RPC_Server->>Store: GetHomeChannel(wallet, asset)
Store-->>RPC_Server: (nil) // not found
RPC_Server-->>Client: 200 { channel: <omitted> } // success with nil
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
# Conflicts: # sdk/ts/test/unit/__snapshots__/public-api-drift.test.ts.snap
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk/ts-compat/src/client.ts (1)
526-541:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t collapse full RPC lookup failures into a false “not found” in
getChannelData.If all
getHomeChannelcalls fail (e.g., transport/auth outage), this loop swallows every exception and ends withChannel ... not found, which is a wrong result and can trigger incorrect caller behavior.🔧 Proposed fix
async getChannelData(_channelId: string): Promise<any> { await this.ensureAssets(); + let hadSuccessfulLookup = false; + let lastLookupError: unknown = null; for (const [, info] of this.assetsBySymbol) { try { const ch = await this.innerClient.getHomeChannel(this.userAddress, info.symbol); + hadSuccessfulLookup = true; if (ch && ch.channelId === _channelId) { return { channel: ch, state: await this.innerClient.getLatestState(this.userAddress, info.symbol, false), }; } - } catch { + } catch (err) { + lastLookupError = err; // transient RPC failure for this asset — skip } } + if (!hadSuccessfulLookup && lastLookupError) { + throw lastLookupError instanceof Error + ? new Error(`[compat] failed to query channels: ${lastLookupError.message}`) + : new Error('[compat] failed to query channels'); + } throw new Error(`Channel ${_channelId} not found`); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/ts-compat/src/client.ts` around lines 526 - 541, The loop in getChannelData swallows all exceptions from innerClient.getHomeChannel and always throws "Channel ... not found", which masks transport/auth failures; modify getChannelData to track RPC errors while iterating assetsBySymbol (e.g., a variable like lastRpcError or a flag rpcErrorOccurred) and only suppress per-asset errors for transient misses, but if any getHomeChannel call throws, rethrow a meaningful error (or the first caught exception) instead of the generic "not found"; keep ensureAssets and the success path (returning channel and latest state via innerClient.getLatestState) unchanged, but replace the unconditional throw at the end with logic that throws the tracked RPC error when present, otherwise throw the original "Channel ... not found" error.
🧹 Nitpick comments (3)
pkg/rpc/client_test.go (1)
134-162: ⚡ Quick winAdd explicit nil-payload success cases for the nullable contract.
These updated fixtures cover non-nil pointer decoding, but the behavior change is specifically about successful responses with omitted/null payload fields. Please add table rows/tests where
channel/state/definitionis absent and assertrequire.NoErrorplusnilfield values.Also applies to: 164-185, 215-246, 299-324
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/rpc/client_test.go` around lines 134 - 162, Add explicit success tests for nil/omitted nullable payloads: create additional subtests (or table rows) for TestClientV1_ChannelsV1GetHomeChannel that register a ChannelsV1GetHomeChannelResponse where Channel.State is nil and another where Channel.State is non-nil but Channel.State.Definition is nil (i.e., omit those fields), using registerSimpleHandlerV1 and the same request via client.ChannelsV1GetHomeChannel; assert require.NoError(t, err) and require.Nil(t, resp.Channel.State) or require.Nil(t, resp.Channel.State.Definition) as appropriate. Apply the same pattern to the other analogous client tests in this file (the other ranges referenced in the review) so each test covers non-nil decoding and explicit nil/omitted payload success cases.sdk/go/client_test.go (1)
20-218: ⚡ Quick winAdd explicit nil-payload tests for new not-found semantics.
These updates validate pointer payloads on happy paths, but this PR’s behavioral contract is “absent resource returns
(nil, nil).” Please add cases forGetHomeChannel,GetEscrowChannel,GetLatestState, andGetAppDefinitionwhere RPC returnsnilpayload fields and asserterr == nilwith nil result.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/go/client_test.go` around lines 20 - 218, Add tests that exercise the "not-found returns (nil, nil)" contract by registering RPC responses with nil payloads and asserting the client returns (nil, nil): for GetHomeChannel register rpc.ChannelsV1GetHomeChannelMethod response rpc.ChannelsV1GetHomeChannelResponse{Channel: nil} then call Client.GetHomeChannel(...) and assert err == nil and result == nil; for GetEscrowChannel register rpc.ChannelsV1GetEscrowChannelMethod response rpc.ChannelsV1GetEscrowChannelResponse{Channel: nil} then call Client.GetEscrowChannel(...) and assert err == nil and result == nil; for GetLatestState register rpc.ChannelsV1GetLatestStateMethod response rpc.ChannelsV1GetLatestStateResponse{State: nil} then call Client.GetLatestState(...) and assert err == nil and result == nil; and for GetAppDefinition register rpc.AppSessionsV1GetAppDefinitionMethod response rpc.AppSessionsV1GetAppDefinitionResponse{Definition: nil} then call Client.GetAppDefinition(...) and assert err == nil and result == nil; reuse NewMockDialer, rpc.NewClient(mockDialer) and the same test structure as the existing happy-path tests.nitronode/api/channel_v1/get_latest_state_test.go (1)
242-242: ⚡ Quick winAdd a doc comment for the new exported test function.
Please add a short doc comment above
TestGetLatestState_NotFound.Suggested patch
+// TestGetLatestState_NotFound verifies that absent latest state returns success with a nil state payload. func TestGetLatestState_NotFound(t *testing.T) {As per coding guidelines,
**/*.go: Followgofmtformatting and add doc comments on all exported names in Go code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nitronode/api/channel_v1/get_latest_state_test.go` at line 242, Add a short doc comment above the exported test function TestGetLatestState_NotFound describing what the test verifies (e.g., that GetLatestState returns a not-found error/HTTP 404 when no state exists), ensure the comment starts with the function name ("TestGetLatestState_NotFound ..."), and then run gofmt/goimports to keep formatting consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nitronode/api/channel_v1/get_latest_state_test.go`:
- Around line 266-267: Replace the non-fatal assertions guarding
ctx.Response.Payload with fail-fast require assertions so the test stops before
any payload dereference: change assert.NotNil(t, ctx.Response.Payload) to
require.NotNil(t, ctx.Response.Payload) and change assert.Nil(t,
ctx.Response.Error()) to require.Nil(t, ctx.Response.Error()), and add/ensure
the testify/require import is present; update the test function (in
get_latest_state_test.go) where ctx.Response is checked to use require.* instead
of assert.*.
---
Outside diff comments:
In `@sdk/ts-compat/src/client.ts`:
- Around line 526-541: The loop in getChannelData swallows all exceptions from
innerClient.getHomeChannel and always throws "Channel ... not found", which
masks transport/auth failures; modify getChannelData to track RPC errors while
iterating assetsBySymbol (e.g., a variable like lastRpcError or a flag
rpcErrorOccurred) and only suppress per-asset errors for transient misses, but
if any getHomeChannel call throws, rethrow a meaningful error (or the first
caught exception) instead of the generic "not found"; keep ensureAssets and the
success path (returning channel and latest state via innerClient.getLatestState)
unchanged, but replace the unconditional throw at the end with logic that throws
the tracked RPC error when present, otherwise throw the original "Channel ...
not found" error.
---
Nitpick comments:
In `@nitronode/api/channel_v1/get_latest_state_test.go`:
- Line 242: Add a short doc comment above the exported test function
TestGetLatestState_NotFound describing what the test verifies (e.g., that
GetLatestState returns a not-found error/HTTP 404 when no state exists), ensure
the comment starts with the function name ("TestGetLatestState_NotFound ..."),
and then run gofmt/goimports to keep formatting consistent.
In `@pkg/rpc/client_test.go`:
- Around line 134-162: Add explicit success tests for nil/omitted nullable
payloads: create additional subtests (or table rows) for
TestClientV1_ChannelsV1GetHomeChannel that register a
ChannelsV1GetHomeChannelResponse where Channel.State is nil and another where
Channel.State is non-nil but Channel.State.Definition is nil (i.e., omit those
fields), using registerSimpleHandlerV1 and the same request via
client.ChannelsV1GetHomeChannel; assert require.NoError(t, err) and
require.Nil(t, resp.Channel.State) or require.Nil(t,
resp.Channel.State.Definition) as appropriate. Apply the same pattern to the
other analogous client tests in this file (the other ranges referenced in the
review) so each test covers non-nil decoding and explicit nil/omitted payload
success cases.
In `@sdk/go/client_test.go`:
- Around line 20-218: Add tests that exercise the "not-found returns (nil, nil)"
contract by registering RPC responses with nil payloads and asserting the client
returns (nil, nil): for GetHomeChannel register
rpc.ChannelsV1GetHomeChannelMethod response
rpc.ChannelsV1GetHomeChannelResponse{Channel: nil} then call
Client.GetHomeChannel(...) and assert err == nil and result == nil; for
GetEscrowChannel register rpc.ChannelsV1GetEscrowChannelMethod response
rpc.ChannelsV1GetEscrowChannelResponse{Channel: nil} then call
Client.GetEscrowChannel(...) and assert err == nil and result == nil; for
GetLatestState register rpc.ChannelsV1GetLatestStateMethod response
rpc.ChannelsV1GetLatestStateResponse{State: nil} then call
Client.GetLatestState(...) and assert err == nil and result == nil; and for
GetAppDefinition register rpc.AppSessionsV1GetAppDefinitionMethod response
rpc.AppSessionsV1GetAppDefinitionResponse{Definition: nil} then call
Client.GetAppDefinition(...) and assert err == nil and result == nil; reuse
NewMockDialer, rpc.NewClient(mockDialer) and the same test structure as the
existing happy-path tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d11ea131-2d5b-4f8c-b770-6f95855a6991
⛔ Files ignored due to path filters (1)
sdk/ts/test/unit/__snapshots__/public-api-drift.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (20)
cerebro/commands.godocs/api.yamlnitronode/api/app_session_v1/get_app_definition.gonitronode/api/app_session_v1/get_app_definition_test.gonitronode/api/channel_v1/get_escrow_channel.gonitronode/api/channel_v1/get_escrow_channel_test.gonitronode/api/channel_v1/get_home_channel.gonitronode/api/channel_v1/get_home_channel_test.gonitronode/api/channel_v1/get_latest_state.gonitronode/api/channel_v1/get_latest_state_test.gopkg/rpc/api.gopkg/rpc/client_test.gosdk/go/app_session.gosdk/go/channel.gosdk/go/client_test.gosdk/go/examples/challenge/main.gosdk/ts-compat/src/client.tssdk/ts/src/client.tssdk/ts/src/rpc/api.tssdk/ts/test/unit/client.test.ts
ihsraham
left a comment
ihsraham
left a comment
There was a problem hiding this comment.
I would keep this open for two closure items.
The backend/API changes and the main Go/TS SDK paths look aligned with the new absence semantics, and the current checks are green. Before this is ready, I would address:
sdk/ts-compatstill swallows realgetHomeChannel/getLatestStatefailures ingetChannelDataand reports them asChannel ... not found(inline).- YNU-915 AC8 asks for
test/integrationabsent-then-present coverage per endpoint, but this diff only adds handler/unit coverage and does not add or update integration tests.
The unresolved assert / require note in get_latest_state_test.go looks non-blocking to me.
| } | ||
| } catch { | ||
| // no channel for this asset | ||
| // transient RPC failure for this asset — skip |
There was a problem hiding this comment.
With the new nullable semantics, getHomeChannel returns null for normal absence, so exceptions from this block are genuine failures. The catch also covers the matched channel’s getLatestState call; if that request fails, getChannelData reports Channel ... not found instead of surfacing the RPC/auth/transport error.
Please rethrow the lookup error, or only throw not-found after all lookups completed successfully and returned no match.
There was a problem hiding this comment.
Yep, good catch. Rewrote getChannelData in 50f812e to track lookup errors per asset and rethrow if every getHomeChannel call failed — only per-asset misses are suppressed now. getLatestState on the matched channel also runs outside the catch so its failures surface as well.
nksazonov
left a comment
nksazonov
left a comment
There was a problem hiding this comment.
Good work — clean approach, and the core change (nil instead of error for absent resources) is implemented consistently across all four endpoints. A few things need attention before landing.
Out-of-diff notes:
sdk/ts/examples/example-app/src/components/WalletDashboard.tsx:209 — Inside the allowance-error recovery path for handleCheckpoint, state.homeLedger.blockchainId is accessed without a null check. getLatestState now returns null for absent state; if this path is hit before a channel exists, it throws TypeError: Cannot read properties of null. Fix: guard with if (!state) throw new Error('no state available for approval'); before the blockchainId access.
sdk/ts/examples/example-app/src/components/ActionModal.tsx:123 — Same issue: latestState.homeLedger.blockchainId immediately after getLatestState(...) with no null check. Same fix pattern.
sdk/ts-compat/src/rpc.ts:434 — parseGetAppDefinitionResponse is a public exported compat function. When the server omits definition (new nil-response case), it returns { params: {} }. Callers who access params.protocol, params.quorum, or params.participants get undefined silently — no error, no null signal. Fix: return { params: null } and document that callers must check before accessing fields.
sdk/ts-compat/test/unit/ — The nil-to-throw bridges added in NitroliteClient.getAppDefinition and NitroliteClient.getEscrowChannel (both throw when inner SDK returns null) have no tests. Add a test for each that mocks innerClient to return null and verifies the expected throw.
- sdk/ts-compat: fix getChannelData to surface lookup errors instead of reporting "not found", and reject matched channels with null latest state (would have leaked state=null to v0.5.3 callers). - nitronode/get_latest_state_test: use require before payload deref in the NotFound test; add doc comment. - pkg/rpc/client_test, sdk/go/client_test, sdk/ts/test/unit/client.test: add absence-semantics coverage for getHomeChannel, getEscrowChannel, getLatestState, getAppDefinition, and the nil-state guards in checkpoint, closeHomeChannel, and submitAppSessionDeposit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@ihsraham both closure items addressed in 50f812e:
Also picked up @nksazonov's test-coverage notes — added |
ihsraham
left a comment
ihsraham
left a comment
There was a problem hiding this comment.
Re-reviewed the latest head 50f812eae05378ee62957cd26c1017df8efc337a and the author replies.
The added Go/RPC/TS unit coverage and the fail-fast assertion change look good. I am not keeping the test/integration item as a blocker if YNU-915 was updated as described in the author reply.
One compat issue remains: the getChannelData fix still converts partial lookup failures into Channel ... not found.
| } | ||
| return { channel: ch, state }; | ||
| } | ||
| if (!sawSuccessfulLookup && lookupError) { |
There was a problem hiding this comment.
This still masks partial lookup failures. If one asset lookup throws and any other asset lookup succeeds without matching, sawSuccessfulLookup is true and this falls through to Channel ... not found even though the requested channel may be behind the failed lookup.
With the nullable contract, misses come back as null; a caught exception means that asset was not inspected. Please rethrow when lookupError is set before emitting not-found, unless a matching channel was already found.
There was a problem hiding this comment.
Yep, you're right — the sawSuccessfulLookup gate let partial failures slip through to "not found". Dropped it in ea3dcb5. Now any unresolved lookupError rethrows before the not-found path, since a caught exception means that asset wasn't inspected and we can't conclude absence.
Per @ihsraham: a caught exception means that asset was not inspected, so a fall-through "not found" misreports state when the requested channel may have been behind a failed lookup. Drop the sawSuccessfulLookup gate and rethrow whenever lookupError is set. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ihsraham
left a comment
ihsraham
left a comment
There was a problem hiding this comment.
Approving from my side. The latest getChannelData update now rethrows any captured lookup error before the not-found path, so the partial-failure case is covered.
I also rechecked the author replies and the added nil-response coverage from the prior commit. No remaining blocker from my previous review.
# Conflicts: # docs/api.yaml # sdk/ts/src/client.ts
## Summary Patch release that re-publishes \`@yellow-org/sdk-compat\` with a correct build. The \`@yellow-org/sdk-compat@1.3.0\` tarball shipped a stale \`dist/\` (built May 5), so it is missing runtime changes from #699 (ts-compat runtime migration surface), #748 (absent-resource GET returns), and #758 (audit final session-key changes). \`@yellow-org/sdk@1.3.0\` rebuilt correctly via its existing \`prepublishOnly\` hook. ## Changes - Bump \`@yellow-org/sdk\`, \`@yellow-org/sdk-compat\`, \`@yellow-org/sdk-mcp\` → \`1.3.1\` (strict-mirror policy) - Add \`prepublishOnly: clean + build:prod\` to \`@yellow-org/sdk-compat\` - Tighten \`@yellow-org/sdk-mcp\` \`prepublishOnly\` to clean \`dist/\` and \`content/\` before build ## Post-merge - [ ] Deprecate \`@yellow-org/sdk-compat@1.3.0\` on npm - [ ] Tag \`v1.3.1\` on stable HEAD - [ ] Local publish \`@yellow-org/sdk@1.3.1\` + \`@yellow-org/sdk-compat@1.3.1\` + \`@yellow-org/sdk-mcp@1.3.1\` 🤖 Generated with [Claude Code](https://claude.com/claude-code)
3 participants
Summary
Four single-resource lookups (
channels.v1.get_latest_state,channels.v1.get_home_channel,channels.v1.get_escrow_channel,app_sessions.v1.get_app_definition) used to respond withRespErrwhen the record was absent. That counted "expected absence" as a failure inrpc_requests_total{result="failed"}, skewing success-rate dashboards, and forced clients to parse error strings to distinguish "not found" from a real failure.This PR makes absence a successful response: the relevant payload field is now optional/nullable. Handlers drop the
nil → Failbranch. Go and TS SDK methods returnnil/nullon absence; callers explicitly check for nil instead of treating the error path as the "no channel yet" branch.Notion: https://www.notion.so/yellow-net/Return-empty-response-instead-of-error-for-absent-resource-GET-endpoints-35d75b8feb0480bcba31d203a64bc208
Changes
pkg/rpc/api.go— pointerizeChannel/State/Definitionresponse fields,omitemptynitronode/api/channel_v1/{get_latest_state,get_home_channel,get_escrow_channel}.go,app_session_v1/get_app_definition.go— dropnil → Fail, return success with empty payloadsdk/go/{channel,app_session}.go,cerebro/commands.go,sdk/go/examples/challenge/main.go— return(nil, nil)on absence; callers (Deposit,Withdraw,Transfer,Acknowledge,CloseHomeChannel,Checkpoint,SubmitAppSessionDeposit) handle nil explicitlysdk/ts/src/{rpc/api.ts,client.ts}— returnPromise<T | null>, drop redundanttry/catcharoundgetLatestStatein high-level ops; explicit nil checks incloseHomeChannel/checkpoint/submitAppSessionDepositsdk/ts-compat/src/client.ts— nil-aware (getChannelData, asset scans,getAppDefinition,getEscrowChannel)docs/api.yaml— mark 4 response fieldsoptional: true, remove*_not_founderrors*_NotFoundpaths now assert success + nil payload; success paths addrequire.NotNil; newNotFoundcases forget_latest_stateandget_escrow_channel; TS public-API drift snapshot refreshedTest plan
go test ./...passesgo vet ./...cleancd sdk/ts && npm testpasses (154/154)cd sdk/ts-compat && npm testpasses (34/34)cd sdk/ts && npm run typecheckcleancd sdk/ts-compat && npm run typecheckcleanget_latest_statefor a wallet with no state returnsRespwithstateomitted,rpc_requests_total{result="success"}incrementsCompatibility
Wire-shape change: response payload fields become optional. SDK ≥ this PR works against new nitronode. Older SDKs see the response unchanged in the success case; they would only fail on absence if they assumed the field was always populated. Roll out nitronode first, then bump SDK consumers.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
Tests
Examples