YNU-900: stamp application_id on writes and expose via SDKs#696
Open
YNU-900: stamp application_id on writes and expose via SDKs#696
Conversation
philanton
requested review from
dimast-x,
ihsraham and
nksazonov
as code owners
Contributor
📝 WalkthroughWalkthroughThreads a client-declared ApplicationID (WS query Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SDK
participant WebsocketNode
participant Handler
participant Store
participant DB
participant Metrics
Client->>SDK: connect (may include ApplicationID)
SDK->>WebsocketNode: WS dial URL (app_id query)
WebsocketNode->>WebsocketNode: parse & validate app_id
WebsocketNode->>Handler: deliver request (context includes applicationID)
Handler->>Store: StoreUserState(state, applicationID)
Store->>DB: INSERT state (application_id = applicationID or NULL)
Store->>Metrics: buffer IncUserState(..., applicationID)
Handler->>Store: RecordTransaction(tx, applicationID)
Store->>DB: INSERT transaction (application_id = applicationID or NULL)
Store->>Metrics: buffer RecordTransaction(..., applicationID)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
clearnode/store/database/database.go (1)
123-125:⚠️ Potential issue | 🟠 MajorPropagate SQLite auto-migration failures.
migrateSqlitereturns theAutoMigrateerror, butconnectToSqliteignores it and logs success. If the newState/Transactionmigration fails, startup continues with an incomplete schema.🐛 Proposed fix
- // Migrate sqlite - migrateSqlite(db) + // Migrate sqlite + if err := migrateSqlite(db); err != nil { + return nil, fmt.Errorf("failed to auto-migrate sqlite: %w", err) + } log.Println("Successfully auto-migrated")As per coding guidelines, Go error handling: always check and return errors, never ignore with
_.Also applies to: 217-217
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/store/database/database.go` around lines 123 - 125, connectToSqlite currently calls migrateSqlite(db) and unconditionally logs "Successfully auto-migrated"; instead capture and handle the error returned by migrateSqlite (from function migrateSqlite) and propagate it up from connectToSqlite (or return a wrapped error) so startup fails on migration errors; update the call site to check the returned error and remove the misleading success log unless migrateSqlite returned nil.clearnode/api/channel_v1/request_creation_test.go (1)
106-119:⚠️ Potential issue | 🟡 MinorAssert the propagated connection application ID instead of accepting anything.
These tests now match the new arity, but
mock.Anythingwould still pass ifRequestCreationstopped using the connection’sapp_id. Setctx.Storagewith a concrete value and expect that exact value inRecordTransaction/StoreUserState.🧪 Proposed test tightening
+ applicationID := "client-app" mockTxStore.On("RecordTransaction", mock.MatchedBy(func(tx core.Transaction) bool { // For home_deposit: fromAccount is homeChannelID, toAccount is userWallet return tx.TxType == core.TransactionTypeHomeDeposit && tx.ToAccount == userWallet && tx.FromAccount != "" // homeChannelID will be set by handler - }), mock.Anything).Return(nil).Once() + }), applicationID).Return(nil).Once() mockTxStore.On("StoreUserState", mock.MatchedBy(func(state core.State) bool { return state.UserWallet == userWallet && state.Asset == asset && state.Version == 1 && state.Epoch == 0 && state.NodeSig != nil && state.HomeChannelID != nil - }), mock.Anything).Return(nil).Once() + }), applicationID).Return(nil).Once() @@ ctx := &rpc.Context{ Context: context.Background(), + Storage: rpc.NewSafeStorage(), Request: rpc.Message{ RequestID: 1, Method: rpc.ChannelsV1RequestCreationMethod.String(), Payload: payload, }, } + ctx.Storage.Set(rpc.ApplicationIDQueryParam, applicationID)Apply the same pattern to the acknowledgement success case’s
StoreUserStateexpectation.Also applies to: 135-142, 261-269, 285-292
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/api/channel_v1/request_creation_test.go` around lines 106 - 119, The test currently uses mock.Anything for the second argument of mockTxStore.RecordTransaction and StoreUserState which lets RequestCreation ignore the connection's app_id; instead set ctx.Storage to a concrete connection application ID before invoking the handler and change the expectations to match that exact value (e.g., expect the provided app_id in the matched Transaction and State objects). Update the mock.MatchedBy predicates used in the RecordTransaction and StoreUserState expectations to assert the concrete ctx.Storage value (and do the same tightening for the acknowledgement-success StoreUserState expectation referenced around the other ranges).clearnode/api/app_session_v1/submit_deposit_state_test.go (1)
180-192:⚠️ Potential issue | 🟡 MinorVerify app-session writes use the session ApplicationID.
mock.Anythinghides regressions where the handler might pass the connectionapp_idor an empty string. Since this path must stampappSession.ApplicationID, assert that exact value; optionally set a different connection tag to prove it is ignored.🧪 Proposed test tightening
mockStore.On("StoreUserState", mock.MatchedBy(func(state core.State) bool { return state.UserWallet == participant1 && state.Version == incomingUserState.Version && state.Transition.Type == core.TransitionTypeCommit && state.NodeSig != nil - }), mock.Anything).Return(nil).Once() + }), existingAppSession.ApplicationID).Return(nil).Once() @@ mockStore.On("RecordTransaction", mock.MatchedBy(func(tx core.Transaction) bool { return tx.TxType == core.TransactionTypeCommit && tx.Amount.Equal(depositAmount) && tx.ToAccount == appSessionID - }), mock.Anything).Return(nil).Once() + }), existingAppSession.ApplicationID).Return(nil).Once() @@ ctx := &rpc.Context{ Context: context.Background(), + Storage: rpc.NewSafeStorage(), Request: rpc.NewRequest(1, string(rpc.AppSessionsV1SubmitDepositStateMethod), payload), } + ctx.Storage.Set(rpc.ApplicationIDQueryParam, "connection-app")Apply the same exact-value assertion in
TestSubmitDepositState_AppRegistryDisabled.Also applies to: 205-208, 662-667, 679-682
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/api/app_session_v1/submit_deposit_state_test.go` around lines 180 - 192, The mocks in submit_deposit_state_test.go currently use mock.Anything for the connection/app ID which hides regressions; update the mocked expectations for StoreUserState and RecordTransaction (and the other occurrences in the file including TestSubmitDepositState_AppRegistryDisabled) to assert the exact app session ApplicationID is used (i.e., replace mock.Anything for the ToAccount/connection argument with a matcher that equals appSession.ApplicationID or a MatchedBy(func(s string) bool { return s == appSession.ApplicationID }) and optionally add a different connection value on the incoming request to prove it is ignored), ensuring the test verifies the handler stamps appSession.ApplicationID rather than the connection app_id or empty string.clearnode/metrics/exporter.go (1)
175-189:⚠️ Potential issue | 🟠 MajorAvoid raw client-controlled values as Prometheus labels.
applicationIDis self-declared by clients, and the currentgetApplicationIDLabelValue()function only provides a fallback for empty values (_DEFAULT). It does not validate or constrain the applicationID against a bounded set. This allows arbitrary client values to flow directly as Prometheus label values, creating unbounded time-series cardinality and potential memory/storage pressure in Prometheus and the exporter.Constrain
applicationIDto a bounded allowlist of registered applications, or bucket unknown values to a fixed set before callingWithLabelValues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/metrics/exporter.go` around lines 175 - 189, The Prometheus labels (used by userStatesTotal, transactionsTotal, transactionsAmountTotal) currently accept arbitrary client-provided application IDs via getApplicationIDLabelValue(); replace that with a bounded mapping: implement or use an allowlist/set of registered application IDs and a bucketing function (e.g., isRegisteredApplication(appID) or registeredApps map) to return the appID if present or a fixed sentinel like "_UNKNOWN_APP" or "_UNREGISTERED" otherwise, and call that mapping before passing the label to WithLabelValues so untrusted values cannot create unbounded label cardinality.
🧹 Nitpick comments (3)
clearnode/config/migrations/postgres/20260420000000_add_application_id_to_writes.sql (1)
1-12: Consider a partial index and harden the Down migration.Two small suggestions:
application_idis nullable and, per the PR objectives, most existing rows and clients without?app_id=will storeNULL. A partial index excludingNULLs will be substantially smaller and faster to maintain for this access pattern (lookups are only meaningful for non-NULL values).- The Down migration uses
IF EXISTSfor the indexes but not for theDROP COLUMNstatements — inconsistent, and will fail noisily if the column was already removed manually. Since index drops will also cascade when the column is dropped, the explicit index drops are arguably redundant.♻️ Proposed diff
-- +goose Up ALTER TABLE channel_states ADD COLUMN application_id VARCHAR(66); ALTER TABLE transactions ADD COLUMN application_id VARCHAR(66); -CREATE INDEX idx_channel_states_app_id ON channel_states(application_id); -CREATE INDEX idx_transactions_app_id ON transactions(application_id); +CREATE INDEX idx_channel_states_app_id ON channel_states(application_id) WHERE application_id IS NOT NULL; +CREATE INDEX idx_transactions_app_id ON transactions(application_id) WHERE application_id IS NOT NULL; -- +goose Down DROP INDEX IF EXISTS idx_transactions_app_id; DROP INDEX IF EXISTS idx_channel_states_app_id; -ALTER TABLE transactions DROP COLUMN application_id; -ALTER TABLE channel_states DROP COLUMN application_id; +ALTER TABLE transactions DROP COLUMN IF EXISTS application_id; +ALTER TABLE channel_states DROP COLUMN IF EXISTS application_id;On large existing
transactions/channel_statestables,CREATE INDEX(withoutCONCURRENTLY) will take anACCESS EXCLUSIVE-blocking lock for writes. If these tables are sizeable in production, consider running index creation via a separate non-transactional migration usingCREATE INDEX CONCURRENTLY(goose supports-- +goose NO TRANSACTION) to avoid write stalls during deploy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/config/migrations/postgres/20260420000000_add_application_id_to_writes.sql` around lines 1 - 12, Change the indexes to partial indexes that exclude NULLs (e.g., create idx_channel_states_app_id and idx_transactions_app_id WITH WHERE application_id IS NOT NULL) so they stay small and faster for lookups on non-NULL application_id; update the Down migration to defensively use IF EXISTS when dropping columns (DROP COLUMN IF EXISTS application_id on transactions and channel_states) or remove explicit index drops since dropping the column cascades indexes, and consider making the index creation non-transactional with CREATE INDEX CONCURRENTLY (use goose NO TRANSACTION) for large tables to avoid ACCESS EXCLUSIVE locks during deploy.sdk/ts/test/unit/config.test.ts (1)
1-47: Align imports with path alias and prefer data-driven tests.Two guideline deviations worth addressing:
- Path alias:
'../../src/config'should use@/configper the test-config import convention.- Data-driven pattern: the six
appendApplicationIDQueryParamcases share identical shape (input URL + app_id → expected assertion) and are a good fit for a.forEach()table-driven test.As per coding guidelines: "Use path alias
@/mapping tosrc/in test configurations for import paths" and "Use data-driven tests with.forEach()pattern and manual mocks instead ofjest.mock()".♻️ Sketch
-import { - APPLICATION_ID_QUERY_PARAM, - appendApplicationIDQueryParam, - type Config, - withApplicationID, -} from '../../src/config'; +import { + APPLICATION_ID_QUERY_PARAM, + appendApplicationIDQueryParam, + type Config, + withApplicationID, +} from '@/config';And for
appendApplicationIDQueryParam, consolidate the happy-path cases into a table:const cases: Array<{ name: string; url: string; appID?: string; check: (out: string) => void }> = [ { name: 'no-op on empty id', url: 'ws://host/path', check: (o) => expect(o).toBe('ws://host/path') }, { name: 'adds app_id with no query', url: 'ws://host/path', appID: 'my-app', check: (o) => expect(o).toBe('ws://host/path?app_id=my-app') }, // ... ]; cases.forEach(({ name, url, appID, check }) => it(name, () => check(appendApplicationIDQueryParam(url, appID))));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts/test/unit/config.test.ts` around lines 1 - 47, Update the imports to use the path alias by replacing '../../src/config' with '@/config' for the imported symbols (APPLICATION_ID_QUERY_PARAM, appendApplicationIDQueryParam, Config, withApplicationID), then refactor the appendApplicationIDQueryParam tests into a data-driven table: create an array of case objects (name, url, optional appID, and a check function) and run cases.forEach(({name,url,appID,check}) => it(name, () => check(appendApplicationIDQueryParam(url, appID)))); keep the existing withApplicationID unit as-is referencing withApplicationID and Config and ensure one of the case checks uses new URL(out).searchParams.get(APPLICATION_ID_QUERY_PARAM) where needed.clearnode/api/app_session_v1/submit_app_state_test.go (1)
355-993: Consider a stricter matcher for at least one test.Using
mock.Anythingfor theapplicationIDargument is fine for the majority of cases, but since the contract is that app-session handlers must stamp the session'sApplicationID(not the connection tag), at least one representative test (e.g.,TestSubmitAppState_WithdrawIntent_SuccessorTestSubmitAppState_CloseIntent_Success) would be more valuable if it matched"test-app"explicitly — otherwise a regression that accidentally passesrpc.GetApplicationID(c)(empty in these tests) would still pass.♻️ Example
-mockStore.On("RecordTransaction", mock.Anything, mock.Anything).Return(nil) +mockStore.On("RecordTransaction", mock.Anything, "test-app").Return(nil)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/api/app_session_v1/submit_app_state_test.go` around lines 355 - 993, The test suite uses mock.Anything for the applicationID in some GetApp expectations which hides regressions; update at least one representative test (e.g., TestSubmitAppState_CloseIntent_Success) so the mock expectation on mockStore uses the explicit application ID "test-app" instead of mock.Anything — locate the mock call mockStore.On("GetApp", ... ) in TestSubmitAppState_CloseIntent_Success and replace the loose matcher with the literal "test-app" to ensure the handler stamps the session's ApplicationID correctly.
🤖 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/store/database/transaction.go`:
- Around line 33-35: The model's ApplicationID (field ApplicationID on the
transaction/state structs) can exceed the DB VARCHAR(66) limit because it is
taken directly from r.URL.Query().Get(ApplicationIDQueryParam); add validation
at ingress where applicationID is read (the code that references
ApplicationIDQueryParam in the RPC node request handling) to enforce or sanitize
length: check len(applicationID) and if >66 either truncate to 66 bytes (or set
to nil/empty) before assigning to the model, or return a 4xx error; also add a
defensive guard right before persistence in the methods that populate
Transaction.ApplicationID / State.ApplicationID to ensure any programmatic
caller cannot bypass the check. Ensure you reference ApplicationIDQueryParam,
the local applicationID var, and the ApplicationID struct fields when applying
the changes.
In `@pkg/rpc/node.go`:
- Line 213: The request handler reads applicationID via
r.URL.Query().Get(ApplicationIDQueryParam) and forwards it to persistence where
transactions.application_id/channel_states.application_id are VARCHAR(66); add
ingress validation to enforce a max length of 66 (e.g., if len(applicationID) >
66 either truncate and process while logging a warning via the request logger or
reject with a 400 and clear error message), and ensure the same
validated/truncated value is used for downstream calls like StoreUserState and
RecordTransaction to avoid per-connection DB errors; update the handler around
the applicationID retrieval to perform this check and log accordingly.
---
Outside diff comments:
In `@clearnode/api/app_session_v1/submit_deposit_state_test.go`:
- Around line 180-192: The mocks in submit_deposit_state_test.go currently use
mock.Anything for the connection/app ID which hides regressions; update the
mocked expectations for StoreUserState and RecordTransaction (and the other
occurrences in the file including TestSubmitDepositState_AppRegistryDisabled) to
assert the exact app session ApplicationID is used (i.e., replace mock.Anything
for the ToAccount/connection argument with a matcher that equals
appSession.ApplicationID or a MatchedBy(func(s string) bool { return s ==
appSession.ApplicationID }) and optionally add a different connection value on
the incoming request to prove it is ignored), ensuring the test verifies the
handler stamps appSession.ApplicationID rather than the connection app_id or
empty string.
In `@clearnode/api/channel_v1/request_creation_test.go`:
- Around line 106-119: The test currently uses mock.Anything for the second
argument of mockTxStore.RecordTransaction and StoreUserState which lets
RequestCreation ignore the connection's app_id; instead set ctx.Storage to a
concrete connection application ID before invoking the handler and change the
expectations to match that exact value (e.g., expect the provided app_id in the
matched Transaction and State objects). Update the mock.MatchedBy predicates
used in the RecordTransaction and StoreUserState expectations to assert the
concrete ctx.Storage value (and do the same tightening for the
acknowledgement-success StoreUserState expectation referenced around the other
ranges).
In `@clearnode/metrics/exporter.go`:
- Around line 175-189: The Prometheus labels (used by userStatesTotal,
transactionsTotal, transactionsAmountTotal) currently accept arbitrary
client-provided application IDs via getApplicationIDLabelValue(); replace that
with a bounded mapping: implement or use an allowlist/set of registered
application IDs and a bucketing function (e.g., isRegisteredApplication(appID)
or registeredApps map) to return the appID if present or a fixed sentinel like
"_UNKNOWN_APP" or "_UNREGISTERED" otherwise, and call that mapping before
passing the label to WithLabelValues so untrusted values cannot create unbounded
label cardinality.
In `@clearnode/store/database/database.go`:
- Around line 123-125: connectToSqlite currently calls migrateSqlite(db) and
unconditionally logs "Successfully auto-migrated"; instead capture and handle
the error returned by migrateSqlite (from function migrateSqlite) and propagate
it up from connectToSqlite (or return a wrapped error) so startup fails on
migration errors; update the call site to check the returned error and remove
the misleading success log unless migrateSqlite returned nil.
---
Nitpick comments:
In `@clearnode/api/app_session_v1/submit_app_state_test.go`:
- Around line 355-993: The test suite uses mock.Anything for the applicationID
in some GetApp expectations which hides regressions; update at least one
representative test (e.g., TestSubmitAppState_CloseIntent_Success) so the mock
expectation on mockStore uses the explicit application ID "test-app" instead of
mock.Anything — locate the mock call mockStore.On("GetApp", ... ) in
TestSubmitAppState_CloseIntent_Success and replace the loose matcher with the
literal "test-app" to ensure the handler stamps the session's ApplicationID
correctly.
In
`@clearnode/config/migrations/postgres/20260420000000_add_application_id_to_writes.sql`:
- Around line 1-12: Change the indexes to partial indexes that exclude NULLs
(e.g., create idx_channel_states_app_id and idx_transactions_app_id WITH WHERE
application_id IS NOT NULL) so they stay small and faster for lookups on
non-NULL application_id; update the Down migration to defensively use IF EXISTS
when dropping columns (DROP COLUMN IF EXISTS application_id on transactions and
channel_states) or remove explicit index drops since dropping the column
cascades indexes, and consider making the index creation non-transactional with
CREATE INDEX CONCURRENTLY (use goose NO TRANSACTION) for large tables to avoid
ACCESS EXCLUSIVE locks during deploy.
In `@sdk/ts/test/unit/config.test.ts`:
- Around line 1-47: Update the imports to use the path alias by replacing
'../../src/config' with '@/config' for the imported symbols
(APPLICATION_ID_QUERY_PARAM, appendApplicationIDQueryParam, Config,
withApplicationID), then refactor the appendApplicationIDQueryParam tests into a
data-driven table: create an array of case objects (name, url, optional appID,
and a check function) and run cases.forEach(({name,url,appID,check}) => it(name,
() => check(appendApplicationIDQueryParam(url, appID)))); keep the existing
withApplicationID unit as-is referencing withApplicationID and Config and ensure
one of the case checks uses new
URL(out).searchParams.get(APPLICATION_ID_QUERY_PARAM) where needed.
🪄 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: 7036bdb4-732d-4ab9-b33c-37f20a5eb9e3
📒 Files selected for processing (41)
clearnode/api/app_session_v1/handler.goclearnode/api/app_session_v1/interface.goclearnode/api/app_session_v1/rebalance_app_sessions.goclearnode/api/app_session_v1/rebalance_app_sessions_test.goclearnode/api/app_session_v1/submit_app_state.goclearnode/api/app_session_v1/submit_app_state_test.goclearnode/api/app_session_v1/submit_deposit_state.goclearnode/api/app_session_v1/submit_deposit_state_test.goclearnode/api/app_session_v1/testing.goclearnode/api/channel_v1/handler.goclearnode/api/channel_v1/interface.goclearnode/api/channel_v1/request_creation.goclearnode/api/channel_v1/request_creation_test.goclearnode/api/channel_v1/submit_state.goclearnode/api/channel_v1/submit_state_test.goclearnode/api/channel_v1/testing.goclearnode/api/metric_store.goclearnode/config/migrations/postgres/20260420000000_add_application_id_to_writes.sqlclearnode/metrics/exporter.goclearnode/metrics/interface.goclearnode/store/database/blockchain_action_test.goclearnode/store/database/channel_test.goclearnode/store/database/database.goclearnode/store/database/db_store_test.goclearnode/store/database/interface.goclearnode/store/database/state.goclearnode/store/database/state_test.goclearnode/store/database/test/postgres_integration_test.goclearnode/store/database/testing.goclearnode/store/database/transaction.goclearnode/store/database/transaction_test.gopkg/rpc/context.gopkg/rpc/context_internal_test.gopkg/rpc/node.gosdk/go/client.gosdk/go/config.gosdk/go/config_test.gosdk/ts/src/client.tssdk/ts/src/config.tssdk/ts/src/index.tssdk/ts/test/unit/config.test.ts
nksazonov
approved these changes
Apr 21, 2026
Contributor
nksazonov
left a comment
nksazonov
left a comment
There was a problem hiding this comment.
Generally approve, although with some suggestions
Treat the WebSocket app_id query parameter as an advisory origin tag. The clearnode reads it during the HTTP upgrade, stores it in per-connection SafeStorage, and persists it on channel_states and transactions writes originating from client requests. App-session handlers use the session's own ApplicationID rather than the connection tag; rebalance now rejects batches mixing sessions from different applications. Metrics (user_states_total, transactions_total, transactions_amount_total) carry an application_id label, using _DEFAULT when unset. The Go and TypeScript SDKs expose a WithApplicationID / withApplicationID option that appends ?app_id= to the dial URL. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- pkg/app: new ApplicationIDRegex (^[a-z0-9_-]{1,66}$) and IsValidApplicationID helper
- pkg/rpc: reject WS upgrade with 400 when app_id query param is malformed
- clearnode/api/app_session_v1: reject create_app_session with invalid application id
- clearnode/api/app_session_v1: move shared applicationID var into useStoreInTx scope in rebalance
- clearnode/api/app_session_v1: document why handlers use session's ApplicationID vs connection tag
- clearnode/metrics: document application_id label cardinality risk
- sdk/ts: re-export APPLICATION_ID_QUERY_PARAM and appendApplicationIDQueryParam
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
philanton
force-pushed
the
feat/app-id-transactions
branch
from
f0c74cd to
d427d73
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
clearnode/store/database/database.go (1)
122-124:⚠️ Potential issue | 🟠 MajorReturn SQLite migration failures instead of logging success.
migrateSqlitenow migratesStateandTransaction, butconnectToSqlitestill ignores its returned error. If the new schema migration fails, startup proceeds with an incomplete schema.Proposed fix
// Migrate sqlite - migrateSqlite(db) + if err := migrateSqlite(db); err != nil { + return nil, fmt.Errorf("failed to auto-migrate sqlite: %w", err) + } log.Println("Successfully auto-migrated")As per coding guidelines, Go error handling: always check and return errors, never ignore with
_.Also applies to: 216-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/store/database/database.go` around lines 122 - 124, connectToSqlite is currently calling migrateSqlite and ignoring its error, so migration failures (for State and Transaction) allow startup to continue with an incomplete schema; update connectToSqlite (and the other call site around lines 216-220) to capture the error returned by migrateSqlite, propagate it (return it) up to the caller instead of discarding it, and ensure any callers of connectToSqlite handle that error; locate the migrateSqlite and connectToSqlite symbols and change the call sites to check "err := migrateSqlite(db)" and return that err if non-nil so startup fails on migration errors.
🧹 Nitpick comments (4)
sdk/ts/src/config.ts (1)
84-88: Consider validating/trimmingappIDinput.
withApplicationID('')will setconfig.applicationID = '', whichappendApplicationIDQueryParamthen treats as falsy and silently ignores. A whitespace-only value would be stored and sent verbatim to the server. Since the server column isVARCHAR(66), consider rejecting empty/whitespace inputs (or inputs exceeding 66 chars) early to fail fast at config time rather than silently dropping or producing a server-side error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts/src/config.ts` around lines 84 - 88, withApplicationID currently assigns the raw appID to config.applicationID allowing empty/whitespace or too-long values; update withApplicationID to trim the input, validate that the trimmed value is non-empty and no longer than 66 characters, and if invalid throw a clear error instead of assigning; set config.applicationID = trimmedValue when valid so appendApplicationIDQueryParam sees a normalized, validated value.sdk/go/config.go (1)
50-62: Optional: validateapplicationIDclient-side before connecting.The server rejects invalid
app_idvalues with HTTP 400 during the WebSocket upgrade (seepkg/rpc/node.goServeHTTP). The SDK user will only see this as an opaquerpcClient.Startfailure far from where they set the option. Sincepkg/app.ApplicationIDRegexis exported, you could surface a clearer error here whenapplicationID != ""and doesn't match, instead of deferring to a dial failure.Not a blocker — advisory.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/go/config.go` around lines 50 - 62, The appendApplicationIDQueryParam function should proactively validate non-empty applicationID against the exported pkg/app.ApplicationIDRegex and return a clear error if it doesn't match instead of deferring to a dial failure; update appendApplicationIDQueryParam to import pkg/app, test applicationID against app.ApplicationIDRegex when applicationID != "", and return a descriptive error (e.g., "invalid applicationID: does not match ApplicationIDRegex") before attempting url.Parse so callers get immediate, actionable feedback.pkg/app/app_v1.go (1)
14-26: Clarify the relationship betweenAppIDV1Regexand the newApplicationIDRegex.
AppIDV1Regex(^[a-z0-9][-a-z0-9]{0,65}$) andApplicationIDRegex(^[a-z0-9_-]{1,66}$) look nearly identical but differ in three ways: the new one permits_, permits leading-/_/digit-only/single-char, and is used for the advisory-tag column. Neither is a subset of the other usage-wise (the advisory tag is looser, so a valid registered ID passes both, but a valid advisory tag may not be a valid registered ID).Consider a short doc note on each regex cross-referencing the other and explicitly stating they are intentionally distinct (registered-identity vs. advisory-tag), so future maintainers don't accidentally use the wrong one. This directly ties into the
create_app_session.gosite where the choice matters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/app/app_v1.go` around lines 14 - 26, Add clarifying comments next to AppIDV1Regex and ApplicationIDRegex stating they are intentionally different: document that AppIDV1Regex enforces the registered application identifier rules (no leading dash, no underscore, length 1..66 with first char letter/digit), while ApplicationIDRegex is the looser advisory-tag format (allows underscores and leading dash/underscore and single-char/digit-only) used for the advisory-tag DB column; cross-reference the other regex symbol and mention that IsValidApplicationID uses ApplicationIDRegex for advisory checks and that create_app_session.go must use AppIDV1Regex when validating registered IDs to avoid mixing usages.clearnode/metrics/interface.go (1)
56-56: Add a doc comment for the exported constructor.
NewNoopRuntimeMetricExporteris exported, so it should have a leading doc comment.📝 Proposed fix
+// NewNoopRuntimeMetricExporter returns a RuntimeMetricExporter that discards all metric updates. func NewNoopRuntimeMetricExporter() RuntimeMetricExporter { return noopRuntimeMetricExporter{} }As per coding guidelines, exported names must have doc comments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/metrics/interface.go` at line 56, Add a doc comment above the exported constructor NewNoopRuntimeMetricExporter describing what the function returns and its purpose (i.e., it constructs and returns a no-op implementation of RuntimeMetricExporter used when metrics are disabled or not available), so the exported symbol has a proper leading doc comment per project guidelines.
🤖 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/app_session_v1/create_app_session.go`:
- Around line 44-47: Change the validation for Definition.Application to use the
V1 app ID rules: replace the current check that relies on app.ApplicationIDRegex
with a validation against app.AppIDV1Regex (or a helper that uses it), i.e.
update the condition around
app.IsValidApplicationID(reqPayload.Definition.Application) to validate using
AppIDV1Regex so registered app IDs follow the V1 pattern (`AppIDV1Regex`) and
will match registry lookups downstream (references: IsValidApplicationID,
reqPayload.Definition.Application, AppIDV1Regex).
In `@clearnode/api/app_session_v1/rebalance_app_sessions.go`:
- Around line 108-114: The current check in rebalance logic uses applicationID
and appSession.ApplicationID and can produce a confusing message when one side
is empty; update the mismatch branch (where rpc.Errorf is returned) to detect if
either applicationID or appSession.ApplicationID is empty and return a clearer
message—e.g. when applicationID == "" return rpc.Errorf("cannot rebalance tagged
session %q with untagged sessions", appSession.ApplicationID) and when
appSession.ApplicationID == "" return rpc.Errorf("cannot rebalance tagged
session %q with untagged sessions", applicationID) otherwise keep the existing
cross-application message; make this change in the same block that compares
applicationID and appSession.ApplicationID so the behavior remains within the
transaction scope.
In `@clearnode/metrics/interface.go`:
- Around line 18-24: getApplicationIDLabelValue currently returns any non-empty
applicationID verbatim (used by metrics in exporter.go for user_states_total,
transactions_total, transactions_amount_total), which risks unbounded Prometheus
label cardinality; change getApplicationIDLabelValue to check the applicationID
against a configured allow-list (or set of knownAppIDs) and return the
applicationID only if it is allowed, otherwise return a stable bucket label like
"unknown_app" or a coarse hashed bucket (e.g., "app_hash_<N>")—ensure the
allow-list source is configurable and referenced by getApplicationIDLabelValue
and update exporter.go to rely on this sanitized value for all metrics.
- Line 39: Rename the parameter in the Metrics interface method
ObserveRPCDuration from durationSecs to duration to satisfy staticcheck ST1011;
locate the ObserveRPCDuration(method, path string, success bool, durationSecs
time.Duration) declaration in the Metrics interface and change the final
parameter name to duration so it matches the implementation in exporter.go and
avoids the unit-suffixed parameter name.
In `@sdk/ts/src/config.ts`:
- Around line 96-103: appendApplicationIDQueryParam currently calls new
URL(wsURL) which will throw a raw TypeError for invalid input; wrap the URL
parsing in a try-catch inside appendApplicationIDQueryParam and rethrow a new,
descriptive Error (e.g. indicating the provided wsURL / url configuration is
malformed) while preserving the original error message or including the invalid
wsURL for debugging; update error text to reference APPLICATION_ID_QUERY_PARAM
context so callers (and Client.create) get a clear, user-facing message instead
of the raw "Invalid URL".
---
Outside diff comments:
In `@clearnode/store/database/database.go`:
- Around line 122-124: connectToSqlite is currently calling migrateSqlite and
ignoring its error, so migration failures (for State and Transaction) allow
startup to continue with an incomplete schema; update connectToSqlite (and the
other call site around lines 216-220) to capture the error returned by
migrateSqlite, propagate it (return it) up to the caller instead of discarding
it, and ensure any callers of connectToSqlite handle that error; locate the
migrateSqlite and connectToSqlite symbols and change the call sites to check
"err := migrateSqlite(db)" and return that err if non-nil so startup fails on
migration errors.
---
Nitpick comments:
In `@clearnode/metrics/interface.go`:
- Line 56: Add a doc comment above the exported constructor
NewNoopRuntimeMetricExporter describing what the function returns and its
purpose (i.e., it constructs and returns a no-op implementation of
RuntimeMetricExporter used when metrics are disabled or not available), so the
exported symbol has a proper leading doc comment per project guidelines.
In `@pkg/app/app_v1.go`:
- Around line 14-26: Add clarifying comments next to AppIDV1Regex and
ApplicationIDRegex stating they are intentionally different: document that
AppIDV1Regex enforces the registered application identifier rules (no leading
dash, no underscore, length 1..66 with first char letter/digit), while
ApplicationIDRegex is the looser advisory-tag format (allows underscores and
leading dash/underscore and single-char/digit-only) used for the advisory-tag DB
column; cross-reference the other regex symbol and mention that
IsValidApplicationID uses ApplicationIDRegex for advisory checks and that
create_app_session.go must use AppIDV1Regex when validating registered IDs to
avoid mixing usages.
In `@sdk/go/config.go`:
- Around line 50-62: The appendApplicationIDQueryParam function should
proactively validate non-empty applicationID against the exported
pkg/app.ApplicationIDRegex and return a clear error if it doesn't match instead
of deferring to a dial failure; update appendApplicationIDQueryParam to import
pkg/app, test applicationID against app.ApplicationIDRegex when applicationID !=
"", and return a descriptive error (e.g., "invalid applicationID: does not match
ApplicationIDRegex") before attempting url.Parse so callers get immediate,
actionable feedback.
In `@sdk/ts/src/config.ts`:
- Around line 84-88: withApplicationID currently assigns the raw appID to
config.applicationID allowing empty/whitespace or too-long values; update
withApplicationID to trim the input, validate that the trimmed value is
non-empty and no longer than 66 characters, and if invalid throw a clear error
instead of assigning; set config.applicationID = trimmedValue when valid so
appendApplicationIDQueryParam sees a normalized, validated value.
🪄 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: aad01075-aa40-4a5d-8f95-02d3588c4b38
📒 Files selected for processing (44)
clearnode/api/app_session_v1/create_app_session.goclearnode/api/app_session_v1/handler.goclearnode/api/app_session_v1/interface.goclearnode/api/app_session_v1/rebalance_app_sessions.goclearnode/api/app_session_v1/rebalance_app_sessions_test.goclearnode/api/app_session_v1/submit_app_state.goclearnode/api/app_session_v1/submit_app_state_test.goclearnode/api/app_session_v1/submit_deposit_state.goclearnode/api/app_session_v1/submit_deposit_state_test.goclearnode/api/app_session_v1/testing.goclearnode/api/channel_v1/handler.goclearnode/api/channel_v1/interface.goclearnode/api/channel_v1/request_creation.goclearnode/api/channel_v1/request_creation_test.goclearnode/api/channel_v1/submit_state.goclearnode/api/channel_v1/submit_state_test.goclearnode/api/channel_v1/testing.goclearnode/api/metric_store.goclearnode/config/migrations/postgres/20260420000000_add_application_id_to_writes.sqlclearnode/metrics/exporter.goclearnode/metrics/interface.goclearnode/store/database/blockchain_action_test.goclearnode/store/database/channel_test.goclearnode/store/database/database.goclearnode/store/database/db_store_test.goclearnode/store/database/interface.goclearnode/store/database/state.goclearnode/store/database/state_test.goclearnode/store/database/test/postgres_integration_test.goclearnode/store/database/testing.goclearnode/store/database/transaction.goclearnode/store/database/transaction_test.gopkg/app/app_v1.gopkg/app/app_v1_test.gopkg/rpc/context.gopkg/rpc/context_internal_test.gopkg/rpc/node.gosdk/go/client.gosdk/go/config.gosdk/go/config_test.gosdk/ts/src/client.tssdk/ts/src/config.tssdk/ts/src/index.tssdk/ts/test/unit/config.test.ts
✅ Files skipped from review due to trivial changes (4)
- clearnode/config/migrations/postgres/20260420000000_add_application_id_to_writes.sql
- clearnode/api/app_session_v1/submit_deposit_state_test.go
- sdk/ts/test/unit/config.test.ts
- pkg/rpc/context_internal_test.go
🚧 Files skipped from review as they are similar to previous changes (20)
- clearnode/store/database/channel_test.go
- clearnode/api/app_session_v1/submit_app_state.go
- clearnode/api/app_session_v1/submit_deposit_state.go
- sdk/ts/src/index.ts
- clearnode/store/database/test/postgres_integration_test.go
- sdk/ts/src/client.ts
- sdk/go/config_test.go
- clearnode/api/app_session_v1/submit_app_state_test.go
- clearnode/api/channel_v1/handler.go
- sdk/go/client.go
- clearnode/api/app_session_v1/interface.go
- clearnode/store/database/interface.go
- clearnode/api/app_session_v1/handler.go
- clearnode/store/database/transaction.go
- clearnode/api/app_session_v1/testing.go
- clearnode/store/database/state.go
- clearnode/metrics/exporter.go
- clearnode/api/metric_store.go
- pkg/rpc/context.go
- clearnode/api/app_session_v1/rebalance_app_sessions_test.go
nksazonov
approved these changes
Apr 21, 2026
- clearnode/api/app_session_v1/rebalance: quote applicationIDs in mismatch error so empty (legacy) tag stays visually distinct from named apps - clearnode/metrics/interface: rename ObserveRPCDuration(durationSecs) -> duration (staticcheck ST1011) - sdk/ts/src/config: wrap new URL(wsURL) with try/catch and rethrow a descriptive error instead of leaking raw TypeError Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/metrics/interface.go`:
- Line 56: Add a godoc comment above the exported constructor
NewNoopRuntimeMetricExporter describing its purpose and return value (e.g.,
"NewNoopRuntimeMetricExporter returns a RuntimeMetricExporter that performs no
operations, useful for tests or disabling metrics") and mention the concrete
type returned (noopRuntimeMetricExporter) so tools and readers understand that
it returns a no-op exporter.
🪄 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: 5dff2749-9af6-40da-a5fb-89c5574645a2
📒 Files selected for processing (4)
clearnode/api/app_session_v1/rebalance_app_sessions.goclearnode/metrics/interface.gosdk/ts/src/config.tssdk/ts/test/unit/config.test.ts
✅ Files skipped from review due to trivial changes (1)
- sdk/ts/test/unit/config.test.ts
| func (noopRuntimeMetricExporter) ObserveRPCDuration(string, string, bool, time.Duration) {} | ||
| func (noopRuntimeMetricExporter) SetRPCConnections(string, string, uint32) {} | ||
| func (noopRuntimeMetricExporter) IncAppStateUpdate(string) {} | ||
| func NewNoopRuntimeMetricExporter() RuntimeMetricExporter { return noopRuntimeMetricExporter{} } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the exported constructor has an adjacent godoc comment.
# Expected: line before NewNoopRuntimeMetricExporter starts with "// NewNoopRuntimeMetricExporter".
awk 'NR>=52 && NR<=58 { printf "%d:%s\n", NR, $0 }' clearnode/metrics/interface.goRepository: layer-3/nitrolite
Length of output: 528
Add a doc comment for the exported constructor.
NewNoopRuntimeMetricExporter is exported but lacks a godoc comment.
Proposed fix
+// NewNoopRuntimeMetricExporter returns a no-op runtime metric exporter.
-func NewNoopRuntimeMetricExporter() RuntimeMetricExporter { return noopRuntimeMetricExporter{} }
+func NewNoopRuntimeMetricExporter() RuntimeMetricExporter { return noopRuntimeMetricExporter{} }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func NewNoopRuntimeMetricExporter() RuntimeMetricExporter { return noopRuntimeMetricExporter{} } | |
| // NewNoopRuntimeMetricExporter returns a no-op runtime metric exporter. | |
| func NewNoopRuntimeMetricExporter() RuntimeMetricExporter { return noopRuntimeMetricExporter{} } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clearnode/metrics/interface.go` at line 56, Add a godoc comment above the
exported constructor NewNoopRuntimeMetricExporter describing its purpose and
return value (e.g., "NewNoopRuntimeMetricExporter returns a
RuntimeMetricExporter that performs no operations, useful for tests or disabling
metrics") and mention the concrete type returned (noopRuntimeMetricExporter) so
tools and readers understand that it returns a no-op exporter.
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
2 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.
Summary
?app_id=on the WebSocket upgrade as an advisory origin tag: clearnode reads it inServeHTTP, stores it in per-connectionSafeStorage, and stamps it ontochannel_states.application_idandtransactions.application_id(nullableVARCHAR(66)) for writes originating from client requests.ApplicationIDrather than the connection tag.rebalance_app_sessionsnow rejects batches that mix sessions from different applications.user_states_total,transactions_total,transactions_amount_totalcarry anapplication_idlabel; unset clients bucket under_DEFAULT.WithApplicationID) and TypeScript SDK (withApplicationID) append?app_id=to the dial URL.Notes
application_idcolumn is advisory — it is self-declared by the client and must not be used for access control.?app_id=continue to work; their writes storeNULL.clearnode/config/migrations/postgres/20260420000000_add_application_id_to_writes.sql.Test plan
go vet ./...cleango test ./...— 23/23 packages green, including newTestGetApplicationID,TestDBStore_StoreUserState_ApplicationID,TestDBStore_RecordTransaction/ApplicationID_*,TestRebalanceAppSessions_Error_DifferentApplications,TestWithApplicationID,TestAppendApplicationIDQueryParamstate_advancer.tsimport-extension errors are unchanged)?app_id=; verify column value on resulting rowsapplication_idlabel on the three affected metrics🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Database Changes
Behavior Change
Metrics
Tests