YNU-869: Refine ts-compat runtime migration surface#699
Conversation
ihsraham
requested review from
dimast-x,
nksazonov and
philanton
as code owners
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR implements v1-compatible RPC request builders and envelope-aware response parsing in the ts-compat layer, adds chain-aware token decimals and viem PublicClient read support to the compat client, converts several legacy helpers to fail-fast migration shims, and updates docs and tests to clarify amount-unit conventions. ChangesTS compat: v1 request / client / amounts
Sequence Diagram(s)sequenceDiagram
participant Caller as Compat helper
participant Signer as MessageSigner
participant Envelope as LegacyEnvelope {req,sig}
participant v1 as v1 RPC server
participant Read as PublicClient (on-chain)
Caller->>Signer: build v1 method + params (signed if required)
Signer-->>Caller: signature
Caller->>Envelope: wrap `{ req: [method, args], sig }`
Caller->>v1: send envelope (or return to caller)
v1-->>Envelope: response `{ res: [...] }`
Envelope->>Caller: received envelope
Caller->>Caller: extractResponsePayload(res) -> {requestId, method, payload}
alt on-chain read (getOpenChannels / token read)
Caller->>Read: readContract / getBalance
Read-->>Caller: chain data (normalized to tokenDecimals)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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 |
|
pushed a small follow up here. gettokenbalance now rounds down to token precision before parseunits, and the compat docs now call out more clearly that createchannel and the workflow style helper shims throw with migration guidance instead of warning or returning fake wire payloads. |
ihsraham
left a comment
ihsraham
left a comment
There was a problem hiding this comment.
@coderabbitai review
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
sdk/ts-compat/src/client.ts (4)
569-572:getTokenAllowanceignores resolved token address casing / canonical form.Minor: the method resolves
infobut then forwards the caller-providedtokenAddresstocheckTokenAllowancerather thaninfo.tokenAddress. Consistency withdeposit/withdrawal(which use the resolved address) would be nice, and it also guards against the caller passing a non-checksummed address in environments where the inner SDK is stricter.🤖 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 569 - 572, getTokenAllowance currently resolves the token via resolveToken but then calls innerClient.checkTokenAllowance with the original tokenAddress, which can miss canonical/case-normalized addresses; change the call in getTokenAllowance to use info.tokenAddress and keep info.chainId and this.userAddress (mirroring deposit/withdrawal behavior) so the innerClient.checkTokenAllowance receives the resolved/canonical token address instead of the caller-provided value.
696-710: Sequentialawaitin balances loop -- parallelize withPromise.all.
getAssetDecimalsForAssetis awaited once per balance. After the first call populates the asset map, each subsequent call is cheap, but theasync/awaitper iteration still serializes microtasks unnecessarily. APromise.all(balances.map(...))(or just an upfrontawait this.ensureAssets()then synchronous map lookups) would be cleaner.♻️ Suggested simplification
- async getBalances(wallet?: Address): Promise<LedgerBalance[]> { - const balances = await this.innerClient.getBalances(wallet ?? this.userAddress); - const result: LedgerBalance[] = []; - - for (const balance of balances) { - const decimals = await this.getAssetDecimalsForAsset(balance.asset); - const rawAmount = balance.balance.mul(new Decimal(10).pow(decimals)).toFixed(0); - result.push({ - asset: balance.asset, - amount: rawAmount, - }); - } - - return result; + async getBalances(wallet?: Address): Promise<LedgerBalance[]> { + const balances = await this.innerClient.getBalances(wallet ?? this.userAddress); + await this.ensureAssets(); + return balances.map((balance) => { + const info = this.assetsBySymbol.get(balance.asset.toLowerCase()); + const decimals = info?.assetDecimals ?? 6; + return { + asset: balance.asset, + amount: balance.balance.mul(new Decimal(10).pow(decimals)).toFixed(0), + }; + }); }🤖 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 696 - 710, The getBalances method is serializing awaits inside the for-loop by calling await this.getAssetDecimalsForAsset(balance.asset) per iteration; replace that with a parallelized approach — either call await this.ensureAssets() up front and do synchronous lookups for decimals, or build an array of promises (e.g., balances.map(b => this.getAssetDecimalsForAsset(b.asset))) and await Promise.all to get decimals before computing rawAmount and pushing to result — update the logic in getBalances to compute rawAmount using the pre-fetched decimals for each balance.asset.
393-405: Consider passingchaintocreatePublicClientand drop the unusedasync.
getReadClientForChainis declaredasyncbut contains noawait; it can be a regular method returningPublicClient. Separately, creating the client without achainmeans viem can't resolve chain-specific features (multicall3, contract defaults) for future calls — if you have access to viem chain definitions viaensureBlockchains()metadata or static maps, passingchainyields better behavior for anything beyond a rawreadContract.🤖 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 393 - 405, getReadClientForChain is marked async but never awaits and should be a synchronous method returning PublicClient; change its signature to non-async and return the client directly. Also pass a viem chain definition into createPublicClient (e.g., resolve a chain via your ensureBlockchains() metadata or a static map keyed by Number(chainId)) so createPublicClient({ chain, transport: http(this.getRPCUrl(key)) }) can enable chain-specific features like multicall3 and contract defaults; keep caching logic using this._publicClients and the same key lookup/update around createPublicClient.
538-549:getOpenChannelsresult typing —bytes32[]returnsreadonly Hex[].The ABI output type is
bytes32[], so viem infersreadonly \0x${string}`[]forchannelIds. The[...channelIds]spread correctly widens to a mutable array, but the declared return isPromise<string[]>— consider returningPromise<Hex[]>so consumers get typed channel IDs (this also matchesLedgerChannel.channel_id` usage elsewhere).🤖 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 538 - 549, The getOpenChannels function returns channel IDs typed as strings while viem infers bytes32[] as readonly Hex[]; update the function signature from Promise<string[]> to Promise<Hex[]> (import Hex from viem where needed) and return the channelIds as Hex[] (you can keep the [...channelIds] spread if you want a mutable array) so types align with the ABI and with LedgerChannel.channel_id usage elsewhere.sdk/ts-compat/src/rpc.ts (4)
93-95: Random request IDs may collide; consider a monotonic counter.
defaultRequestId()usesMath.floor(Math.random() * Number.MAX_SAFE_INTEGER). Collisions across concurrent in-flight legacy helpers are astronomically unlikely, but a monotonic counter is cheaper, eliminates the collision class entirely, and produces ordered IDs that are easier to correlate in logs. Optional for the compat surface, but worth considering since all legacy helpers now default through this one function.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts-compat/src/rpc.ts` around lines 93 - 95, Replace the non-deterministic defaultRequestId implementation with a module-level monotonic counter to avoid collisions and provide ordered IDs: introduce a top-level let (e.g., requestIdCounter) initialized to 0 and have defaultRequestId() return ++requestIdCounter (or equivalent post-increment), optionally wrapping when exceeding Number.MAX_SAFE_INTEGER back to 1; update any references to defaultRequestId() to rely on the new monotonic behavior. Ensure the counter is defined in the same module as defaultRequestId so it persists across calls but is not exported.
232-239: Silent swallow of parse errors loses migration signal.
parseAnyRPCResponsecatches all errors and returns a generic{ method: 'error', params: { error: 'parse failed' } }. This hides both the original error message (useful for migration) and the raw payload (useful for diagnosis of server-side wire drift). At minimum, include the underlying error text and — ideally — log a warning once per session so consumers migrating off these helpers can see why parsing failed.💡 Suggested tweak
- } catch { - return { requestId: 0, method: 'error', params: { error: 'parse failed' } }; - } + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + return { requestId: 0, method: 'error', params: { error: `parse failed: ${message}` } }; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts-compat/src/rpc.ts` around lines 232 - 239, parseAnyRPCResponse currently swallows all parse errors and returns a generic error payload; change it to surface the underlying error message and the raw input and emit a single-session warning so migrations can see why parsing failed. Update parseAnyRPCResponse to catch the error from extractResponsePayload, include error.message (or String(error)) and the raw string in the returned params (e.g., params: { error: ..., raw }), and add a module-level boolean flag (e.g., warnedParseError) so you log processLogger.warn or console.warn only once per session when a parse failure first occurs; keep extractResponsePayload usage and signatures intact.
365-389: RedundantrequireSubmitVersioncall increateCloseAppSessionMessage.
requireSubmitVersion('createCloseAppSessionMessage', params.version)is invoked inline here, and thenbuildSubmitAppStateParamscalls it again internally (with the already-validated number). Not a bug, but a minor duplication — either drop the inline call or pass a pre-validated params object tobuildSubmitAppStateParams.♻️ Suggested cleanup
buildSubmitAppStateParams( { app_session_id: params.app_session_id, intent: RPCAppStateIntent.Close, - version: requireSubmitVersion('createCloseAppSessionMessage', params.version), + version: params.version, allocations: params.allocations, session_data: params.session_data, quorum_sigs: params.quorum_sigs, } as SubmitAppStateRequestParamsV04, RPCAppStateIntent.Close, params.quorum_sigs, ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts-compat/src/rpc.ts` around lines 365 - 389, The createCloseAppSessionMessage function redundantly calls requireSubmitVersion inline before passing into buildSubmitAppStateParams which itself validates the version; remove the inline requireSubmitVersion call and instead pass the raw params.version (or call requireSubmitVersion once beforehand and pass that validated number) into buildSubmitAppStateParams so validation happens only once — update createCloseAppSessionMessage to pass version directly and ensure buildSubmitAppStateParams still performs validation.
42-55:legacyJSONReplacerkey-based coercion may over-stringify response fields on round-trips.The replacer unconditionally stringifies any numeric
blockchain_id,epoch,version, ornonce. That's fine when serializing outbound requests, but this replacer is used viaserializeMessageonly on outbound messages — which is the correct scope. However, nothing enforces that: if someone later reuseslegacyJSONReplacerto re-serialize a parsed response (e.g., for logging), integerversion/noncefields would become strings and quietly diverge from input types. Consider scoping the coercion to request params only (e.g., pass apathprefix), or renaming torequestJSONReplacerto make intent explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts-compat/src/rpc.ts` around lines 42 - 55, The replacer legacyJSONReplacer currently coerces numeric fields unconditionally which can corrupt round-tripped responses; modify legacyJSONReplacer to accept an explicit context flag (e.g., isRequest: boolean) or a path prefix parameter and only perform string coercion when that flag indicates outbound request serialization, update serializeMessage (the caller) to pass the request context (true) when serializing outgoing messages, and leave default behavior as no coercion to avoid accidental reuse on parsed responses; update any call sites and tests that use legacyJSONReplacer accordingly and consider renaming to requestJSONReplacer to make intent explicit.
🤖 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/src/client.ts`:
- Around line 382-405: getRPCUrl currently falls back to
walletClient.chain.rpcUrls without verifying the chain id, causing wrong-chain
RPCs to be used and cached by getReadClientForChain; update getRPCUrl to only
use walletClient.chain?.rpcUrls.public/default if walletClient.chain?.id (coerce
to number) === chainId, otherwise skip that fallback and throw the existing
error (or require blockchainRPCs); ensure getReadClientForChain still uses
Number(chainId) as the cache key but relies on the corrected getRPCUrl so cached
clients always match the requested chain; reference: getRPCUrl,
getReadClientForChain, _blockchainRPCs, walletClient.chain, _publicClients.
In `@sdk/ts-compat/src/rpc.ts`:
- Around line 61-82: In extractResponsePayload, the raw-array branch uses the
wrong tuple indices (data[1], data[2], data[3]) causing an off-by-one misread;
change that branch to map data[0] → requestId, data[1] → method, data[2] →
payload so it matches the LegacyRPCEnvelope { res: [...] } branch, and add a
unit test that passes a bare-array response through
parseEnvelope/extractResponsePayload (or parseAnyRPCResponse/parse*Response) to
assert correct requestId, method, and payload extraction to prevent regression.
In `@sdk/ts-compat/test/unit/rpc-wire-shape.test.ts`:
- Around line 229-247: The test fixtures use 3-element "res" tuples so the
parsers are validated against a non-v1 wire shape; update the JSON passed to
parseGetAppSessionsResponse, parseCreateAppSessionResponse, and
parseGetAppDefinitionResponse to use 4-element res tuples by appending a
timestamp (e.g., a numeric epoch or Date.now()) as the fourth element in each
res array so the inputs match the v1 wire-compatible format.
---
Nitpick comments:
In `@sdk/ts-compat/src/client.ts`:
- Around line 569-572: getTokenAllowance currently resolves the token via
resolveToken but then calls innerClient.checkTokenAllowance with the original
tokenAddress, which can miss canonical/case-normalized addresses; change the
call in getTokenAllowance to use info.tokenAddress and keep info.chainId and
this.userAddress (mirroring deposit/withdrawal behavior) so the
innerClient.checkTokenAllowance receives the resolved/canonical token address
instead of the caller-provided value.
- Around line 696-710: The getBalances method is serializing awaits inside the
for-loop by calling await this.getAssetDecimalsForAsset(balance.asset) per
iteration; replace that with a parallelized approach — either call await
this.ensureAssets() up front and do synchronous lookups for decimals, or build
an array of promises (e.g., balances.map(b =>
this.getAssetDecimalsForAsset(b.asset))) and await Promise.all to get decimals
before computing rawAmount and pushing to result — update the logic in
getBalances to compute rawAmount using the pre-fetched decimals for each
balance.asset.
- Around line 393-405: getReadClientForChain is marked async but never awaits
and should be a synchronous method returning PublicClient; change its signature
to non-async and return the client directly. Also pass a viem chain definition
into createPublicClient (e.g., resolve a chain via your ensureBlockchains()
metadata or a static map keyed by Number(chainId)) so createPublicClient({
chain, transport: http(this.getRPCUrl(key)) }) can enable chain-specific
features like multicall3 and contract defaults; keep caching logic using
this._publicClients and the same key lookup/update around createPublicClient.
- Around line 538-549: The getOpenChannels function returns channel IDs typed as
strings while viem infers bytes32[] as readonly Hex[]; update the function
signature from Promise<string[]> to Promise<Hex[]> (import Hex from viem where
needed) and return the channelIds as Hex[] (you can keep the [...channelIds]
spread if you want a mutable array) so types align with the ABI and with
LedgerChannel.channel_id usage elsewhere.
In `@sdk/ts-compat/src/rpc.ts`:
- Around line 93-95: Replace the non-deterministic defaultRequestId
implementation with a module-level monotonic counter to avoid collisions and
provide ordered IDs: introduce a top-level let (e.g., requestIdCounter)
initialized to 0 and have defaultRequestId() return ++requestIdCounter (or
equivalent post-increment), optionally wrapping when exceeding
Number.MAX_SAFE_INTEGER back to 1; update any references to defaultRequestId()
to rely on the new monotonic behavior. Ensure the counter is defined in the same
module as defaultRequestId so it persists across calls but is not exported.
- Around line 232-239: parseAnyRPCResponse currently swallows all parse errors
and returns a generic error payload; change it to surface the underlying error
message and the raw input and emit a single-session warning so migrations can
see why parsing failed. Update parseAnyRPCResponse to catch the error from
extractResponsePayload, include error.message (or String(error)) and the raw
string in the returned params (e.g., params: { error: ..., raw }), and add a
module-level boolean flag (e.g., warnedParseError) so you log processLogger.warn
or console.warn only once per session when a parse failure first occurs; keep
extractResponsePayload usage and signatures intact.
- Around line 365-389: The createCloseAppSessionMessage function redundantly
calls requireSubmitVersion inline before passing into buildSubmitAppStateParams
which itself validates the version; remove the inline requireSubmitVersion call
and instead pass the raw params.version (or call requireSubmitVersion once
beforehand and pass that validated number) into buildSubmitAppStateParams so
validation happens only once — update createCloseAppSessionMessage to pass
version directly and ensure buildSubmitAppStateParams still performs validation.
- Around line 42-55: The replacer legacyJSONReplacer currently coerces numeric
fields unconditionally which can corrupt round-tripped responses; modify
legacyJSONReplacer to accept an explicit context flag (e.g., isRequest: boolean)
or a path prefix parameter and only perform string coercion when that flag
indicates outbound request serialization, update serializeMessage (the caller)
to pass the request context (true) when serializing outgoing messages, and leave
default behavior as no coercion to avoid accidental reuse on parsed responses;
update any call sites and tests that use legacyJSONReplacer accordingly and
consider renaming to requestJSONReplacer to make intent explicit.
🪄 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: 6d7e06b9-446d-495c-8eab-49f0fdf56ed4
📒 Files selected for processing (10)
sdk/ts-compat/README.mdsdk/ts-compat/docs/migration-offchain.mdsdk/ts-compat/docs/migration-onchain.mdsdk/ts-compat/docs/migration-overview.mdsdk/ts-compat/src/client.tssdk/ts-compat/src/rpc.tssdk/ts-compat/src/types.tssdk/ts-compat/test/unit/amount-semantics.test.tssdk/ts-compat/test/unit/client-mapping.test.tssdk/ts-compat/test/unit/rpc-wire-shape.test.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/test/unit/rpc-wire-shape.test.ts`:
- Around line 49-98: Add assertions that the unsigned helper functions do not
invoke the signer: after calling createGetChannelsMessage in the first test add
expect(signer).not.toHaveBeenCalled(); and after createGetAppSessionsMessage and
createGetAppDefinitionMessage in the third test add
expect(signer).not.toHaveBeenCalled(); to ensure createGetChannelsMessage,
createGetAppSessionsMessage, and createGetAppDefinitionMessage never call the
signer (use the existing signer mock), placing these assertions immediately
after parsing the raw messages so they verify no wallet prompts occurred.
- Around line 145-218: The test currently only checks request tuples but misses
asserting that signatures are emitted and that signer isn't invoked on
validation failure; update the test for createSubmitAppStateMessage and
createCloseAppSessionMessage to also assert the parsed message includes a
non-empty sig (e.g., check submitRaw/closeRaw or parseCompatRequest(...).sig
exists and is a string) and when calling the functions that should reject for
missing version, verify the signer mock/spied function was not called (use the
signer passed into createSubmitAppStateMessage/createCloseAppSessionMessage and
assert its call count is unchanged) so you validate both correct signing and
that validation short-circuits before signing.
- Around line 122-142: The test currently only asserts parts of the parsed RPC
tuple and should pin the full 4-element JSON-RPC tuple shape returned by
parseCompatRequest: add assertions for parsed.req[0] (the REQUEST_ID) and
parsed.req[3] (the TIMESTAMP) to match the values supplied in the raw request
used in the test, so the test verifies req === [REQUEST_ID,
'app_sessions.v1.create_app_session', PARAMS, TIMESTAMP] exactly; update the
assertions near parsed.sig and parsed.req[1]/[2] accordingly so
parseCompatRequest, parsed.req, and the create_app_session tuple are fully
validated.
🪄 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: 0f0e92e8-b8ef-4883-af59-0c438e1627ff
📒 Files selected for processing (4)
sdk/ts-compat/src/client.tssdk/ts-compat/src/rpc.tssdk/ts-compat/test/unit/client-mapping.test.tssdk/ts-compat/test/unit/rpc-wire-shape.test.ts
✅ Files skipped from review due to trivial changes (2)
- sdk/ts-compat/test/unit/client-mapping.test.ts
- sdk/ts-compat/src/client.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- sdk/ts-compat/src/rpc.ts
nksazonov
left a comment
nksazonov
left a comment
There was a problem hiding this comment.
LGTM with some comments
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/client.ts (1)
1111-1118:⚠️ Potential issue | 🟠 Major
transfer()divides by token decimals, but should use asset decimals to matchgetBalances()semantics.The test fixture reveals the issue: YUSD has
assetDecimals=6andtokenDecimals=8on chain 84532.getBalances()returns amounts in raw asset-decimal units (multiplies byassetDecimals), buttransfer()divides bytokenDecimals. When a caller naturally chains them—getBalances()output →transfer(..., [{ asset, amount }])—they pass asset-decimal units to a method expecting token-decimal units, resulting in amounts off by10^(tokenDecimals - assetDecimals).The test passes only because it explicitly constructs token-decimal-formatted inputs ('500000000'), not because it validates the intended roundtrip from
getBalances(). Switch line 1114 to usegetAssetDecimalsForAssetinstead.🛠 Proposed fix
async transfer(destination: Address, allocations: TransferAllocation[]): Promise<void> { for (const alloc of allocations) { - const decimals = await this.getTokenDecimalsForAsset(alloc.asset); + const decimals = await this.getAssetDecimalsForAsset(alloc.asset); const humanAmount = new Decimal(alloc.amount).div(new Decimal(10).pow(decimals)); await this.innerClient.transfer(destination, alloc.asset, humanAmount); } }🤖 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 1111 - 1118, The transfer() implementation uses getTokenDecimalsForAsset to scale amounts, but getBalances() returns asset-decimal raw amounts so transfer should use asset decimals; update transfer() to call getAssetDecimalsForAsset(alloc.asset) instead of getTokenDecimalsForAsset(alloc.asset) when computing humanAmount (the Decimal division before calling innerClient.transfer) so amounts match getBalances() semantics and a balance→transfer roundtrip works correctly.
🧹 Nitpick comments (2)
sdk/ts-compat/src/rpc.ts (1)
1-524: LGTM overall on the RPC compat layer.The envelope-aware
extractResponsePayload, consistentnewUnsignedMessage/newSignedMessagebuilders,legacyJSONReplacerhandling for bigints/stringified numeric fields, and the switch tounsupportedHelperErrorforcreateTransferMessage/createCreateChannelMessage/createCloseChannelMessage/createResizeChannelMessageall make the compat surface meaningfully more honest. The tuple-index fix from the prior review is correctly applied inextractResponsePayload.One small typing nit (optional):
parseEnvelope's return typeLegacyRPCEnvelope | unknowncollapses tounknown, so theLegacyRPCEnvelopeannotation conveys no information at the type level — downstream still has to narrow manually. Considerunknownor a discriminated union if you want the shape documented.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts-compat/src/rpc.ts` around lines 1 - 524, The parseEnvelope return type is declared as LegacyRPCEnvelope | unknown which effectively collapses to unknown and doesn't convey the LegacyRPCEnvelope shape; update the parseEnvelope signature to return unknown (or a proper discriminated union describing the tuple/object shapes) and adjust any consumer typings if needed (notably extractResponsePayload which currently narrows the result) so TypeScript no longer treats the value as opaque; locate the parseEnvelope function and change its declared return type and, if you choose a discriminated union, define that union type and use it for safer narrowing downstream.sdk/ts-compat/src/client.ts (1)
402-414: Minor:PublicClientcreated without achain, andgetReadClientForChainisasyncwithout awaiting anything.
createPublicClient({ transport: http(...) })omits thechainoption, so chain-aware helpers on the returned client (multicall batching config, fee/block defaults, block-explorer helpers) won't have chain context. For the currentreadContractcall ingetOpenChannelsthis is harmless, but if this client gets reused later it's worth wiring up. You can resolve the chain fromensureBlockchains()/walletClient.chainwhen the ids match, or from a viem chain registry.Also,
getReadClientForChainis declaredasyncbut has noawait— either dropasyncor move it to once the chain resolution becomes async.🛠 Suggested tweak
- private async getReadClientForChain(chainId: bigint): Promise<PublicClient> { + private getReadClientForChain(chainId: bigint): PublicClient { const key = Number(chainId); const cached = this._publicClients.get(key); if (cached) { return cached; } + const walletChain = this.walletClient.chain; const client = createPublicClient({ + ...(walletChain && Number(walletChain.id) === key ? { chain: walletChain } : {}), transport: http(this.getRPCUrl(key)), }); this._publicClients.set(key, client); return client; }Call sites would drop their
await.🤖 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 402 - 414, The getReadClientForChain function currently creates a PublicClient via createPublicClient({ transport: ... }) without a chain and is marked async without any awaits; update it to resolve the appropriate viem Chain (e.g. find the chain whose id matches Number(chainId) from ensureBlockchains() or from this.walletClient.chain when ids match) and pass that chain into createPublicClient({ chain, transport: http(...) }) so returned clients are chain-aware; if resolving the chain is synchronous keep the function non-async (drop the async) or, if you must await ensureBlockchains(), keep async and await that resolution—adjust call sites accordingly.
🤖 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/client.ts`:
- Around line 1111-1118: The transfer() implementation uses
getTokenDecimalsForAsset to scale amounts, but getBalances() returns
asset-decimal raw amounts so transfer should use asset decimals; update
transfer() to call getAssetDecimalsForAsset(alloc.asset) instead of
getTokenDecimalsForAsset(alloc.asset) when computing humanAmount (the Decimal
division before calling innerClient.transfer) so amounts match getBalances()
semantics and a balance→transfer roundtrip works correctly.
---
Nitpick comments:
In `@sdk/ts-compat/src/client.ts`:
- Around line 402-414: The getReadClientForChain function currently creates a
PublicClient via createPublicClient({ transport: ... }) without a chain and is
marked async without any awaits; update it to resolve the appropriate viem Chain
(e.g. find the chain whose id matches Number(chainId) from ensureBlockchains()
or from this.walletClient.chain when ids match) and pass that chain into
createPublicClient({ chain, transport: http(...) }) so returned clients are
chain-aware; if resolving the chain is synchronous keep the function non-async
(drop the async) or, if you must await ensureBlockchains(), keep async and await
that resolution—adjust call sites accordingly.
In `@sdk/ts-compat/src/rpc.ts`:
- Around line 1-524: The parseEnvelope return type is declared as
LegacyRPCEnvelope | unknown which effectively collapses to unknown and doesn't
convey the LegacyRPCEnvelope shape; update the parseEnvelope signature to return
unknown (or a proper discriminated union describing the tuple/object shapes) and
adjust any consumer typings if needed (notably extractResponsePayload which
currently narrows the result) so TypeScript no longer treats the value as
opaque; locate the parseEnvelope function and change its declared return type
and, if you choose a discriminated union, define that union type and use it for
safer narrowing downstream.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d93dd94d-12ed-4f34-a07d-1bb2c6c28ecc
📒 Files selected for processing (2)
sdk/ts-compat/src/client.tssdk/ts-compat/src/rpc.ts
ihsraham
changed the title
ihsraham
force-pushed
the
ts-compat-runtime-remediation
branch
from
d057a64 to
41652f2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
sdk/ts-compat/docs/migration-onchain.md (1)
79-81: ⚡ Quick winConsider clarifying token identifier formats.
The examples mix different token identifier conventions:
- Client methods use
tokenAddress(presumably hex addresses like'0x...')- Transfer allocations use symbolic names like
asset: 'usdc'While this distinction may be intentional and correct, consider adding a brief note explaining when to use token addresses versus symbolic asset names to help migrating developers avoid confusion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/ts-compat/docs/migration-onchain.md` around lines 79 - 81, Clarify the token identifier conventions used in the docs: add a short note near the Transfer allocations and App-session allocations sections explaining that transfer allocations use symbolic asset identifiers in the `asset` field (e.g., 'usdc' with raw asset-unit `amount: '5000000'`) while client methods and on-chain operations expect `tokenAddress` (hex like '0x...'); mention when to prefer `asset` vs `tokenAddress` and give one concrete sentence mapping symbolic names to on‑chain token addresses to avoid confusion for migrating developers.
🤖 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 `@sdk/ts-compat/docs/migration-onchain.md`:
- Around line 79-81: Clarify the token identifier conventions used in the docs:
add a short note near the Transfer allocations and App-session allocations
sections explaining that transfer allocations use symbolic asset identifiers in
the `asset` field (e.g., 'usdc' with raw asset-unit `amount: '5000000'`) while
client methods and on-chain operations expect `tokenAddress` (hex like '0x...');
mention when to prefer `asset` vs `tokenAddress` and give one concrete sentence
mapping symbolic names to on‑chain token addresses to avoid confusion for
migrating developers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b79e013b-1017-4670-964a-4303cc3cbf85
📒 Files selected for processing (11)
sdk/ts-compat/README.mdsdk/ts-compat/docs/migration-offchain.mdsdk/ts-compat/docs/migration-onchain.mdsdk/ts-compat/docs/migration-overview.mdsdk/ts-compat/src/client.tssdk/ts-compat/src/rpc.tssdk/ts-compat/src/types.tssdk/ts-compat/test/unit/amount-semantics.test.tssdk/ts-compat/test/unit/client-mapping.test.tssdk/ts-compat/test/unit/client.test.tssdk/ts-compat/test/unit/rpc-wire-shape.test.ts
✅ Files skipped from review due to trivial changes (4)
- sdk/ts-compat/test/unit/amount-semantics.test.ts
- sdk/ts-compat/docs/migration-overview.md
- sdk/ts-compat/README.md
- sdk/ts-compat/test/unit/rpc-wire-shape.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- sdk/ts-compat/test/unit/client-mapping.test.ts
- sdk/ts-compat/src/types.ts
- sdk/ts-compat/src/client.ts
- sdk/ts-compat/src/rpc.ts
## Summary Adds protocol, TS SDK, and compat SDK drift guards on top of current `main`. #699 and #701 are now merged, so this branch has been rebased directly on `main` and preserves current behavior for: - `nitronode` naming and paths - #699 compat runtime semantics and migration surface - #701 `appDefinition` mapping and SDK `applicationID` exports - package versions at `1.2.1` ## Change Areas ### Drift Guards - Adds static guards for RPC method coverage, RPC DTO shape, public API snapshots, consumed ABI surface, app/session-key signing vectors, and response transforms. - Adds compat drift coverage for v1 app-session mapping, legacy flat fallback behavior, empty-session behavior, asset decimal conversion, config pass-through, and session-key public APIs. - Adds runtime smoke coverage that builds and starts an isolated local `nitronode`, then verifies live SDK and compat calls. ### SDK Validation - Validates key-state `states` containers before mapping. - Validates app-session payloads, allocations, app definitions, participants, and quorum at the wire boundary. - Rejects arrays in session key-state record transforms. ### Nitronode Database - Propagates SQLite migration errors from `connectToSqlite`. - Returns Postgres migration errors instead of panicking. - Keeps SQLite auto-migration aligned with current runtime tables, including state, transaction, app participants, and key-state tables. - Uses a parameterized schema existence check and quoted schema identifiers for Postgres schema setup. ### Runtime Smoke And CI - Builds `./nitronode` and uses `NITRONODE_RUNTIME_SMOKE_*` env names in new scripts/workflows/docs. - Requires `NITRONODE_RUNTIME_SMOKE_PRIVATE_KEY` for external smoke mode. - Keeps the hardcoded Anvil key only for isolated local smoke. - Restricts runtime-smoke log directories to repo-local paths. - Passes an allowlisted child-process env instead of forwarding all parent env. - Captures compat logs for failure diagnostics. - Builds Foundry artifacts before TS SDK/static drift ABI checks in CI. ## Review Notes Addressed current actionable review feedback around SDK response validation, database migration error handling, runtime-smoke env/log safety, public API tautology tests, ABI drift-test stability, and stale Clearnode wording in PR-added files. Deferred CodeRabbit docstring coverage and shared public API test utility extraction because they are broader cleanup items, not blockers for this PR. ## Verification Local checks run from a fresh worktree: - `npm --prefix sdk/ts run typecheck` - `npm --prefix sdk/ts test` - `npm --prefix sdk/ts run lint` - `npm --prefix sdk/ts run build:ci` - `npm --prefix sdk/ts-compat run typecheck` - `npm --prefix sdk/ts-compat test` - `npm --prefix sdk/ts-compat run lint` - `npm --prefix sdk/ts-compat run build:ci` - `forge build` for local ABI artifacts - `./scripts/check-protocol-drift.sh --static` - `./scripts/check-protocol-drift.sh --runtime` - `go test ./nitronode/... ./pkg/rpc/...` - `git diff --check` GitHub checks have started on the rebased branch.
3 participants
This is part of the ongoing refinements to the TS SDKs, focused on the compat TypeScript SDK. It is now rebased on current
mainafter #701.It makes the compat runtime surface more honest and more useful during migration:
transfer()andgetBalances()use canonical asset raw-unit strings, on-chain token helpers use raw token-unitbigint, and app-session allocations stay human-decimal stringsappDefinitionmapping and currentnitronodenamingValidation run locally:
npm --prefix sdk/ts-compat run typechecknpm --prefix sdk/ts-compat testnpm --prefix sdk/ts-compat run lintSummary by CodeRabbit
New Features
Behavior Changes
Documentation
Tests