feat: send wearable links from the client to the shop - #9672
feat: send wearable links from the client to the shop#9672juanmahidalgo wants to merge 1 commit into
Conversation
🚦 CI StatusBuild skipped — no changes detected under Warnings not reduced: 13676 => 13676 — remove at least 1 warning to merge. Warnings/errors in files changed by this PR (10)All Unity tests passed ✅
|
|
Slack notification sent to #explorer-ext-contributions for external review. |
decentraland-bot
left a comment
There was a problem hiding this comment.
STEP 2 — Root-cause check
PASS. Wearable entry points sent users to the legacy marketplace; they should land on the Shop, which is where wearables and emotes are sold. The diff fixes this directly by changing the URL destination and route pattern.
STEP 3 — Design & integration
PASS. No new long-lived units, no lifecycle changes, no persistent state. Pure URL-routing change across three existing classes.
Teardown trace: OnOpenShop is wired via AddListener in the constructor and cleaned up via RemoveAllListeners() in Dispose() — unchanged by this PR. GetShopLink is a stateless extension method with no subscriptions or resources.
Pre-existing duplication: PhotoDetailUtility.GetShopLink and EquippedItemsPassportModuleController.GetMarketplaceLink (private) now implement identical URN-parsing + /item/{contract}/{id} logic against DecentralandUrl.ShopLink. The extension method is the better home; the passport controller's private copy could call it instead. This duplication predates this PR.
STEP 4 — Member audit
| Member | Consumers | Verdict |
|---|---|---|
GetShopLink (public extension, renamed) |
1 (EquippedWearableController) |
OK — centralizes shop-link derivation for IWearable; single use acceptable for an extension method |
OnOpenShop (private, renamed) |
1 (click listener) | OK — callback |
shopSourceParam (private readonly, renamed) |
1 (OnOpenShop) |
OK — reusable URL parameter |
STEP 5 — Line-level review
Two P2 findings (see inline comments with suggestion blocks). No P0 or P1 issues.
Security review: No security issues found. URLs are built from server-controlled IDecentralandUrlsSource, not user input. Contract/item-ID validation is preserved. No secrets, PII, or open-redirect risk.
STEP 6 — Complexity assessment
SIMPLE. 3 files, ~30 lines. URL destination swap — no ECS, async, plugin, or system changes.
STEP 7 — QA assessment
QA_REQUIRED: YES. User-facing navigation changes — backpack and photo-detail buy buttons now open different URLs. Manual verification needed per the PR test plan.
STEP 8 — Non-blocking warnings
None. Main scene not modified.
STEP 9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: URL routing change across backpack and photo-detail controllers — no ECS, async, or architectural changes
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by juanmahidalgo via GitHub
| // The Shop and not the marketplace: this button lives in the avatar section, so what it is asked for is | ||
| // always wearables or emotes, and those are what the Shop sells. LAND keeps pointing at the marketplace | ||
| // elsewhere — the Shop carries no parcels or estates. | ||
| private void OnOpenShop() |
There was a problem hiding this comment.
[P2] Over-explanatory comment (CLAUDE.md §11 — comments must explain what the annotated code does, not narrate caller/external behavior). OnOpenShop + DecentralandUrl.ShopLink already communicate the destination. The comment narrates the avatar section's UI content, what the Shop sells, and what happens with LAND — all external to this method.
| // The Shop and not the marketplace: this button lives in the avatar section, so what it is asked for is | |
| // always wearables or emotes, and those are what the Shop sells. LAND keeps pointing at the marketplace | |
| // elsewhere — the Shop carries no parcels or estates. | |
| private void OnOpenShop() | |
| private void OnOpenShop() |
| /// The Shop and not the marketplace: the subject here is always a wearable, which is what the Shop | ||
| /// sells and prices in USD. LAND still belongs to the marketplace — the Shop carries no parcels. | ||
| /// </para> | ||
| /// <para> | ||
| /// Note the route differs, so this is not a host swap: the marketplace addresses an item as | ||
| /// <c>/contracts/{contract}/items/{id}</c> and the Shop as <c>/item/{contract}/{id}</c>. Keeping the | ||
| /// old shape against the new host is a 404, not a redirect. | ||
| /// </para> | ||
| /// </summary> | ||
| public static string GetMarketplaceLink(this IWearable wearable, IDecentralandUrlsSource decentralandUrlsSource) | ||
| public static string GetShopLink(this IWearable wearable, IDecentralandUrlsSource decentralandUrlsSource) |
There was a problem hiding this comment.
[P2] Verbose XML doc (CLAUDE.md §11). The first <para> narrates business context about what the Shop sells — external to this method. The route-shape note is genuinely useful (it prevents reverting to the marketplace path pattern), but can be condensed into the summary line.
| /// The Shop and not the marketplace: the subject here is always a wearable, which is what the Shop | |
| /// sells and prices in USD. LAND still belongs to the marketplace — the Shop carries no parcels. | |
| /// </para> | |
| /// <para> | |
| /// Note the route differs, so this is not a host swap: the marketplace addresses an item as | |
| /// <c>/contracts/{contract}/items/{id}</c> and the Shop as <c>/item/{contract}/{id}</c>. Keeping the | |
| /// old shape against the new host is a 404, not a redirect. | |
| /// </para> | |
| /// </summary> | |
| public static string GetMarketplaceLink(this IWearable wearable, IDecentralandUrlsSource decentralandUrlsSource) | |
| public static string GetShopLink(this IWearable wearable, IDecentralandUrlsSource decentralandUrlsSource) | |
| /// Builds the Shop link for a wearable worn in a photo. The route is | |
| /// <c>/item/{contract}/{id}</c>, not the marketplace's <c>/contracts/{contract}/items/{id}</c>. |
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — feat: send wearable links from the client to the shop
Step 2 — Root-cause check: PASS
The PR correctly identifies that wearable/emote entry points still send users to the legacy marketplace instead of the Shop. The fix directly addresses the cause by swapping the URL source (DecentralandUrl.Market → DecentralandUrl.ShopLink) and updating the route pattern (/contracts/{contract}/items/{id} → /item/{contract}/{id}). This is a correct root-cause fix, not a symptom workaround.
Step 3 — Design & integration: PASS
No new long-lived units introduced. The changes are surgical URL redirections in existing controllers and a utility extension method. No lifecycle, ECS, or architectural changes.
view.marketplaceButton(serialized Unity field) is correctly left unrenamed — renaming it would break Unity serialization references.- The sidebar's marketplace button is deliberately unchanged — the Shop carries no LAND. Sound product boundary.
DecentralandUrl.ShopLinkalready exists inDecentralandUrlsSource.csand is used by the passport'sEquippedItemsPassportModuleController. No new enum members needed.
Step 4 — Member audit: PASS
shopSourceParam— single consumer (OnOpenShop). Legitimate field for URL parameter reuse.OnOpenShop()— wired toview.marketplaceButton.onClick; single-use private handler. Correct and appropriate.GetShopLink(extension method) — 1 consumer (EquippedWearableController.BuyWearableButtonClicked). Public extension, appropriate scope.
No single-use-merge, absent≠false, or redundant-guard issues.
Step 5 — Line-level findings
3 findings, all P2. See inline comments with suggestion blocks.
Pre-existing duplication (non-blocking observation): EquippedItemsPassportModuleController.GetMarketplaceLink(string id) (line 436) contains identical URL-building logic — same DecentralandUrl.ShopLink, same /item/{0}/{1} pattern, same URN-parsing and validation. It also still carries the old name GetMarketplaceLink despite already pointing at the Shop. This PR was a natural consolidation point; consider unifying via the existing GetShopLink extension method in a follow-up.
Security review: No issues found
URL construction uses a first-party domain from DecentralandUrl.ShopLink. Contract validated with 0x prefix check, item validated as integer. Invalid URNs safely return empty string. No secrets, no open redirect, no injection vectors.
CI status
Edit-mode tests and rsp-files checks show as cancelled (not failed from this PR's changes). Approval checks pending (expected for a new PR).
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: URL redirection across 3 files — no ECS, async, lifecycle, or architectural changes.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by unknown via Slack
| // Still "backpack" even though the destination is now the Shop: this names where the click came FROM, | ||
| // and keeping it is what lets the web side see this traffic move from the marketplace over to the Shop. | ||
| private readonly URLParameter shopSourceParam = new ("utm_source", "backpack"); |
There was a problem hiding this comment.
[P2] Comment narrates external behavior (CLAUDE.md §11: "A comment must state only what the annotated code itself does or guarantees, never what callers or upper layers will do"). The second line describes what "the web side" does with the UTM parameter — that behavior is external and could change independently, making the comment stale.
| // Still "backpack" even though the destination is now the Shop: this names where the click came FROM, | |
| // and keeping it is what lets the web side see this traffic move from the marketplace over to the Shop. | |
| private readonly URLParameter shopSourceParam = new ("utm_source", "backpack"); | |
| // Identifies the originating surface, not the destination — value stays "backpack" across redirects. | |
| private readonly URLParameter shopSourceParam = new ("utm_source", "backpack"); |
| // The Shop and not the marketplace: this button lives in the avatar section, so what it is asked for is | ||
| // always wearables or emotes, and those are what the Shop sells. LAND keeps pointing at the marketplace | ||
| // elsewhere — the Shop carries no parcels or estates. |
There was a problem hiding this comment.
[P2] The second and third lines narrate behavior of other code paths ("LAND keeps pointing at the marketplace elsewhere") — this is external behavior per CLAUDE.md §11. The first clause, that the button's context is always wearables/emotes, is a valid guarantee of the annotated method.
| // The Shop and not the marketplace: this button lives in the avatar section, so what it is asked for is | |
| // always wearables or emotes, and those are what the Shop sells. LAND keeps pointing at the marketplace | |
| // elsewhere — the Shop carries no parcels or estates. | |
| // Opens the Shop: this button lives in the avatar section, so the context is always wearables or emotes. |
| /// The Shop and not the marketplace: the subject here is always a wearable, which is what the Shop | ||
| /// sells and prices in USD. LAND still belongs to the marketplace — the Shop carries no parcels. | ||
| /// </para> | ||
| /// <para> | ||
| /// Note the route differs, so this is not a host swap: the marketplace addresses an item as |
There was a problem hiding this comment.
[P2] The first <para> narrates the Shop's product scope ("LAND still belongs to the marketplace") — external behavior per CLAUDE.md §11 that could change independently of this method. The summary line already establishes this builds a Shop link for a wearable. Remove this paragraph; the second <para> (route difference) documents what the method itself does and is the valuable part.
| /// The Shop and not the marketplace: the subject here is always a wearable, which is what the Shop | |
| /// sells and prices in USD. LAND still belongs to the marketplace — the Shop carries no parcels. | |
| /// </para> | |
| /// <para> | |
| /// Note the route differs, so this is not a host swap: the marketplace addresses an item as | |
| /// Builds the Shop link for a wearable worn in a photo. |
|
Fixed in a separate PR, closing |
The client still sends people to the legacy marketplace when what they are after is a wearable. The Shop is
where wearables and emotes are sold and priced in USD, so those entry points should land there. LAND is
unchanged and stays on the marketplace — the Shop carries no parcels or estates.
The passport's equipped-item link already points at the Shop; this brings the remaining wearable entry
points in line with it.
Changes
section, so what it is asked for is always a wearable or an emote.
swap: the marketplace addresses an item as
/contracts/{contract}/items/{id}and the Shop as/item/{contract}/{id}, so keeping the old shape against the new host would 404. Verified against theShop's routes.
GetMarketplaceLinkrenamed toGetShopLinkso the name matches where it goes.utm_source=backpackis kept as-is. It names where the click came FROM, not where it lands, and keeping itis what makes the shift in destination visible on the web side.
Deliberately NOT changed
currently offers no way through to LAND. Repointing it would strand anyone who opened it looking for a
parcel. That needs a product decision first, plus either a link from the Shop to the marketplace or a
second entry in the sidebar.
GoShoppingWithMarketplaceCredits. It looks like it should move, and it must not: marketplace creditsonly spend in the legacy marketplace, so sending that button to the Shop would drop the user somewhere
their credits do not work.
item.url), so its shape isserver-controlled and cannot be rewritten safely from here. It could be derived from the item's
urninstead, which is worth doing separately.
Test plan
decentraland.org/shop?utm_source=backpackdecentraland.org/shop/item/<contract>/<id>and the pageresolves (not a 404)