feat: sdk teleportTo realm coords support - #10047
Conversation
🚦 CI StatusWindows and Mac built successfully in Unity Cloud.
Warnings not reduced: 12025 => 12025 — remove at least 1 warning to merge. Warnings/errors in files changed by this PR (1)Lint run · full InspectCode report · took 25m 34s All Unity tests passed ✅
Tests time sums the test cases; Job time is the job's wall clock including checkout, licensing and asset import. Slowest tests
Full report: run summary · results + editor logs: editmode · playmode 🏁 Bare-metal benchmark finished — run #34525474443. Full reportPR #10047, run #34525474443 Overall: ✅ no significant changes Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Apple M1
Intel Core i5
On demand — comment |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — feat: sdk teleportTo realm coords support
Inspected commits: base 2831b06 → head 141ff48
STEP 1 — Scope
The diff touches 9 files (5 hand-written C#, 1 JS bridge, 1 auto-generated protocol C#, 2 package manifest files). Read the implementation, interface, wrappers, tests, JS module, the ChangeRealmPromptController + its Params, the callback wiring in DynamicWorldContainer, and ChatTeleporter.TeleportToRealmAsync. Searched consumers of TryTeleportTo — 3 callers: RestrictedActionsAPIWrapper, UserActionsWrapper, and tests.
STEP 2 — Root-cause check
PASS. This is a protocol-driven feature addition: the SDK's TeleportTo restricted action gains an optional realm field (and makes worldCoordinates optional). The diff implements the new routing — realm-present requests go through the existing change-realm consent prompt; coords-only requests keep the existing teleport prompt. This is a genuine feature, not a symptom fix.
STEP 3 — Design & integration
PASS. No new long-lived units introduced. The change extends the existing TryTeleportTo method signature and reuses the established ChangeRealmAsync → ChangeRealmPromptController → changeRealmCallback → ChatTeleporter.TeleportToRealmAsync pipeline.
- The
ChangeRealmPromptController.Paramsstruct already had thePositionfield (prior PR). - The callback in
DynamicWorldContainer(line 719–728) already branches onposition.HasValue, routing toChatTeleporter.TeleportToRealmAsync(realmUrl, position.Value, …)with position or the chat-command path without. - The
ChangeRealmAsyncprivate method gains an optionalVector2Int? positionparameter — backward-compatible with the existingTryChangeRealmcaller (which passes no position).
No lifecycle duplication, no per-frame reconciliation, no new persistent state. Clean integration into the existing architecture.
STEP 4 — Member audit
| Member | Consumers | Verdict |
|---|---|---|
IRestrictedActionsAPI.TryTeleportTo(Vector2Int?, string?) |
RestrictedActionsAPIWrapper.TeleportTo, UserActionsWrapper.RequestTeleport, 4 test methods |
Appropriate public surface — each combination of optional params is tested |
ChangeRealmAsync(…, Vector2Int? position = null) |
TryTeleportTo, TryChangeRealm |
Optional parameter preserves backward compatibility |
No single-use intermediaries, no redundant predicates.
STEP 5 — Line-level findings
See inline comments below.
STEP 6 — Complexity
COMPLEX — modifies a public interface (IRestrictedActionsAPI) used across assemblies, the JS→C# bridge, and the protocol message layer.
STEP 7 — QA assessment
QA_REQUIRED: YES — changes runtime teleport behavior (user-facing navigation).
STEP 8 — Non-blocking warnings
None. Main scene is not modified.
Security Review
Scope: Traced the attacker-controlled realm string from the SDK scene message through JS bridge → RestrictedActionsAPIWrapper → TryTeleportTo → ChangeRealmAsync → ChangeRealmPromptController.
- Consent prompt gate: Every realm-based teleport goes through
ChangeRealmPromptController.ShowAsync, which shows a user consent UI before proceeding. No path bypasses it. - Realm display sanitization: The controller already disables rich-text parsing on message and realm text fields (SEC-003) and strips userinfo from URL realms to prevent consent-prompt spoofing (SEC-004,
DestinationHostFor). These controls cover this new entry point. - IsCurrent guard: Present and covers all branches.
- Dependency bump:
@dcl/protocolupdated from1.0.0-33874985952.commit-4f4e0abto1.0.0-34381335718.commit-3c838bb— Decentraland-owned package, expected protocol update for the new optional fields. No new dependencies added. No binaries changed. No advisory matches.
Limits: npm audit not run (read-only review). No native binaries affected.
DEPENDENCY_REVIEW: PASS
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies public IRestrictedActionsAPI interface, JS↔C# bridge layer, and protocol message handling across restricted-actions subsystem
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
…ative assertion, complete XML doc - Replace multi-line comments that narrate external behavior with concise descriptions of what the annotated code itself does - Add DidNotReceive check for TeleportPromptController in TeleportToRealmDefaultSpawnWithoutCoordinates test - Document the both-absent rejection case in IRestrictedActionsAPI XML doc Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Review: feat: sdk teleportTo realm coords support
STEP 2 — Root-cause check
PASS. This PR adds a genuine new feature: SDK scenes can now request teleportation to a specific parcel in another realm via the TeleportTo restricted action. The protocol update makes world_coordinates optional and adds an optional realm field. The implementation routes to the correct consent prompt based on which fields are present. This is a new capability, not a symptom fix.
STEP 3 — Design & integration
PASS. The PR makes excellent use of existing infrastructure:
ChangeRealmPromptController.Paramsalready had aVector2Int? Positionproperty (pre-existing, not added by this PR).ChangeRealmAsyncis extended with an optionalpositionparameter (defaultnull), keeping backward compatibility with the existingTryChangeRealmcaller.- The
DynamicWorldContainercallback already branched onposition.HasValueto route throughChatTeleporter.TeleportToRealmAsync(realmUrl, position.Value, ct)vs the chat-command path. - No new lifecycle units, systems, controllers, or managers are introduced. The routing logic is a straightforward addition to the existing
TryTeleportTomethod.
Owner search: TryTeleportTo already owned the teleport decision; the new realm branch delegates to ChangeRealmAsync, which was already the private helper for TryChangeRealm. No lifecycle duplication.
STEP 4 — Member audit
| Member | Consumers | Status |
|---|---|---|
IRestrictedActionsAPI.TryTeleportTo(Vector2Int?, string?) |
RestrictedActionsAPIWrapper.TeleportTo, UserActionsWrapper.RequestTeleport, tests |
Interface method, multiple callers ✓ |
ChangeRealmAsync(string, string, Vector2Int?) |
TryTeleportTo (with position), TryChangeRealm (without — uses default null) |
Private helper, two distinct use cases ✓ |
RestrictedActionsAPIWrapper.TeleportTo(int?, int?, string?) |
JS bridge (RestrictedActions.js) |
Single caller, justified — it's the JS↔C# interop boundary ✓ |
No single-use-merge, absent≠false, or redundant-guard issues.
STEP 5 — Line-level findings
All findings are P2 (non-blocking). No P0 or P1 issues found.
[P2-1] Pre-existing parameter name mismatch between interface and implementation. The interface declares Vector2Int? newCoords while the implementation uses Vector2Int? coords. This was already present before this PR (base SHA confirmed), but since this PR touches both signatures, it's a good opportunity to align them. coords is the better name — the new prefix was vestigial from when the method only teleported within the current realm. (See inline comment.)
[P2-2] JS bridge comment narrates external behavior (CLAUDE.md §11). The comment on RestrictedActions.js lines 56–57 describes what the C# implementation does with the values, not what the JS bridge function itself does. The interface XML doc already documents the contract. (See inline comment.)
[P2-3] Wrapper silently discards partial coordinates. RestrictedActionsAPIWrapper.TeleportTo treats x.HasValue && y.HasValue as the guard — if only one is provided, coords become null. This is defensible since the JS layer always sends both or neither (they come from the same coords object), so partial coordinates can only arise from a JS-side bug. Low risk.
STEP 5 — Security review
All consent-prompt paths are preserved. Every code path through TryTeleportTo terminates at a user-facing consent prompt (ChangeRealmPromptController or TeleportPromptController). The sceneStateProvider.IsCurrent guard prevents non-focused scenes from invoking the action.
Realm string injection: mitigated. The attacker-controlled realm string passes through the existing SEC-003/SEC-004 defenses: rich-text is disabled on both prompt text fields, and DestinationHostFor() strips URL userinfo to prevent consent-prompt spoofing. The message parameter is hardcoded to string.Empty (line 115), so scenes cannot control the prompt body text via this path — slightly more restrictive than the existing changeRealm action, which is good.
Downstream validation: intact. After consent, the realm string flows through ChatEnvironmentValidator.ValidateTeleport which rejects URLs outside the configured base domain and explicitly rejects URLs containing @ (userinfo).
Supply chain:
| Dependency | Old version | New version | Source | Integrity |
|---|---|---|---|---|
@dcl/protocol |
1.0.0-33874985952.commit-4f4e0ab |
1.0.0-34381335718.commit-3c838bb |
npmjs.org (@dcl scope, first-party) |
sha512 present |
The generated C# changes (RestrictedActions.gen.cs) are consistent with the expected protocol update: TeleportToRequest gains an optional realm field (field number 2) and world_coordinates becomes optional. No unexpected fields, services, or code patterns added. No binaries, native plugins, UPM packages, or assembly changes.
DEPENDENCY_REVIEW: PASS
STEP 6 — Complexity
SIMPLE. The PR touches 5 meaningful source files (~80 lines of hand-written changes), plus generated protobuf code and a lockfile bump. Changes are straightforward routing logic in an existing API — no ECS systems, async patterns, containers, or architectural changes.
STEP 7 — QA assessment
QA_REQUIRED: YES. The changes affect runtime teleportation behavior. The PR description includes specific QA test steps: enter sdk7testscenes.dcl.eth at position 1,2, verify portals render, click a portal, confirm teleportation to the correct scene.
STEP 8 — Warnings
No warnings. Main.unity is not modified.
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Straightforward routing-logic extension to the existing TeleportTo restricted action, reusing pre-existing ChangeRealmPromptController infrastructure.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
…omment - Rename interface parameter `newCoords` → `coords` to match implementation and update paramref in XML doc - Replace JS bridge comment that narrated C# behavior with a concise description of what the bridge function itself does Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — feat: sdk teleportTo realm coords support
STEP 1 — Scope
Files inspected (9): RestrictedActionsAPIImplementation.cs, IRestrictedActionsAPI.cs, RestrictedActionsAPIWrapper.cs, UserActionsWrapper.cs, RestrictedActions.js, RestrictedActions.gen.cs, RestrictedActionsAPIImplementationShould.cs, scripts/package.json, scripts/package-lock.json.
Surrounding context loaded: ChangeRealmPromptController.cs, ChangeRealmPromptController.Params.cs, ISceneRuntime.cs (consumer registration), DeepLinkHandleImplementation.cs (existing ChangeRealmAsync + position usage). Repo-wide rg for TryTeleportTo, ChangeRealmPromptController, and IRestrictedActionsAPI consumers to map all call sites.
Conventions loaded: CLAUDE.md, review-instructions.md.
STEP 2 — Root-cause check
This PR implements new protocol support — making worldCoordinates optional and adding an optional realm field to the TeleportTo restricted action. The diff addresses the feature at the right level: the protocol schema gains the field, the JS bridge unpacks the new optional, the C# interface/implementation branch on the new parameter, and existing consent-prompt infrastructure is reused for the realm path. PASS — this is a cause, not a symptom.
STEP 3 — Design & integration
No new units introduced. The diff modifies the existing TryTeleportTo method signature and adds branching logic inside it. No new systems, managers, controllers, or helpers.
Lifecycle reuse confirmed:
- Realm-based teleport reuses
ChangeRealmAsync→ChangeRealmPromptController, which already acceptedVector2Int? Positionin itsParamsstruct (confirmed inChangeRealmPromptController.Params.cs). ThechangeRealmCallbackinChangeRealmPromptController.csalready forwardsinputData.Positionto the realm-switch callback. - Coordinate-only teleport reuses existing
TeleportAsync→TeleportPromptController.
No new subscriptions, connections, or resources — nothing to trace for teardown.
PASS — no design issues.
STEP 4 — Member audit
IRestrictedActionsAPI.TryTeleportTo(Vector2Int? coords, string? realm) — signature changed from (Vector2Int newCoords). Consumers found and all updated:
RestrictedActionsAPIWrapper.TeleportTo(JS bridge) — updated ✓UserActionsWrapper.RequestTeleport— passesnullrealm ✓RestrictedActionsAPIImplementation(impl) — updated ✓- Test file — updated with 3 new tests + existing test adapted ✓
ChangeRealmAsync(string, string, Vector2Int?) — added optional third parameter with default null. Backward-compatible; existing callers (TryChangeRealm, MVCManagerMenusAccessFacade) continue to work without change.
No single-use members, no absent-≠-false predicates, no redundant guards. No issues.
STEP 5 — Line-level review
Two passes completed. No blocking issues found. Detailed checks:
A. Blocking-issue pass:
-
Null safety —
TryTeleportTo:string.IsNullOrEmpty(realm)correctly handles bothnulland"".coords.HasValuecorrectly gatescoords.Value. No null-dereference paths. -
Partial-coords defense in wrapper (
x.HasValue && y.HasValue) — correct. Per protobuf,worldCoordinatesis an optionalVector2message, so x and y arrive together or not at all. The defensive AND is harmless and follows the pattern used byMovePlayerToin the same wrapper. -
JS bridge —
coords != undefinedcorrectly distinguishes set/unset protobuf optional message fields. The loose equality!=also catchesnull. Field-by-field guarding is consistent with the existingmovePlayerTopattern.message.realm != undefinedcorrectly forwards the optional string. -
.Forget()onChangeRealmAsync— consistent with existing.Forget()usage inTryChangeRealmand all other async prompt launches in this class. These are UI-prompt fire-and-forget operations on the main thread — the established pattern. Not detached essential work (CLAUDE.md §9). -
Security: realm is scene-controlled (untrusted) — the realm string flows to
ChangeRealmPromptController, which already disablesrichText(SEC-003) and strips userinfo spoofing viaDestinationHostFor()(SEC-004). No new attack surface. -
System.Text.RegularExpressionsimport in test file — used bynew Regex("TeleportTo")inIgnoreTeleportWithNeitherCoordinatesNorRealmviaLogAssert.Expect. Not unused. -
Resource/subscription leaks — no new subscriptions, event hookups, or disposables added.
B. Design/encapsulation/naming pass:
-
Comment quality —
// Realm present → route through the change-realm consent prompt, carrying the optional parcel.describes what this code does (routes through the consent prompt), not what callers do with the result. Acceptable per CLAUDE.md §11. -
XML doc on
IRestrictedActionsAPI.TryTeleportTo— accurately describes the three cases (coords-only, realm-only, both-absent rejection). Parameter names match implementation. ✓ -
Test naming —
TeleportToParcelInAnotherRealm,TeleportToRealmDefaultSpawnWithoutCoordinates,IgnoreTeleportWithNeitherCoordinatesNorRealm— descriptive, follow existing naming convention in the file. AAA pattern, NUnit + NSubstitute. ✓ -
No magic values, no naming issues, no encapsulation leaks.
STEP 6 — Complexity
SIMPLE — 9 files changed, but the core logic is ~20 lines of branching in TryTeleportTo. The generated protobuf code is the bulk of the diff. No ECS systems, no async pattern changes, no plugin/container wiring, no assembly changes.
STEP 7 — QA assessment
QA_REQUIRED: YES — changes runtime code affecting teleportation behavior (user-facing consent prompts, realm switching with optional coordinates). The PR description includes specific test steps for a ZONE/SEPOLIA world.
STEP 8 — Non-blocking warnings
None. Main.unity not modified.
Security & Dependency Review
Inventory:
| Dependency | Version | Source | Type | Scope | Risk |
|---|---|---|---|---|---|
@dcl/protocol |
1.0.0-34381335718.commit-3c838bb |
registry.npmjs.org/@dcl/protocol |
npm | build (codegen) | Low |
- Provenance:
@dcl/protocolis published under the@dclnpm scope (Decentraland's official scope). Resolved from the public npm registry with integrity hash. The package is used only at build time for protobuf code generation; the generatedRestrictedActions.gen.csships in the player. - Generated code:
RestrictedActions.gen.cschanges are consistent with adding an optionalstring realmfield toTeleportToRequest. The protobuf descriptor base64, field numbers (1 = worldCoordinates, 2 = realm), wire types, andHasRealm/ClearRealmpattern match standardprotocoutput for an optional string field. - No new binaries, plugins, native payloads, or assembly changes.
- No new execution hooks,
InitializeOnLoad, or build callbacks. - Security controls intact: Scene-controlled
realmstring goes through the existingChangeRealmPromptControllerconsent prompt with rich-text disabled and userinfo-stripping (DestinationHostFor). No new input vectors.
DEPENDENCY_REVIEW: PASS
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Signature change to TryTeleportTo with branching logic; reuses existing ChangeRealm/Teleport prompt infrastructure, no new systems or lifecycle units.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
pravusjif
added
clean-build
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — feat: sdk teleportTo realm coords support
Inspected commits: base 81afe2b → head 0170ec7 (4 commits, including 2 follow-up fixes and a merge from dev)
STEP 1 — Scope
Files changed (9): RestrictedActionsAPIImplementation.cs, IRestrictedActionsAPI.cs, RestrictedActionsAPIWrapper.cs, UserActionsWrapper.cs, RestrictedActions.js, RestrictedActions.gen.cs (auto-generated protobuf), RestrictedActionsAPIImplementationShould.cs, scripts/package.json, scripts/package-lock.json.
Context loaded: Full source of all changed files. ChangeRealmPromptController.cs and its Params.cs partial (confirms Vector2Int? Position pre-exists). DynamicWorldContainer's changeRealmCallback registration (already branches on position.HasValue). Repo-wide rg for all TryTeleportTo consumers: RestrictedActionsAPIWrapper.TeleportTo, UserActionsWrapper.RequestTeleport, and 4 test methods. CLAUDE.md and review-instructions.md loaded.
STEP 2 — Root-cause check
PASS. This is a protocol-driven feature addition: the SDK's TeleportTo restricted action gains an optional realm field and makes worldCoordinates optional. The diff implements the new routing at the correct layers — protocol schema, JS bridge, C# interface/implementation — and reuses existing consent-prompt infrastructure for the realm path. Genuine feature, not a symptom fix.
STEP 3 — Design & integration
PASS. No new long-lived units introduced. The change extends the existing TryTeleportTo method with branching logic.
Lifecycle reuse confirmed:
- Realm-based teleport reuses
ChangeRealmAsync→ChangeRealmPromptController, which already acceptedVector2Int? Positionin itsParamsstruct (ChangeRealmPromptController.Params.cs, line 13). The controller'sOnViewShow(line 49) already forwardsinputData.Positionto thechangeRealmCallback. - Coordinate-only teleport reuses existing
TeleportAsync→TeleportPromptController. ChangeRealmAsyncgains an optionalVector2Int? position = nullparameter — backward-compatible with the existingTryChangeRealmcaller (line 133, which passes no position).
No lifecycle duplication, no per-frame reconciliation, no new persistent state, no new subscriptions or resources to trace.
STEP 4 — Member audit
| Member | Consumers | Verdict |
|---|---|---|
IRestrictedActionsAPI.TryTeleportTo(Vector2Int?, string?) |
RestrictedActionsAPIWrapper.TeleportTo, UserActionsWrapper.RequestTeleport, 4 test methods |
Appropriate public surface — each combination of optional params is tested |
ChangeRealmAsync(string, string, Vector2Int?) |
TryTeleportTo (with position), TryChangeRealm (without — uses default null) |
Optional parameter preserves backward compatibility |
RestrictedActionsAPIWrapper.TeleportTo(int?, int?, string?) |
JS bridge (RestrictedActions.js) |
Single caller, justified — JS↔C# interop boundary |
No single-use intermediaries, no absent-≠-false predicates, no redundant guards.
STEP 5 — Line-level review
Two passes completed. No issues found.
A. Blocking-issue pass:
-
Null safety —
string.IsNullOrEmpty(realm)correctly handles bothnulland""(an SDK scene explicitly settingrealm = ""is correctly treated as absent).coords.HasValuecorrectly gatescoords.Value. No null-dereference paths. -
Partial-coords defense in wrapper (
x.HasValue && y.HasValue) — correct. Per protobuf,worldCoordinatesis an optionalVector2message, so x and y arrive together or not at all. The defensive AND is harmless and follows the pattern used byMovePlayerToin the same wrapper. -
JS bridge —
coords != undefinedcorrectly distinguishes set/unset protobuf optional message fields. The loose!=also catchesnull.message.realm != undefinedcorrectly forwards the optional string. Pattern is consistent withmovePlayerTo. -
.Forget()onChangeRealmAsync(line 115) — consistent with all other.Forget()calls in this class (lines 73, 125, 133, 223, 264). These are UI consent-prompt fire-and-forget operations on the main thread — the established pattern. Not detached essential work (CLAUDE.md §9). -
IsCurrentguard — present at line 109, covers all branches (realm, coords, and neither). ✓ -
Resource/subscription leaks — no new subscriptions, event hookups, connections, CancellationTokenSources, or disposables added. ✓
-
System.Text.RegularExpressionsimport in test file — used bynew Regex("TeleportTo")inLogAssert.Expect. Not unused. ✓
B. Design/encapsulation/naming pass:
-
Comment quality — All comments describe what the annotated code does, not external behavior: "Realm present → route through the change-realm consent prompt, carrying the optional parcel" (line 112), "Unpack optional fields and forward to the C# bridge; both are nullable" (JS line 56). Compliant with CLAUDE.md §11. ✓
-
XML doc on
IRestrictedActionsAPI.TryTeleportTo— accurately describes all three cases including the both-absent rejection. Parameter names (coords,realm) match the implementation. ✓ -
Naming — PascalCase for methods/properties, camelCase for locals/params.
Iprefix on interface. Follows conventions. ✓ -
Test naming and patterns —
TeleportToParcelInAnotherRealm,TeleportToRealmDefaultSpawnWithoutCoordinates,IgnoreTeleportWithNeitherCoordinatesNorRealm— descriptive, follow existing naming convention. AAA pattern, NUnit + NSubstitute. Negative assertions present on all tests where applicable. ✓ -
No magic values, no naming issues, no encapsulation leaks, no constants needed. ✓
Prior review findings: All 6 P2 findings from the initial review (comments narrating external behavior, missing negative assertion, incomplete XML doc, parameter name mismatch, JS comment) were addressed by the two follow-up commits (cf54ace, 8af11ed). No unresolved findings remain.
Security Review
Input trace: The attacker-controlled realm string flows: SDK scene message → protobuf TeleportToRequest.realm → JS bridge (message.realm) → RestrictedActionsAPIWrapper.TeleportTo(realm) → TryTeleportTo(coords, realm) → ChangeRealmAsync(string.Empty, realm, coords) → ChangeRealmPromptController.Params(message, realm, position) → consent prompt shown to user.
Controls verified:
- Consent prompt gate: Every realm-based teleport goes through
ChangeRealmPromptController.ShowAsync(line 298), which shows a user consent UI before proceeding. No path bypasses it. ✓ - Rich-text disabled (SEC-003):
viewInstance.MessageText.richText = falseandviewInstance.RealmText.richText = falseinOnViewInstantiated(controller lines 37–38). Covers this new entry point. ✓ - Userinfo stripping (SEC-004):
DestinationHostFor()(controller lines 72–93) strips URL userinfo to prevent consent-prompt spoofing. Covers this new entry point. ✓ - IsCurrent guard: Line 109, covers all branches. ✓
- Message hardcoded:
string.Emptypassed as message (line 115) — scenes cannot control the prompt body text via this path (more restrictive thanTryChangeRealm). ✓
Supply chain:
| Dependency | Old version | New version | Source | Type |
|---|---|---|---|---|
@dcl/protocol |
1.0.0-33874985952.commit-4f4e0ab |
1.0.0-34381335718.commit-3c838bb |
registry.npmjs.org (@dcl scope) |
npm, build-time codegen |
- First-party Decentraland package (
@dclnpm scope).sha512integrity hash present in lockfile. ✓ - Generated
RestrictedActions.gen.cschanges are consistent with adding an optionalrealmstring field (field number 2) and makingworldCoordinatesoptional. Protobuf descriptor, field numbers, wire types,HasRealm/ClearRealmpattern match standardprotocoutput. ✓ - No new binaries, native plugins, UPM packages, or assembly definition changes. ✓
- No new execution hooks,
InitializeOnLoad, or build callbacks. ✓
Coverage limits: npm audit not executed (read-only review). No native binaries affected.
DEPENDENCY_REVIEW: PASS
STEP 6 — Complexity
SIMPLE — The core logic change is ~20 lines of branching in TryTeleportTo, plus a 1-parameter addition to ChangeRealmAsync. The bulk of the diff (116 lines) is auto-generated protobuf code. No ECS systems, async pattern changes, plugin/container wiring, or assembly changes.
STEP 7 — QA assessment
QA_REQUIRED: YES — Changes runtime teleportation behavior (user-facing consent prompts, realm switching with optional coordinates). The PR description includes specific test steps for a ZONE/SEPOLIA world.
STEP 8 — Non-blocking warnings
None. Main.unity not modified.
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Straightforward routing-logic extension to existing TryTeleportTo, reusing pre-existing ChangeRealmPromptController infrastructure; no new systems or lifecycle units.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — feat: sdk teleportTo realm coords support
Inspected commits: base 19d6924 → head 28d1273 (5 commits: initial feature, 2 review-feedback fixes, 2 merges from dev)
STEP 1 — Scope
Files inspected (9): RestrictedActionsAPIImplementation.cs, IRestrictedActionsAPI.cs, RestrictedActionsAPIWrapper.cs, UserActionsWrapper.cs, RestrictedActions.js, RestrictedActions.gen.cs (generated), RestrictedActionsAPIImplementationShould.cs, scripts/package.json, scripts/package-lock.json.
Context files read in full: ChangeRealmPromptController.cs, ChangeRealmPromptController.Params.cs, ChangeRealmPromptPlugin.cs, DynamicWorldContainer.cs (composition-root callback at L719–729), DeepLinkHandleImplementation.cs. Searched all callers of TryTeleportTo, ChangeRealmAsync, ChangeRealmPromptController, and IRestrictedActionsAPI across the repo.
STEP 2 — Root-cause check
PASS. This PR adds a genuine new feature: SDK scenes can now request teleportation to a specific parcel in another realm via the TeleportTo restricted action. The diff extends TeleportToRequest (protocol) with an optional realm field, and the implementation routes realm-bearing requests through the pre-existing ChangeRealmPromptController consent flow. No symptom-masking or workaround patterns.
STEP 3 — Design & integration
PASS. No new long-lived units are introduced. The change adds a routing branch inside the existing RestrictedActionsAPIImplementation.TryTeleportTo method, reusing:
ChangeRealmAsynchelper — already existed in the same class (used byTryChangeRealm); this PR adds theVector2Int? position = nullparameter to thread the optional parcel through.ChangeRealmPromptController.Params.Position— the struct already had this optional field (used byDeepLinkHandleImplementation.ShowRealmChangePromptAsync).Action<string, Vector2Int?> changeRealmCallback— the composition-root callback inDynamicWorldContainer(L719–729) already branches onposition.HasValueto callChatTeleporter.TeleportToRealmAsyncwith the parcel or fall back to the chat-command route.
The routing logic lives in the implementation class (not the wrapper), consistent with TryChangeRealm and every other Try* method in the same file. RestrictedActionsAPIWrapper.TeleportTo stays a pure JS↔C# type-conversion shim — not a same-layer bridge anti-pattern.
Lifecycle / teardown: ChangeRealmPromptController is a long-lived singleton registered once in ChangeRealmPromptPlugin, reused per ShowAsync call. No new subscriptions, event hookups, or disposable resources are introduced.
Call-site sweep: All three callers of the old TryTeleportTo(Vector2Int) (RestrictedActionsAPIWrapper.TeleportTo, UserActionsWrapper.RequestTeleport, and the interface declaration) were consistently updated to the new (Vector2Int?, string?) signature. No stale callers remain.
STEP 4 — Member audit
| Member | Consumers | Notes |
|---|---|---|
IRestrictedActionsAPI.TryTeleportTo(Vector2Int?, string?) |
RestrictedActionsAPIWrapper.TeleportTo, UserActionsWrapper.RequestTeleport, tests |
All updated consistently |
RestrictedActionsAPIImplementation.ChangeRealmAsync(string, string, Vector2Int?) |
TryTeleportTo (with position), TryChangeRealm (without — uses default null) |
Optional param avoids breaking existing caller |
No single-use accessor issues. No absent-≠-false confusion — both coords and realm are genuinely nullable, and the method correctly rejects the both-absent case.
STEP 5 — Line-level review
See inline comments below. Summary:
- 0 P0 (blockers)
- 0 P1 (major)
- 3 P2 (minor, non-blocking)
STEP 6 — Complexity
SIMPLE. Touches 5 hand-written files + 1 generated + 2 package files. Straightforward feature addition that reuses existing consent-prompt infrastructure. Does not modify ECS systems, async patterns, plugins, containers, or assembly structure.
STEP 7 — QA assessment
QA_REQUIRED: YES. Modifies runtime code affecting teleportation behavior — the new realm+coords routing and the existing coords-only path must be manually verified in a live world.
STEP 8 — Non-blocking warnings
None. Main scene not modified.
Security review
Consent bypass: Not found. Both branches of TryTeleportTo route exclusively through user-facing consent prompts (TeleportPromptController or ChangeRealmPromptController). The prompt only fires the realm change after the user clicks Approve.
Realm string sanitization: Adequate, unchanged. ChangeRealmPromptController.OnViewInstantiated disables richText (SEC-003) and DestinationHostFor strips userinfo/path/query to prevent consent-prompt spoofing (SEC-004).
JS bridge input handling: Consistent with existing sibling calls. Number() coercion for coords, direct string pass-through for realm (same pattern as changeRealm handler).
Injection risk: None identified. Coordinates are strictly int/Vector2Int. Realm string display is sanitized; realm string handling for the actual network connection is pre-existing, unchanged code.
@dcl/protocol bump: 1.0.0-33874985952.commit-4f4e0ab → 1.0.0-34381335718.commit-3c838bb. Build/codegen dependency only (scripts/package.json), not shipped in the Unity client. Integrity hash updated consistently. The transitive protobufjs advisories are pre-existing across all @dcl/protocol versions (not introduced by this PR) and only execute during offline code generation, never in the shipped client.
DEPENDENCY_REVIEW: PASS
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Adds a routing branch to TryTeleportTo and threads an optional parcel through the pre-existing ChangeRealmAsync / ChangeRealmPromptController consent flow — no new systems, plugins, or async patterns.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — feat: sdk teleportTo realm coords support
Inspected commits: base dev → head ab11728e (re-review after follow-up fixes)
STEP 1 — Scope
9 files changed (5 hand-written C#, 1 JS bridge, 1 auto-generated protocol C#, 2 package manifests). Inspected full context of all changed files plus surrounding files: ChangeRealmPromptController + Params, ChangeRealmPromptPlugin, DynamicWorldContainer (callback wiring), DeepLinkHandleImplementation, and ChatTeleporter.TeleportToRealmAsync. Searched consumers of TryTeleportTo — 3 callers: RestrictedActionsAPIWrapper, UserActionsWrapper, and tests.
STEP 2 — Root-cause check
PASS. This is a protocol-driven feature addition: the SDK's TeleportTo restricted action gains an optional realm field (and makes worldCoordinates optional). The diff implements the new routing — realm-present requests go through the existing change-realm consent prompt; coords-only requests keep the existing teleport prompt. This is a genuine feature, not a symptom fix.
STEP 3 — Design & integration
PASS. No new long-lived units introduced. The change extends the existing TryTeleportTo method signature and reuses the established ChangeRealmAsync → ChangeRealmPromptController → changeRealmCallback → ChatTeleporter.TeleportToRealmAsync pipeline.
- The
ChangeRealmPromptController.Paramsstruct already had thePositionfield (prior PR). - The callback in
DynamicWorldContainer(lines 719–728) already branches onposition.HasValue, routing toChatTeleporter.TeleportToRealmAsync(realmUrl, position.Value, …)with position or the chat-command path without. - The
ChangeRealmAsyncprivate method gains an optionalVector2Int? positionparameter — backward-compatible with the existingTryChangeRealmcaller (which passes no position).
No lifecycle duplication, no per-frame reconciliation, no new persistent state. Clean integration into the existing architecture.
STEP 4 — Member audit
| Member | Consumers | Status |
|---|---|---|
TryTeleportTo(Vector2Int?, string?) |
RestrictedActionsAPIWrapper, UserActionsWrapper, tests (3) |
Clean — routes to two existing async paths based on parameters |
ChangeRealmAsync(string, string, Vector2Int?) |
TryTeleportTo (with position), TryChangeRealm (default null) (2) |
Clean — optional parameter with sane default |
No single-use intermediates, no re-derived state, no absent-vs-false conflation.
STEP 5 — Line-level review
No new findings. All P2 issues identified in the prior review (head 141ff48) have been addressed by the follow-up commits:
| Prior finding | Addressed in |
|---|---|
Comment narrating external behavior (RestrictedActionsAPIImplementation.cs) |
cf54ace — trimmed to describe what the annotated code does |
Missing negative assertion in TeleportToRealmDefaultSpawnWithoutCoordinates |
cf54ace + ab11728 — added DidNotReceive for TeleportPromptController (type param fixed in ab11728) |
| Incomplete XML doc (both-absent case) | cf54ace — appended "both absent is rejected" |
Interface param name mismatch newCoords → coords |
8af11ed — aligned to match implementation |
| JS bridge comment narrating C# behavior | 8af11ed — trimmed to "Unpack optional fields and forward to the C# bridge" |
Partial-coords edge case (RestrictedActionsAPIWrapper.TeleportTo): if only x or only y is non-null, the wrapper maps to null coords rather than surfacing a malformed call. As noted in the prior review, this is academic — protobuf Vector2 delivers both or neither, and the JS bridge mirrors that. The current handling (treat partial as absent) is the safest default. No code change needed.
STEP 6 — Complexity
SIMPLE. Straightforward extension of an existing method signature with routing logic. Does not touch ECS systems/components/queries, async patterns, plugin registration, or any complex subsystem.
STEP 7 — QA assessment
YES. Changes affect runtime teleport behavior — user-facing UI (consent prompts) and navigation.
STEP 8 — Non-blocking warnings
None. No main scene changes.
Security Review
Consent flow: Preserved. All realm-carrying teleport requests route through ChangeRealmAsync → ChangeRealmPromptController, which shows a consent prompt. The controller disables richText on both message and realm labels (SEC-003/034/050) and strips userinfo from URL realms via DestinationHostFor (SEC-004). The sceneStateProvider.IsCurrent gate is unchanged.
Input validation: string.IsNullOrEmpty(realm) correctly handles both null and empty-string realm. The JS bridge (RestrictedActions.js:57-61) correctly maps undefined to null before crossing into C#. RestrictedActionsAPIWrapper uses HasValue checks for nullable int? parameters.
Supply chain — @dcl/protocol bump:
| Field | Value |
|---|---|
| Package | @dcl/protocol |
| Old version | 1.0.0-33874985952.commit-4f4e0ab |
| New version | 1.0.0-34381335718.commit-3c838bb |
| Publisher | decentraland (npm) |
| Integrity | sha512-0QUcs/F1K60GmB9Qnc... (lockfile verified) |
| Scope | Build-time codegen only — not shipped in the player |
The transitive protobufjs vulnerability (GHSA-h755-8qp9-cq85 and others) is pre-existing — identical npm audit output on the dev baseline. Not introduced by this PR.
No binary, assembly, plugin, .meta, .asmdef, or .asmref changes.
DEPENDENCY_REVIEW: PASS
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Extends an existing restricted-action method signature with an optional realm parameter and routing logic; no ECS, async, or subsystem changes
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
2 participants
optionaluse of coordiantes in theTeleportTorestricted action.QA TEST STEPS
Use the build from this PR to enter the ZONE/SEPOLIA world
sdk7testscenes.dcl.ethat position1,2and confirm: