feat: in game shop - #9982
Conversation
🚦 CI StatusBuild failed! Check the logs to see what went wrong.
Warnings not reduced: 12025 => 12061 — remove at least 37 warnings to merge. Warnings/errors in files changed by this PR (63)Lint run · took 25m 43s 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 #33892386090. Full reportPR #9982, run #33892386090 Overall: 🟢 GPU average improved on Intel Core i5; GPU 1% worst improved on Intel Core i5 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.
Review — feat: in game shop
STEP 2 — Root-cause check
This PR adds an in-game shop as a new ISection in the explore panel, with a cart, multi-group checkout, and analytics. The change is a new feature, not a fix.
STEP 3 — Design & integration
PASS. The architecture follows established patterns:
-
ShopController implements
ISection— the same pattern used by Navmap, Backpack, Events, etc. It plugs intoExplorePanelController's section selector. Searched:ExplorePanelController.SetupExploreSectionsAsync,SectionSelectorController. The explore panel IS the lifecycle owner of its sections; ShopController belongs there. -
ShopCart is a new domain concept (in-memory shopping cart). No existing owner manages the cart lifecycle — it is created in
DynamicWorldContainer.CreateAsync, shared acrossExplorePanelPlugin(for the shop UI) andCreditPurchasePlugin(for the cart modal), and owned/disposed byCreditPurchasePlugin. Ownership is clear; no double-dispose. -
CreditsCartCheckoutService — new service handling the multi-group reserve→sign→settle flow. Created alongside ShopCart in DynamicWorldContainer, disposed by CreditPurchasePlugin.
-
UseCreditsExecutor — cleanly extracted from
CreditsPurchaseService. The signing, settlement polling, and credit-release logic that was previously inlined inCreditsPurchaseService.PurchaseInternalAsyncis now a reusable struct consumed by both the single-item flow and the cart checkout. Good consolidation — no code duplication. -
ShopCatalogService — TTL cache with in-flight coalescing, owned by ShopController. No lifecycle duplication.
Subscription/disposal trace (all confirmed):
- ShopController: 10 event subs in ctor, all unsubscribed in Dispose ✓
- ShopOverviewController: 5 event subs, all unsubscribed in Dispose ✓
- ShopCollectiblesController: 11 event subs, all unsubscribed in Dispose ✓
- ShopCart: 2 identity subs, unsubscribed in Dispose ✓
- ShopAnalytics: 7 subs, unsubscribed in Dispose ✓
- ShopCartAnalytics: 6 subs, unsubscribed in Dispose (via MVC decorator) ✓
- ShopCartModalController: subscribes OnViewShow, unsubscribes OnViewClose ✓
- All CancellationTokenSources have SafeCancelAndDispose paths ✓
STEP 4 — Member audit
| Member | Consumers | Status |
|---|---|---|
ShopItemCardModel.IsPrimary |
Presenter, CartLine, Model factories | Multi-use ✓ |
ShopItemCardModel.IsNotForSale |
Presenter, Overview, Collectibles | Multi-use ✓ |
ShopCart.Contains (3 overloads) |
Presenter, Cart, Overview | Multi-use ✓ |
ShopCatalogService.Invalidate |
ShopController | Single consumer, clear intent ✓ |
UseCreditsExecutor.ReleaseIntentAsync |
CreditsPurchaseService, CreditsCartCheckoutService | Multi-use ✓ |
ShopItemCardPresenter.PurchasesEnabled |
ShopController.Activate | Single setter, read-only by cards — acceptable |
No single-use-merge, absent-≠-false, or redundant-guard issues found.
STEP 5 — Line-level findings
Overall code quality is high. Tests cover the cart (ShopCartShould), checkout (CreditsCartCheckoutServiceShould), trade encoder (CreditsTradeEncoderShould), and API client (MarketplaceShopAPIClientShould). Naming, async patterns, error handling, and resource lifecycle follow CLAUDE.md conventions. Three P2 observations below.
Security review
No security issues found. No secrets committed, no injection surfaces, RandomSalt uses RandomNumberGenerator.Create (cryptographically secure), auth flows use the existing web3 identity system, all API data is server-sourced with proper deserialization.
STEP 6 — Complexity
COMPLEX — adds plugin/container wiring, new assemblies, async checkout flow, dependency injection changes across 100+ files.
STEP 7 — QA assessment
QA_REQUIRED: YES — user-facing shop UI, cart, animations, checkout flow, and runtime code.
STEP 8 — Non-blocking warnings
None. Main scene not modified.
STEP 9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Adds a full in-game shop (catalog, cart, multi-group checkout, analytics) with plugin/container wiring across multiple assemblies.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
| UniTaskCompletionSource<T>? inFlight = slot.InFlight; | ||
|
|
||
| if (inFlight == null) | ||
| { |
There was a problem hiding this comment.
[P2] Slot.Clear() does not discard the in-flight completion source. A fetch that started before Invalidate() (e.g. before a purchase settled) completes afterward and silently writes its result — so the "invalidated" data is replaced by pre-invalidation server state. The window is short in practice (settlement takes seconds, the fetch is long done), but the intent of invalidation is weakened.
Add a comment documenting this trade-off so future maintainers don't assume Invalidate guarantees a fresh fetch:
| { | |
| public void Clear() | |
| { | |
| Value = null; | |
| FetchedAtUtc = DateTime.MinValue; | |
| // InFlight is intentionally kept: a fetch already in progress will deliver near-current | |
| // data, and nulling the completion source would orphan callers awaiting it. The next | |
| // GetOrFetchAsync after the in-flight settles will see the stale FetchedAtUtc and re-fetch. | |
| } |
| private readonly MarketplaceShopAPIClient api; | ||
| private readonly TimeSpan ttl; | ||
| private readonly System.Random random = new (); | ||
| private readonly Slot<IReadOnlyList<ShopItemCardModel>> trending = new (); |
There was a problem hiding this comment.
[P2] ShopCatalogService holds mutable state (three Slot<T> instances, a System.Random) but its XML summary omits the threading contract. Per CLAUDE.md Step 5B: "A stateful class with caches or mutable fields whose XML summary omits its threading contract."
| private readonly Slot<IReadOnlyList<ShopItemCardModel>> trending = new (); | |
| /// <summary> | |
| /// The overview datasets (trending, new creations, outfits) behind a TTL cache with in-flight coalescing, so | |
| /// reopening the shop within the TTL is instant and concurrent openers share one request. A purchase | |
| /// invalidates everything: stock and prices moved. | |
| /// <para>Not thread-safe — all calls must happen on the Unity main thread.</para> | |
| /// </summary> | |
| public class ShopCatalogService |
| /// </summary> | ||
| public sealed class ShopCart : IDisposable | ||
| { | ||
| private readonly IWeb3IdentityCache identityCache; |
There was a problem hiding this comment.
[P2] ShopCart holds mutable collections and identity-tracking state but lacks a threading-contract note. Per CLAUDE.md Step 5B.
| private readonly IWeb3IdentityCache identityCache; | |
| /// <summary> | |
| /// The in-memory shopping cart, shared by the shop UI (adds) and the checkout (removes what settled). It | |
| /// belongs to the signed-in account: a different address signing in starts from an empty cart, signing out | |
| /// keeps it. | |
| /// <para>Not thread-safe — all calls must happen on the Unity main thread.</para> | |
| /// </summary> | |
| public sealed class ShopCart : IDisposable |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
In-Game Shop — Code Review
Summary
This PR introduces a complete in-game shop for the Unity Explorer:
- Shopping cart (
ShopCart) with identity-based ownership, quantity management, and stock-cap enforcement - Batch credits checkout (
CreditsCartCheckoutService) — group reservation → sequential sign/settle per group, with correct partial-failure handling - Marketplace API clients extended for unified listings, catalog items, outfits, and trending content
- Shop UI — overview page with carousels, collectibles grid with filters/search/pagination, category tree, and cart modal
- Analytics — shop and cart funnel events (Segment-compatible with the web shop)
- Plugin wiring in
CreditPurchasePluginandExplorePanelPlugin
The feature is well-structured with clean MVC separation. 49 unit tests across 7 test files cover the checkout flow, trade encoding, API client URL building, cart operations, catalog caching, card model construction, and query mapping.
Root-Cause Check
N/A — new feature, not a bugfix.
Design & Integration
Strengths:
- Two-phase checkout (reserve all → sign/settle) correctly handles partial failures — unsigned groups release their credits, broadcast groups conservatively keep them
Slot<T>TTL cache with in-flight coalescing inShopCatalogServiceprevents duplicate fetches- Object pooling in views (chips, category rows, cards) follows project performance conventions
- Event subscription/unsubscription is symmetric in all Dispose methods
ShopCartLinesnapshots the listing at add-time, isolating the cart from external mutations- Nonce sequencing via
nextMinNoncecorrectly prevents nonce reuse across sequential groups RandomSalt()usesRandomNumberGenerator(cryptographically secure)IsCheckoutInFlightas a plain bool is safe under UniTask's single-threaded cooperative model
Concerns (see inline comments):
- Several services created inside constructors rather than injected (DI conventions)
- Shared instances disposed by a single plugin — disposal order is load-bearing
Test Coverage
Good coverage of critical paths (49 test methods). Missing direct tests for UseCreditsExecutor (tested indirectly via checkout service mocks) and UI controllers (understandable for MonoBehaviour-heavy code).
Findings Summary
| Severity | Count | Category |
|---|---|---|
| P1 | 3 | Input validation in financial arithmetic |
| P1 | 2 | DI / lifecycle conventions |
| P2 | 5 | Defensive hardening |
Not in diff but worth noting (existing code exposed by new callers):
CreditsTradeEncoder.CeilToCentscastsBigIntegertointwithout overflow protection — extreme server values silently wrapCreditsTradeEncoder.UsdWeiToManaWeidivides byrate.Ratewithout a zero-guard on the public APICreditsTradeEncoder.RoundUpToWholeCreditdivides bycentsPerCreditwithout a zero-guard
Non-Blocking Warnings
ShopCreatorNameCacheaccumulates name mappings without eviction — acceptable for current scaleShopCart.RemoveAllis O(n×m) — acceptable for realistic cart sizesShopCollectiblesFilters.BuildAnalyticsSignatureallocates aStringBuilderper call — fine for filter-change frequency- The
lifetimeCtsvsuiCtcancellation split in Phase B is correct but the comment at line 262 ("buyer's cancellation is ignored") is misleading —uiCtIS still checked between groups (line 273); only in-flight signing/settlement ignores it
DEPENDENCY_REVIEW: PASS
REVIEW_RESULT: PASS
COMPLEXITY: L
COMPLEXITY_REASON: ~145 files, ~8600 lines of new C#; new in-game shop spanning cart, checkout, API clients, UI, analytics, and plugin wiring
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
| BigInteger authorizedCap = BigInteger.Parse(response.maxCreditedValue) | ||
| + CreditsTradeEncoder.UncreditedValue(response.maxCreditedValue, response.credit.availableAmount); |
There was a problem hiding this comment.
[P1 — Input Validation] BigInteger.Parse(response.maxCreditedValue) is called without validating that the server response field is non-null and numeric. A malformed response throws FormatException, caught as UnknownError rather than AuthorizationFailed. Validate immediately after deserialization; apply the same pattern to credit.amount, credit.availableAmount, and credit.signature.
| BigInteger authorizedCap = BigInteger.Parse(response.maxCreditedValue) | |
| + CreditsTradeEncoder.UncreditedValue(response.maxCreditedValue, response.credit.availableAmount); | |
| if (!BigInteger.TryParse(response.maxCreditedValue, out BigInteger maxCredited)) | |
| return AuthorizationFailure(group, CreditsPurchaseError.AuthorizationFailed, "Invalid maxCreditedValue from server"); | |
| BigInteger authorizedCap = maxCredited | |
| + CreditsTradeEncoder.UncreditedValue(response.maxCreditedValue, response.credit.availableAmount); |
| authorizedCap = BigInteger.Parse(request.MaxCreditedValue) | ||
| + CreditsTradeEncoder.UncreditedValue(request.MaxCreditedValue, credit.availableAmount); |
There was a problem hiding this comment.
[P1 — Input Validation] Same issue: BigInteger.Parse(request.MaxCreditedValue) can throw on malformed data after the credit has already been authorized. The intent gets released by the catch block, but the error surfaces as a generic exception rather than a typed failure.
| authorizedCap = BigInteger.Parse(request.MaxCreditedValue) | |
| + CreditsTradeEncoder.UncreditedValue(request.MaxCreditedValue, credit.availableAmount); | |
| if (!BigInteger.TryParse(request.MaxCreditedValue, out BigInteger maxCredited)) | |
| return new UseCreditsOutcome(CreditsPurchaseError.EncodingFailed, "Invalid MaxCreditedValue"); | |
| authorizedCap = maxCredited | |
| + CreditsTradeEncoder.UncreditedValue(request.MaxCreditedValue, credit.availableAmount); |
| public void Clear() | ||
| { | ||
| Value = null; | ||
| FetchedAtUtc = DateTime.MinValue; | ||
| } |
There was a problem hiding this comment.
[P2 — Race Condition] Clear() resets Value and FetchedAtUtc but leaves InFlight intact. If Invalidate() is called while a fetch is in progress, the completing fetch writes stale data back into Value/FetchedAtUtc, defeating the invalidation. This can happen when a purchase settles while the overview is loading.
| public void Clear() | |
| { | |
| Value = null; | |
| FetchedAtUtc = DateTime.MinValue; | |
| } | |
| public void Clear() | |
| { | |
| Value = null; | |
| FetchedAtUtc = DateTime.MinValue; | |
| InFlight = null; | |
| } |
| public static string BuildItemUrl(IDecentralandUrlsSource urlsSource, string contractAddress, string itemId) => | ||
| $"{urlsSource.Url(DecentralandUrl.ShopLink)}/item/{contractAddress}/{itemId}?utm_source=client"; |
There was a problem hiding this comment.
[P2 — Input Sanitization] contractAddress and itemId are embedded in the URL path without encoding. While these come from API responses (low risk), a compromised server could inject path traversal or query parameters.
| public static string BuildItemUrl(IDecentralandUrlsSource urlsSource, string contractAddress, string itemId) => | |
| $"{urlsSource.Url(DecentralandUrl.ShopLink)}/item/{contractAddress}/{itemId}?utm_source=client"; | |
| public static string BuildItemUrl(IDecentralandUrlsSource urlsSource, string contractAddress, string itemId) => | |
| $"{urlsSource.Url(DecentralandUrl.ShopLink)}/item/{Uri.EscapeDataString(contractAddress)}/{Uri.EscapeDataString(itemId)}?utm_source=client"; |
| cartCheckoutService.Dispose(); | ||
| shopCart.Dispose(); |
There was a problem hiding this comment.
[P1 — Lifecycle] ShopCart and ICreditsCartCheckoutService are shared with ExplorePanelPlugin/ShopController, but only CreditPurchasePlugin.Dispose() disposes them. If this plugin disposes first, the other consumers operate on disposed objects. Per CLAUDE.md §11, plugins should not own shared container-level lifecycles.
Consider moving disposal of these shared instances to DynamicWorldContainer (which created them), and remove the dispose calls here:
| cartCheckoutService.Dispose(); | |
| shopCart.Dispose(); | |
| // ShopCart and cartCheckoutService are shared — disposed by the container, not by this plugin. |
| catalog = new ShopCatalogService(api); | ||
| var creatorNames = new ShopCreatorNameCache(profileRepositoryWrapper); |
There was a problem hiding this comment.
[P1 — DI Convention] ShopCatalogService and ShopCreatorNameCache are created internally rather than injected. Their lifetimes are invisible to the composition root and cannot be substituted in tests. ShopCatalogService holds an in-flight coalescing cache whose lifetime couples to the controller — in-flight fetches may complete after the controller is disposed.
Consider creating these in the plugin/container and injecting them:
| catalog = new ShopCatalogService(api); | |
| var creatorNames = new ShopCreatorNameCache(profileRepositoryWrapper); | |
| // TODO: inject ShopCatalogService and ShopCreatorNameCache from the container | |
| catalog = new ShopCatalogService(api); | |
| var creatorNames = new ShopCreatorNameCache(profileRepositoryWrapper); |
| this.identityCache = identityCache; | ||
| this.creditsFeatureAccess = creditsFeatureAccess; | ||
| this.isFeatureEnabled = isFeatureEnabled; | ||
| executor = new UseCreditsExecutor(creditsAPIClient, metaTxRelayer, settlementPoller); |
There was a problem hiding this comment.
[P1 — DI Convention] UseCreditsExecutor is newed internally here and also in CreditsPurchaseService. Both services share the same three collaborators (creditsAPIClient, metaTxRelayer, settlementPoller) but create independent executor instances, hiding the dependency from tests. Create a single UseCreditsExecutor in the container and inject it into both services.
2 participants
Pull Request Description
What does this PR change?
Test Instructions
Steps (standard run):
metaforge explorer run XXXX # ← replace with this PR numberExpected result:
Steps (fresh account):
metaforge account create --clear metaforge explorer run XXXX # ← replace with this PR numberExpected result:
Automation (if applicable):
metaforge explorer test XXXXPrerequisites
Test Steps
Additional Testing Notes
Quality Checklist
Code Review Reference
Please review our Branch & PR Standards before submitting. It explains the automated review flow, QA/DEV approval requirements, and what each label does — especially useful for first-time contributors.