Fix TS SDK raw RPC drift and on-chain balance surface#698
Conversation
ihsraham
requested review from
dimast-x,
nksazonov and
philanton
as code owners
📝 WalkthroughWalkthroughThe pull request adds a new client method for querying on-chain token balances, removes the app session close RPC functionality, transitions the TypeScript SDK to ESM module support with updated build and test configurations, and adds corresponding test coverage. Documentation reflects the revised feature scope. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ihsraham
force-pushed
the
ts-sdk-core-truth-fixes
branch
from
bf853e3 to
9b51b68
Compare
|
pushed a small follow up here. added two more getonchainbalance tests so we cover init ordering and error propagation, and updated the pr description with an explicit release note for the raw close_app_session removal since there is no package changelog file in this repo to land that in. |
|
@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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk/ts/package.json (1)
61-67:⚠️ Potential issue | 🟠 MajorRemove
jest-utilfromdependencies— it is an internal Jest helper package that should never be a runtime dependency.
jest-utilis an internal utility used by Jest's own packages and is already pulled in transitively. It has no place in the SDK's runtime dependencies and will unnecessarily pollute the dependency tree of every consumer. The initial search confirms no code insrc/ortest/directly importsjest-util, so it can be safely removed entirely.Proposed fix
"dependencies": { "abitype": "^1.2.3", "decimal.js": "^10.4.3", - "jest-util": "^30.3.0", "viem": "^2.46.1", "zod": "^4.3.6" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts/package.json` around lines 61 - 67, Remove the inappropriate runtime dependency "jest-util" from the package.json "dependencies" block (the entry currently alongside "abitype", "decimal.js", "viem", and "zod"); delete the "jest-util" line, run your package manager to update the lockfile (npm/yarn/pnpm) so the lockfile no longer lists it, and ensure no src/ or test/ files import "jest-util" (search for usages) before committing the change.
🧹 Nitpick comments (2)
sdk/ts/test/unit/client.test.ts (1)
5-83: Tests cover the new surface well; one small robustness tweak on the init-ordering test.The three cases (delegation, init-ordering, error propagation) are appropriately scoped, and using
Object.create(Client.prototype)with injected internals is a reasonable way to avoid standing up an RPC/WebSocket stack for a pure delegator. Two observations:
The init-ordering test relies on ordering in the microtask queue when asserting
getTokenBalancehas not been called yet (line 55). That assertion runs synchronously right afterclient.getOnChainBalance(...)is invoked, before any awaited continuation runs, so it's correct — but aPromise.resolve()tick before the assertion would make the "waits for initialization" intent more explicit and less reliant on JS scheduling semantics.These tests reach into private-ish fields (
blockchainClients,initializeBlockchainClient). That's fine for a unit test of this size, but if the internal caching shape changes later (e.g., replaced by a factory), all three tests will need updating in lockstep. Consider a follow-up with a thin seam (e.g., an injectable factory) when/if more balance/chain methods land.♻️ Optional tightening of the ordering assertion
const balancePromise = client.getOnChainBalance(chainId, 'usdc', wallet); + // Let any synchronous microtasks settle so the assertion reflects + // the explicit "awaiting initializeBlockchainClient" state. + await Promise.resolve(); expect(initializeBlockchainClient).toHaveBeenCalledWith(chainId); expect(getTokenBalance).not.toHaveBeenCalled();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts/test/unit/client.test.ts` around lines 5 - 83, The init-ordering test should explicitly yield a microtask before asserting getTokenBalance hasn't been called: after invoking client.getOnChainBalance(...) (which internally calls client.initializeBlockchainClient), await a Promise.resolve() (or use process.nextTick()/setImmediate equivalent) to ensure the initialization microtask runs before checking that getTokenBalance wasn't invoked; this change touches the test that calls client.getOnChainBalance, asserts initializeBlockchainClient was called, and then checks getTokenBalance, so add the single microtask yield between those steps to make the ordering assert deterministic.sdk/ts/eslint.config.cjs (1)
22-23: Consider a narrower relaxation ofno-unused-vars.Turning off
@typescript-eslint/no-unused-varsproject-wide silences genuinely unused symbols in new code, not just the intended RPC wire-type cases. A common middle ground is keeping the rule enabled with an underscore ignore pattern:♻️ Proposed refactor
- '@typescript-eslint/no-explicit-any': 'off', - '@typescript-eslint/no-unused-vars': 'off', + '@typescript-eslint/no-explicit-any': 'off', + '@typescript-eslint/no-unused-vars': [ + 'warn', + { argsIgnorePattern: '^_', varsIgnorePattern: '^_', caughtErrorsIgnorePattern: '^_' }, + ],
no-explicit-anybeing off is defensible per the repo guideline allowinganyfor RPC wire types, so leaving that as-is is fine.As per coding guidelines: "Strict TypeScript — no
anyunless unavoidable (e.g., RPC wire types)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts/eslint.config.cjs` around lines 22 - 23, The rule '@typescript-eslint/no-unused-vars' is currently turned off broadly; re-enable it and apply an underscore-ignore pattern so only intentionally prefixed unused symbols are suppressed. Update the ESLint config entry for '@typescript-eslint/no-unused-vars' to a strict setting (error or warn) with options like argsIgnorePattern, varsIgnorePattern (and caughtErrorsIgnorePattern) set to '^_' so RPC wire-type uses of `any` remain allowed while genuine unused symbols are still reported; leave '@typescript-eslint/no-explicit-any' as-is.
🤖 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/package.json`:
- Around line 61-67: Remove the inappropriate runtime dependency "jest-util"
from the package.json "dependencies" block (the entry currently alongside
"abitype", "decimal.js", "viem", and "zod"); delete the "jest-util" line, run
your package manager to update the lockfile (npm/yarn/pnpm) so the lockfile no
longer lists it, and ensure no src/ or test/ files import "jest-util" (search
for usages) before committing the change.
---
Nitpick comments:
In `@sdk/ts/eslint.config.cjs`:
- Around line 22-23: The rule '@typescript-eslint/no-unused-vars' is currently
turned off broadly; re-enable it and apply an underscore-ignore pattern so only
intentionally prefixed unused symbols are suppressed. Update the ESLint config
entry for '@typescript-eslint/no-unused-vars' to a strict setting (error or
warn) with options like argsIgnorePattern, varsIgnorePattern (and
caughtErrorsIgnorePattern) set to '^_' so RPC wire-type uses of `any` remain
allowed while genuine unused symbols are still reported; leave
'@typescript-eslint/no-explicit-any' as-is.
In `@sdk/ts/test/unit/client.test.ts`:
- Around line 5-83: The init-ordering test should explicitly yield a microtask
before asserting getTokenBalance hasn't been called: after invoking
client.getOnChainBalance(...) (which internally calls
client.initializeBlockchainClient), await a Promise.resolve() (or use
process.nextTick()/setImmediate equivalent) to ensure the initialization
microtask runs before checking that getTokenBalance wasn't invoked; this change
touches the test that calls client.getOnChainBalance, asserts
initializeBlockchainClient was called, and then checks getTokenBalance, so add
the single microtask yield between those steps to make the ordering assert
deterministic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a69fb4e-54ce-4526-9c84-cf89542796bc
📒 Files selected for processing (10)
sdk/ts/README.mdsdk/ts/eslint.config.cjssdk/ts/jest.config.cjssdk/ts/package.jsonsdk/ts/src/client.tssdk/ts/src/core/state_advancer.tssdk/ts/src/rpc/api.tssdk/ts/src/rpc/client.tssdk/ts/src/rpc/methods.tssdk/ts/test/unit/client.test.ts
💤 Files with no reviewable changes (3)
- sdk/ts/src/rpc/methods.ts
- sdk/ts/src/rpc/client.ts
- sdk/ts/src/rpc/api.ts
3 participants
This is part of the ongoing refinements to the TS SDKs, focused on the main TypeScript SDK.
It adds a high-level
getOnChainBalance(...)surface, removes the unsupported rawapp_sessions.v1.close_app_sessionRPC from the TS SDK, tightens the README parity wording, and aligns the package-local test and lint setup so the TS SDK can be validated cleanly.release note: this removes the previously exposed raw
app_sessions.v1.close_app_sessionmethod, request/response types, and client helper from@yellow-org/sdk. that RPC is not supported by clearnode. app session close continues to be modeled throughsubmit_app_statewith close intent.Summary by CodeRabbit
New Features
Removals
Documentation
Tests
Chores