sdk/compat: follow-up to #573 and residual review cleanup#582
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements real auth RPC builders and EIP‑712 signing, adds deterministic app-session hashing and quorum signature helpers, expands RPC types/transactions, enhances NitroliteClient app-session and session-key flows, and adds comprehensive examples and docs for the compat layer. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant AuthBuilder as Auth Message Builder
participant Signer as Message Signer
participant Wallet as Wallet Client
participant RPC as NitroliteRPC
App->>AuthBuilder: createAuthRequestMessage(params)
AuthBuilder->>AuthBuilder: generate requestId & timestamp
AuthBuilder->>AuthBuilder: build NitroliteRPC request tuple
App->>Signer: createEIP712AuthMessageSigner(...)
AuthBuilder->>Signer: sign request (challenge flow)
Signer->>Wallet: signTypedData(EIP-712 payload)
Wallet-->>Signer: signature
Signer-->>AuthBuilder: signature (formatted)
AuthBuilder->>RPC: signRequestMessage(msg, signer)
RPC-->>App: JSON RPC message (with sig)
sequenceDiagram
participant App as Application
participant Client as NitroliteClient
participant Pack as App Signing Utils
participant S1 as Signer A
participant S2 as Signer B
participant S3 as Signer C
participant Inner as Inner Client
App->>Client: createAppSession(def, allocations, quorumSigs)
Client->>Pack: packCreateAppSessionHash(params)
Pack-->>Client: hash
Client->>S1: sign(hash)
Client->>S2: sign(hash)
Client->>S3: sign(hash)
S1-->>Client: sig1
S2-->>Client: sig2
S3-->>Client: sig3
Client->>Pack: toWalletQuorumSignature([sig1,sig2,sig3])
Pack-->>Client: formatted quorumSigs
Client->>Inner: submitCreateAppSession(..., quorum_sigs)
Inner-->>Client: app session result
Client-->>App: app session
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Summary of ChangesHello @ihsraham, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the compatibility SDK by implementing previously stubbed authentication helpers and cleaning up client-side logic. It also updates documentation to accurately reflect the finalized behavior introduced in a previous PR, ensuring clarity and correctness for developers migrating to the new SDK version. The changes aim to improve the robustness and maintainability of the compatibility layer. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Code Review
This pull request is a follow-up to a previous one, primarily focused on replacing auth stubs with real implementations and cleaning up documentation and code. The changes in sdk/compat/src/auth.ts are significant, introducing v0.5.3-compatible authentication logic. Other changes include various cleanups and refactorings in sdk/compat/src/client.ts and updates to RPC helpers and types. My review focuses on potential issues with data types, type safety, and maintainability in the new implementations.
sdk/ts-compat/src/auth.ts
Outdated
Comment on lines
+36
to
+38
| return JSON.stringify(request, (_, value) => | ||
| typeof value === 'bigint' ? Number(value) : value, | ||
| ); |
Contributor
There was a problem hiding this comment.
The replacer function in JSON.stringify converts bigint values to Number. This can lead to a loss of precision for large bigint values that exceed Number.MAX_SAFE_INTEGER. While this may be acceptable for timestamps for the near future, it could become a problem later or for other bigint values. To preserve precision, consider converting bigint to a string if the receiving API supports it.
Suggested change
| return JSON.stringify(request, (_, value) => | |
| typeof value === 'bigint' ? Number(value) : value, | |
| ); | |
| return JSON.stringify(request, (_, value) => | |
| typeof value === 'bigint' ? value.toString() : value, | |
| ); |
| _domain: any, | ||
| ): any { | ||
| return async () => '0x'; | ||
| walletClient: any, |
Contributor
There was a problem hiding this comment.
The walletClient parameter is typed as any, which reduces type safety and makes the code harder to maintain. It appears to be a viem WalletClient. Please provide a specific type for it to improve maintainability and enable static analysis. You will likely need to add import type { WalletClient } from 'viem'; at the top of the file.
sdk/compat/src/rpc.ts
Outdated
| async signRequestMessage(msg: NitroliteRPCMessage, _signer: MessageSigner): Promise<NitroliteRPCMessage> { | ||
| return msg; | ||
| async signRequestMessage(msg: NitroliteRPCMessage, signer: MessageSigner): Promise<NitroliteRPCMessage> { | ||
| const signature = await signer(msg.req as any); |
Contributor
There was a problem hiding this comment.
The use of msg.req as any to call the signer bypasses a type mismatch. The MessageSigner type expects a Uint8Array, but msg.req is a tuple [number, string, any, number]. This any cast reduces type safety and should be addressed, perhaps by updating the MessageSigner type to reflect its actual usage throughout the compatibility layer.
Contributor
There was a problem hiding this comment.
Code Review
This pull request is a follow-up that implements feedback from a previous review, primarily by replacing authentication stubs with functional compatibility helpers and cleaning up related documentation and code. The changes are logical and significantly improve the compatibility layer's functionality. I've identified one area for improvement regarding type-safety with MessageSigner that is currently bypassed using a type assertion. Addressing this point will enhance code quality and robustness. Overall, this is a solid contribution.
sdk/compat/src/rpc.ts
Outdated
| async signRequestMessage(msg: NitroliteRPCMessage, _signer: MessageSigner): Promise<NitroliteRPCMessage> { | ||
| return msg; | ||
| async signRequestMessage(msg: NitroliteRPCMessage, signer: MessageSigner): Promise<NitroliteRPCMessage> { | ||
| const signature = await signer(msg.req as any); |
Contributor
There was a problem hiding this comment.
The as any cast here hides a type mismatch between the MessageSigner type and its usage. MessageSigner is typed as (payload: Uint8Array) => Promise<string>, but it's being called with msg.req, which is of type [number, string, any, number].
For better type safety and to remove the need for this cast, the MessageSigner type definition in sdk/compat/src/types.ts should be updated to reflect its actual usage. For example:
export type MessageSigner = (payload: NitroliteRPCMessage['req']) => Promise<string>;Or a union type could be used if the Uint8Array payload is still required elsewhere.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
sdk/compat/src/auth.ts (1)
83-99: Payload is accessed as a tuple despiteMessageSignertyping it asUint8Array.
payload[1]andpayload[2]assume the tuple structure[requestId, method, params, timestamp], confirming theMessageSignertype mismatch flagged inrpc.ts. This signer will only work when called throughNitroliteRPC.signRequestMessage(which passesmsg.req as any), not through any code path that actually passes aUint8Array.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/compat/src/auth.ts` around lines 83 - 99, The signer in auth.ts treats the incoming payload as a tuple (accessing payload[1], payload[2]) but MessageSigner is typed as Uint8Array elsewhere; fix by aligning types and parsing: update the MessageSigner typing (or the specific signer signature returned by the async function) to accept the tuple shape [requestId, method, params, timestamp] used by NitroliteRPC.signRequestMessage, or if you must accept a Uint8Array decode/deserialize it into that tuple before accessing indices; ensure the signer function (the returned async (payload: any) => { ... }) and rpc.ts/NitroliteRPC.signRequestMessage agree on the concrete type so payload[1]/payload[2] are valid and keep the existing validation of method and params.challenge.
🧹 Nitpick comments (6)
sdk/compat/src/auth.ts (5)
107-120: Error wrapping re-throws a newError, discarding the original stack trace.On line 119, a new
Erroris thrown with the message concatenated, which loses the original error's stack trace. Consider preserving causality:♻️ Preserve original error as cause
- throw new Error(`EIP-712 signing failed: ${errorMessage}`); + throw new Error(`EIP-712 signing failed: ${errorMessage}`, { cause: err });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/compat/src/auth.ts` around lines 107 - 120, The catch block around walletClient.signTypedData currently throws a new Error which discards the original stack; instead preserve causality by rethrowing the original error or constructing the new Error with the original error as its cause (e.g., new Error(`EIP-712 signing failed: ${errorMessage}`, { cause: err })), so update the catch in auth.ts (the try calling walletClient.signTypedData with EIP712AuthTypes and primaryType 'Policy') to include the original error as the cause or rethrow err after logging.
73-82:walletClient: anybypasses type safety — preferWalletClientfrom viem.The rest of the codebase (e.g.,
client.tsline 13) properly imports and typesWalletClientfrom viem. Usinganyhere loses type checking onwalletClient.account,walletClient.signTypedData, etc.♻️ Suggested fix
+import type { WalletClient } from 'viem'; + export function createEIP712AuthMessageSigner( - walletClient: any, + walletClient: WalletClient, partialMessage: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/compat/src/auth.ts` around lines 73 - 82, Replace the loose walletClient: any in createEIP712AuthMessageSigner with the proper WalletClient type from viem: import WalletClient and change the function signature to walletClient: WalletClient so TypeScript enforces walletClient.account, walletClient.signTypedData, etc.; ensure any uses of account/address and signTypedData in createEIP712AuthMessageSigner match the WalletClient API and adjust types for return values or casts if necessary.
21-23:generateRequestIdmixes timestamp with small random offset — collision-prone under burst traffic.For concurrent requests within the same millisecond window, the random offset (0–9999) added to
Date.now()provides limited uniqueness. This is likely acceptable for auth flows (low frequency), but if this helper is reused for high-throughput RPC calls, collisions are plausible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/compat/src/auth.ts` around lines 21 - 23, The generateRequestId function's current approach (Date.now() + small Math.random() offset) can collide under burst traffic; replace it with a stronger unique-id strategy: use a UUID (crypto.randomUUID()) returning a string, or if UUID isn't available, compose a string from Date.now() plus cryptographically-secure random bytes (via crypto.getRandomValues/crypto.randomBytes) or a process-wide monotonic counter to guarantee uniqueness; update generateRequestId's return type and any callers in auth.ts to accept a string ID if switching from number to string.
25-39:createAuthRequestMessageisasyncbut contains noawait.The function is marked
asyncbut performs no asynchronous operations — it synchronously builds and serializes the message. While this isn't a bug (the return typePromise<string>is still honored), it's slightly misleading. If theasyncis intentional for API consistency with othercreate*functions, that's fine to keep.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/compat/src/auth.ts` around lines 25 - 39, The function createAuthRequestMessage is marked async but does no asynchronous work; remove the async keyword and change its return type from Promise<string> to string (update the signature and any callers if they relied on a Promise), so it synchronously builds and returns the serialized request string; alternatively, if API consistency is required, keep async but add a comment explaining why it's intentionally async—prefer removing async and updating the signature in createAuthRequestMessage.
36-38: Align bigint serialization with codebase convention.The
expires_atfield (a Unix timestamp) is safe as aNumbersince it will never exceedNumber.MAX_SAFE_INTEGER. However, the rest of the codebase usesvalue.toString()for bigint serialization (sdk/ts/examples/example-app/src/utils.tsand integration tests). Update this file to match the preferred pattern for consistency:♻️ Align with codebase convention
return JSON.stringify(request, (_, value) => - typeof value === 'bigint' ? Number(value) : value, + typeof value === 'bigint' ? value.toString() : value, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/compat/src/auth.ts` around lines 36 - 38, Change the bigint-to-primitive serialization used in the JSON.stringify call so it matches the codebase convention: replace the current conversion that uses Number(value) with value.toString() (i.e., when typeof value === 'bigint' return value.toString()). Keep in mind the expires_at Unix timestamp remains safe as a Number elsewhere, but update the JSON.stringify callback in sdk/compat/src/auth.ts (the anonymous replacer passed to JSON.stringify) to use toString() for consistency with other code and tests.sdk/compat/src/types.ts (1)
343-343:LedgerAccountTypeis redundant —Addressis already`0x${string}`.viem's
Addresstype is defined as`0x${string}`, soAddress | \0x${string}`simplifies to justAddress. If the intent is to accept non-checksummed hex strings,Address` already covers that. Consider simplifying:♻️ Simplify type
-export type LedgerAccountType = Address | `0x${string}`; +export type LedgerAccountType = Address;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/compat/src/types.ts` at line 343, The type alias LedgerAccountType is redundant because viem's Address is already the template literal `0x${string}`; replace the current declaration export type LedgerAccountType = Address | `0x${string}`; with export type LedgerAccountType = Address (or remove the alias if unused) and update any usages expecting the union to accept Address; ensure imports for Address remain and run type checks to confirm no regressions in places referencing LedgerAccountType.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk/compat/src/rpc.ts`:
- Around line 47-50: The MessageSigner type is incorrect for how
signRequestMessage calls it with msg.req (a tuple) and how
createEIP712AuthMessageSigner expects tuple indices; update the MessageSigner
definition in types.ts to accept the actual request shape (e.g., (payload:
Uint8Array | [number, string, any, number]) => Promise<string> or better,
(payload: Uint8Array | NitroliteRPCRequest) => Promise<string>) so
createECDSAMessageSigner and createEIP712AuthMessageSigner conform to the same
contract, and ensure signRequestMessage calls signer with the matching type (or
alternatively serialize msg.req to Uint8Array here and update both signer
implementations to consume bytes).
In `@sdk/compat/src/types.ts`:
- Around line 345-355: The RPCTransaction interface declares createdAt as Date
but JSON RPC responses deliver date fields as strings; update the RPCTransaction
type for createdAt to accept string | Date (or switch to string if you prefer to
enforce explicit parsing elsewhere) and adjust any code that consumes
RPCTransaction (e.g., deserialization/validation paths) to parse the string into
a Date when needed so runtime values match the TypeScript type; reference
RPCTransaction and the createdAt property when making this change.
---
Duplicate comments:
In `@sdk/compat/src/auth.ts`:
- Around line 83-99: The signer in auth.ts treats the incoming payload as a
tuple (accessing payload[1], payload[2]) but MessageSigner is typed as
Uint8Array elsewhere; fix by aligning types and parsing: update the
MessageSigner typing (or the specific signer signature returned by the async
function) to accept the tuple shape [requestId, method, params, timestamp] used
by NitroliteRPC.signRequestMessage, or if you must accept a Uint8Array
decode/deserialize it into that tuple before accessing indices; ensure the
signer function (the returned async (payload: any) => { ... }) and
rpc.ts/NitroliteRPC.signRequestMessage agree on the concrete type so
payload[1]/payload[2] are valid and keep the existing validation of method and
params.challenge.
---
Nitpick comments:
In `@sdk/compat/src/auth.ts`:
- Around line 107-120: The catch block around walletClient.signTypedData
currently throws a new Error which discards the original stack; instead preserve
causality by rethrowing the original error or constructing the new Error with
the original error as its cause (e.g., new Error(`EIP-712 signing failed:
${errorMessage}`, { cause: err })), so update the catch in auth.ts (the try
calling walletClient.signTypedData with EIP712AuthTypes and primaryType
'Policy') to include the original error as the cause or rethrow err after
logging.
- Around line 73-82: Replace the loose walletClient: any in
createEIP712AuthMessageSigner with the proper WalletClient type from viem:
import WalletClient and change the function signature to walletClient:
WalletClient so TypeScript enforces walletClient.account,
walletClient.signTypedData, etc.; ensure any uses of account/address and
signTypedData in createEIP712AuthMessageSigner match the WalletClient API and
adjust types for return values or casts if necessary.
- Around line 21-23: The generateRequestId function's current approach
(Date.now() + small Math.random() offset) can collide under burst traffic;
replace it with a stronger unique-id strategy: use a UUID (crypto.randomUUID())
returning a string, or if UUID isn't available, compose a string from Date.now()
plus cryptographically-secure random bytes (via
crypto.getRandomValues/crypto.randomBytes) or a process-wide monotonic counter
to guarantee uniqueness; update generateRequestId's return type and any callers
in auth.ts to accept a string ID if switching from number to string.
- Around line 25-39: The function createAuthRequestMessage is marked async but
does no asynchronous work; remove the async keyword and change its return type
from Promise<string> to string (update the signature and any callers if they
relied on a Promise), so it synchronously builds and returns the serialized
request string; alternatively, if API consistency is required, keep async but
add a comment explaining why it's intentionally async—prefer removing async and
updating the signature in createAuthRequestMessage.
- Around line 36-38: Change the bigint-to-primitive serialization used in the
JSON.stringify call so it matches the codebase convention: replace the current
conversion that uses Number(value) with value.toString() (i.e., when typeof
value === 'bigint' return value.toString()). Keep in mind the expires_at Unix
timestamp remains safe as a Number elsewhere, but update the JSON.stringify
callback in sdk/compat/src/auth.ts (the anonymous replacer passed to
JSON.stringify) to use toString() for consistency with other code and tests.
In `@sdk/compat/src/types.ts`:
- Line 343: The type alias LedgerAccountType is redundant because viem's Address
is already the template literal `0x${string}`; replace the current declaration
export type LedgerAccountType = Address | `0x${string}`; with export type
LedgerAccountType = Address (or remove the alias if unused) and update any
usages expecting the union to accept Address; ensure imports for Address remain
and run type checks to confirm no regressions in places referencing
LedgerAccountType.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
sdk/compat/docs/migration-offchain.mdsdk/compat/docs/migration-overview.mdsdk/compat/src/auth.tssdk/compat/src/client.tssdk/compat/src/index.tssdk/compat/src/rpc.tssdk/compat/src/types.ts
Comment on lines
+47
to
50
| async signRequestMessage(msg: NitroliteRPCMessage, signer: MessageSigner): Promise<NitroliteRPCMessage> { | ||
| const signature = await signer(msg.req as any); | ||
| return { ...msg, sig: signature }; | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type=ts -C3 'MessageSigner' --glob '!node_modules/**'Repository: erc7824/nitrolite
Length of output: 11305
🏁 Script executed:
# Get the complete implementation of createEIP712AuthMessageSigner
sed -n '73,100p' sdk/compat/src/auth.ts
# Get NitroliteRPCMessage type definition
rg -n 'interface NitroliteRPCMessage|type NitroliteRPCMessage' -A 5 sdk/compat/src/types.ts
# Check what msg.req actually contains and how it's structured
rg -n 'msg\.req' sdk/compat/src/ -B 2 -A 2Repository: erc7824/nitrolite
Length of output: 1558
🏁 Script executed:
# Check how createECDSAMessageSigner is used in tests
rg -n 'createECDSAMessageSigner' -A 5 -B 1 test/integration/
# Check if createECDSAMessageSigner is used with signRequestMessage
rg -n 'signRequestMessage' sdk/compat/src/ -B 5 -A 5
# Check if createECDSAMessageSigner payload parameter is actually invoked
rg -n 'messageSKSigner\|messageWalletSigner' test/integration/common/ -B 2 -A 2Repository: erc7824/nitrolite
Length of output: 3712
🏁 Script executed:
# Search for usage of messageSKSigner and messageWalletSigner
rg -n 'messageSKSigner|messageWalletSigner' test/integration/ -B 2 -A 3
# Find and examine createCleanupSessionKeyCacheMessage
rg -n 'createCleanupSessionKeyCacheMessage|export.*createCleanupSessionKeyCacheMessage' -A 10 sdk/compat/src/Repository: erc7824/nitrolite
Length of output: 34669
🏁 Script executed:
# Get full createECDSAMessageSigner implementation
sed -n '34,50p' sdk/compat/src/signers.tsRepository: erc7824/nitrolite
Length of output: 326
Type mismatch: MessageSigner signature does not match actual usage.
MessageSigner is defined as (payload: Uint8Array) => Promise<string>, but signRequestMessage calls it with msg.req (a [number, string, any, number] tuple), hidden by an as any cast.
This creates a contract violation between implementations:
createECDSAMessageSignercorrectly implements the type as(payload: Uint8Array)and callsaccount.signMessage({ message: { raw: payload } }), expecting bytes.createEIP712AuthMessageSignerviolates the type by declaring(payload: any)and accessingpayload[1]andpayload[2]as tuple indices.
When createECDSAMessageSigner is passed to signRequestMessage (as seen in create_channel.test.ts:82 and other tests), it receives the tuple instead of Uint8Array, leading to incorrect behavior.
Recommended fix: Update MessageSigner to reflect the actual contract:
♻️ Suggested type fix in types.ts
-export type MessageSigner = (payload: Uint8Array) => Promise<string>;
+export type MessageSigner = (payload: any) => Promise<string>;This aligns the type with how both implementations and callers actually use it. Alternatively, serialize msg.req to Uint8Array before signing and update implementations to deserialize.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/compat/src/rpc.ts` around lines 47 - 50, The MessageSigner type is
incorrect for how signRequestMessage calls it with msg.req (a tuple) and how
createEIP712AuthMessageSigner expects tuple indices; update the MessageSigner
definition in types.ts to accept the actual request shape (e.g., (payload:
Uint8Array | [number, string, any, number]) => Promise<string> or better,
(payload: Uint8Array | NitroliteRPCRequest) => Promise<string>) so
createECDSAMessageSigner and createEIP712AuthMessageSigner conform to the same
contract, and ensure signRequestMessage calls signer with the matching type (or
alternatively serialize msg.req to Uint8Array here and update both signer
implementations to consume bytes).
Comment on lines
+345
to
+355
| export interface RPCTransaction { | ||
| id: number; | ||
| txType: RPCTxType; | ||
| fromAccount: LedgerAccountType; | ||
| fromAccountTag?: string; | ||
| toAccount: LedgerAccountType; | ||
| toAccountTag?: string; | ||
| asset: string; | ||
| amount: string; | ||
| createdAt: Date; | ||
| } |
Contributor
There was a problem hiding this comment.
createdAt: Date may cause issues when deserializing from JSON.
JSON payloads deliver dates as strings, not Date objects. If RPCTransaction is used to type raw RPC responses, consumers will get a string at runtime while TypeScript believes it's a Date. Consider using string | Date or string with a note that consumers should convert, unless this type is only used after explicit deserialization logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/compat/src/types.ts` around lines 345 - 355, The RPCTransaction interface
declares createdAt as Date but JSON RPC responses deliver date fields as
strings; update the RPCTransaction type for createdAt to accept string | Date
(or switch to string if you prefer to enforce explicit parsing elsewhere) and
adjust any code that consumes RPCTransaction (e.g., deserialization/validation
paths) to parse the string into a Date when needed so runtime values match the
TypeScript type; reference RPCTransaction and the createdAt property when making
this change.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk/compat/examples/lifecycle.ts (1)
102-138:⚠️ Potential issue | 🟡 MinorAlign wallet client chain with
CHAIN_ID(currently fixed to Sepolia).
Line 134 hard-codessepoliawhile Line 104 allows arbitraryCHAIN_ID, so running on non‑Sepolia will build clients against the wrong chain/RPC. Either guard against non‑Sepolia values or mapCHAIN_IDto the correct viem chain.🛠️ Suggested guard to avoid mismatched chains
const CHAIN_ID = parseInt(process.env.CHAIN_ID || '11155111', 10); if (Number.isNaN(CHAIN_ID)) { console.error('CHAIN_ID must be a valid integer'); process.exit(1); } +if (CHAIN_ID !== sepolia.id) { + console.error(`This example currently targets Sepolia only (CHAIN_ID=${sepolia.id}).`); + process.exit(1); +}#!/bin/bash # Verify whether the compat client relies on walletClient.chain or chainId internally. rg -n "walletClient" sdk/compat/src/client.ts -C3 rg -n "chainId" sdk/compat/src/client.ts -C3Also applies to: 395-396
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/compat/examples/lifecycle.ts` around lines 102 - 138, The code hard-codes the viem chain "sepolia" when calling createWalletClient in main (and similar places), which can mismatch the parsed CHAIN_ID; update the logic to derive the viem chain from CHAIN_ID or fail fast: either map CHAIN_ID to the appropriate viem chain object (e.g., look up by id from known chains or a helper like getChainById) and pass that chain to createWalletClient, or check if CHAIN_ID === sepolia.id and process.exit(1) with an error if not supported; adjust the createWalletClient calls (and any other walletClient creation sites) to use the resolved chain variable instead of the hard-coded sepolia.
🧹 Nitpick comments (2)
sdk/compat/package.json (1)
46-46: LGTM —tsxcorrectly placed as adevDependency.
tsx(TypeScript Execute, Node.js enhanced with esbuild to run TypeScript & ESM files) is at4.21.0on npm, so^4.21.0is pinned to the current latest — valid. The package is excluded from the published artifact sincefilesonly includesdist/andREADME.md.One optional nicety: since
tsxis added specifically to runexamples/lifecycle.ts(per the commit message) but noscriptsentry wraps it, contributors must discover the invocation on their own. Consider adding a convenience script:✨ Optional: add a convenience example script
"scripts": { "build": "tsc", "build:prod": "tsc -p tsconfig.prod.json", "typecheck": "tsc --noEmit", + "example": "tsx examples/lifecycle.ts", "clean": "node -e \"require('fs').rmSync('dist',{recursive:true,force:true})\"" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/compat/package.json` at line 46, Add a convenience npm script to package.json so contributors can run the lifecycle example easily: add a new script key (e.g., "example:lifecycle") that invokes the installed devDependency "tsx" to run "examples/lifecycle.ts" (use the project-local tsx binary so it works for contributors without global installs); update the "scripts" object in package.json to include this new entry.sdk/compat/examples/lifecycle.ts (1)
345-373: Use proper types from@erc7824/nitroliteinstead ofas anyto keep the example aligned with API changes.The inline objects passed to
client.innerClient.createAppSession()andclient.innerClient.submitAppState()matchAppDefinitionV1andAppStateUpdateV1structures respectively. Replace theas anycasts with proper typed objects:import type { AppDefinitionV1, AppStateUpdateV1, AppParticipantV1, AppAllocationV1 } from '@erc7824/nitrolite'; // For createAppSession: const definition: AppDefinitionV1 = { application: appName, participants: appParticipants as AppParticipantV1[], quorum: appQuorum, nonce: appNonce, }; await client.innerClient.createAppSession(definition, appSessionData, [sig]); // For submitAppState: const appUpdate: AppStateUpdateV1 = { appSessionId: createdSessionId, intent: 0, version: 2n, allocations: [], sessionData: '{"round":1}', }; await client.innerClient.submitAppState(appUpdate, [operateSig]);This ensures the example breaks at compile-time if the API contract changes, keeping it in sync with the underlying SDK surface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/compat/examples/lifecycle.ts` around lines 345 - 373, Replace the loose any casts around the objects passed to client.innerClient.createAppSession and client.innerClient.submitAppState with proper typed values from `@erc7824/nitrolite`: import AppDefinitionV1, AppStateUpdateV1, AppParticipantV1, AppAllocationV1 and build a const definition: AppDefinitionV1 = { application: appName, participants: appParticipants as AppParticipantV1[], quorum: appQuorum, nonce: appNonce } and a const appUpdate: AppStateUpdateV1 = { appSessionId: createdSessionId as Hex, intent: 0, version: 2n, allocations: [] as AppAllocationV1[], sessionData: '{"round":1}' } (and similarly for the close update), then pass those typed variables into createAppSession(...) and submitAppState(...) instead of using as any.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@sdk/compat/examples/lifecycle.ts`:
- Around line 102-138: The code hard-codes the viem chain "sepolia" when calling
createWalletClient in main (and similar places), which can mismatch the parsed
CHAIN_ID; update the logic to derive the viem chain from CHAIN_ID or fail fast:
either map CHAIN_ID to the appropriate viem chain object (e.g., look up by id
from known chains or a helper like getChainById) and pass that chain to
createWalletClient, or check if CHAIN_ID === sepolia.id and process.exit(1) with
an error if not supported; adjust the createWalletClient calls (and any other
walletClient creation sites) to use the resolved chain variable instead of the
hard-coded sepolia.
---
Nitpick comments:
In `@sdk/compat/examples/lifecycle.ts`:
- Around line 345-373: Replace the loose any casts around the objects passed to
client.innerClient.createAppSession and client.innerClient.submitAppState with
proper typed values from `@erc7824/nitrolite`: import AppDefinitionV1,
AppStateUpdateV1, AppParticipantV1, AppAllocationV1 and build a const
definition: AppDefinitionV1 = { application: appName, participants:
appParticipants as AppParticipantV1[], quorum: appQuorum, nonce: appNonce } and
a const appUpdate: AppStateUpdateV1 = { appSessionId: createdSessionId as Hex,
intent: 0, version: 2n, allocations: [] as AppAllocationV1[], sessionData:
'{"round":1}' } (and similarly for the close update), then pass those typed
variables into createAppSession(...) and submitAppState(...) instead of using as
any.
In `@sdk/compat/package.json`:
- Line 46: Add a convenience npm script to package.json so contributors can run
the lifecycle example easily: add a new script key (e.g., "example:lifecycle")
that invokes the installed devDependency "tsx" to run "examples/lifecycle.ts"
(use the project-local tsx binary so it works for contributors without global
installs); update the "scripts" object in package.json to include this new
entry.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
sdk/compat/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
sdk/compat/README.mdsdk/compat/examples/lifecycle.tssdk/compat/examples/output.txtsdk/compat/examples/tsconfig.jsonsdk/compat/package.json
✅ Files skipped from review due to trivial changes (2)
- sdk/compat/examples/tsconfig.json
- sdk/compat/examples/output.txt
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
sdk/compat/src/types.ts (1)
350-360:⚠️ Potential issue | 🟡 Minor
RPCTransaction.createdAtlikely mismatches wire runtime shape.On Line 359,
createdAt: Dateis risky for RPC/JSON payload typing; these fields typically arrive as strings unless explicitly deserialized first.#!/bin/bash set -euo pipefail # Inspect RPCTransaction declaration and createdAt typing. rg -n --type=ts -C2 'interface\s+RPCTransaction|createdAt' sdk/compat/src/types.ts # Inspect where transfer-notification/transaction payloads are constructed or consumed. rg -n --type=ts -C3 'TransferNotificationResponseParams|RPCTransaction|createdAt|transactions' sdk/compat/src🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/compat/src/types.ts` around lines 350 - 360, RPCTransaction.createdAt is typed as Date but RPC/JSON payloads usually deliver timestamps as strings; change the RPC shape to accept a string (e.g., createdAt: string or createdAt: string | Date) and update any deserialization sites to convert that string to a Date when constructing domain objects (look for uses of RPCTransaction, TransferNotificationResponseParams, and any transaction consumers that read createdAt and call new Date(...) or parseISO). Ensure downstream code that expects a Date receives a proper Date instance after parsing rather than relying on the raw RPC type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk/compat/src/app-signing.ts`:
- Around line 127-131: The current startsWith(WALLET_QUORUM_PREFIX) check can
mis-detect a raw 65-byte ECDSA signature that happens to begin with the same
bytes; update the detection in the function that returns a wrapped signature so
it only treats an input as already-wrapped when the prefix is present AND the
remaining payload has the expected raw-signature length (i.e., validate
signature.slice(0, WALLET_QUORUM_PREFIX.length) === WALLET_QUORUM_PREFIX and
that signature.length equals the prefix length + expected raw-signature hex
length (e.g. 2 + 65*2 for 0x + 65 bytes) or decode/check the remaining bytes
decode to 65 bytes); otherwise call concatHex([WALLET_QUORUM_PREFIX, signature
as Hex]) as before. Ensure you reference WALLET_QUORUM_PREFIX and concatHex when
making the change.
In `@sdk/compat/src/client.ts`:
- Around line 573-575: The code assigns (err as Error).message to
this._lastAppSessionsListError which is unsafe if non-Error values are thrown;
update the error extraction in the block that sets _lastAppSessionsListError and
compares to _lastAppSessionsListErrorLogged to normalize the thrown value (e.g.
derive a stable string via String(err) or err?.message ?? JSON.stringify(err)
fallback) so _lastAppSessionsListError is never undefined and comparisons
against _lastAppSessionsListErrorLogged behave predictably; locate the
assignment and replace the direct cast with the guarded conversion in the same
function/method that updates _lastAppSessionsListError and
_lastAppSessionsListErrorLogged.
- Around line 557-568: The fallback path in getAppSessionsList drops the
requested status filter (effectiveStatus) and returns all sessions; instead,
when retrying use this.innerClient.getAppSessions with the same status filter
(e.g., this.innerClient.getAppSessions({ wallet: participant, status:
effectiveStatus })), keep setting this._lastAppSessionsListError = null and
returning mapSessions(sessions), and update the log messages to reflect the
status-aware fallback (include effectiveStatus and sessions.length) so the API
behavior remains consistent under error conditions.
---
Duplicate comments:
In `@sdk/compat/src/types.ts`:
- Around line 350-360: RPCTransaction.createdAt is typed as Date but RPC/JSON
payloads usually deliver timestamps as strings; change the RPC shape to accept a
string (e.g., createdAt: string or createdAt: string | Date) and update any
deserialization sites to convert that string to a Date when constructing domain
objects (look for uses of RPCTransaction, TransferNotificationResponseParams,
and any transaction consumers that read createdAt and call new Date(...) or
parseISO). Ensure downstream code that expects a Date receives a proper Date
instance after parsing rather than relying on the raw RPC type.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
sdk/compat/README.mdsdk/compat/src/app-signing.tssdk/compat/src/client.tssdk/compat/src/index.tssdk/compat/src/types.ts
sdk/compat/src/app-signing.ts
Outdated
Comment on lines
+127
to
+131
| if (signature.toLowerCase().startsWith(WALLET_QUORUM_PREFIX)) { | ||
| return signature as Hex; | ||
| } | ||
|
|
||
| return concatHex([WALLET_QUORUM_PREFIX, signature as Hex]); |
Contributor
There was a problem hiding this comment.
Prefix detection can silently skip wrapping valid raw signatures.
On Line 127, using startsWith('0xa1') to detect “already wrapped” signatures is ambiguous: a normal 65-byte ECDSA signature may naturally start with a1. In that case, the function returns an unwrapped signature and quorum verification can fail.
💡 Proposed fix
export function toWalletQuorumSignature(signature: Hex | string): Hex {
- if (!signature.startsWith('0x')) {
+ const normalized = signature.toLowerCase();
+ if (!normalized.startsWith('0x')) {
throw new Error('Signature must be a hex string with 0x prefix');
}
- if (signature.toLowerCase().startsWith(WALLET_QUORUM_PREFIX)) {
- return signature as Hex;
+ // Treat only 1-byte prefix + 65-byte signature as already wrapped.
+ if (normalized.startsWith(WALLET_QUORUM_PREFIX) && normalized.length === 134) {
+ return normalized as Hex;
}
- return concatHex([WALLET_QUORUM_PREFIX, signature as Hex]);
+ if (normalized.length !== 132) {
+ throw new Error('Expected a 65-byte wallet signature (0x + 130 hex chars)');
+ }
+ return concatHex([WALLET_QUORUM_PREFIX, normalized as Hex]);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/compat/src/app-signing.ts` around lines 127 - 131, The current
startsWith(WALLET_QUORUM_PREFIX) check can mis-detect a raw 65-byte ECDSA
signature that happens to begin with the same bytes; update the detection in the
function that returns a wrapped signature so it only treats an input as
already-wrapped when the prefix is present AND the remaining payload has the
expected raw-signature length (i.e., validate signature.slice(0,
WALLET_QUORUM_PREFIX.length) === WALLET_QUORUM_PREFIX and that signature.length
equals the prefix length + expected raw-signature hex length (e.g. 2 + 65*2 for
0x + 65 bytes) or decode/check the remaining bytes decode to 65 bytes);
otherwise call concatHex([WALLET_QUORUM_PREFIX, signature as Hex]) as before.
Ensure you reference WALLET_QUORUM_PREFIX and concatHex when making the change.
sdk/compat/src/client.ts
Outdated
Comment on lines
+573
to
+575
| const message = (err as Error).message; | ||
| this._lastAppSessionsListError = message; | ||
| if (this._lastAppSessionsListErrorLogged !== message) { |
Contributor
There was a problem hiding this comment.
Error message extraction is unsafe for non-Error throws.
On Line 573, (err as Error).message can be undefined when upstream throws strings/objects. Prefer a guarded conversion to keep _lastAppSessionsListError stable.
💡 Proposed fix
- const message = (err as Error).message;
+ const message = err instanceof Error ? err.message : String(err);📝 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
| const message = (err as Error).message; | |
| this._lastAppSessionsListError = message; | |
| if (this._lastAppSessionsListErrorLogged !== message) { | |
| const message = err instanceof Error ? err.message : String(err); | |
| this._lastAppSessionsListError = message; | |
| if (this._lastAppSessionsListErrorLogged !== message) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/compat/src/client.ts` around lines 573 - 575, The code assigns (err as
Error).message to this._lastAppSessionsListError which is unsafe if non-Error
values are thrown; update the error extraction in the block that sets
_lastAppSessionsListError and compares to _lastAppSessionsListErrorLogged to
normalize the thrown value (e.g. derive a stable string via String(err) or
err?.message ?? JSON.stringify(err) fallback) so _lastAppSessionsListError is
never undefined and comparisons against _lastAppSessionsListErrorLogged behave
predictably; locate the assignment and replace the direct cast with the guarded
conversion in the same function/method that updates _lastAppSessionsListError
and _lastAppSessionsListErrorLogged.
| if (channels.length > 0) { | ||
| try { | ||
| const ch = channels[0]; | ||
| const data = await client.getChannelData(ch.channel_id); |
| console.log('\n-- 10. Signer Helpers --'); | ||
|
|
||
| try { | ||
| const wss = new WalletStateSigner(walletClient); |
|
|
||
| try { | ||
| const sessionKey = generatePrivateKey(); | ||
| const sessionSigner = createECDSAMessageSigner(sessionKey); |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
sdk/ts-compat/src/types.ts (1)
352-362:⚠️ Potential issue | 🟡 Minor
RPCTransaction.createdAtis too strict for wire data.At Line 361,
createdAt: Datecan mismatch runtime JSON payloads (typically strings), which can mislead consumers and cause unsafe assumptions.💡 Proposed type-safe adjustment
export interface RPCTransaction { id: number; txType: RPCTxType; fromAccount: LedgerAccountType; fromAccountTag?: string; toAccount: LedgerAccountType; toAccountTag?: string; asset: string; amount: string; - createdAt: Date; + createdAt: string | Date; }#!/bin/bash # Verify whether transfer-notification payloads are explicitly deserialized into Date. rg -n -C3 'interface RPCTransaction|TransferNotificationResponseParams|createdAt' sdk/ts-compat/src rg -n -C3 'new Date\(|Date\.parse\(|createdAt\s*[:=]' sdk/ts-compat/src🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts-compat/src/types.ts` around lines 352 - 362, The RPCTransaction.createdAt field is too strict as Date for wire/JSON payloads; change the type on the RPCTransaction interface (createdAt) to accept both serialized and runtime forms (e.g., string | Date) so runtime JSON payloads won't break typings, and update any call sites that consume RPCTransaction (e.g., parsing in TransferNotificationResponseParams handlers) to explicitly parse the string into a Date (new Date(createdAt)) when a Date is required.sdk/ts-compat/examples/lifecycle.ts (2)
223-223:⚠️ Potential issue | 🟡 MinorRemove the unused
databinding.Line 223 assigns
databut never reads it.💡 Proposed fix
- const data = await client.getChannelData(ch.channel_id); + await client.getChannelData(ch.channel_id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts-compat/examples/lifecycle.ts` at line 223, The variable `data` is assigned from `client.getChannelData(ch.channel_id)` but never used; remove the unused binding by either calling `await client.getChannelData(ch.channel_id)` without assigning its result or rename it to a deliberately ignored identifier (e.g., `_data`) in the same line where `client.getChannelData` is invoked; update the occurrence in the `lifecycle.ts` example where `const data = await client.getChannelData(ch.channel_id);` appears (referencing the `client.getChannelData` call and the `data` identifier).
554-561:⚠️ Potential issue | 🟡 MinorUnused signer helper variables are still present.
wssandsessionSignerare created but not used.💡 Proposed fix
- const wss = new WalletStateSigner(walletClient); + new WalletStateSigner(walletClient); ok('WalletStateSigner()', `created for ${walletClient.account?.address?.slice(0, 10)}...`); @@ const sessionKey = generatePrivateKey(); const sessionSigner = createECDSAMessageSigner(sessionKey); - ok('createECDSAMessageSigner()', 'session signer created'); + const sampleSig = await sessionSigner(new Uint8Array([0x01])); + ok('createECDSAMessageSigner()', `session signer created, sampleSig=${sampleSig.slice(0, 12)}...`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts-compat/examples/lifecycle.ts` around lines 554 - 561, The variables wss (from WalletStateSigner(walletClient)) and sessionSigner (from createECDSAMessageSigner(sessionKey) where sessionKey is from generatePrivateKey()) are created but never used; remove the unused assignments or use them (e.g., perform a simple assertion/log or call a method) to satisfy linter and correctness—specifically edit the block that constructs WalletStateSigner and the block that generates sessionKey/createECDSAMessageSigner to either (a) drop the variable assignment and just call the constructor/factory if side-effects are required, or (b) keep the variables and use them in a follow-up action (like a sanity check or one-line log/assert) so wss, WalletStateSigner, sessionSigner, sessionKey, generatePrivateKey, and createECDSAMessageSigner are no longer unused.
🧹 Nitpick comments (1)
sdk/ts-compat/examples/lifecycle.ts (1)
57-100: Prefer exported app-signing helpers over local duplicate implementations.
packCreateRequest,packAppStateUpdate, andwalletSignduplicate logic already provided bysdk/ts-compat/src/app-signing.ts. Using the exported helpers will reduce drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts-compat/examples/lifecycle.ts` around lines 57 - 100, The three local helpers packCreateRequest, packAppStateUpdate, and walletSign duplicate logic available in the exported helpers in sdk/ts-compat/src/app-signing.ts; remove these local implementations (and the WALLET_SIG_PREFIX constant) and import and use the exported functions from that module instead (update the file's imports to pull the app-signing helpers and replace all calls to packCreateRequest, packAppStateUpdate, and walletSign with the imported functions so there is a single source of truth).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk/ts-compat/examples/lifecycle.ts`:
- Around line 134-137: The code hard-codes the Sepolia chain when constructing
the wallet client (createWalletClient -> walletClient) causing mismatches with a
configurable CHAIN_ID; change the chain argument to derive from the configured
chain id (e.g., use a helper or mapping like getChainForId(chainId) or a runtime
lookup from the chains library) instead of using the literal sepolia symbol, and
apply the same replacement for the other occurrences around where account and
transport are set so the walletClient is created for the actual configured
chain.
In `@sdk/ts-compat/README.md`:
- Around line 156-163: The README currently contradicts itself by introducing
app-session signing helpers (packCreateAppSessionHash, packSubmitAppStateHash,
toWalletQuorumSignature) while elsewhere describing “RPC Stubs/Auth Stubs” as
no-ops; update the document so the semantics are consistent by either (A)
labeling the three functions explicitly as "compatibility helpers" that provide
deterministic hashing and prefixing for quorum signing and adding
cross-references from the RPC/Auth Stubs section to explain they remain no-ops
but rely on these helpers for client-side compatibility, or (B) if the stubs are
meant to perform work, change the RPC/Auth Stubs description to reflect that
they now call these helpers. Make the change by editing the README text around
the helper table and the sections describing RPC Stubs/Auth Stubs so the
relationship between packCreateAppSessionHash, packSubmitAppStateHash,
toWalletQuorumSignature and the stubs is unambiguous.
In `@sdk/ts-compat/src/app-signing.ts`:
- Around line 29-30: The code currently force-casts params.appSessionId to Hex
during hash packing which hides invalid inputs; change the params contract so
appSessionId is typed as Hex (replace "appSessionId: Hex | string" with
"appSessionId: Hex") or, if widening the public API is required, add an explicit
runtime check in the hash-packing routine that validates params.appSessionId is
a proper Hex (use an isHex helper) and throw a clear error before ABI encoding;
update usages of appSessionId (the hash-packing function that reads
params.appSessionId and any callers that construct params) to satisfy the
stricter type or perform the validation.
In `@sdk/ts-compat/src/auth.ts`:
- Around line 36-38: The JSON.stringify replacer currently converts all bigints
to Number unconditionally causing silent precision loss; update the replacer in
sdk/ts-compat/src/auth.ts (the JSON.stringify(request, ...) call) to detect
bigints and if value > BigInt(Number.MAX_SAFE_INTEGER) throw a clear error (or
otherwise refuse conversion) indicating the field name and that the bigint
exceeds Number.MAX_SAFE_INTEGER, otherwise safely convert with Number(value);
ensure the thrown error mentions the request/context so callers can handle the
large uint64 (e.g., expires_at) instead of losing precision.
In `@sdk/ts-compat/src/client.ts`:
- Around line 765-783: The loop over v1Allocations misses implicit decreases
when an existing allocation is omitted; update the logic to iterate over the
union of keys from currentByParticipantAndAsset and v1Allocations (normalize
participant and asset casing the same way you build keys) so you compute delta =
nextAmount.minus(currentAmount) where nextAmount defaults to new Decimal(0) if
the allocation is missing; push positive deltas to positiveDeltas and negative
ones to negativeDeltas as before. Apply the same union-key iteration/fallback
logic to the other allocation comparison block referenced by the comment (the
similar loop at 790-792) so omitted allocations are captured as negative deltas
there as well. Ensure you still use currentByParticipantAndAsset.get(key) and
Decimal for arithmetic and preserve the participant.toLowerCase()/asset
normalization used when building keys.
---
Duplicate comments:
In `@sdk/ts-compat/examples/lifecycle.ts`:
- Line 223: The variable `data` is assigned from
`client.getChannelData(ch.channel_id)` but never used; remove the unused binding
by either calling `await client.getChannelData(ch.channel_id)` without assigning
its result or rename it to a deliberately ignored identifier (e.g., `_data`) in
the same line where `client.getChannelData` is invoked; update the occurrence in
the `lifecycle.ts` example where `const data = await
client.getChannelData(ch.channel_id);` appears (referencing the
`client.getChannelData` call and the `data` identifier).
- Around line 554-561: The variables wss (from WalletStateSigner(walletClient))
and sessionSigner (from createECDSAMessageSigner(sessionKey) where sessionKey is
from generatePrivateKey()) are created but never used; remove the unused
assignments or use them (e.g., perform a simple assertion/log or call a method)
to satisfy linter and correctness—specifically edit the block that constructs
WalletStateSigner and the block that generates
sessionKey/createECDSAMessageSigner to either (a) drop the variable assignment
and just call the constructor/factory if side-effects are required, or (b) keep
the variables and use them in a follow-up action (like a sanity check or
one-line log/assert) so wss, WalletStateSigner, sessionSigner, sessionKey,
generatePrivateKey, and createECDSAMessageSigner are no longer unused.
In `@sdk/ts-compat/src/types.ts`:
- Around line 352-362: The RPCTransaction.createdAt field is too strict as Date
for wire/JSON payloads; change the type on the RPCTransaction interface
(createdAt) to accept both serialized and runtime forms (e.g., string | Date) so
runtime JSON payloads won't break typings, and update any call sites that
consume RPCTransaction (e.g., parsing in TransferNotificationResponseParams
handlers) to explicitly parse the string into a Date (new Date(createdAt)) when
a Date is required.
---
Nitpick comments:
In `@sdk/ts-compat/examples/lifecycle.ts`:
- Around line 57-100: The three local helpers packCreateRequest,
packAppStateUpdate, and walletSign duplicate logic available in the exported
helpers in sdk/ts-compat/src/app-signing.ts; remove these local implementations
(and the WALLET_SIG_PREFIX constant) and import and use the exported functions
from that module instead (update the file's imports to pull the app-signing
helpers and replace all calls to packCreateRequest, packAppStateUpdate, and
walletSign with the imported functions so there is a single source of truth).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
sdk/ts-compat/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
sdk/ts-compat/.gitignoresdk/ts-compat/README.mdsdk/ts-compat/docs/migration-offchain.mdsdk/ts-compat/docs/migration-onchain.mdsdk/ts-compat/docs/migration-overview.mdsdk/ts-compat/examples/lifecycle.tssdk/ts-compat/examples/output.txtsdk/ts-compat/examples/tsconfig.jsonsdk/ts-compat/package.jsonsdk/ts-compat/src/app-signing.tssdk/ts-compat/src/auth.tssdk/ts-compat/src/client.tssdk/ts-compat/src/config.tssdk/ts-compat/src/errors.tssdk/ts-compat/src/events.tssdk/ts-compat/src/index.tssdk/ts-compat/src/rpc.tssdk/ts-compat/src/signers.tssdk/ts-compat/src/types.tssdk/ts-compat/tsconfig.jsonsdk/ts-compat/tsconfig.prod.json
✅ Files skipped from review due to trivial changes (1)
- sdk/ts-compat/examples/output.txt
Comment on lines
+134
to
+137
| const walletClient = createWalletClient({ | ||
| chain: sepolia, | ||
| transport: http(process.env.RPC_URL || 'https://1rpc.io/sepolia'), | ||
| account, |
Contributor
There was a problem hiding this comment.
CHAIN_ID is configurable, but wallet clients are hard-wired to Sepolia.
Line 135 (and Lines 395-396) always use sepolia, so non-Sepolia CHAIN_ID runs are inconsistent and can fail in confusing ways.
💡 Minimal guard fix
const CHAIN_ID = parseInt(process.env.CHAIN_ID || '11155111', 10);
@@
if (Number.isNaN(CHAIN_ID)) {
console.error('CHAIN_ID must be a valid integer');
process.exit(1);
}
+if (CHAIN_ID !== sepolia.id) {
+ console.error(`This example currently supports Sepolia only (CHAIN_ID=${sepolia.id}). Received ${CHAIN_ID}.`);
+ process.exit(1);
+}Also applies to: 395-396
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/ts-compat/examples/lifecycle.ts` around lines 134 - 137, The code
hard-codes the Sepolia chain when constructing the wallet client
(createWalletClient -> walletClient) causing mismatches with a configurable
CHAIN_ID; change the chain argument to derive from the configured chain id
(e.g., use a helper or mapping like getChainForId(chainId) or a runtime lookup
from the chains library) instead of using the literal sepolia symbol, and apply
the same replacement for the other occurrences around where account and
transport are set so the walletClient is created for the actual configured
chain.
Comment on lines
+156
to
+163
| ### App Session Signing Helpers | ||
|
|
||
| | Helper | Description | | ||
| |---|---| | ||
| | `packCreateAppSessionHash(params)` | Deterministic hash for `createAppSession` quorum signing | | ||
| | `packSubmitAppStateHash(params)` | Deterministic hash for `submitAppState` quorum signing | | ||
| | `toWalletQuorumSignature(signature)` | Prefixes wallet signature to compat app-session quorum format | | ||
|
|
Contributor
There was a problem hiding this comment.
README has conflicting helper semantics in the same document.
This new helper section implies functional compatibility helpers, but Line 308 and Line 327 still describe “RPC Stubs/Auth Stubs” as no-ops. Please align those sections to avoid misleading migration users.
📝 Suggested doc alignment
-## RPC Stubs
+## RPC Compatibility Helpers
-The following functions exist so that any remaining v0.5.3 `create*Message` / `parse*Response` imports compile. They are intentionally **no-ops** — prefer calling `NitroliteClient` methods directly:
+The following functions exist so that remaining v0.5.3 `create*Message` / `parse*Response` imports compile. They are **compatibility helpers**; prefer calling `NitroliteClient` methods directly for new code.
-## Auth Stubs
+## Auth Compatibility Helpers
-v1.0.0 handles authentication internally — there is no public auth API. These stubs allow existing auth code to compile while doing nothing at runtime:
+v1.0.0 handles authentication internally. For legacy auth paths, compat auth helpers remain available to preserve older integration patterns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/ts-compat/README.md` around lines 156 - 163, The README currently
contradicts itself by introducing app-session signing helpers
(packCreateAppSessionHash, packSubmitAppStateHash, toWalletQuorumSignature)
while elsewhere describing “RPC Stubs/Auth Stubs” as no-ops; update the document
so the semantics are consistent by either (A) labeling the three functions
explicitly as "compatibility helpers" that provide deterministic hashing and
prefixing for quorum signing and adding cross-references from the RPC/Auth Stubs
section to explain they remain no-ops but rely on these helpers for client-side
compatibility, or (B) if the stubs are meant to perform work, change the
RPC/Auth Stubs description to reflect that they now call these helpers. Make the
change by editing the README text around the helper table and the sections
describing RPC Stubs/Auth Stubs so the relationship between
packCreateAppSessionHash, packSubmitAppStateHash, toWalletQuorumSignature and
the stubs is unambiguous.
Comment on lines
+29
to
+30
| appSessionId: Hex | string; | ||
| intent: RPCAppStateIntent | 'close' | number; |
Contributor
There was a problem hiding this comment.
Avoid unchecked appSessionId cast in hash packing.
Line 107 force-casts params.appSessionId to Hex. This hides invalid inputs until ABI encoding throws at runtime. Prefer a strict Hex type in the params contract (or explicit validation before encoding).
💡 Proposed type-safe fix
export interface SubmitAppStateHashParams {
- appSessionId: Hex | string;
+ appSessionId: Hex;
intent: RPCAppStateIntent | 'close' | number;
version: bigint | number;
allocations: SubmitAppStateHashAllocation[];
sessionData?: string;
}
@@
- params.appSessionId as Hex,
+ params.appSessionId,Also applies to: 107-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/ts-compat/src/app-signing.ts` around lines 29 - 30, The code currently
force-casts params.appSessionId to Hex during hash packing which hides invalid
inputs; change the params contract so appSessionId is typed as Hex (replace
"appSessionId: Hex | string" with "appSessionId: Hex") or, if widening the
public API is required, add an explicit runtime check in the hash-packing
routine that validates params.appSessionId is a proper Hex (use an isHex helper)
and throw a clear error before ABI encoding; update usages of appSessionId (the
hash-packing function that reads params.appSessionId and any callers that
construct params) to satisfy the stricter type or perform the validation.
sdk/ts-compat/src/auth.ts
Outdated
Comment on lines
+36
to
+38
| return JSON.stringify(request, (_, value) => | ||
| typeof value === 'bigint' ? Number(value) : value, | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find and examine expires_at usage
rg -nP '\bexpires_at\b' sdk/ts-compat/src sdk/ts-compat/examples
# Also search for the auth-related types and interfaces
rg -nP 'AuthRequest|AuthVerify|EIP712AuthTypes|auth_request|auth_verify' sdk/ts-compat/src
# Get file structure
echo "=== File structure ==="
find sdk/ts-compat/src -type f -name "*.ts" | head -20Repository: erc7824/nitrolite
Length of output: 2323
🏁 Script executed:
#!/bin/bash
# Read the auth.ts file around the problematic lines
wc -l sdk/ts-compat/src/auth.ts
cat -n sdk/ts-compat/src/auth.tsRepository: erc7824/nitrolite
Length of output: 4988
🏁 Script executed:
#!/bin/bash
# Read types.ts to see EIP712AuthTypes definition
sed -n '375,400p' sdk/ts-compat/src/types.ts
# Also look for test files
find sdk/ts-compat -type f \( -name "*.test.ts" -o -name "*.spec.ts" \) | head -10
# Look for examples with expires_at
find sdk/ts-compat/examples -type f -name "*.ts" -o -name "*.js" 2>/dev/null | xargs ls -la 2>/dev/null || echo "No examples dir"
# Check if there are any comments or documentation about expires_at values
rg -A 2 -B 2 'expires_at' sdk/ts-compat/src/types.tsRepository: erc7824/nitrolite
Length of output: 1017
🏁 Script executed:
#!/bin/bash
# Let's look at how expires_at is used in examples and test files
find sdk/ts-compat -type f \( -name "*.ts" -o -name "*.js" -o -name "*.json" \) | xargs grep -l 'expires_at' 2>/dev/null
# Check the complete EIP712AuthTypes definition
cat -n sdk/ts-compat/src/types.ts | sed -n '375,395p'Repository: erc7824/nitrolite
Length of output: 907
🏁 Script executed:
#!/bin/bash
# Check the lifecycle.ts example for how expires_at is used
rg -A 5 -B 5 'expires_at' sdk/ts-compat/examples/lifecycle.tsRepository: erc7824/nitrolite
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Let's check if there are any tests and what values they use
find sdk/ts-compat -type f \( -name "*.test.ts" -o -name "*.spec.ts" -o -name "*.test.js" \)
# Check the package.json to understand test setup
cat sdk/ts-compat/package.json 2>/dev/null | head -30Repository: erc7824/nitrolite
Length of output: 934
🏁 Script executed:
#!/bin/bash
# Check lifecycle.ts for expires_at usage
cat sdk/ts-compat/examples/lifecycle.ts | grep -A 10 -B 10 'expires_at' || echo "expires_at not found in lifecycle.ts"
# Let's search more broadly for any test or example files
find sdk/ts-compat -type f \( -name "*.ts" -o -name "*.js" \) -exec grep -l 'createAuthRequestMessage\|AuthRequestParams' {} \;Repository: erc7824/nitrolite
Length of output: 150
🏁 Script executed:
#!/bin/bash
# Let's check how createAuthRequestMessage is actually called
rg 'createAuthRequestMessage' -A 5 -B 2 sdk/ts-compat/Repository: erc7824/nitrolite
Length of output: 2855
🏁 Script executed:
#!/bin/bash
# Let's understand the real-world usage pattern by checking RPC creation
rg -A 10 'createRequest' sdk/ts-compat/src/rpc.ts | head -40Repository: erc7824/nitrolite
Length of output: 521
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation or comments about expires_at semantics
rg -B 5 -A 5 'uint64' sdk/ts-compat/src/types.ts | head -50Repository: erc7824/nitrolite
Length of output: 471
🏁 Script executed:
#!/bin/bash
# Let's verify if JSON.stringify actually throws on bigint
node <<'EOF'
// Test 1: Does JSON.stringify throw on bigint?
try {
const obj = { value: 1n };
JSON.stringify(obj);
console.log("No error - bigint serialized");
} catch (e) {
console.log("Error on bigint:", e.message);
}
// Test 2: Does the replacer work?
const obj = { value: 1n, safe: 123, nested: { expires_at: 9007199254740991n } };
try {
const result = JSON.stringify(obj, (_, v) => typeof v === 'bigint' ? Number(v) : v);
console.log("Replacer result:", result);
} catch (e) {
console.log("Replacer error:", e.message);
}
// Test 3: Precision loss demonstration
const unsafeValue = 9007199254740992n; // MAX_SAFE_INTEGER + 1
const asNumber = Number(unsafeValue);
console.log("\nPrecision loss test:");
console.log("Original bigint:", unsafeValue.toString());
console.log("As number:", asNumber);
console.log("Back to bigint:", BigInt(asNumber).toString());
console.log("Lost precision?", unsafeValue !== BigInt(asNumber));
// Test 4: Real-world timestamp example
const futureTimestamp = 32503680000n; // Year 3000 in seconds
console.log("\nYear 3000 timestamp test:");
console.log("Original:", futureTimestamp.toString());
console.log("As number:", Number(futureTimestamp));
console.log("Safe?", futureTimestamp <= 9007199254740991n);
EOFRepository: erc7824/nitrolite
Length of output: 418
🏁 Script executed:
#!/bin/bash
# Let's verify the exact scenario - what happens to expires_at in createAuthRequestMessage
cat sdk/ts-compat/src/auth.ts | sed -n '25,39p'
# Check if there are other places where bigint is serialized
rg 'JSON.stringify' sdk/ts-compat/src/ -A 2 -B 2Repository: erc7824/nitrolite
Length of output: 2358
🏁 Script executed:
#!/bin/bash
# Let's verify the maximum possible values and practical ranges
node <<'EOF'
// Check MAX_SAFE_INTEGER and practical timestamp ranges
console.log("Number.MAX_SAFE_INTEGER:", Number.MAX_SAFE_INTEGER);
console.log("Number.MAX_SAFE_INTEGER in seconds:", Number.MAX_SAFE_INTEGER / 1000, "(year", new Date(Number.MAX_SAFE_INTEGER * 1000).getFullYear(), ")");
// What's the practical range for expires_at?
const now = Math.floor(Date.now() / 1000); // current unix timestamp in seconds
const year2100 = new Date(2100, 0, 1).getTime() / 1000;
const year2286 = new Date(2286, 0, 1).getTime() / 1000;
console.log("\nPractical timestamp ranges:");
console.log("Now (2026):", now, "within safe?", now < 9007199254740991n);
console.log("Year 2100:", Math.floor(year2100), "within safe?", Math.floor(year2100) < 9007199254740991);
console.log("Year 2286:", Math.floor(year2286), "within safe?", Math.floor(year2286) < 9007199254740991);
// Verify the specific precision loss scenario
console.log("\nSpecific precision loss check:");
const testValue = 9007199254740992n; // MAX_SAFE + 1
console.log("Value > MAX_SAFE:", testValue.toString());
console.log("As Number:", Number(testValue));
console.log("Number.isSafeInteger check:", Number.isSafeInteger(Number(testValue)));
EOFRepository: erc7824/nitrolite
Length of output: 446
Guard bigint→number conversion to prevent silent precision loss.
Line 37 converts bigint to number without validation. Values exceeding Number.MAX_SAFE_INTEGER (9,007,199,254,740,991) will lose precision silently, potentially corrupting expires_at or other large uint64 values.
While practical timestamp values remain safe for centuries, the type signature (expires_at: bigint) and EIP712 specification (uint64) indicate the field should support the full range. The current conversion silently truncates values without warning.
Proposed fix
- return JSON.stringify(request, (_, value) =>
- typeof value === 'bigint' ? Number(value) : value,
- );
+ return JSON.stringify(request, (_, value) => {
+ if (typeof value !== 'bigint') return value;
+ const asNumber = Number(value);
+ if (!Number.isSafeInteger(asNumber)) {
+ throw new Error(`BigInt value exceeds Number.MAX_SAFE_INTEGER: ${value.toString()}`);
+ }
+ return asNumber;
+ });📝 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
| return JSON.stringify(request, (_, value) => | |
| typeof value === 'bigint' ? Number(value) : value, | |
| ); | |
| return JSON.stringify(request, (_, value) => { | |
| if (typeof value !== 'bigint') return value; | |
| const asNumber = Number(value); | |
| if (!Number.isSafeInteger(asNumber)) { | |
| throw new Error(`BigInt value exceeds Number.MAX_SAFE_INTEGER: ${value.toString()}`); | |
| } | |
| return asNumber; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/ts-compat/src/auth.ts` around lines 36 - 38, The JSON.stringify replacer
currently converts all bigints to Number unconditionally causing silent
precision loss; update the replacer in sdk/ts-compat/src/auth.ts (the
JSON.stringify(request, ...) call) to detect bigints and if value >
BigInt(Number.MAX_SAFE_INTEGER) throw a clear error (or otherwise refuse
conversion) indicating the field name and that the bigint exceeds
Number.MAX_SAFE_INTEGER, otherwise safely convert with Number(value); ensure the
thrown error mentions the request/context so callers can handle the large uint64
(e.g., expires_at) instead of losing precision.
Comment on lines
+765
to
+783
| for (const allocation of v1Allocations) { | ||
| const key = `${allocation.participant.toLowerCase()}::${allocation.asset.toLowerCase()}`; | ||
| const currentAmount = currentByParticipantAndAsset.get(key) ?? new Decimal(0); | ||
| const delta = allocation.amount.minus(currentAmount); | ||
| if (delta.greaterThan(0)) { | ||
| positiveDeltas.push({ | ||
| participant: allocation.participant.toLowerCase(), | ||
| asset: allocation.asset, | ||
| amount: delta, | ||
| }); | ||
| } else if (delta.lessThan(0)) { | ||
| negativeDeltas.push({ | ||
| participant: allocation.participant.toLowerCase(), | ||
| asset: allocation.asset, | ||
| amount: delta, | ||
| }); | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Deposit delta validation misses implicit decreases when allocations are omitted.
This loop compares only keys present in v1Allocations. If an existing allocation is omitted from the new state, that implicit drop is not captured in negativeDeltas, yet the update is still submitted.
💡 Proposed fix (compare union of current + next keys)
- type PositiveDelta = { participant: string; asset: string; amount: Decimal };
+ type PositiveDelta = { participant: string; asset: string; amount: Decimal };
const positiveDeltas: PositiveDelta[] = [];
const negativeDeltas: PositiveDelta[] = [];
- for (const allocation of v1Allocations) {
- const key = `${allocation.participant.toLowerCase()}::${allocation.asset.toLowerCase()}`;
- const currentAmount = currentByParticipantAndAsset.get(key) ?? new Decimal(0);
- const delta = allocation.amount.minus(currentAmount);
+ const nextByParticipantAndAsset = new Map<string, Decimal>();
+ for (const allocation of v1Allocations) {
+ nextByParticipantAndAsset.set(
+ `${allocation.participant.toLowerCase()}::${allocation.asset.toLowerCase()}`,
+ allocation.amount,
+ );
+ }
+
+ const allKeys = new Set<string>([
+ ...currentByParticipantAndAsset.keys(),
+ ...nextByParticipantAndAsset.keys(),
+ ]);
+
+ for (const key of allKeys) {
+ const [participant, asset] = key.split('::');
+ const currentAmount = currentByParticipantAndAsset.get(key) ?? new Decimal(0);
+ const nextAmount = nextByParticipantAndAsset.get(key) ?? new Decimal(0);
+ const delta = nextAmount.minus(currentAmount);
if (delta.greaterThan(0)) {
positiveDeltas.push({
- participant: allocation.participant.toLowerCase(),
- asset: allocation.asset,
+ participant,
+ asset,
amount: delta,
});
} else if (delta.lessThan(0)) {
negativeDeltas.push({
- participant: allocation.participant.toLowerCase(),
- asset: allocation.asset,
+ participant,
+ asset,
amount: delta,
});
}
}Also applies to: 790-792
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/ts-compat/src/client.ts` around lines 765 - 783, The loop over
v1Allocations misses implicit decreases when an existing allocation is omitted;
update the logic to iterate over the union of keys from
currentByParticipantAndAsset and v1Allocations (normalize participant and asset
casing the same way you build keys) so you compute delta =
nextAmount.minus(currentAmount) where nextAmount defaults to new Decimal(0) if
the allocation is missing; push positive deltas to positiveDeltas and negative
ones to negativeDeltas as before. Apply the same union-key iteration/fallback
logic to the other allocation comparison block referenced by the comment (the
similar loop at 790-792) so omitted allocations are captured as negative deltas
there as well. Ensure you still use currentByParticipantAndAsset.get(key) and
Decimal for arithmetic and preserve the participant.toLowerCase()/asset
normalization used when building keys.
alessio
approved these changes
Feb 27, 2026
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk/ts-compat/src/index.ts (1)
120-127:⚠️ Potential issue | 🟡 MinorExport
OngoingStateTransitionErrorfrom the barrel for consistency.A new public error class was added in
sdk/ts-compat/src/errors.ts, but it isn’t re-exported here, unlike the other compat errors.💡 Suggested export addition
export { CompatError, AllowanceError, UserRejectedError, InsufficientFundsError, NotInitializedError, + OngoingStateTransitionError, getUserFacingMessage, } from './errors';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts-compat/src/index.ts` around lines 120 - 127, The barrel export list in index.ts is missing the new OngoingStateTransitionError; update the export block that currently re-exports CompatError, AllowanceError, UserRejectedError, InsufficientFundsError, NotInitializedError, and getUserFacingMessage to also include OngoingStateTransitionError so the new public error class from errors.ts is exposed consistently.
♻️ Duplicate comments (1)
sdk/ts-compat/src/client.ts (1)
828-845:⚠️ Potential issue | 🟠 MajorDeposit delta check still misses implicit decreases from omitted allocations.
Line 828 computes deltas only for keys present in
v1Allocations. If an existing allocation is omitted, that negative delta is never captured, so the guard at Lines 853-855 can be bypassed.💡 Suggested fix (iterate union of current + next keys)
- for (const allocation of v1Allocations) { - const key = `${allocation.participant.toLowerCase()}::${allocation.asset.toLowerCase()}`; - const currentAmount = currentByParticipantAndAsset.get(key) ?? new Decimal(0); - const delta = allocation.amount.minus(currentAmount); + const nextByParticipantAndAsset = new Map<string, Decimal>(); + for (const allocation of v1Allocations) { + nextByParticipantAndAsset.set( + `${allocation.participant.toLowerCase()}::${allocation.asset.toLowerCase()}`, + allocation.amount, + ); + } + + const allKeys = new Set<string>([ + ...currentByParticipantAndAsset.keys(), + ...nextByParticipantAndAsset.keys(), + ]); + + for (const key of allKeys) { + const [participant, asset] = key.split('::'); + const currentAmount = currentByParticipantAndAsset.get(key) ?? new Decimal(0); + const nextAmount = nextByParticipantAndAsset.get(key) ?? new Decimal(0); + const delta = nextAmount.minus(currentAmount); if (delta.greaterThan(0)) { positiveDeltas.push({ - participant: allocation.participant.toLowerCase(), - asset: allocation.asset, + participant, + asset, amount: delta, }); } else if (delta.lessThan(0)) { negativeDeltas.push({ - participant: allocation.participant.toLowerCase(), - asset: allocation.asset, + participant, + asset, amount: delta, }); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts-compat/src/client.ts` around lines 828 - 845, The current loop over v1Allocations only computes deltas for keys present in v1Allocations, missing implicit decreases when an existing allocation is omitted; update the logic that builds positiveDeltas/negativeDeltas by iterating the union of keys from v1Allocations and currentByParticipantAndAsset (e.g., build a map from v1Allocations keyed by `${participant}::${asset}` or iterate current keys as well), compute currentAmount = currentByParticipantAndAsset.get(key) ?? new Decimal(0) and nextAmount = v1Map.get(key)?.amount ?? new Decimal(0), then compute delta = nextAmount.minus(currentAmount) and push into positiveDeltas or negativeDeltas accordingly (referencing v1Allocations, currentByParticipantAndAsset, positiveDeltas, negativeDeltas).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@sdk/ts-compat/src/index.ts`:
- Around line 120-127: The barrel export list in index.ts is missing the new
OngoingStateTransitionError; update the export block that currently re-exports
CompatError, AllowanceError, UserRejectedError, InsufficientFundsError,
NotInitializedError, and getUserFacingMessage to also include
OngoingStateTransitionError so the new public error class from errors.ts is
exposed consistently.
---
Duplicate comments:
In `@sdk/ts-compat/src/client.ts`:
- Around line 828-845: The current loop over v1Allocations only computes deltas
for keys present in v1Allocations, missing implicit decreases when an existing
allocation is omitted; update the logic that builds
positiveDeltas/negativeDeltas by iterating the union of keys from v1Allocations
and currentByParticipantAndAsset (e.g., build a map from v1Allocations keyed by
`${participant}::${asset}` or iterate current keys as well), compute
currentAmount = currentByParticipantAndAsset.get(key) ?? new Decimal(0) and
nextAmount = v1Map.get(key)?.amount ?? new Decimal(0), then compute delta =
nextAmount.minus(currentAmount) and push into positiveDeltas or negativeDeltas
accordingly (referencing v1Allocations, currentByParticipantAndAsset,
positiveDeltas, negativeDeltas).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
sdk/ts-compat/src/app-signing.tssdk/ts-compat/src/client.tssdk/ts-compat/src/errors.tssdk/ts-compat/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- sdk/ts-compat/src/app-signing.ts
- expand examples/lifecycle.ts into a comprehensive compat API walkthrough\n- add examples/tsconfig.json and examples/output.txt for runnable setup + expected output\n- add Build Size metrics to sdk/compat/README.md from build:prod + npm pack --dry-run\n- add tsx dev dependency updates required to run the example workflow
b2e150e to
e6b7ce3
Compare
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.
Context
Follow-up to PR #573 (
feat: get_channels RPC, GetAppSessions fix, compat SDK & tests).This PR applies the remaining review feedback/residual comments on the compat SDK and aligns docs/comments with the finalized behavior introduced after #573.
Summary
sdk/compat/src/auth.ts.sdk/compat/src/client.ts.sdk/compat/src/index.tssdk/compat/src/rpc.tssdk/compat/src/types.tsstub/no-oplanguage with accuratecompatibility helperwording in:sdk/compat/src/index.tssdk/compat/src/rpc.tssdk/compat/docs/migration-overview.mdsdk/compat/docs/migration-offchain.mdValidation
cd sdk/compat && npm run typecheckcd appstore && npm run buildcd beatwav && npm run buildcd nexus && npm run buildSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Additions in this update (February 24, 2026)
Build Sizesection tosdk/compat/README.mdwith measurednpm pack --dry-runanddistbyte stats.sdk/compat/examplesbundle:lifecycle.ts: comprehensive lifecycle walkthrough covering the compat API surface (client lifecycle, assets, balances/channels, transfers, app sessions including multi-party flow,EventPoller, error classification, signers, enums, and inner client access).tsconfig.json: standalone example compiler config with local path aliases.output.txt: expected console output template for quick verification.tsxtosdk/compatdev dependencies so the lifecycle example can be run directly.Validation for this update
cd sdk/compat && npm run build:prodcd sdk/compat && npx tsc -p examples/tsconfig.json --noEmitAdditions in this update (March 2, 2026)
toSessionKeyQuorumSignature(0xa2) insdk/ts-compat/src/app-signing.ts.toSessionKeyQuorumSignaturefromsdk/ts-compat/src/index.ts.channelSessionKeySignerinsdk/ts-compat/src/client.ts.sdk/ts-compat/src/client.ts:signChannelSessionKeyStatesubmitChannelSessionKeyStategetLastChannelKeyStatessignSessionKeyStatesubmitSessionKeyStategetLastKeyStatessdk/ts-compat/src/errors.ts.Validation for this update
cd sdk/ts-compat && npm run typecheck