feat: let scenes offer marketplace items for credits - #9725
feat: let scenes offer marketplace items for credits#9725juanmahidalgo wants to merge 10 commits into
Conversation
🚦 CI Status
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. Warnings not reduced: 13156 => 13162 — remove at least 7 warnings to merge. Warnings/errors in files changed by this PR (15)All Unity tests passed ✅
|
decentraland-bot
added
the
ext-contribution
|
Slack notification sent to #explorer-ext-contributions for external review. |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review: feat: let scenes offer marketplace items for credits
STEP 2 — Root-cause check: PASS ✅
This is a new feature, not a bug fix. The diff adds a second entry point to the existing credit-purchase pipeline so SDK7 scenes can offer marketplace items in-world. The implementation reuses the existing modal, purchase service, and analytics events rather than duplicating any of them. No symptom-masking detected.
STEP 3 — Design & integration: PASS ✅
Owner search for SceneItemPurchaseBridge:
The lifecycle of the purchase flow is owned by CreditPurchasePlugin (created in InitializeAsync, torn down in Dispose()). The consumer is RestrictedActionsAPIImplementation, which lives in the DCL.CrdtEcsBridge assembly — constructed during scene runtime setup, before the plugin initializes, and in a different assembly. The static bridge pattern mirrors the existing CreditsFeatureAccess.Instance convention used for the same cross-assembly late-binding reason. Register/Unregister are correctly placed at the plugin's lifecycle boundaries.
Files searched: CreditPurchasePlugin.cs (owner), RestrictedActionsAPIImplementation.cs (consumer), CreditsFeatureAccess.cs (precedent), DynamicWorldContainer.cs (composition root).
CreditPurchasePlugin implementing ISceneItemPurchaseFlow directly is correct — the plugin already owns the modal controller, shop API client, image controller, and all required dependencies. Extracting a separate service would be a bridge-on-the-same-layer anti-pattern (CLAUDE.md §11).
Event subscription teardown trace:
PurchaseCompleted += OnCompleted→PurchaseCompleted -= OnCompleted(infinallyblock, line 244) ✅PurchaseFailed += OnFailed→PurchaseFailed -= OnFailed(infinallyblock, line 245) ✅PurchaseCancelled += OnCancelled→PurchaseCancelled -= OnCancelled(infinallyblock, line 246) ✅
All subscriptions are properly cleaned up in a finally block — no leak.
SceneItemPurchaseBridge.Register / Unregister:
Register(this)at end ofInitializeAsync(line 125) ✅Unregister()at start ofDispose()(line 80) ✅
STEP 4 — Member audit
| Member | Consumers | Verdict |
|---|---|---|
SceneItemPurchaseBridge.IsAvailable |
0 | ❌ Dead code — see comment below |
SceneItemPurchaseBridge.OpenAsync |
1 (RestrictedActionsAPIImplementation) |
✅ Justified — the bridge is the cross-assembly seam |
SceneItemPurchaseBridge.Register |
1 (CreditPurchasePlugin.InitializeAsync) |
✅ |
SceneItemPurchaseBridge.Unregister |
1 (CreditPurchasePlugin.Dispose) |
✅ |
ISceneItemPurchaseFlow |
1 impl (CreditPurchasePlugin) |
✅ Justified for assembly decoupling (interface lives in CrdtEcsBridge, impl in PluginSystem) |
HasRecentUserGesture() |
2 (TryOpenExplorerUi, TryOpenItemPurchaseAsync) |
✅ DRY extraction |
CreditPurchaseModalControllerParams.SOURCE_SDK_SCENE |
1 | ✅ Named constant |
STEP 5 — Line-level findings
See inline comments. All findings are P2.
Security review: PASS ✅
- URN-only input: the scene supplies only a URN; price is resolved server-side from the catalog. No scene-supplied price or transaction data. ✅
- Coarse verdict: a purchase that could not be completed is always
OipFailed, never a reason. This prevents balance probing via differential pricing. ✅ - Portable experience rejection: PX scenes are explicitly refused (
sceneData.IsPortableExperience()check), preventing phishing from scenes that follow the player. Stricter thanopenExplorerUi. ✅ - User gesture gate: requires a recent pointer gesture within
USER_GESTURE_WINDOW_TICKS, preventing autonomous purchase prompts. ✅ - Current-scene gate:
sceneStateProvider.IsCurrentprevents background scenes from raising purchases. ✅ - Event matching:
ReferenceEquals(dto, listing)ensures each event belongs to the listing this call opened, preventing cross-purchase interference. ✅ - No secrets or credentials in the diff. ✅
STEP 6 — Complexity: COMPLEX
Touches plugin wiring, async/UniTask patterns, MVC modal lifecycle, cross-assembly concerns, JS module bridge, and protobuf protocol. 18 files changed, +1648 −105.
STEP 7 — QA: YES
Modifies runtime code: new user-facing purchase flow triggered from SDK scenes, shared modal with passport, thumbnail loading, and purchase verdict pipeline.
STEP 8 — Non-blocking warnings
None. Main.unity is not modified.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches plugin wiring (CreditPurchasePlugin + DynamicWorldContainer), async/UniTask purchase flow, MVC modal lifecycle, cross-assembly static bridge, JS module system, and protobuf protocol generation.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by unknown via Slack
Co-authored-by: Muna <44584806+decentraland-bot@users.noreply.github.com> Signed-off-by: davidejensen <davidejensen@live.it>
…ementation/RestrictedActions/SceneItemPurchase.cs Co-authored-by: Muna <44584806+decentraland-bot@users.noreply.github.com> Signed-off-by: davidejensen <davidejensen@live.it>
davidejensen
removed
the
ext-contribution
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Review: feat: let scenes offer marketplace items for credits
STEP 1 — Context & scope
Loaded CLAUDE.md, repo review instructions, and the relevant subsystem docs. The diff touches 18 files across 6 areas: restricted-actions API layer (CrdtEcsBridge), scene runtime wrapper, credit purchase plugin, protocol-generated protobuf code, the JS module bridge, and the protocol package dependency. The hand-authored C# changes span ~5 files; the rest are generated code and package manifests.
Surrounding context inspected: RestrictedActionsAPIWrapper.cs (full file — Dispose() pattern for CTS fields), RestrictedActionsAPIImplementation.cs (full file — gate-check patterns, HasRecentUserGesture), CreditPurchasePlugin.cs (full file from PR head — plugin lifecycle, OpenAsync, thumbnail cache, event wiring).
STEP 2 — Root-cause check
Feature PR — adds a new restricted action (openItemPurchase). Not a bug fix. PASS.
STEP 3 — Design & integration
Static bridge pattern (SceneItemPurchaseBridge):
The bridge follows the established CreditsFeatureAccess pattern for crossing the scene-runtime ↔ plugin assembly boundary. The scene runtime (RestrictedActionsAPIImplementation, CrdtEcsBridge assembly) is constructed per-scene before the global CreditPurchasePlugin (PluginSystem assembly) initializes. A static bridge is the canonical late-binding pattern for this construction order.
Lifecycle owners searched:
CreditPurchasePlugin.InitializeAsync→ callsSceneItemPurchaseBridge.Register(this)— found.CreditPurchasePlugin.Dispose→ callsSceneItemPurchaseBridge.Unregister()— found.- The bridge itself is stateless (no persistent collections, no per-frame work). It does not reconcile, poll, or scan.
PASS — the bridge is not a new long-lived unit; it’s a service locator with proper registration/unregistration matching the plugin lifecycle.
Teardown / consumption trace:
| Opener | Mirror | ✓ |
|---|---|---|
SceneItemPurchaseBridge.Register(this) |
SceneItemPurchaseBridge.Unregister() in Dispose() |
✓ |
PurchaseCompleted += OnCompleted |
-= OnCompleted in finally |
✓ |
PurchaseFailed += OnFailed |
-= OnFailed in finally |
✓ |
PurchaseCancelled += OnCancelled |
-= OnCancelled in finally |
✓ |
thumbnailsByUrl (textures + sprites) |
Disposed and destroyed in Dispose() |
✓ |
openItemPurchaseCancellationToken (wrapper) |
NOT cancelled/disposed in Dispose() |
Note on wrapper CTS lifecycle: openItemPurchaseCancellationToken follows the same pattern as movePlayerToCancellationToken and the other CTS fields in RestrictedActionsAPIWrapper — none are cancelled in Dispose(). The disposeCts from JsApiWrapper detaches the JS promise, but the underlying ct passed to TryOpenItemPurchaseAsync stays live. For the existing short-lived actions (move, emote) this is benign. For a purchase flow — which fetches a listing, loads a thumbnail, and shows a modal — the flow could continue after the scene unloads. Worth considering in a follow-up: linking the per-call CTS to disposeCts so the purchase flow cancels when the scene is torn down.
STEP 4 — Member audit
| Member | Consumers | Verdict |
|---|---|---|
HasRecentUserGesture() (private) |
2 — TryOpenExplorerUi, TryOpenItemPurchaseAsync |
Good extraction. Shared logic, well-named. |
ISceneItemPurchaseFlow.OpenAsync |
1 impl (CreditPurchasePlugin), 1 consumer (bridge) |
Interface justified by assembly boundary. |
SceneItemPurchaseBridge.Register |
1 consumer (InitializeAsync) |
Registration endpoint. |
SceneItemPurchaseBridge.Unregister |
1 consumer (Dispose) |
Cleanup endpoint. |
SceneItemPurchaseBridge.OpenAsync |
1 consumer (TryOpenItemPurchaseAsync) |
Forwarding call. |
SOURCE_SDK_SCENE |
1 consumer (OpenAsync) |
Named constant for analytics source. |
No single-use predicates re-deriving existing logic, no absent ≠ false/null issues.
STEP 5 — Line-level findings
See inline comments. Two P2 findings:
Texture2DRefleak on cancellation betweenLoadTextureAsyncand cache insertion- Double blank line in
SceneItemPurchase.cs
Security assessment
The security design is solid:
- URN-only input prevents price manipulation — the client resolves the price from the catalog server-side.
- Coarse verdict (
Purchased/Dismissed/Failed) prevents balance probing — a scene cannot distinguish insufficient credits from other failures. - Portable experience rejection prevents phishing — a PX following the player everywhere cannot raise purchase confirmations in arbitrary parcels.
- User gesture gate prevents programmatic spam — requires a pointer input within one tick.
ReferenceEquals(dto, listing)check prevents cross-purchase event contamination — a scene cannot observe a passport purchase it never initiated.- Sticky terminal verdicts prevent the
PurchaseCancelled-after-PurchaseFailedbug from misreporting the outcome.
No secrets exposed, no injection risks (URN parsed by TryParseCollectionItem), no auth bypass paths.
STEP 6 — Complexity
COMPLEX — async flow across CrdtEcsBridge ↔ PluginSystem assemblies, modal event wiring with sticky verdicts, new protocol types, plugin lifecycle extension.
STEP 7 — QA assessment
YES — runtime code that triggers a user-facing purchase modal from scenes. Requires manual testing of the purchase flow, modal interaction, and edge cases (modal already visible, scene unload during flow).
STEP 8 — Non-blocking warnings
None. Main scene not modified.
STEP 9 — Verdict
Overall the PR is well-designed and carefully implemented. The security properties are strong, the async error handling is thorough, the sticky verdict logic correctly handles the PurchaseCancelled-after-PurchaseFailed edge case, and the HasRecentUserGesture extraction is a clean DRY refactor. The thumbnail cache with URL-keyed retention is a good solution to the ref-count issue.
Two P2 findings in the inline comments — both are low-impact improvements.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Async flow across CrdtEcsBridge ↔ PluginSystem assemblies, modal event wiring with sticky verdicts, new protocol types, plugin lifecycle extension.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by davidejensen via GitHub
|
PR #9725, run #31808353580 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Exception breakdown
Apple M1
|
3 participants
Depends on the protocol contract in decentraland/protocol#462.
What this does
Adds the
openItemPurchaserestricted action, so an SDK7 scene can ask the client to sell a marketplace item for credits in-world — a vending machine, a shop counter, a quest reward — and learn the outcome so the world can react.The scene supplies only the URN. The client resolves the price from the catalog, runs the same confirmation modal the passport already uses, signs and relays. Nothing about price, balance, credits or transactions crosses back — just a coarse verdict.
No new money code: the whole purchase pipeline is the existing one, reached from a second entry point.
Why it's shaped this way
Failed, never a reason. Reporting "insufficient credits" separately would let scene code probe a wallet's balance by offering items at different prices, and the client already tells the player what happened.openExplorerUi, which has no equivalent check.openExplorerUi: current scene + a user gesture within one tick. The gesture check was extracted intoHasRecentUserGesture()and is now shared by both actions.SceneItemPurchaseBridgeis static for the same reasonCreditsFeatureAccessis: the flow is one client-wide service whose owner initializes long after the scene runtime is built. It carries an interface (ISceneItemPurchaseFlow), so no assembly needs a new reference — and there are no asmdef changes in this PR.Notes for the reviewer
Five defects were found by running this against Amoy, not by reading it. Each has a comment at the site explaining the constraint, because none are locally obvious:
PersistentSetting,LoadTextureAsynccreates an entity in the global ECS world synchronously before its first await, and the modal is Unity UI. Hence theSwitchToMainThreadbefore any of it. The passport reaches all of this from a UI callback, so it never had to switch.Texture2DRefwhen its modal closed drove the texture's reference count negative. They are now kept keyed by url — which also bounds retention by distinct items rather than growing per offer.MVCManager.ShowAsyncreturns silently when its controller is not hidden. Without theViewHiddencheck, a busy modal was indistinguishable from the player dismissing the offer.Purchasedfrom a passport purchase it never showed — and hand over an item nobody paid for.PurchaseCancelled(it is only suppressed on success), which overwroteFailedwithDismissed— a broken purchase reported back as if the player had declined. Terminal verdicts are now sticky.Known cosmetic gap: the card's rarity frame and category icon stay unset, since those come from ScriptableObject mappings assigned per plugin in the Inspector. The modal skips them when null.
Follow-up
SceneItemPurchaseResultmirrorsOpenItemPurchaseResultfrom the proto by hand, with the values in the same order. It is not generated yet because this repo pins a protocol build offexperimental, while the proto change landed onmain: regenerating from a main-based build here would revert the experimental-only changes tocomms/rfc4,avatar_shapeandlight_source. Once a protocol bump carries the new type, swapping to it is a three-line change. That is what keeps this a draft.Test plan
How to test: