MF-M08: fix(rpc): replace Origin label with application_id on connection gauge#745
Merged
Merged
Conversation
Replace the client-controlled Origin header label on the rpc_connections_active gauge with application_id, which is parsed and validated against IsValidApplicationID at WebSocket upgrade. The hub now tracks per-application counts and deletes the gauge series when an application's count drops to zero, so disconnected buckets do not accumulate as zero-valued time series. The connection-level Origin field is retained for logging. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds CI drift guards and scripts; updates Solidity and ABI; introduces paginated key-state APIs with SDK transforms; implements DB pointer table with locks and caps; escrow-operation gating; event user-sig backfill; WS read-limit/rate-limiter and metrics relabeling; DB SSL config; docs updates. ChangesUnified Change Set
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nitronode/store/database/channel_session_key_state.go (1)
64-83:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake history insert + pointer upsert atomic.
Line 64-83 currently allows partial commit (state inserted, pointer not updated). A transient pointer-upsert failure can permanently stale latest-state reads because retries hit duplicate insert first.
💡 Suggested fix
- if err := s.db.Create(&dbState).Error; err != nil { - return fmt.Errorf("failed to store channel session key state: %w", err) - } - - if len(state.Assets) > 0 { - assets := make([]ChannelSessionKeyAssetV1, len(state.Assets)) - for i, asset := range state.Assets { - assets[i] = ChannelSessionKeyAssetV1{ - SessionKeyStateID: id, - Asset: strings.ToLower(asset), - } - } - if err := s.db.Create(&assets).Error; err != nil { - return fmt.Errorf("failed to store channel session key assets: %w", err) - } - } - - if err := upsertCurrentSessionKeyState(s.db, userAddress, sessionKey, SessionKeyKindChannel, state.Version); err != nil { - return err - } - - return nil + if err := s.db.Transaction(func(tx *gorm.DB) error { + if err := tx.Create(&dbState).Error; err != nil { + return fmt.Errorf("failed to store channel session key state: %w", err) + } + + if len(state.Assets) > 0 { + assets := make([]ChannelSessionKeyAssetV1, len(state.Assets)) + for i, asset := range state.Assets { + assets[i] = ChannelSessionKeyAssetV1{ + SessionKeyStateID: id, + Asset: strings.ToLower(asset), + } + } + if err := tx.Create(&assets).Error; err != nil { + return fmt.Errorf("failed to store channel session key assets: %w", err) + } + } + + if err := upsertCurrentSessionKeyState(tx, userAddress, sessionKey, SessionKeyKindChannel, state.Version); err != nil { + return err + } + return nil + }); err != nil { + return err + } + + return nil🤖 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/store/database/channel_session_key_state.go` around lines 64 - 83, The insert of dbState and assets plus the call to upsertCurrentSessionKeyState must be executed inside a single DB transaction to avoid partial commits: begin a transaction from s.db, replace s.db.Create calls for dbState and assets with tx.Create, and call a transaction-aware version of upsertCurrentSessionKeyState (or modify upsertCurrentSessionKeyState to accept a *gorm.DB and pass tx) so the pointer upsert uses the same tx; on any error roll back the tx and return the error, otherwise commit the tx. Use the existing symbols dbState, assets, ChannelSessionKeyAssetV1, upsertCurrentSessionKeyState (or a new upsertCurrentSessionKeyStateTx that takes tx), SessionKeyKindChannel, userAddress, sessionKey and state.Version when implementing this change.
🧹 Nitpick comments (8)
sdk/ts/test/unit/app-signing-drift.test.ts (1)
40-251: ⚡ Quick winRefactor repeated drift assertions to data-driven
.forEach()cases.This suite is highly repetitive and can be collapsed into case tables for vector and adversarial checks, which will make updates to expected hashes easier and keep style consistent with repo test guidelines.
As per coding guidelines "Use data-driven tests with
.forEach()pattern and manual mocks instead ofjest.mock()".🤖 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/test/unit/app-signing-drift.test.ts` around lines 40 - 251, The tests repeat many expect(...) assertions for packCreateAppSessionRequestV1, generateAppSessionIDV1, packAppStateUpdateV1, and packAppSessionKeyStateV1; collapse these into data-driven tables (arrays of case objects) and iterate with .forEach() (or test.each) to drive assertions, and convert the adversarial comparisons into similar case pairs in arrays so each vector and adversarial check is a single table-driven loop; update references to AppStateUpdateIntent, packAppStateUpdateV1, packCreateAppSessionRequestV1, generateAppSessionIDV1, and packAppSessionKeyStateV1 in the new case objects to locate inputs, and ensure any mocking follows the guideline by using manual mocks rather than jest.mock() if mocking is needed.sdk/ts/test/unit/abi-drift.test.ts (1)
112-278: ⚡ Quick winConvert repeated adversarial cases into a single table-driven
.forEach()suite.The current pattern repeats near-identical assertion scaffolding. A shared case matrix would make future ABI drift cases easier to add and maintain.
As per coding guidelines "Use data-driven tests with
.forEach()pattern and manual mocks instead ofjest.mock()".🤖 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/test/unit/abi-drift.test.ts` around lines 112 - 278, Consolidate the repeated adversarial test cases at the end of the "contract ABI drift guards" suite into a single table-driven array and iterate with .forEach(): collect each case's inputs and expected output (cases currently implemented as individual it(...) blocks that call diffConsumedFunctions or diffSdkSubsetAgainstManifest), then in the loop call the appropriate function (diffConsumedFunctions or diffSdkSubsetAgainstManifest) and assert toEqual(expected); reference the existing identifiers to find code: the suite name "contract ABI drift guards", the helper functions diffConsumedFunctions and diffSdkSubsetAgainstManifest, and the individual test names like 'reports adversarial ChannelHub function signature changes...', 'reports adversarial ERC20 missing consumed functions', and 'reports adversarial AppRegistry manifest signature changes' to build the case matrix and replace the separate it(...) blocks with a single data-driven .forEach() test.sdk/ts/test/unit/public-api-drift.test.ts (1)
181-247: ⚡ Quick winRefactor adversarial drift checks into data-driven
.forEach()cases.These tests share the same structure and can be condensed into parameterized cases to simplify future maintenance.
As per coding guidelines "Use data-driven tests with
.forEach()pattern and manual mocks instead ofjest.mock()".🤖 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/test/unit/public-api-drift.test.ts` around lines 181 - 247, Condense the repeated "proves adversarial..." test cases into a data-driven array and iterate with .forEach to generate the individual it() tests: collect test descriptors (name, precomputed api via serializePublicApi(), target member name like 'Client'/'Config'/'packAppStateUpdateV1'/'AppStateUpdateIntent' or expected predicate, and the specific mutation + assertion), then replace each explicit it(...) block with a single .forEach that calls expect/mutates accordingly; reference serializePublicApi(), publicApi and the existing property checks (e.g., checking properties for 'ping:', kind === 'interface' for 'Config', signatures for 'packAppStateUpdateV1', members for 'AppStateUpdateIntent', and adding '__FakeExport') to implement each case, and ensure you follow the guideline to use manual setup/mocks instead of jest.mock() while keeping assertions identical.nitronode/store/database/db_store.go (1)
214-215: 💤 Low valueConsider extracting the mutual lock check for readability.
The combined condition correctly validates both home and escrow channel versions, but could be extracted for clarity.
♻️ Optional refactor
- if result.HomeChannelVersion == nil || result.StateVersion != *result.HomeChannelVersion || - result.EscrowChannelVersion == nil || result.StateVersion != *result.EscrowChannelVersion { + homeCaughtUp := result.HomeChannelVersion != nil && result.StateVersion == *result.HomeChannelVersion + escrowCaughtUp := result.EscrowChannelVersion != nil && result.StateVersion == *result.EscrowChannelVersion + if !homeCaughtUp || !escrowCaughtUp { return fmt.Errorf("mutual lock is still ongoing") }🤖 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/store/database/db_store.go` around lines 214 - 215, Extract the combined mutual-lock condition into a small helper to improve readability: create a function (e.g., isMutuallyLocked or validateMutualLock) that accepts the same result type and returns bool using the existing logic that checks result.HomeChannelVersion, result.EscrowChannelVersion and compares both to result.StateVersion; then replace the inline condition (the long if that references result.HomeChannelVersion, result.EscrowChannelVersion and result.StateVersion) with a call to the new helper and keep behavior identical.sdk/ts/test/unit/rpc-drift.test.ts (1)
49-52: 💤 Low valueIndentation-dependent regex is brittle but adequately guarded.
The regex on line 50 hardcodes exactly 2 spaces (
^\s{2}). If client method indentation changes, this will silently break. The guard at line 130 (expect(clientMethods.size).toBeGreaterThan(20)) mitigates this, but consider using a more flexible pattern like^\s+with additional context to match class methods more robustly.🤖 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/test/unit/rpc-drift.test.ts` around lines 49 - 52, The regex in extractTsClientMethods is brittle because it requires exactly two leading spaces; update the pattern used in extractTsClientMethods to allow flexible indentation (e.g. use ^\s+ instead of ^\s{2}) and keep the existing method-capturing groups (still matching optional "static"/"async" and the identifier) so it robustly finds class methods even if indentation changes; ensure the new pattern still uses the gm flags and preserves the return type Set<string> behavior.contracts/test/mocks/OversizedReturnERC20.sol (1)
23-39: ⚡ Quick winAdd NatSpec comments for public functions.
transferandmintare public, but they currently have no function-level NatSpec blocks.Proposed update
+ /** + * `@notice` Transfers tokens and returns custom-sized returndata for transfer-behavior tests. + * `@dev` Performs `_transfer` only when `FIRST_WORD != 0`; then returns `RETURN_DATA_SIZE` bytes. + */ function transfer(address to, uint256 amount) public override returns (bool) { if (FIRST_WORD != 0) { _transfer(msg.sender, to, amount); } @@ } + /** + * `@notice` Mints tokens for test setup. + */ function mint(address to, uint256 amount) public { _mint(to, amount); }As per coding guidelines
**/*.sol: Include NatSpec comments on all public/external functions in Solidity.🤖 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 `@contracts/test/mocks/OversizedReturnERC20.sol` around lines 23 - 39, Add Solidity NatSpec comments for the public functions transfer and mint: add a `@notice` describing what each function does, `@param` tags for the address and amount parameters (e.g., to, amount), and for transfer include a `@return` tag describing the boolean result; place these function-level NatSpec blocks immediately above the transfer and mint function declarations in OversizedReturnERC20.sol (identify the functions by name: transfer and mint) following the repository's NatSpec style guidelines.nitronode/api/app_session_v1/interface.go (1)
55-59: 💤 Low valueMissing doc comments on new/changed exported interface methods.
LockSessionKeyState,CountSessionKeysForUser, and the updatedGetLastAppSessionKeyStatesare exported interface methods but lack doc comments, unlike the adjacentCheckActiveChannel/EnsureNoOngoingEscrowOperationwhich were documented in this PR. Sibling interfacechannel_v1/interface.goalready documents the equivalents (lines 80-86, 95-97), so the asymmetry seems unintentional.📝 Proposed doc comments
// App Session key state operations + // LockSessionKeyState locks the (user, session_key, kind) pointer row for the surrounding + // transaction, returning the current version (0 if newly created). LockSessionKeyState(userAddress, sessionKey string, kind database.SessionKeyKind) (uint64, error) + // CountSessionKeysForUser returns the number of distinct session keys for the wallet + // across both kinds, used to enforce the per-user cap at submit time. CountSessionKeysForUser(userAddress string) (uint32, error) StoreAppSessionKeyState(state app.AppSessionKeyStateV1) error GetLastAppSessionKeyVersion(wallet, sessionKey string) (uint64, error) + // GetLastAppSessionKeyStates retrieves the latest app-session key states for a user, + // optionally filtered by session key. Results are paginated; the second return is the + // unpaginated total. GetLastAppSessionKeyStates(wallet string, sessionKey *string, limit, offset uint32) ([]app.AppSessionKeyStateV1, uint32, error)As per coding guidelines: "Follow
gofmtformatting 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/app_session_v1/interface.go` around lines 55 - 59, Add Go doc comments for the exported interface methods that are missing documentation: LockSessionKeyState, CountSessionKeysForUser, and GetLastAppSessionKeyStates (also review GetLastAppSessionKeyVersion and StoreAppSessionKeyState to ensure they have comments). Place concise Godoc-style comments immediately above each method in the app_session_v1 interface mirroring the style/phrasing used in the sibling channel_v1/interface.go (describe what the method does, its important parameters and return values) so all exported methods have proper documentation.pkg/rpc/connection.go (1)
408-419: 💤 Low valueConsider logging frame size against the configured limit.
The warning log records
frame_bytesbut not the limiter's configured rate/threshold, which makes it hard to distinguish "client sent a huge frame" from "limiter ran out of tokens." Optional: pass an explicit reason or the limiter's effective settings into the log to help operators triageframe rate limit exceededalerts.🤖 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/connection.go` around lines 408 - 419, The warning only logs frame_bytes but not the limiter's configured limit or reason; update the block where conn.frameRateLimiter.Admit(...) is checked (the conn.frameRateLimiter.Admit call and the conn.logger.Warn invocation) to include the limiter's configured settings and/or remaining tokens in the log message (e.g., add fields like "limit_bytes", "burst", or "tokens_remaining" and an explicit "reason" such as "token_exhausted" vs "oversize_frame"); keep the same close flow that sends to conn.closeConnCh and calls handleClosure(nil).
🤖 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/app_session_v1/get_last_key_states_test.go`:
- Around line 45-85: The tests
TestGetLastKeyStates_DefaultsToPageOneOnEmptyResult and
TestGetLastKeyStates_PaginationMetadata_AlignedOffset set expectations on
mockStore via mockStore.On(...) but never verify them; add
mockStore.AssertExpectations(t) at the end of each test to ensure the mocked
GetLastAppSessionKeyStates call was invoked as expected (i.e., call
mockStore.AssertExpectations(t) in both
TestGetLastKeyStates_DefaultsToPageOneOnEmptyResult and
TestGetLastKeyStates_PaginationMetadata_AlignedOffset).
In `@nitronode/store/database/current_session_key_state.go`:
- Around line 98-117: The non-Postgres fallback has a race where two callers
both see gorm.ErrRecordNotFound and both run s.db.Create(&seed) which can fail
on PK conflict; update the seed creation logic in the CurrentSessionKeyStateV1
path (the block that runs after checking err == gorm.ErrRecordNotFound from
s.db.Where(...).First(&existing).Error) to be idempotent—either use GORM's
OnConflict{DoNothing: true} when creating the seed or catch/ignore the unique
constraint/primary-key conflict error from s.db.Create(&seed) so the second
concurrent insert doesn't return an error, then return version 0 as before.
In `@pkg/rpc/rate_limiter.go`:
- Around line 64-69: The Admit method currently converts size to cost and
updates b.tokens, but it lacks a guard against negative sizes which would
erroneously increase tokens; update Admit (in pkg/rpc/rate_limiter.go) to check
the incoming size parameter and immediately return false (or an error boolean)
if size is negative before computing cost or mutating b.tokens, ensuring the
existing logic that checks b.tokens < cost and subtracts cost only runs for
non-negative sizes.
In `@scripts/drift/generate-app-signing-vectors.go`:
- Around line 134-137: The must function currently discards the error returned
by fmt.Fprintf; change it to capture that return (e.g., writeErr :=
fmt.Fprintf(...)) and handle it explicitly: if writeErr != nil, attempt a
fallback (for example write the combined message to os.Stdout or use fmt.Println
to emit both the writeErr and the original err), then call os.Exit(1) as before
so the function still terminates; update the function name must accordingly to
check and handle the fprintf error rather than ignoring it.
In `@sdk/ts/src/session_key_state_transforms.ts`:
- Around line 4-56: Replace the custom runtime validators (asRecord,
requireStringField, requireStringArrayField) with zod schemas and use
schema.safeParse(raw) inside transformChannelSessionKeyState and
transformAppSessionKeyState; define a Zod object for the
ChannelSessionKeyStateV1 shape and one for AppSessionKeyStateV1 (matching fields
user_address, session_key, version, assets, expires_at, user_sig and for app:
application_ids, app_session_ids) and on safeParse failure throw an Error with
the parse error message, otherwise return the parsed.data object; remove the
helper functions and ensure the transforms return the typed parsed result from
the Zod schemas.
In `@sdk/ts/src/utils.ts`:
- Around line 367-373: transformAppSessionInfo currently maps raw.app_session_id
directly to appSessionId without checking type; validate that raw.app_session_id
exists and is a string (and optionally non-empty) before returning — if it's
missing or not a string, throw a descriptive error (e.g., "invalid
app_session_id") or handle it according to SDK conventions; update the mapping
(appSessionId) and any tests to rely on this validation and reference
transformAppSessionInfo and raw.app_session_id when making the change.
---
Outside diff comments:
In `@nitronode/store/database/channel_session_key_state.go`:
- Around line 64-83: The insert of dbState and assets plus the call to
upsertCurrentSessionKeyState must be executed inside a single DB transaction to
avoid partial commits: begin a transaction from s.db, replace s.db.Create calls
for dbState and assets with tx.Create, and call a transaction-aware version of
upsertCurrentSessionKeyState (or modify upsertCurrentSessionKeyState to accept a
*gorm.DB and pass tx) so the pointer upsert uses the same tx; on any error roll
back the tx and return the error, otherwise commit the tx. Use the existing
symbols dbState, assets, ChannelSessionKeyAssetV1, upsertCurrentSessionKeyState
(or a new upsertCurrentSessionKeyStateTx that takes tx), SessionKeyKindChannel,
userAddress, sessionKey and state.Version when implementing this change.
---
Nitpick comments:
In `@contracts/test/mocks/OversizedReturnERC20.sol`:
- Around line 23-39: Add Solidity NatSpec comments for the public functions
transfer and mint: add a `@notice` describing what each function does, `@param` tags
for the address and amount parameters (e.g., to, amount), and for transfer
include a `@return` tag describing the boolean result; place these function-level
NatSpec blocks immediately above the transfer and mint function declarations in
OversizedReturnERC20.sol (identify the functions by name: transfer and mint)
following the repository's NatSpec style guidelines.
In `@nitronode/api/app_session_v1/interface.go`:
- Around line 55-59: Add Go doc comments for the exported interface methods that
are missing documentation: LockSessionKeyState, CountSessionKeysForUser, and
GetLastAppSessionKeyStates (also review GetLastAppSessionKeyVersion and
StoreAppSessionKeyState to ensure they have comments). Place concise Godoc-style
comments immediately above each method in the app_session_v1 interface mirroring
the style/phrasing used in the sibling channel_v1/interface.go (describe what
the method does, its important parameters and return values) so all exported
methods have proper documentation.
In `@nitronode/store/database/db_store.go`:
- Around line 214-215: Extract the combined mutual-lock condition into a small
helper to improve readability: create a function (e.g., isMutuallyLocked or
validateMutualLock) that accepts the same result type and returns bool using the
existing logic that checks result.HomeChannelVersion,
result.EscrowChannelVersion and compares both to result.StateVersion; then
replace the inline condition (the long if that references
result.HomeChannelVersion, result.EscrowChannelVersion and result.StateVersion)
with a call to the new helper and keep behavior identical.
In `@pkg/rpc/connection.go`:
- Around line 408-419: The warning only logs frame_bytes but not the limiter's
configured limit or reason; update the block where
conn.frameRateLimiter.Admit(...) is checked (the conn.frameRateLimiter.Admit
call and the conn.logger.Warn invocation) to include the limiter's configured
settings and/or remaining tokens in the log message (e.g., add fields like
"limit_bytes", "burst", or "tokens_remaining" and an explicit "reason" such as
"token_exhausted" vs "oversize_frame"); keep the same close flow that sends to
conn.closeConnCh and calls handleClosure(nil).
In `@sdk/ts/test/unit/abi-drift.test.ts`:
- Around line 112-278: Consolidate the repeated adversarial test cases at the
end of the "contract ABI drift guards" suite into a single table-driven array
and iterate with .forEach(): collect each case's inputs and expected output
(cases currently implemented as individual it(...) blocks that call
diffConsumedFunctions or diffSdkSubsetAgainstManifest), then in the loop call
the appropriate function (diffConsumedFunctions or diffSdkSubsetAgainstManifest)
and assert toEqual(expected); reference the existing identifiers to find code:
the suite name "contract ABI drift guards", the helper functions
diffConsumedFunctions and diffSdkSubsetAgainstManifest, and the individual test
names like 'reports adversarial ChannelHub function signature changes...',
'reports adversarial ERC20 missing consumed functions', and 'reports adversarial
AppRegistry manifest signature changes' to build the case matrix and replace the
separate it(...) blocks with a single data-driven .forEach() test.
In `@sdk/ts/test/unit/app-signing-drift.test.ts`:
- Around line 40-251: The tests repeat many expect(...) assertions for
packCreateAppSessionRequestV1, generateAppSessionIDV1, packAppStateUpdateV1, and
packAppSessionKeyStateV1; collapse these into data-driven tables (arrays of case
objects) and iterate with .forEach() (or test.each) to drive assertions, and
convert the adversarial comparisons into similar case pairs in arrays so each
vector and adversarial check is a single table-driven loop; update references to
AppStateUpdateIntent, packAppStateUpdateV1, packCreateAppSessionRequestV1,
generateAppSessionIDV1, and packAppSessionKeyStateV1 in the new case objects to
locate inputs, and ensure any mocking follows the guideline by using manual
mocks rather than jest.mock() if mocking is needed.
In `@sdk/ts/test/unit/public-api-drift.test.ts`:
- Around line 181-247: Condense the repeated "proves adversarial..." test cases
into a data-driven array and iterate with .forEach to generate the individual
it() tests: collect test descriptors (name, precomputed api via
serializePublicApi(), target member name like
'Client'/'Config'/'packAppStateUpdateV1'/'AppStateUpdateIntent' or expected
predicate, and the specific mutation + assertion), then replace each explicit
it(...) block with a single .forEach that calls expect/mutates accordingly;
reference serializePublicApi(), publicApi and the existing property checks
(e.g., checking properties for 'ping:', kind === 'interface' for 'Config',
signatures for 'packAppStateUpdateV1', members for 'AppStateUpdateIntent', and
adding '__FakeExport') to implement each case, and ensure you follow the
guideline to use manual setup/mocks instead of jest.mock() while keeping
assertions identical.
In `@sdk/ts/test/unit/rpc-drift.test.ts`:
- Around line 49-52: The regex in extractTsClientMethods is brittle because it
requires exactly two leading spaces; update the pattern used in
extractTsClientMethods to allow flexible indentation (e.g. use ^\s+ instead of
^\s{2}) and keep the existing method-capturing groups (still matching optional
"static"/"async" and the identifier) so it robustly finds class methods even if
indentation changes; ensure the new pattern still uses the gm flags and
preserves the return type Set<string> behavior.
🪄 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: 8fd6f7d9-21ae-4218-8e70-8b987ede82f0
⛔ Files ignored due to path filters (3)
sdk/ts-compat/test/unit/__snapshots__/public-api-drift.test.ts.snapis excluded by!**/*.snapsdk/ts/package-lock.jsonis excluded by!**/package-lock.jsonsdk/ts/test/unit/__snapshots__/public-api-drift.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (105)
.github/workflows/main-pr.yml.github/workflows/main-push.yml.github/workflows/protocol-drift-external-smoke.yml.github/workflows/test-sdk.ymlcontracts/SECURITY.mdcontracts/src/ChannelHub.solcontracts/src/Utils.solcontracts/src/interfaces/Types.solcontracts/test/ChannelHub_Node.t.solcontracts/test/ChannelHub_challenge/ChannelHub_challengeSessionKeyValidator.t.solcontracts/test/ChannelHub_lifecycle/ChannelHub_singlechain.lifecycle.t.solcontracts/test/ChannelHub_nonRevertingPushFunds.t.solcontracts/test/ChannelHub_sigValidator.t.solcontracts/test/Utils.t.solcontracts/test/mocks/MalformedReturningERC20.solcontracts/test/mocks/OversizedReturnERC20.soldocs/api.yamldocs/data_models.mmddocs/protocol/enforcement.mddocs/protocol/overview.mddocs/protocol/security-and-limitations.mdnitronode/README.mdnitronode/api/app_session_v1/README.mdnitronode/api/app_session_v1/create_app_session_test.gonitronode/api/app_session_v1/get_last_key_states.gonitronode/api/app_session_v1/get_last_key_states_test.gonitronode/api/app_session_v1/handler.gonitronode/api/app_session_v1/interface.gonitronode/api/app_session_v1/rebalance_app_sessions_test.gonitronode/api/app_session_v1/submit_app_state_test.gonitronode/api/app_session_v1/submit_deposit_state.gonitronode/api/app_session_v1/submit_deposit_state_test.gonitronode/api/app_session_v1/submit_session_key_state.gonitronode/api/app_session_v1/submit_session_key_state_test.gonitronode/api/app_session_v1/testing.gonitronode/api/channel_v1/get_last_key_states.gonitronode/api/channel_v1/get_last_key_states_test.gonitronode/api/channel_v1/handler.gonitronode/api/channel_v1/interface.gonitronode/api/channel_v1/submit_session_key_state.gonitronode/api/channel_v1/submit_session_key_state_test.gonitronode/api/channel_v1/submit_state.gonitronode/api/channel_v1/submit_state_test.gonitronode/api/channel_v1/testing.gonitronode/api/rate_limits.gonitronode/api/rpc_router.gonitronode/chart/README.mdnitronode/chart/config/stress-v1/nitronode.yaml.gotmplnitronode/chart/templates/helpers/_common.tplnitronode/chart/values.yamlnitronode/config/migrations/postgres/20260507000000_add_current_session_key_states.sqlnitronode/event_handlers/service.gonitronode/event_handlers/service_test.gonitronode/event_handlers/testing.gonitronode/main.gonitronode/metrics/exporter.gonitronode/metrics/interface.gonitronode/runtime.gonitronode/store/database/app_session_key_state.gonitronode/store/database/app_session_key_state_test.gonitronode/store/database/channel.gonitronode/store/database/channel_session_key_state.gonitronode/store/database/channel_session_key_state_test.gonitronode/store/database/channel_test.gonitronode/store/database/current_session_key_state.gonitronode/store/database/current_session_key_state_test.gonitronode/store/database/database.gonitronode/store/database/database_test.gonitronode/store/database/db_store.gonitronode/store/database/db_store_test.gonitronode/store/database/interface.gonitronode/store/database/state.gonitronode/store/database/testing.gopkg/blockchain/evm/channel_hub_reactor.gopkg/blockchain/evm/channel_hub_reactor_test.gopkg/core/event.gopkg/core/interface.gopkg/rpc/api.gopkg/rpc/connection.gopkg/rpc/connection_hub.gopkg/rpc/connection_test.gopkg/rpc/node.gopkg/rpc/rate_limiter.gopkg/rpc/rate_limiter_test.gopkg/rpc/types.goprotocol-description.mdscripts/check-protocol-drift.shscripts/drift/generate-app-signing-vectors.goscripts/drift/runtime-smoke.mjssdk/PROTOCOL_DRIFT_GUARDS.mdsdk/ts-compat/package.jsonsdk/ts-compat/test/unit/client.test.tssdk/ts-compat/test/unit/public-api-drift.test.tssdk/ts/package.jsonsdk/ts/src/blockchain/evm/channel_hub_abi.tssdk/ts/src/client.tssdk/ts/src/rpc/api.tssdk/ts/src/session_key_state_transforms.tssdk/ts/src/utils.tssdk/ts/test/unit/abi-drift.test.tssdk/ts/test/unit/app-signing-drift.test.tssdk/ts/test/unit/public-api-drift.test.tssdk/ts/test/unit/rpc-drift.test.tssdk/ts/test/unit/rpc-dto-drift.test.tssdk/ts/test/unit/transform-drift.test.ts
nksazonov
reviewed
May 13, 2026
Contributor
nksazonov
left a comment
nksazonov
left a comment
There was a problem hiding this comment.
Clean approach to bounding metric cardinality. Two things need attention before this lands.
Field naming: rpcConnectionsTotal *prometheus.GaugeVec (line 180 of exporter.go) names a gauge as if it were a counter — minor but confusing. Consider renaming to rpcConnectionsActive alongside this change.
ConnectionHub.Add and Remove invoked observeConnections (-> Prometheus SetRPCConnections, which takes metric-internal mutexes) while still holding hub.mu, so all readers — including Publish — serialized behind Prometheus writes during connection churn. Snapshot the post-mutation count, release hub.mu, then call the observer outside the lock. Also gate Remove's observer call on whether the bucket actually changed, so map-miss / already-zero paths no longer emit a spurious DeleteLabelValues for an app_id the gauge never tracked. Annotate the rpc_connections_active Help with the remaining cardinality bound: the format regex on application_id plus disconnect-driven series shedding — no registry check and no per-app connection cap. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
nksazonov
pushed a commit
that referenced
this pull request
May 13, 2026
- MF-L01: fix(contracts/ChannelHub): cap ERC20 transfer returndata copy to 32 bytes (#726) - MF-H01: fix(nitronode): paginate get_last_key_states endpoints (#724) - MF-I01-I02: fix(contracts): address security audit findings I-01 and I-02 (#728) - MF-C01: rpc: cap inbound WebSocket frame size and rate-limit per connection (#723) - MF-L02: docs(protocol): qualify enforcement guarantee for intent-specific execution paths (#737) - MF-L02-I03-I04_I05: fix(contracts): add more Node trust assumptions and requirements (#738) - MF-M01: backfill state user_sig from on-chain events (#731) - MF-M02: fix(rpc): release Serve wait group on processSink overflow (#732) - Fix SDK acknowledgement before home channel creation (#734) - MF-I06: fix(nitronode): gate escrow transitions on home channel onchain materialization (#730) - MF-M05: fix(nitronode): enforce TLS by default for Postgres (#733) - MF-M07: Unblock receiver states after finalized escrow operations (#735) - MF-M04: feat: provide tooling for and enhance docs on ValidatorRegistered event (#744) - MF-L04: fix(contracts): reject redundant native value (#741) - MF-H02: bind session key registration to a single owner per kind (#739) - MF-I07: fix(contracts): enforce max challenge duration (#752) - MF-M08: fix(rpc): replace Origin label with application_id on connection gauge (#745) - MF-C02: fix(core): add ChannelStatusClosing to gate post-finalize state transitions (#746) - MF-L06: fix(contracts): clear stale challengeExpireAt on cooperative escrow finalization (#754) - MF-I08: docs: document ChannelClosed event orientation ambiguity during abandoned migration (#755) - MF-M09: fix(nitronode): auto-challenge home channel on withheld escrow finalize (#753) - MF-L09: fix(nitronode): validate parsed app session nonce (#751) - MF-L05: docs(contracts): document informational events not guaranteed to emit (#756) - MF-L08: fix(nitronode/api): default get_last_key_states to active-only with include_inactive opt-in (#749) - MF-L10: fix: emit escrowIds array in EscrowDepositsPurged event and handle it in Nitronode (#757)
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
Originheader label onrpc_connections_activewithapplication_id, sourced from theapp_idquery parameter validated at WebSocket upgrade. Unauthenticated remotes can no longer drive unbounded Prometheus label cardinality by sending uniqueOriginheaders.ConnectionHuband callDeleteLabelValuesonce a bucket reaches zero, so series for disconnected applications are shed instead of lingering as zero-valued gauges.ApplicationID()on therpc.Connectioninterface (alongside the existingOrigin()) so the hub can label without re-parsing the request. The rawOriginfield is retained for logging only.Test plan
go build ./...go vet ./...go test ./pkg/rpc/... ./nitronode/metrics/...rpc_connections_active{application_id="..."}on a running node and that the series disappears after the last connection for that app closes.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation
decimals()are rejected.