Conversation
📝 WalkthroughWalkthroughThis PR refactors the SDK registration mechanism across multiple network implementations (EVM, Solana, Sui, Definitions) by replacing implicit side-effect imports with explicit named imports and deferred registration calls. The registration flow now tracks explicit invocations separately from auto-registration and reserves deprecation warnings for implicit use. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
sdk/definitions/src/index.ts (1)
14-38:⚠️ Potential issue | 🟠 MajorThe deferred registration contradicts "Backward-compatible: auto-register on import."
setTimeout(..., 0)delaysregisterPayloadTypes(...)to the next macrotask. While the current test suite doesn't fail because all payload operations use asyncgetProtocol()calls (which execute after the timer), the implementation still violates the stated guarantee: true import-time registration is synchronous, not deferred. Any consumer that performs synchronous payload operations immediately after import could encounter an empty factory.The suggested fix is sound: register synchronously and defer only the warning by using the deprecated parameter to distinguish explicit calls from the internal fallback.
Possible fix shape
-export function register(_deprecatedTopLevel?: boolean): void { - _explicitlyRegistered = true; +export function register(_deprecatedTopLevel = false): void { + if (!_deprecatedTopLevel) { + _explicitlyRegistered = true; + } if (!payloadFactory.has(composeLiteral("Ntt", nttNamedPayloads[0]![0]))) { registerPayloadTypes("Ntt", nttNamedPayloads); } if ( !payloadFactory.has( composeLiteral("MultiTokenNtt", multiTokenNttNamedPayloads[0]![0]) ) ) { registerPayloadTypes("MultiTokenNtt", multiTokenNttNamedPayloads); } } -setTimeout(() => { - if (!_explicitlyRegistered) { - console.warn( - "@wormhole-foundation/sdk-definitions-ntt: auto-registration on import is deprecated. Import { register } and call it explicitly." - ); - } - register(); -}, 0); +register(true); +setTimeout(() => { + if (!_explicitlyRegistered) { + console.warn( + "@wormhole-foundation/sdk-definitions-ntt: auto-registration on import is deprecated. Import { register } and call it explicitly." + ); + } +}, 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/definitions/src/index.ts` around lines 14 - 38, The register function currently defers payload registration via setTimeout, causing import-time registration to be asynchronous; change the behavior so register() is invoked synchronously on import (call register() directly) while only deferring the deprecation warning so callers who explicitly passed the deprecatedTopLevel flag are not warned—use the existing _explicitlyRegistered flag and the deprecatedTopLevel parameter to detect explicit calls (keep registerPayloadTypes("Ntt", ...) and registerPayloadTypes("MultiTokenNtt", ...) logic inside register) and move the console.warn into a deferred timer that runs only when _explicitlyRegistered is false after performing the synchronous register call.sui/ts/src/index.ts (1)
13-34:⚠️ Potential issue | 🟠 MajorImport-time registration timing has changed in a way that contradicts the backward-compatibility goal.
While the actual
registerProtocol(...)calls are still executed before any real consumer code runs (since all protocol lookups are async), the registration is now deferred to a macrotask instead of happening synchronously during module initialization. This breaks the documented contract that protocols are available on import.The previous implementation (commit c3774e0) called
register(true)synchronously. The current deferred approach usingsetTimeout(..., 0)still works in practice because all SDK consumers useawait chain.getProtocol(), but it violates the semantic promise of "auto-register on import" for anything that might synchronously check protocol availability during the same event tick.If deprecating synchronous auto-registration is intentional, update the comment to clarify that the async deferral is permanent (not temporary), and consider whether the deprecation message is still accurate. If backward compatibility is required, restore synchronous registration while keeping only the warning deferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sui/ts/src/index.ts` around lines 13 - 34, The module currently defers auto-registration via setTimeout which breaks the promise of synchronous "auto-register on import"; restore backward-compatible behavior by invoking register(true) synchronously during module initialization (use the existing register(_deprecatedTopLevel?: boolean) API and pass true) and keep only the deprecation warning deferred using setTimeout (so remove the deferred register() call but retain the console.warn in the macrotask that checks _explicitlyRegistered); alternatively, if the async deferral is intentional, update the top comment and the warning to explicitly state that registration is permanently deferred and adjust logic to avoid claiming "auto-register on import" is supported (reference symbols: register, _explicitlyRegistered, setTimeout, registerProtocol, protocolIsRegistered, SuiNtt, SuiNttWithExecutor).solana/ts/sdk/index.ts (1)
14-35:⚠️ Potential issue | 🟠 MajorDeferred fallback registration breaks import-time compatibility.
Moving the fallback
register()behindsetTimeout(..., 0)meansNtt/NttWithExecutorare no longer registered during module evaluation. Any existing consumer that imports this module and immediately uses SDK lookups in the same turn can now race the timer;solana/tests/anchor/anchor.test.tsis an in-repo example of that pattern. Please keep the fallback registration synchronous and defer only the warning, e.g. by using the deprecated parameter to distinguish internal auto-registration from explicitregister()calls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@solana/ts/sdk/index.ts` around lines 14 - 35, The deferred setTimeout registration causes race conditions; make the auto-registration synchronous while deferring only the warning: if _explicitlyRegistered is false call register(true) immediately (so register can detect the deprecated internal auto-registration via its _deprecatedTopLevel param and avoid re-triggering the warning), and keep the setTimeout only for console.warn; update register to respect the _deprecatedTopLevel flag (use _explicitlyRegistered and skip emitting the deprecation warning when _deprecatedTopLevel is true) and ensure protocolIsRegistered/registerProtocol/SolanaNtt/SolanaNttWithExecutor logic remains unchanged.
🤖 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/definitions/src/index.ts`:
- Around line 14-38: The register function currently defers payload registration
via setTimeout, causing import-time registration to be asynchronous; change the
behavior so register() is invoked synchronously on import (call register()
directly) while only deferring the deprecation warning so callers who explicitly
passed the deprecatedTopLevel flag are not warned—use the existing
_explicitlyRegistered flag and the deprecatedTopLevel parameter to detect
explicit calls (keep registerPayloadTypes("Ntt", ...) and
registerPayloadTypes("MultiTokenNtt", ...) logic inside register) and move the
console.warn into a deferred timer that runs only when _explicitlyRegistered is
false after performing the synchronous register call.
In `@solana/ts/sdk/index.ts`:
- Around line 14-35: The deferred setTimeout registration causes race
conditions; make the auto-registration synchronous while deferring only the
warning: if _explicitlyRegistered is false call register(true) immediately (so
register can detect the deprecated internal auto-registration via its
_deprecatedTopLevel param and avoid re-triggering the warning), and keep the
setTimeout only for console.warn; update register to respect the
_deprecatedTopLevel flag (use _explicitlyRegistered and skip emitting the
deprecation warning when _deprecatedTopLevel is true) and ensure
protocolIsRegistered/registerProtocol/SolanaNtt/SolanaNttWithExecutor logic
remains unchanged.
In `@sui/ts/src/index.ts`:
- Around line 13-34: The module currently defers auto-registration via
setTimeout which breaks the promise of synchronous "auto-register on import";
restore backward-compatible behavior by invoking register(true) synchronously
during module initialization (use the existing register(_deprecatedTopLevel?:
boolean) API and pass true) and keep only the deprecation warning deferred using
setTimeout (so remove the deferred register() call but retain the console.warn
in the macrotask that checks _explicitlyRegistered); alternatively, if the async
deferral is intentional, update the top comment and the warning to explicitly
state that registration is permanently deferred and adjust logic to avoid
claiming "auto-register on import" is supported (reference symbols: register,
_explicitlyRegistered, setTimeout, registerProtocol, protocolIsRegistered,
SuiNtt, SuiNttWithExecutor).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 218b6f00-6785-4853-aa46-99ab34390a30
📒 Files selected for processing (7)
cli/src/commands/token-transfer.tscli/src/index.tsevm/ts/src/index.tssdk/definitions/__tests__/registerWarning.test.tssdk/definitions/src/index.tssolana/ts/sdk/index.tssui/ts/src/index.ts
3 participants
Summary
setTimeout(..., 0)so explicitregister()calls (including dynamic import flows) do not log false-positive warningsregisterbackward-compatible with an optional deprecated parameter signaturesdk-definitionsto lock warning behavior (explicit register => no warning,side-effect import => warning)Why
Running
ntt --versionwas printing loud deprecation warnings even when consumers follow the explicit-registration migration path. This change preserves the deprecation signal for legacy usage while removing noise for correct usage.Validation
ntt --versionprints clean output without deprecation warningsbun run typecheck:clibun run --filter "@wormhole-foundation/sdk-definitions-ntt" test -- __tests__/registerWarning.test.ts