Conversation
…stry, gated actions Add 15+ new compat facade methods to bridge the v1.2.0 TS SDK features to the v0.5.3-style API surface: - Security token locking: lockSecurityTokens, initiateSecurityTokensWithdrawal, cancelSecurityTokensWithdrawal, withdrawSecurityTokens, approveSecurityToken, getLockedBalance (with bigint<->Decimal conversion and amount validation) - App registry: getApps, registerApp - Additional queries: getBlockchains, getActionAllowances, getEscrowChannel, checkTokenAllowance - Channel: acknowledge - App sessions: rebalanceAppSessions - Lifecycle: waitForClose Bug fixes: - Fix application -> applicationId field rename in AppDefinitionV1 - Export OngoingStateTransitionError from barrel and add to classifyError - Add blockchain list caching (ensureBlockchains) to avoid redundant RPCs - Add console.warn on locking token decimals fallback Housekeeping: - Update stale @layer-3/nitrolite references to @yellow-org/sdk where referring to the current peer dependency (keep old name for v0.5.3 context) - Bump version 1.1.1 -> 1.2.0, peer dep @yellow-org/sdk >=1.2.0 Made-with: Cursor
ihsraham
requested review from
alessio,
dimast-x,
nksazonov and
philanton
as code owners
|
Warning Rate limit exceeded
⌛ 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. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe ts-compat package was rebranded to the Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/App
participant Client as NitroliteClient
participant SDK as Core SDK
participant RPC as Blockchain RPC
User->>Client: lockSecurityTokens(targetWallet, chainId, amount)
Client->>Client: ensureBlockchains() / getLockingTokenDecimals(chainId)
Client->>SDK: getBlockchains()
SDK-->>Client: blockchain metadata
Client->>RPC: eth_call ERC20.decimals (via configured RPC)
RPC-->>Client: decimals
Client->>Client: convert human amount → smallest unit
Client->>SDK: call lockSecurityTokens RPC (with converted amount)
SDK->>RPC: submit transaction
RPC-->>SDK: txHash
SDK-->>Client: txHash
Client-->>User: Promise<string> (txHash)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
sdk/ts-compat/src/client.ts (1)
975-980: Memoize the in-flight blockchain lookup too.
ensureBlockchains()only writes_blockchainsaftergetBlockchains()resolves, so the first burst of parallel locking calls can still fan out to multiple RPC requests. If this cache is meant to suppress redundant lookups, store the promise and clear it on failure.🤖 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 975 - 980, ensureBlockchains currently only sets this._blockchains after await this.innerClient.getBlockchains(), allowing parallel callers to trigger multiple RPCs; change it to memoize the in-flight promise (e.g., store a temp like this._blockchainsPromise) so subsequent calls return the same promise, and on success set this._blockchains and clear the promise, while on failure clear the promise and rethrow the error so callers can retry. Update references in ensureBlockchains to use the promise guard and ensure innerClient.getBlockchains() errors do not leave a stale promise.sdk/ts-compat/package.json (1)
38-38: Prefer a bounded peer range for@yellow-org/sdk.Line 38 uses
>=1.2.0, which also accepts future2.xreleases. Since this package is wrapping the v1 SDK surface, npm can treat an unvalidated breaking major as compatible here. Use^1.2.0or>=1.2.0 <2unless v2 compatibility has already been verified.Suggested manifest change
- "@yellow-org/sdk": ">=1.2.0", + "@yellow-org/sdk": ">=1.2.0 <2",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts-compat/package.json` at line 38, The dependency declaration for "@yellow-org/sdk" currently uses an unbounded range ">=1.2.0" which would allow unverified major v2 releases; update the package.json dependency entry for "@yellow-org/sdk" (the version string) to a bounded range such as "^1.2.0" or ">=1.2.0 <2" to constrain to v1-compatible releases and prevent accidental acceptance of v2 breaking changes; choose "^1.2.0" if SemVer caret semantics are acceptable or use the explicit "<2" upper bound if you want to allow all 1.x >=1.2.0.
🤖 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/README.md`:
- Around line 334-361: The README is inconsistent about amount units: the
Security Token examples state APIs take raw bigint units but elsewhere the
transfer() example uses a human string; update the examples and wording so the
unit contract is clear — either (A) change the transfer() example
(NitroliteClient.transfer) to use raw bigint units like the other calls
(approveSecurityToken, lockSecurityTokens, getLockedBalance,
initiateSecurityTokensWithdrawal, cancelSecurityTokensWithdrawal,
withdrawSecurityTokens), or (B) explicitly document that transfer accepts
human-readable strings which the compat layer scales by token decimals and show
both variants with a clear label; ensure every code snippet and the surrounding
text unambiguously state which APIs expect raw bigint vs human-readable string
inputs.
In `@sdk/ts-compat/src/client.ts`:
- Around line 1069-1094: getLockingTokenDecimals currently picks the first asset
with the same chainId and memoizes 6 on any miss/error; instead resolve decimals
from the actual locking token and only cache verified values: inside
getLockingTokenDecimals (using ensureBlockchains, ensureAssets, assetsByToken,
chain.lockingContractAddress) find the asset whose token/contract address equals
the chain.lockingContractAddress (compare normalized casing), if found set and
return its info.decimals and store that in _lockingTokenDecimals; if not found
or on transient errors do not overwrite the memoized value with 6 (return 6 as a
non-cached fallback) so functions like approveSecurityToken, lockSecurityTokens,
and getLockedBalance keep correct cached scale once discovered.
---
Nitpick comments:
In `@sdk/ts-compat/package.json`:
- Line 38: The dependency declaration for "@yellow-org/sdk" currently uses an
unbounded range ">=1.2.0" which would allow unverified major v2 releases; update
the package.json dependency entry for "@yellow-org/sdk" (the version string) to
a bounded range such as "^1.2.0" or ">=1.2.0 <2" to constrain to v1-compatible
releases and prevent accidental acceptance of v2 breaking changes; choose
"^1.2.0" if SemVer caret semantics are acceptable or use the explicit "<2" upper
bound if you want to allow all 1.x >=1.2.0.
In `@sdk/ts-compat/src/client.ts`:
- Around line 975-980: ensureBlockchains currently only sets this._blockchains
after await this.innerClient.getBlockchains(), allowing parallel callers to
trigger multiple RPCs; change it to memoize the in-flight promise (e.g., store a
temp like this._blockchainsPromise) so subsequent calls return the same promise,
and on success set this._blockchains and clear the promise, while on failure
clear the promise and rethrow the error so callers can retry. Update references
in ensureBlockchains to use the promise guard and ensure
innerClient.getBlockchains() errors do not leave a stale promise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a898291-6c2e-4090-953d-2b0134e7db0a
📒 Files selected for processing (8)
sdk/ts-compat/README.mdsdk/ts-compat/docs/migration-offchain.mdsdk/ts-compat/docs/migration-overview.mdsdk/ts-compat/examples/lifecycle.tssdk/ts-compat/examples/tsconfig.jsonsdk/ts-compat/package.jsonsdk/ts-compat/src/client.tssdk/ts-compat/src/index.ts
dimast-x
left a comment
dimast-x
left a comment
There was a problem hiding this comment.
Please address the second AI comment
…mount docs
1. getLockingTokenDecimals: Query the locking contract on-chain via
contract.asset() + token.decimals() instead of guessing from the
asset map. Throw on missing RPC config instead of silent fallback.
Cache only verified decimals. (Addresses dimast-x + CodeRabbit review)
2. createAppSession: Pass through optional ownerSig field added in
SDK v1.2.0 (opts?: { ownerSig?: string }). Also add owner_sig to
CreateAppSessionRequestParams type.
3. README: Clarify amount conventions — table distinguishing raw bigint
vs raw string inputs across method groups. Fix transfer example to
use raw units.
Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
sdk/ts-compat/README.md (1)
89-94:⚠️ Potential issue | 🟠 MajorFix the
transfer()amount wording.Line 89 still says “human-readable” amounts, but the compat client treats
TransferAllocation.amountas raw units and scales it down before delegating. The example/comment on Line 93 is raw-unit based, so the text and example currently contradict each other and will mislead callers about what to pass.🤖 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 89 - 94, Update the README wording to state that transfer() expects amounts in raw token units (the smallest denomination) rather than “human-readable” values; mention that TransferAllocation.amount is treated as raw units (string) and the compat layer scales it down before delegating. Reference the transfer() call and the TransferAllocation.amount field so readers know which parameter to pass, and ensure the example comment matches this wording (e.g., '5000000' represents 5 USDC in 6-decimal raw units).
🤖 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/README.md`:
- Around line 336-370: Update the README locking example to avoid the removed
fixed-decimal assumption: replace the hardcoded 100_000_000n amount with a
token-aware approach that either (a) shows how to resolve the token and its
decimals via the client/RPC for the target chain and compute rawAmount =
humanAmount * 10**decimals, or (b) use a concrete token example (e.g., USDC on
chain X) and clearly state the exact raw amount for that token; also add a short
note that approveSecurityToken, lockSecurityTokens, getLockedBalance,
initiateSecurityTokensWithdrawal, cancelSecurityTokensWithdrawal and
withdrawSecurityTokens require blockchainRPCs configured for the chain and will
throw if RPCs are missing so readers know to set blockchainRPCs before calling
these methods.
---
Duplicate comments:
In `@sdk/ts-compat/README.md`:
- Around line 89-94: Update the README wording to state that transfer() expects
amounts in raw token units (the smallest denomination) rather than
“human-readable” values; mention that TransferAllocation.amount is treated as
raw units (string) and the compat layer scales it down before delegating.
Reference the transfer() call and the TransferAllocation.amount field so readers
know which parameter to pass, and ensure the example comment matches this
wording (e.g., '5000000' represents 5 USDC in 6-decimal raw units).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3374dc66-9065-47d4-a2a3-031e488e5b22
📒 Files selected for processing (3)
sdk/ts-compat/README.mdsdk/ts-compat/src/client.tssdk/ts-compat/src/types.ts
- transfer(): Change "human-readable" to "raw-unit string amounts" to match actual behavior (divides by decimals internally) - Locking example: Replace hardcoded USDC/6-decimal amount with Yellow token (18 decimals), note that blockchainRPCs must be configured Made-with: Cursor
sdk/ts-compat/README.md
Outdated
|
|
||
| ### 4. Transfer off-chain | ||
|
|
||
| `transfer()` accepts raw-unit string amounts (smallest denomination). The compat layer divides by token decimals internally before delegating to the v1 SDK: |
There was a problem hiding this comment.
The actual transfer() signature is:
async transfer(recipientWallet: string, asset: string, amount: Decimal): Promise<core.State>
It takes a Decimal amount (human-readable units like 50 for 50 USDC), not a raw-unit string.
There's no array of {asset, amount} objects either — asset and amount are separate parameters.
The correct usage is:
const state = await client.transfer('0xRecipient...', 'usdc', new Decimal(50));
Claude message:
The guide is wrong. The TS SDK uses human-readable Decimal amounts, not raw-unit strings. The
conversion to raw units (smallest denomination) happens internally in two places:
1. For on-chain signing (state_packer.ts:51): decimalToBigInt() multiplies by 10^decimals when
ABI-encoding the state for contract submission. E.g., Decimal(50) with 6 decimals becomes 50000000n.
2. For RPC to the node (client.ts:1911): state.transition.amount.toString() just serializes the
Decimal as-is (e.g., "50"), so the node also works with human-readable amounts.
The flow is:
transfer('0xRecip', 'usdc', new Decimal(50))
→ applyTransferSendTransition(state, recipient, Decimal(50)) // stores Decimal in state
→ packState() → decimalToBigInt(Decimal(50), 6) → 50000000n // for signing
→ transformStateToRPC() → amount: "50" // for node RPC
There was a problem hiding this comment.
That's the main SDK's signature, yes. The compat layer wraps it differently to preserve the v0.5.3 calling convention:
// Compat transfer (v0.5.3 style)
async transfer(destination: Address, allocations: TransferAllocation[]): Promise<void>Internally at line 929-934, it takes the raw-unit string from alloc.amount, divides by token decimals, then calls innerClient.transfer(dest, asset, humanDecimal).
So both are correct at their own layer. The README now makes this clear and shows innerClient as the escape hatch for direct SDK access.
There was a problem hiding this comment.
Alright. Was a bit confusing for me as I expected in v0.5.3 they also used human readable amounts for transfer (as this was unified-balance layer operation). If it is okay for devs then I am good with it.
There was a problem hiding this comment.
The lifecycle example uses parseUnits to produce raw amounts, so the pattern is established and documented. I'll add it as a new feature in the future release if devs ask for it.
Compiles and builds clean.
I'll merge it now
Make the compat transfer() docs explicit: v0.5.3-style array-of- allocations with raw-unit strings, compat divides by decimals, SDK uses Decimal. Show innerClient escape hatch for direct SDK access. Made-with: Cursor
…ording - Make quorum_sigs required in CreateAppSessionRequestParams and CloseAppSessionRequestParams (not optional) - Remove rebalanceAppSessions (not available on deployment, will be deprecated per Dmytro) - Fix README: quorumSigs shown as required in method cheat sheet, clean up contradictory amount-convention wording
There was a problem hiding this comment.
Actionable comments posted: 1
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/README.md (1)
490-495:⚠️ Potential issue | 🟡 MinorUpdate README peer dependency version for
@yellow-org/sdkto match package.json.The README shows
@yellow-org/sdk >= 1.0.0but package.json specifies>=1.2.0. Line 494 should be updated to reflect the correct peer dependency constraint.🤖 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 490 - 495, Update the Peer Dependencies table entry for `@yellow-org/sdk` in the README so the version constraint matches package.json: change the table cell currently showing `@yellow-org/sdk | >=1.0.0` to `@yellow-org/sdk | >=1.2.0` (i.e., update the `@yellow-org/sdk` version string in the Peer Dependencies table row).
🧹 Nitpick comments (1)
sdk/ts-compat/src/client.ts (1)
316-316: Consider tightening the error classification pattern.The current pattern matches "ongoing" OR "state transition" independently. The word "ongoing" alone is fairly generic and could match unrelated errors (e.g., "ongoing maintenance", "ongoing operation"). Consider requiring both terms or a more specific phrase:
-if (lower.includes('ongoing') || lower.includes('state transition')) return new OngoingStateTransitionError(msg); +if (lower.includes('ongoing') && lower.includes('state transition')) return new OngoingStateTransitionError(msg);Alternatively, if the server uses a specific error message format, match that phrase directly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts-compat/src/client.ts` at line 316, The current classifier in client.ts uses a loose check (if lower.includes('ongoing') || lower.includes('state transition')) to return OngoingStateTransitionError, which can false-positive on generic "ongoing" messages; tighten it by requiring a more specific match (e.g., both terms present with &&, or look for the exact phrase "ongoing state transition" or the server's canonical error format) when evaluating the lower string before returning new OngoingStateTransitionError(msg), so update the condition around the lower variable and the OngoingStateTransitionError return to use the stricter pattern.
🤖 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 705-706: The v1Def construction incorrectly falls back to (def as
any).protocol for applicationId, which can set applicationId to a protocol
string; update the v1Def assignment (AppDefinitionV1 / v1Def) to stop using
def.protocol and instead validate RPCAppDefinition.application: if
def.application is missing throw a clear Error (or at minimum use
def.application || ''), ensuring applicationId is populated only from
def.application and not from def.protocol.
---
Outside diff comments:
In `@sdk/ts-compat/README.md`:
- Around line 490-495: Update the Peer Dependencies table entry for
`@yellow-org/sdk` in the README so the version constraint matches package.json:
change the table cell currently showing `@yellow-org/sdk | >=1.0.0` to
`@yellow-org/sdk | >=1.2.0` (i.e., update the `@yellow-org/sdk` version string
in the Peer Dependencies table row).
---
Nitpick comments:
In `@sdk/ts-compat/src/client.ts`:
- Line 316: The current classifier in client.ts uses a loose check (if
lower.includes('ongoing') || lower.includes('state transition')) to return
OngoingStateTransitionError, which can false-positive on generic "ongoing"
messages; tighten it by requiring a more specific match (e.g., both terms
present with &&, or look for the exact phrase "ongoing state transition" or the
server's canonical error format) when evaluating the lower string before
returning new OngoingStateTransitionError(msg), so update the condition around
the lower variable and the OngoingStateTransitionError return to use the
stricter pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 008b60c1-277d-4f1e-a8d1-a6c683b8eee2
📒 Files selected for processing (3)
sdk/ts-compat/README.mdsdk/ts-compat/src/client.tssdk/ts-compat/src/types.ts
3 participants
Summary
Reconciles
@yellow-org/sdk-compatwith@yellow-org/sdkv1.2.0, adding all missing API surface and fixing several issues. Version bump: 1.1.1 → 1.2.0 (minor, no breaking changes).New Compat Methods (15+)
lockSecurityTokens,initiateSecurityTokensWithdrawal,cancelSecurityTokensWithdrawal,withdrawSecurityTokens,approveSecurityToken,getLockedBalance— accept rawbigintamounts (v0.5.3 style), convert toDecimalinternally, withamount > 0validationgetApps,registerAppgetBlockchains,getActionAllowances,getEscrowChannel,checkTokenAllowanceacknowledgerebalanceAppSessionswaitForCloseBug Fixes
application→applicationIdfield rename inAppDefinitionV1(was broken against SDK v1.2.0)OngoingStateTransitionErrorfrom barrel + add toclassifyErrordetectionconsole.warnwhen locking token decimals lookup falls back to default (6)Improvements
ensureBlockchains()cache to avoid redundant RPC calls in locking methods@layer-3/nitrolitepackage references to@yellow-org/sdk(keep old name only in v0.5.3 migration context)@yellow-org/sdk >=1.2.0Files Changed
src/client.tssrc/index.tsOngoingStateTransitionErrorexport, package name updatespackage.jsonREADME.mddocs/migration-overview.mddocs/migration-offchain.mdexamples/lifecycle.tsexamples/tsconfig.jsonTest Plan
npm run typecheckpassesnpm run buildpassesSummary by CodeRabbit
New Features
Documentation
Behavior Changes
Chores
Other