YNU-941: apply final round audit findings

[philanton]

Summary by CodeRabbit

• New Features

  • Session-key registrations now require and validate both wallet and session-key-holder signatures.
  • ValidatorRegistered event watcher added (SDK + tooling) for subscription/replay.
  • Key-state queries gain pagination and an include_inactive option; responses include metadata.

• Improvements

  • New channel lifecycle state: closing.
  • WebSocket controls: max message size and per-connection byte-rate limits.
  • Per-user session-key registration limits enforced.

• Security / Enforcement

  • Stricter escrow/msg.value and challenge-duration validations; purge events now list purged escrow IDs.

[coderabbitai]

📝 Walkthrough

Do you want the full hidden review stack artifact (very large; can be split across multiple messages) or only the visible walkthrough, cohorts table, and metadata (no hidden artifact)?

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/audit-findings-final

[coderabbitai]

Actionable comments posted: 10

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)

contracts/test/mocks/MalformedReturningERC20.sol (3)

42-44: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add NatSpec documentation to public functions.

The mint function lacks NatSpec documentation. As per coding guidelines, all public/external functions in Solidity must include NatSpec comments.

📝 Suggested NatSpec addition

+    /// `@notice` Mints tokens to specified address for test setup
+    /// `@param` to Recipient address
+    /// `@param` amount Token amount to mint
     function mint(address to, uint256 amount) public {

As per coding guidelines, 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/MalformedReturningERC20.sol` around lines 42 - 44, The
public function mint is missing NatSpec comments; add a NatSpec block
immediately above the mint function (function name: mint) including at least
`@notice` describing what the function does, `@dev` for any implementation notes,
and `@param` to document the "to" and "amount" parameters (and `@return` if you
later add returns). Ensure the comment block uses the triple-slash or /** */
NatSpec format and sits directly above the mint declaration so tooling and
linters pick it up.

---

16-27: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add NatSpec documentation to public functions.

The transfer function lacks NatSpec documentation. As per coding guidelines, all public/external functions in Solidity must include NatSpec comments documenting parameters, return values, and behavior.

📝 Suggested NatSpec addition

+    /// `@notice` Returns malformed 1-byte data without performing transfer
+    /// `@dev` Simulates non-compliant token that could cause abi.decode to revert
+    /// `@return` Always returns 1 byte (0x01) instead of standard 32-byte bool
     function transfer(address, uint256) public pure override returns (bool) {

As per coding guidelines, 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/MalformedReturningERC20.sol` around lines 16 - 27, The
public function transfer in MalformedReturningERC20.sol is missing NatSpec
comments; add a Solidity NatSpec block above the transfer function (the public
pure override transfer(address, uint256) returns (bool) declaration) documenting
the purpose/behavior, each parameter (`@param` for the address and uint256), the
return value (`@return`) and any important notes about its malformed behavior
(that it returns a single byte and does not perform an actual transfer) so
consumers understand the intentionally abnormal behavior.

---

29-40: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add NatSpec documentation to public functions.

The transferFrom function lacks NatSpec documentation. As per coding guidelines, all public/external functions in Solidity must include NatSpec comments.

📝 Suggested NatSpec addition

+    /// `@notice` Returns malformed 1-byte data without performing transfer
+    /// `@dev` Simulates non-compliant token that could cause abi.decode to revert
+    /// `@return` Always returns 1 byte (0x01) instead of standard 32-byte bool
     function transferFrom(address, address, uint256) public pure override returns (bool) {

As per coding guidelines, 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/MalformedReturningERC20.sol` around lines 29 - 40, The
public function transferFrom is missing NatSpec; add a Solidity NatSpec comment
block above the transferFrom function in MalformedReturningERC20.sol including
at minimum `@notice` describing the mock behavior, `@dev` explaining it returns a
single byte without transferring (malformed token simulation), `@param`
annotations for the address parameters and uint256, and `@return` describing the
malformed boolean-like return value so tooling and consumers can understand the
purpose and behavior of transferFrom.

pkg/rpc/node.go (1)

33-38: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add doc comment to exported ByteTokenBucket.

The exported struct ByteTokenBucket is missing a doc comment. As per coding guidelines, all exported names in Go must have doc comments.

📝 Suggested doc comment

+// ByteTokenBucket is a token bucket on bytes read. One bucket per connection.
+// Not safe for concurrent use; the connection's read goroutine is the sole
+// caller of Admit.
 type ByteTokenBucket struct {
 	bytesPerSec float64
 	burst       float64
 	tokens      float64
 	last        time.Time
 }

As per coding guidelines: "Follow gofmt formatting 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 `@pkg/rpc/node.go` around lines 33 - 38, Add a proper Go doc comment for the
exported struct ByteTokenBucket describing its purpose and usage (what it
represents, how tokens/bytes are consumed/refilled, and any concurrency
guarantees) immediately above the type declaration for ByteTokenBucket so it
satisfies Go exported-name documentation rules; ensure the comment starts with
"ByteTokenBucket" and is formatted as a full sentence.

🧹 Nitpick comments (11)

contracts/src/Utils.sol (1)

19-31: ⚡ Quick win

Verify struct layout invariant is maintained.

The getChannelId function uses hardcoded 0xC0 (192 bytes) based on ChannelDefinition having 6 fields. The warning comment in Types.sol is good, but consider adding a compile-time assertion or a test that verifies the struct size matches this assumption. If fields are added/removed in the future, the mismatch would silently produce wrong channel IDs.

🛡️ Consider adding a size verification test

Add a test in contracts/test/Utils.t.sol that verifies the memory layout assumption:

function test_channelDefinitionSizeInvariant() public {
    // Verify that ChannelDefinition memory layout matches Utils.getChannelId assumption
    ChannelDefinition memory def = ChannelDefinition({...});
    bytes memory encoded = abi.encode(def);
    // The encoded size should match 6 * 32 = 192 bytes for the struct data
    // (excluding the initial offset/length words that abi.encode adds)
}

This test would catch any struct modifications that break the assembly optimization.

🤖 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/src/Utils.sol` around lines 19 - 31, Add a verification that the
hardcoded 0xC0 size used in getChannelId matches the actual ChannelDefinition
layout: implement either a compile-time/assertion or a unit test that computes
the struct size for ChannelDefinition and fails if it differs from 6*32 (192) so
future edits can’t silently break getChannelId; reference the getChannelId
function and the ChannelDefinition struct when adding the check (e.g., a
solidity constant or assert in an initialization/test function that compares
abi.encode(def).length or a computed SIZE constant against 192).

contracts/test/mocks/OversizedReturnERC20.sol (1)

11-12: ⚡ Quick win

Clarify that the zero-return-failure pattern deviates from standard ERC20.

The comment states "a zero return value signals failure without a state change," but standard ERC20 tokens revert on failure rather than returning zero. Consider clarifying that this test mock intentionally deviates from standard behavior for testing purposes.

📝 Suggested clarification

- *      The actual transfer is performed only when firstWord != 0, matching normal token semantics
- *      (a zero return value signals failure without a state change).
+ *      The actual transfer is performed only when firstWord != 0. This deviates from standard
+ *      ERC20 behavior (which reverts on failure) to test edge-case handling of non-reverting failures.

🤖 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 11 - 12, Update
the comment in OversizedReturnERC20 (the test mock contract) to explicitly state
that the “a zero return value signals failure without a state change” pattern is
a deliberate deviation from the ERC20 standard used only for testing; reference
the contract name OversizedReturnERC20 and its transfer/transferFrom behavior so
readers know this mock intentionally returns 0 instead of reverting to simulate
legacy/non-standard token behavior for tests.

docs/api.yaml (2)

847-863: ⚡ Quick win

Clarify validation behavior for unsupported sort field.

Same issue as in the channels endpoint - the description states sort is not supported and must be omitted, but it's optional in the type definition. Clarify the validation behavior.

💡 Suggested clarification

-                  description: Pagination parameters (offset, limit). Default limit 10, max 10. The `sort` field is not supported by this endpoint and must be omitted.
+                  description: Pagination parameters (offset, limit). Default limit 10, max 10. The `sort` field is not supported and will be rejected if provided.

🤖 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 `@docs/api.yaml` around lines 847 - 863, The docs currently say the pagination
`sort` field "is not supported and must be omitted" but the type allows it;
update the endpoint description for the pagination field to explicitly state the
validation behavior: clarify whether the server will reject requests containing
`pagination.sort` (returning a 400 with a validation error like "pagination.sort
is not supported") or will silently ignore it; then make the text precise (e.g.,
"The server validates and will reject requests that include `pagination.sort`
with HTTP 400: 'pagination.sort is not supported'") and apply the same wording
to the `pagination` description under this endpoint so consumers know the exact
validation outcome.

---

648-664: ⚡ Quick win

Clarify validation behavior for unsupported sort field.

The description states "The sort field is not supported by this endpoint and must be omitted," but pagination_params includes sort as an optional field. Consider clarifying whether:

• Requests with sort will be rejected with a validation error
• Or if sort is silently ignored

💡 Suggested clarification

-                  description: Pagination parameters (offset, limit). Default limit 10, max 10. The `sort` field is not supported by this endpoint and must be omitted.
+                  description: Pagination parameters (offset, limit). Default limit 10, max 10. The `sort` field is not supported and will be rejected if provided.

🤖 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 `@docs/api.yaml` around lines 648 - 664, Clarify whether the unsupported `sort`
field in the `pagination_params` is rejected or ignored: update the endpoint
description for the `user_address`/`session_key` retrieval to state explicitly
whether requests containing `pagination.sort` will produce a validation error or
will be silently ignored, and ensure the documentation text references
`pagination_params` and the `sort` field and matches the actual API validation
behavior (or update validation to match the doc) so clients know to omit `sort`
when calling this endpoint.

nitronode/api/app_session_v1/submit_session_key_state.go (1)

89-122: 💤 Low value

Document the session key cap race condition trade-off.

The TODO comment at lines 107-113 documents that concurrent registration of different new keys for the same user can bypass the cap by at most (concurrent writers - 1) keys. This is acceptable as a soft DOS bound, but the documented mitigation (per-user advisory lock) should be tracked if a hard quota is ever required.

The implementation correctly prevents the cap check from blocking legitimate key rotation (existing keys can always be updated regardless of cap).

🤖 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/submit_session_key_state.go` around lines 89 -
122, The TODO about the race condition when registering different new session
keys needs to be expanded to document the trade-off and a tracked mitigation:
update the comment near LockSessionKeyState/CountSessionKeysForUser to state
that concurrent new-key writers can exceed h.maxSessionKeysPerUser by up to
(concurrent writers - 1), mark this as an intentional soft DOS bound, and add a
tracked follow-up (e.g., TODO/ISSUE) recommending using a per-user advisory lock
via pg_advisory_xact_lock(hashtext(user_address)) if a hard quota is later
required; reference the symbols LockSessionKeyState, CountSessionKeysForUser and
h.maxSessionKeysPerUser in the comment so maintainers can find the relevant code
and the suggested fix.

nitronode/api/channel_v1/submit_state.go (1)

178-186: 💤 Low value

Consider consolidating the home channel status checks.

The MutualLock branch duplicates the channelStatus != Open check from lines 73-81. Since the earlier check already rejects non-Open channels with "home channel is not materialized onchain," this second check at line 183 is unreachable when the transition is enabled.

When enabling MutualLock support, consider removing this redundant check to simplify the code path.

🤖 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/submit_state.go` around lines 178 - 186, The
MutualLock case duplicates the earlier check for channelStatus !=
core.ChannelStatusOpen (which already returns "home channel is not materialized
onchain"); remove the redundant if-block inside the TransitionTypeMutualLock
branch so the code relies on the prior check, leaving only the
MutualLock-specific logic (or, if you are implementing MutualLock support now,
replace the duplicate guard with the actual MutualLock handling instead of
returning the duplicated error). Ensure you update/remove the duplicated return
rpc.Errorf("home channel is not materialized onchain") and keep the surrounding
comment about needing the home channel materialized for MutualLock.

nitronode/blockchain_worker.go (1)

49-57: ⚡ Quick win

Add an exported doc comment for NewBlockchainWorker.

NewBlockchainWorker is exported and was modified, but it still has no GoDoc comment.

As per coding guidelines, **/*.go: Follow gofmt formatting and include doc comments on all exported names in Go.

🤖 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/blockchain_worker.go` around lines 49 - 57, Add a GoDoc comment for
the exported constructor NewBlockchainWorker: insert a comment immediately above
the NewBlockchainWorker function that starts with "NewBlockchainWorker" and
briefly describes what the constructor returns and its purpose (e.g., creates
and initializes a BlockchainWorker for the given blockchainID using the provided
client, store, channelSigner, assetStore, logger, and metrics exporter). Keep
the comment concise, follow GoDoc style (first word is the function name) and
ensure it reads as documentation for callers.

nitronode/runtime.go (1)

87-88: 💤 Low value

Doc/code mismatch on the byte-rate "disable" sentinel.

Line 87 comment says Set <0 to disable, but the gate at line 251 uses if bytesPerSec > 0, so 0 also disables byte limiting. Either tighten the gate to >= 0 or relax the doc to match actual behavior — operators following the doc and setting -1 will get the same result as 0, which is fine, but anyone parsing the doc literally as "must be >= 0 to enable" will be confused.

📝 Suggested doc fix

-	// WsBytesPerSec is the steady-state byte budget per connection. Set <0 to disable.
+	// WsBytesPerSec is the steady-state byte budget per connection. Set <=0 to disable.
 	WsBytesPerSec float64 `yaml:"ws_bytes_per_sec" env:"NITRONODE_WS_BYTES_PER_SEC" env-default:"262144"`

Also applies to: 251-251

🤖 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/runtime.go` around lines 87 - 88, Doc/code mismatch: update the
WsBytesPerSec field comment to match the runtime gate that enables byte-limiting
only when bytesPerSec > 0. Change the comment on WsBytesPerSec (the struct field
named WsBytesPerSec) to state that values <= 0 disable byte limiting (or
explicitly "only values > 0 enable byte limiting"), so the docs and the check in
the runtime (the bytesPerSec > 0 gate) are consistent.

nitronode/config/migrations/postgres/20260513000000_add_channel_status_closing.sql (1)

14-14: ⚡ Quick win

Coordinate this enum remap with the application rollout.

This migration silently changes the on-disk encoding of Closed from 3 to 4. If the migration runs before the new application binary that knows Closed = 4 is fully rolled out (or vice versa), live rows will be read with the wrong status: any in-flight reads on the old binary that see status = 4 will misinterpret it, and new-binary writes that land before the UPDATE flips existing rows will leave a mix of encodings for Closed.

Worth confirming the deploy plan ensures all writers/readers are on the new enum before this migration runs, or that the service is briefly drained during apply.

🤖 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/config/migrations/postgres/20260513000000_add_channel_status_closing.sql`
at line 14, This migration remaps the numeric enum for Closed from 3 to 4
(UPDATE channels SET status = 4 WHERE status = 3) and must be applied only when
all readers/writers understand the new encoding; change the rollout to a safe,
coordinated two-step migration: 1) deploy an app update that treats both 3 and 4
as Closed (make Channel status parsing in the app accept old and new values), 2)
run the SQL backfill (the existing UPDATE in
20260513000000_add_channel_status_closing.sql) to rewrite rows to 4, and 3)
deploy a final app that treats 4 as canonical and removes 3 support;
alternatively add a short drain/maintenance window or a runtime migration guard
that verifies all nodes are the new version before executing the UPDATE.

pkg/rpc/node.go (1)

135-139: 💤 Low value

Consider clarifying where the default is applied.

The doc comment states "Non-positive values fall back to the default" and documents "default: 128 KiB", but this function doesn't contain the default-assignment logic. Readers may expect to see validation or default handling here.

If the default is applied in NewWebsocketConnection, consider adding a cross-reference: "Non-positive values are normalized to 128 KiB in NewWebsocketConnection."

🤖 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/node.go` around lines 135 - 139, The comment for WsConnMaxMessageSize
is ambiguous about where the default is applied; update the docstring to say
that non-positive values are normalized to the 128 KiB default in
NewWebsocketConnection so readers know where default/validation logic lives,
e.g., mention "Non-positive values are normalized to 128 KiB in
NewWebsocketConnection"; reference WsConnMaxMessageSize and
NewWebsocketConnection to help locate the handling.

sdk/go/app_session.go (1)

417-429: 💤 Low value

Consider validating session key address matches signer.

The function accepts a sessionKeySigner but doesn't verify that sessionKeySigner.PublicKey().Address() matches state.SessionKey. While the doc comment states "The signer must be the holder of the session key," a runtime check would catch caller errors earlier.

🛡️ Optional validation

 func SignAppSessionKeyOwnership(state app.AppSessionKeyStateV1, sessionKeySigner *sign.EthereumMsgSigner) (string, error) {
+	signerAddr := sessionKeySigner.PublicKey().Address().String()
+	if signerAddr != state.SessionKey {
+		return "", fmt.Errorf("signer address %s does not match state.SessionKey %s", signerAddr, state.SessionKey)
+	}
 	packed, err := app.PackAppSessionKeyStateV1(state)

🤖 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/app_session.go` around lines 417 - 429, SignAppSessionKeyOwnership
currently signs without verifying the signer actually owns the session key; add
a runtime check inside SignAppSessionKeyOwnership that compares
sessionKeySigner.PublicKey().Address() (or the appropriate
PublicKey().Address()/AddressHex() accessor) to state.SessionKey and return a
descriptive error if they differ before packing/signing; keep the existing
packing and Sign call (use the existing sessionKeySigner and state types) so
callers get an immediate error when the signer does not match the declared
SessionKey.

🤖 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 `@contracts/test/mocks/OversizedReturnERC20.sol`:
- Around line 38-40: The public function mint in the OversizedReturnERC20
contract is missing NatSpec documentation; add a Solidity NatSpec comment block
(/// `@notice`, `@dev`, and `@param` tags at minimum) immediately above the mint
function declaration to describe its behavior, any important notes, and the
meaning of the parameters (e.g., to and amount), so tools and reviewers can
understand its intent.
- Around line 23-36: The public function transfer lacks NatSpec documentation;
add a NatSpec comment block above the transfer function describing its purpose,
parameters (address to, uint256 amount), return behavior (custom oversized
return data controlled by RETURN_DATA_SIZE and FIRST_WORD), and any side effects
(calls _transfer when FIRST_WORD != 0) so callers and static analyzers
understand the nonstandard return payload and state changes in transfer.

In `@nitronode/api/app_session_v1/get_last_key_states_test.go`:
- Around line 45-85: Both tests set expectations on mockStore but never assert
them; add mockStore.AssertExpectations(t) at the end of
TestGetLastKeyStates_DefaultsToPageOneOnEmptyResult and at the end of
TestGetLastKeyStates_PaginationMetadata_AlignedOffset so the mocked
GetLastAppSessionKeyStates call is verified (locate functions
TestGetLastKeyStates_DefaultsToPageOneOnEmptyResult and
TestGetLastKeyStates_PaginationMetadata_AlignedOffset and append
mockStore.AssertExpectations(t) after the existing asserts).

In `@nitronode/blockchain_worker.go`:
- Around line 216-227: submitChallenge currently dereferences w.assetStore and
w.channelSigner (and then calls w.client.Challenge) without guarding for nil,
causing runtime panics; add precondition checks at the top of submitChallenge to
verify w.assetStore != nil and w.channelSigner != nil (and optionally w.client
!= nil) and return a clear, wrapped error (e.g., "missing channelSigner" /
"missing assetStore") instead of proceeding to core.PackChallengeState or Sign;
update error messages to reference submitChallenge and the offending field to
make failures explicit.

In `@nitronode/event_handlers/service.go`:
- Around line 314-326: When lastSignedState exists with a newer Version but
lastSignedState.EscrowLedger == nil, the handler currently only logger.Warns and
skips auto-defense; change that branch to (a) log an error/emit a metric via
logger.Error and then (b) call
s.scheduleHomeChannelChallengeForEscrowDeposit(ctx, tx, chanID,
event.StateVersion) so the home-chain defense is scheduled (instead of falling
through), keeping the existing
tx.ScheduleFinalizeEscrowDeposit(lastSignedState.ID, ...) path for the non-nil
EscrowLedger case and preserving error returns on failures.

In `@pkg/blockchain/evm/validator_watcher.go`:
- Around line 71-83: The code advances the duplicate-skip watermark (headBlock)
before successfully fetching historical logs, so if client.FilterLogs fails the
live loop later drops events <= headBlock and they are lost; change the logic so
headBlock (and any fromBlock advancement) is only updated after FilterLogs
returns successfully and the returned histLogs have been processed (i.e., move
the headBlock = header.Number.Uint64() assignment and any fromBlock update into
the err == nil branch after processing histLogs), and ensure the live-check that
drops logs <= headBlock in the live loop respects this (do not advance headBlock
on FilterLogs error).

In `@sdk/go/channel.go`:
- Around line 986-1000: SignChannelSessionKeyOwnership currently dereferences
sessionKeySigner without checking for nil, which can panic; add an explicit nil
guard at the start of SignChannelSessionKeyOwnership that returns a clear error
(e.g., "nil sessionKeySigner") if sessionKeySigner == nil, then proceed with
computing metadataHash (core.GetChannelSessionKeyAuthMetadataHashV1), packing
(core.PackChannelKeyStateV1) and signing (sessionKeySigner.Sign) as before;
ensure the returned error wraps context so callers get actionable failure
information.

In `@sdk/go/validator_watcher.go`:
- Around line 45-48: The code currently calls ethclient.Dial(rpcURL) which
ignores the provided context; replace that call with ethclient.DialContext(ctx,
rpcURL) so the connection respects cancellation/deadlines from the caller's ctx,
keeping the rest of the error handling (returning fmt.Errorf with chainID)
intact and still assigning the result to ethCl.

In `@sdk/ts/src/client.ts`:
- Around line 1697-1712: The signChannelSessionKeyOwnership function currently
trusts the provided sessionKeySigner and can produce a bad signature if the
signer does not control state.session_key; update signChannelSessionKeyOwnership
to first derive or query the signer's address (using the EthereumMsgSigner API
on the signer instance) and compare it to state.session_key (normalized to the
same hex/address format), and if they differ throw a clear error (e.g.,
"sessionKeySigner does not match state.session_key") so callers fail fast; apply
the same check to the analogous signing routine around the 1794–1800 block.
- Around line 1892-1900: createEVMClients currently always builds the
publicClient with http(rpcUrl), which fails for ws:// or wss:// RPC endpoints
used by watchValidatorRegistered; update createEVMClients to detect the rpcUrl
scheme and use webSocket({ url: rpcUrl }) when the URL starts with "ws://" or
"wss://", otherwise continue using http({ url: rpcUrl }), ensure the returned
publicClient (and any other client created there) uses the chosen transport, and
import/use viem's webSocket helper alongside http so WebSocket endpoints
properly support push-based delivery to watchValidatorRegistered.

---

Outside diff comments:
In `@contracts/test/mocks/MalformedReturningERC20.sol`:
- Around line 42-44: The public function mint is missing NatSpec comments; add a
NatSpec block immediately above the mint function (function name: mint)
including at least `@notice` describing what the function does, `@dev` for any
implementation notes, and `@param` to document the "to" and "amount" parameters
(and `@return` if you later add returns). Ensure the comment block uses the
triple-slash or /** */ NatSpec format and sits directly above the mint
declaration so tooling and linters pick it up.
- Around line 16-27: The public function transfer in MalformedReturningERC20.sol
is missing NatSpec comments; add a Solidity NatSpec block above the transfer
function (the public pure override transfer(address, uint256) returns (bool)
declaration) documenting the purpose/behavior, each parameter (`@param` for the
address and uint256), the return value (`@return`) and any important notes about
its malformed behavior (that it returns a single byte and does not perform an
actual transfer) so consumers understand the intentionally abnormal behavior.
- Around line 29-40: The public function transferFrom is missing NatSpec; add a
Solidity NatSpec comment block above the transferFrom function in
MalformedReturningERC20.sol including at minimum `@notice` describing the mock
behavior, `@dev` explaining it returns a single byte without transferring
(malformed token simulation), `@param` annotations for the address parameters and
uint256, and `@return` describing the malformed boolean-like return value so
tooling and consumers can understand the purpose and behavior of transferFrom.

In `@pkg/rpc/node.go`:
- Around line 33-38: Add a proper Go doc comment for the exported struct
ByteTokenBucket describing its purpose and usage (what it represents, how
tokens/bytes are consumed/refilled, and any concurrency guarantees) immediately
above the type declaration for ByteTokenBucket so it satisfies Go exported-name
documentation rules; ensure the comment starts with "ByteTokenBucket" and is
formatted as a full sentence.

---

Nitpick comments:
In `@contracts/src/Utils.sol`:
- Around line 19-31: Add a verification that the hardcoded 0xC0 size used in
getChannelId matches the actual ChannelDefinition layout: implement either a
compile-time/assertion or a unit test that computes the struct size for
ChannelDefinition and fails if it differs from 6*32 (192) so future edits can’t
silently break getChannelId; reference the getChannelId function and the
ChannelDefinition struct when adding the check (e.g., a solidity constant or
assert in an initialization/test function that compares abi.encode(def).length
or a computed SIZE constant against 192).

In `@contracts/test/mocks/OversizedReturnERC20.sol`:
- Around line 11-12: Update the comment in OversizedReturnERC20 (the test mock
contract) to explicitly state that the “a zero return value signals failure
without a state change” pattern is a deliberate deviation from the ERC20
standard used only for testing; reference the contract name OversizedReturnERC20
and its transfer/transferFrom behavior so readers know this mock intentionally
returns 0 instead of reverting to simulate legacy/non-standard token behavior
for tests.

In `@docs/api.yaml`:
- Around line 847-863: The docs currently say the pagination `sort` field "is
not supported and must be omitted" but the type allows it; update the endpoint
description for the pagination field to explicitly state the validation
behavior: clarify whether the server will reject requests containing
`pagination.sort` (returning a 400 with a validation error like "pagination.sort
is not supported") or will silently ignore it; then make the text precise (e.g.,
"The server validates and will reject requests that include `pagination.sort`
with HTTP 400: 'pagination.sort is not supported'") and apply the same wording
to the `pagination` description under this endpoint so consumers know the exact
validation outcome.
- Around line 648-664: Clarify whether the unsupported `sort` field in the
`pagination_params` is rejected or ignored: update the endpoint description for
the `user_address`/`session_key` retrieval to state explicitly whether requests
containing `pagination.sort` will produce a validation error or will be silently
ignored, and ensure the documentation text references `pagination_params` and
the `sort` field and matches the actual API validation behavior (or update
validation to match the doc) so clients know to omit `sort` when calling this
endpoint.

In `@nitronode/api/app_session_v1/submit_session_key_state.go`:
- Around line 89-122: The TODO about the race condition when registering
different new session keys needs to be expanded to document the trade-off and a
tracked mitigation: update the comment near
LockSessionKeyState/CountSessionKeysForUser to state that concurrent new-key
writers can exceed h.maxSessionKeysPerUser by up to (concurrent writers - 1),
mark this as an intentional soft DOS bound, and add a tracked follow-up (e.g.,
TODO/ISSUE) recommending using a per-user advisory lock via
pg_advisory_xact_lock(hashtext(user_address)) if a hard quota is later required;
reference the symbols LockSessionKeyState, CountSessionKeysForUser and
h.maxSessionKeysPerUser in the comment so maintainers can find the relevant code
and the suggested fix.

In `@nitronode/api/channel_v1/submit_state.go`:
- Around line 178-186: The MutualLock case duplicates the earlier check for
channelStatus != core.ChannelStatusOpen (which already returns "home channel is
not materialized onchain"); remove the redundant if-block inside the
TransitionTypeMutualLock branch so the code relies on the prior check, leaving
only the MutualLock-specific logic (or, if you are implementing MutualLock
support now, replace the duplicate guard with the actual MutualLock handling
instead of returning the duplicated error). Ensure you update/remove the
duplicated return rpc.Errorf("home channel is not materialized onchain") and
keep the surrounding comment about needing the home channel materialized for
MutualLock.

In `@nitronode/blockchain_worker.go`:
- Around line 49-57: Add a GoDoc comment for the exported constructor
NewBlockchainWorker: insert a comment immediately above the NewBlockchainWorker
function that starts with "NewBlockchainWorker" and briefly describes what the
constructor returns and its purpose (e.g., creates and initializes a
BlockchainWorker for the given blockchainID using the provided client, store,
channelSigner, assetStore, logger, and metrics exporter). Keep the comment
concise, follow GoDoc style (first word is the function name) and ensure it
reads as documentation for callers.

In
`@nitronode/config/migrations/postgres/20260513000000_add_channel_status_closing.sql`:
- Line 14: This migration remaps the numeric enum for Closed from 3 to 4 (UPDATE
channels SET status = 4 WHERE status = 3) and must be applied only when all
readers/writers understand the new encoding; change the rollout to a safe,
coordinated two-step migration: 1) deploy an app update that treats both 3 and 4
as Closed (make Channel status parsing in the app accept old and new values), 2)
run the SQL backfill (the existing UPDATE in
20260513000000_add_channel_status_closing.sql) to rewrite rows to 4, and 3)
deploy a final app that treats 4 as canonical and removes 3 support;
alternatively add a short drain/maintenance window or a runtime migration guard
that verifies all nodes are the new version before executing the UPDATE.

In `@nitronode/runtime.go`:
- Around line 87-88: Doc/code mismatch: update the WsBytesPerSec field comment
to match the runtime gate that enables byte-limiting only when bytesPerSec > 0.
Change the comment on WsBytesPerSec (the struct field named WsBytesPerSec) to
state that values <= 0 disable byte limiting (or explicitly "only values > 0
enable byte limiting"), so the docs and the check in the runtime (the
bytesPerSec > 0 gate) are consistent.

In `@pkg/rpc/node.go`:
- Around line 135-139: The comment for WsConnMaxMessageSize is ambiguous about
where the default is applied; update the docstring to say that non-positive
values are normalized to the 128 KiB default in NewWebsocketConnection so
readers know where default/validation logic lives, e.g., mention "Non-positive
values are normalized to 128 KiB in NewWebsocketConnection"; reference
WsConnMaxMessageSize and NewWebsocketConnection to help locate the handling.

In `@sdk/go/app_session.go`:
- Around line 417-429: SignAppSessionKeyOwnership currently signs without
verifying the signer actually owns the session key; add a runtime check inside
SignAppSessionKeyOwnership that compares sessionKeySigner.PublicKey().Address()
(or the appropriate PublicKey().Address()/AddressHex() accessor) to
state.SessionKey and return a descriptive error if they differ before
packing/signing; keep the existing packing and Sign call (use the existing
sessionKeySigner and state types) so callers get an immediate error when the
signer does not match the declared SessionKey.

🪄 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: 0e87d920-8753-46d1-ae21-86af4121c2ef

📥 Commits

Reviewing files that changed from the base of the PR and between 8449372 and ba58e89.

⛔ Files ignored due to path filters (2)

• sdk/ts-compat/test/unit/__snapshots__/public-api-drift.test.ts.snap is excluded by !**/*.snap
• sdk/ts/test/unit/__snapshots__/public-api-drift.test.ts.snap is excluded by !**/*.snap

📒 Files selected for processing (141)

• cerebro/commands.go
• contracts/SECURITY.md
• contracts/src/ChannelHub.sol
• contracts/src/Utils.sol
• contracts/src/interfaces/Types.sol
• contracts/test/ChannelHub_Node.t.sol
• contracts/test/ChannelHub_challenge/ChannelHub_challengeSessionKeyValidator.t.sol
• contracts/test/ChannelHub_lifecycle/ChannelHub_singlechain.lifecycle.t.sol
• contracts/test/ChannelHub_nonRevertingPushFunds.t.sol
• contracts/test/ChannelHub_sigValidator.t.sol
• contracts/test/ChannelHub_units/ChannelHub_challenge.t.sol
• contracts/test/ChannelHub_units/ChannelHub_createChannel.t.sol
• contracts/test/ChannelHub_units/ChannelHub_finalizeEscrowDeposit.t.sol
• contracts/test/ChannelHub_units/ChannelHub_finalizeEscrowWithdrawal.t.sol
• contracts/test/ChannelHub_units/ChannelHub_initiateEscrowDeposit.t.sol
• contracts/test/Utils.t.sol
• contracts/test/mocks/MalformedReturningERC20.sol
• contracts/test/mocks/OversizedReturnERC20.sol
• docs/api.yaml
• docs/data_models.mmd
• docs/protocol/enforcement.md
• docs/protocol/overview.md
• docs/protocol/security-and-limitations.md
• nitronode/README.md
• nitronode/api/app_session_v1/README.md
• nitronode/api/app_session_v1/create_app_session.go
• nitronode/api/app_session_v1/create_app_session_test.go
• nitronode/api/app_session_v1/get_last_key_states.go
• nitronode/api/app_session_v1/get_last_key_states_test.go
• nitronode/api/app_session_v1/handler.go
• nitronode/api/app_session_v1/interface.go
• nitronode/api/app_session_v1/rebalance_app_sessions_test.go
• nitronode/api/app_session_v1/submit_app_state_test.go
• nitronode/api/app_session_v1/submit_deposit_state.go
• nitronode/api/app_session_v1/submit_deposit_state_test.go
• nitronode/api/app_session_v1/submit_session_key_state.go
• nitronode/api/app_session_v1/submit_session_key_state_test.go
• nitronode/api/app_session_v1/testing.go
• nitronode/api/app_session_v1/utils.go
• nitronode/api/channel_v1/get_channels.go
• nitronode/api/channel_v1/get_home_channel.go
• nitronode/api/channel_v1/get_home_channel_test.go
• nitronode/api/channel_v1/get_last_key_states.go
• nitronode/api/channel_v1/get_last_key_states_test.go
• nitronode/api/channel_v1/handler.go
• nitronode/api/channel_v1/interface.go
• nitronode/api/channel_v1/request_creation.go
• nitronode/api/channel_v1/request_creation_test.go
• nitronode/api/channel_v1/submit_session_key_state.go
• nitronode/api/channel_v1/submit_session_key_state_test.go
• nitronode/api/channel_v1/submit_state.go
• nitronode/api/channel_v1/submit_state_test.go
• nitronode/api/channel_v1/testing.go
• nitronode/api/channel_v1/utils.go
• nitronode/api/rate_limits.go
• nitronode/api/rpc_router.go
• nitronode/blockchain_worker.go
• nitronode/chart/README.md
• nitronode/chart/config/stress-v1/nitronode.yaml.gotmpl
• nitronode/chart/templates/helpers/_common.tpl
• nitronode/chart/values.yaml
• nitronode/config/migrations/postgres/20251222000000_initial_schema.sql
• nitronode/config/migrations/postgres/20260507000000_add_current_session_key_states.sql
• nitronode/config/migrations/postgres/20260508000000_session_key_ownership_constraints.sql
• nitronode/config/migrations/postgres/20260513000000_add_channel_status_closing.sql
• nitronode/event_handlers/service.go
• nitronode/event_handlers/service_test.go
• nitronode/event_handlers/testing.go
• nitronode/main.go
• nitronode/metrics/exporter.go
• nitronode/metrics/interface.go
• nitronode/runtime.go
• nitronode/runtime_config_test.go
• nitronode/store/database/app_session_key_state.go
• nitronode/store/database/app_session_key_state_test.go
• nitronode/store/database/blockchain_action.go
• nitronode/store/database/channel.go
• nitronode/store/database/channel_session_key_state.go
• nitronode/store/database/channel_session_key_state_test.go
• nitronode/store/database/channel_test.go
• nitronode/store/database/current_session_key_state.go
• nitronode/store/database/current_session_key_state_test.go
• nitronode/store/database/database.go
• nitronode/store/database/database_test.go
• nitronode/store/database/db_store.go
• nitronode/store/database/db_store_test.go
• nitronode/store/database/interface.go
• nitronode/store/database/state.go
• nitronode/store/database/testing.go
• pkg/app/session_key_v1.go
• pkg/app/session_key_v1_test.go
• pkg/blockchain/evm/channel_hub_reactor.go
• pkg/blockchain/evm/channel_hub_reactor_test.go
• pkg/blockchain/evm/validator_watcher.go
• pkg/core/event.go
• pkg/core/interface.go
• pkg/core/session_key.go
• pkg/core/session_key_test.go
• pkg/core/types.go
• pkg/core/utils.go
• pkg/rpc/api.go
• pkg/rpc/connection.go
• pkg/rpc/connection_hub.go
• pkg/rpc/connection_test.go
• pkg/rpc/node.go
• pkg/rpc/rate_limiter.go
• pkg/rpc/rate_limiter_test.go
• pkg/rpc/types.go
• pkg/sign/eth_msg_signer.go
• protocol-description.md
• sdk/go/README.md
• sdk/go/app_session.go
• sdk/go/channel.go
• sdk/go/examples/app_sessions/lifecycle.go
• sdk/go/examples/validator_watcher/main.go
• sdk/go/utils.go
• sdk/go/validator_watcher.go
• sdk/ts-compat/README.md
• sdk/ts-compat/src/client.ts
• sdk/ts-compat/src/types.ts
• sdk/ts-compat/test/unit/client-mapping.test.ts
• sdk/ts-compat/test/unit/public-api-drift.test.ts
• sdk/ts/README.md
• sdk/ts/examples/app_sessions/lifecycle.ts
• sdk/ts/examples/example-app/README.md
• sdk/ts/examples/example-app/src/components/WalletDashboard.tsx
• sdk/ts/src/app/types.ts
• sdk/ts/src/blockchain/evm/channel_hub_abi.ts
• sdk/ts/src/blockchain/evm/index.ts
• sdk/ts/src/blockchain/evm/validator_watcher.ts
• sdk/ts/src/client.ts
• sdk/ts/src/core/event.ts
• sdk/ts/src/core/types.ts
• sdk/ts/src/core/utils.ts
• sdk/ts/src/rpc/api.ts
• sdk/ts/src/rpc/types.ts
• sdk/ts/src/session_key_state_transforms.ts
• sdk/ts/src/utils.ts
• sdk/ts/test/unit/blockchain/evm/validator_watcher.test.ts
• sdk/ts/test/unit/rpc-drift.test.ts
• sdk/ts/test/unit/transform-drift.test.ts

[coderabbitai]

🧹 Nitpick comments (1)

pkg/blockchain/evm/channel_hub_reactor_test.go (1)

416-418: ⚡ Quick win

Tighten checkpointed-event matcher to include channel/version fields.

The matcher currently validates only UserSig, so a miswired ChannelID/StateVersion could still pass this test.

Suggested diff

-	handler.On("HandleHomeChannelCheckpointed", mock.Anything, mock.Anything, mock.MatchedBy(func(ev *core.HomeChannelCheckpointedEvent) bool {
-		return ev.UserSig == hexutil.Encode([]byte{0xde, 0xad, 0xbe, 0xef})
-	})).Return(nil)
+	handler.On("HandleHomeChannelCheckpointed", mock.Anything, mock.Anything, mock.MatchedBy(func(ev *core.HomeChannelCheckpointedEvent) bool {
+		return ev.ChannelID == hexutil.Encode(channelID[:]) &&
+			ev.StateVersion == 7 &&
+			ev.UserSig == hexutil.Encode([]byte{0xde, 0xad, 0xbe, 0xef})
+	})).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 `@pkg/blockchain/evm/channel_hub_reactor_test.go` around lines 416 - 418, The
mock matcher for handler.On("HandleHomeChannelCheckpointed", ...) only asserts
UserSig and should also assert the checkpointed event's ChannelID and
StateVersion to prevent false positives; update the MatchedBy anonymous function
that takes *core.HomeChannelCheckpointedEvent to additionally compare
ev.ChannelID and ev.StateVersion against the test's expected values (the same
variables used to create the checkpoint in this test) along with the existing
UserSig check so all three fields must match before returning true.

🤖 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.

Nitpick comments:
In `@pkg/blockchain/evm/channel_hub_reactor_test.go`:
- Around line 416-418: The mock matcher for
handler.On("HandleHomeChannelCheckpointed", ...) only asserts UserSig and should
also assert the checkpointed event's ChannelID and StateVersion to prevent false
positives; update the MatchedBy anonymous function that takes
*core.HomeChannelCheckpointedEvent to additionally compare ev.ChannelID and
ev.StateVersion against the test's expected values (the same variables used to
create the checkpoint in this test) along with the existing UserSig check so all
three fields must match before returning true.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6177bce3-6913-4c01-bb67-abc1f9336b87

📥 Commits

Reviewing files that changed from the base of the PR and between 6152e74 and af32b71.

📒 Files selected for processing (12)

• contracts/src/ChannelHub.sol
• contracts/test/ChannelHub_escrowDepositPurge/ChannelHub_purgeEscrowDeposits.t.sol
• contracts/test/ChannelHub_units/ChannelHub_finalizeEscrowDeposit.t.sol
• contracts/test/ChannelHub_units/ChannelHub_finalizeEscrowWithdrawal.t.sol
• nitronode/event_handlers/service.go
• nitronode/event_handlers/service_test.go
• pkg/blockchain/evm/channel_hub_abi.go
• pkg/blockchain/evm/channel_hub_reactor.go
• pkg/blockchain/evm/channel_hub_reactor_test.go
• pkg/core/event.go
• pkg/core/interface.go
• sdk/ts/src/blockchain/evm/channel_hub_abi.ts

🚧 Files skipped from review as they are similar to previous changes (2)

• contracts/test/ChannelHub_units/ChannelHub_finalizeEscrowDeposit.t.sol
• pkg/blockchain/evm/channel_hub_reactor.go