feat: new credits topup#9439
Merged
Merged
Conversation
Contributor
|
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. |
davidejensen
marked this pull request as ready for review
July 22, 2026 07:45
decentraland-bot
self-requested a review
July 22, 2026 07:46
decentraland-bot
left a comment
decentraland-bot
left a comment
Contributor
There was a problem hiding this comment.
PR Review: feat: add credits topup
STEP 2 — Root-cause check
PASS. This PR introduces a new feature (credits top-up via Stripe checkout), not a fix. The implementation directly addresses the stated goal: letting users buy credits from the client via the "+" icon in the credits balance or when purchasing an item without enough credits.
STEP 3 — Design & integration
PASS. New units introduced:
-
CreditsTopUpService— manages the async checkout → poll → credited flow. This is a standalone service (not ECS) created and disposed byCreditPurchasePlugin. The lifecycle it manages (Stripe checkout + order polling) is user-triggered and has no existing owner in the codebase. It does not duplicate any existing lifecycle. -
CreditsTopUpModalController— standard MVC controller followingControllerBase<TView, TParams>. Created inCreditPurchasePluginand registered withmvcManager. Follows the existing pattern (e.g.,CreditPurchaseModalController). -
CreditsTopUpAnalytics— analytics relay wired viaMVCManagerAnalyticsDecorator. Follows the existing pattern (e.g.,MarketplaceCreditsAnalytics).
All new units are created at the composition root (CreditPurchasePlugin) and disposed properly. No lifecycle duplication detected.
Teardown trace — all subscriptions matched:
StatusChanged +=in constructor →-=inDispose()✅RetryButton.onClick.AddListenerinOnViewShow→RemoveListenerinOnViewClose✅BuyButton.onClick.AddListenerinBindPackItems→RemoveAllListenersinOnViewClose✅- Analytics events
+=inCreditsTopUpAnalyticsconstructor →-=inDispose()✅ lifeCtscreated inOnViewShow→SafeCancelAndDisposeinOnViewClose✅cts/skipForegroundCtsin service →SafeCancelAndDisposeinDispose()✅
STEP 4 — Member audit
Public members reviewed:
ICreditsTopUpServiceinterface — 6 members, all consumed byCreditsTopUpModalControllerand tests. No single-use concerns.CreditsTopUpStatusfactory methods — each maps to a distinct stage. Clean value-type state machine.CreditPackCatalog.PACKS— static catalog, consumed by the controller and tests. Clearly documented as temporary (server-driven in future).- Analytics events on the controller (
BuyCreditsStarted,RedirectedToStripe, etc.) — each consumed byCreditsTopUpAnalytics. Clean relay pattern.
No absent-≠-false, single-use-merge, or re-derive issues found.
STEP 5 — Line-level findings
See inline comments below. All findings are P2 (minor).
STEP 6 — Security review
No security issues found. No hardcoded secrets, no PII in logs or analytics, server-signed requests for checkout, generic user-facing error messages, feature properly gated behind alfa-credits-topup flag.
STEP 7 — Consumer impact
UnityAppWebBrowser.OpenUrlMainThreadOnlychanged behavior (double-encoding fix) — backward-compatible improvement.CreditsPanelControllerconstructor expanded with new parameters — the only caller (ExplorePanelPlugin) is updated in this diff.MarketplaceCreditsAPIClient.GetUserCreditsAsyncmadevirtual— additive, backward-compatible.- New
virtualmethods onMarketplaceCreditsAPIClient— additive.
No breaking changes.
STEP 8 — Non-blocking warnings
None. Main scene not modified.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: New async flow (UniTask polling with CTS management), MVC controller, API endpoints, analytics, plugin wiring, and feature flag integration across 50 files.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
…tsTopUpModalController.cs Co-authored-by: Muna <44584806+decentraland-bot@users.noreply.github.com> Signed-off-by: davidejensen <davidejensen@live.it>
Contributor
|
Warnings not reduced: 14668 => 14707 — remove at least one warning to merge. Warnings/errors in files changed by this PR (91)
…and 41 more (see the |
decentraland-bot
left a comment
decentraland-bot
left a comment
Contributor
There was a problem hiding this comment.
STEP 2 — Root-cause check
This PR introduces a new credits top-up feature (buy credits via Stripe). The diff addresses the stated goal directly — it is net-new functionality, not a symptom fix. PASS.
STEP 3 — Design & integration
New long-lived units introduced:
-
CreditsTopUpService— manages the checkout→poll→credit lifecycle for a single purchase.- Lifecycle managed: Stripe checkout session creation + order status polling + balance refresh.
- Searched for existing owners:
CreditPurchasePlugin(owns the credit purchase modal lifecycle but has no checkout-poll concept),MarketplaceCreditsAPIClient(HTTP calls only, no lifecycle management),CreditsPanelController(read-only balance display). None manage a checkout-poll lifecycle. The new unit is justified.
-
CreditsTopUpModalController— MVC controller for the top-up popup.- Follows the established
ControllerBase<TView, TInputData>pattern. Justified.
- Follows the established
Teardown / consumption trace:
topUpService.StatusChanged += OnServiceStatusChangedin ctor →-=inDispose(). ✓viewInstance.RetryButton.onClick.AddListener(OnRetryClicked)inOnViewShow→RemoveListenerinOnViewClose. ✓packItem.BuyButton.onClick.AddListener(…)inBindPackItems→RemoveAllListeners()inOnViewClose. ✓CreditsTopUpAnalytics: all 5+=subscriptions → matched by 5-=inDispose(). ✓CreditsPanelController.view.GetCreditsButton.onClick.AddListener→RemoveListenerinDispose. ✓CancellationTokenSource cts→SafeCancelAndDispose()inDispose(). ✓CancellationTokenSource skipForegroundCts→SafeCancelAndDispose()inDispose()and after foreground poll. ✓
No subscription or resource leaks found. PASS.
STEP 4 — Member audit
| Member | Consumers | Verdict |
|---|---|---|
IsOrderInFlight |
StartTopUp, OnPackClicked |
2 consumers, re-entry guard — justified |
CurrentStatus |
Service internals, controller ApplyStatus, tests |
Multiple — justified |
StatusChanged event |
Controller relay, tests | Multiple — justified |
BuyCreditsStarted/Redirected/Completed/Pending/Failed events |
CreditsTopUpAnalytics (1 each) |
Event-based analytics pattern — justified |
No single-use intermediaries, absent-vs-false conflation, or redundant guards. PASS.
STEP 5 — Line-level review
See inline comments below. All findings are P2 (DTO convention compliance per CLAUDE.md § serialized JSON DTOs).
Security review
- ✅ No hardcoded secrets or credentials
- ✅ API calls use signed fetch (authenticated)
- ✅ No injection vectors — pack IDs come from hardcoded
CreditPackCatalog, not user input - ✅ Checkout URL originates from first-party server via signed fetch; opened in system browser
- ✅ No insecure deserialization (
JsonUtility) - ✅ State machine prevents concurrent checkout race conditions
- ✅ Feature gated behind
alfa-credits-topupfeature flag - ✅ No PII/sensitive data in logging
No security issues found.
STEP 6 — Complexity assessment
COMPLEX — async checkout→poll lifecycle with cancellation, MVC controller state machine, plugin/container wiring, analytics event hookups, and feature flag integration across multiple assemblies.
STEP 7 — QA assessment
QA_REQUIRED: YES — new user-facing popup and HUD button, runtime code changes, payment flow (Stripe checkout redirect + order status polling).
STEP 8 — Non-blocking warnings
None. Main.unity is not in the changed file list.
STEP 9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Introduces async checkout-poll lifecycle with cancellation, MVC controller with state machine, plugin wiring, and analytics across multiple assemblies.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by davidejensen via GitHub
DafGreco
approved these changes
Jul 23, 2026
DafGreco
left a comment
DafGreco
left a comment
There was a problem hiding this comment.
✔️ PR reviewed and approved by QA on both platforms following instructions playing both happy and un-happy path
Regressions for this ticket had been performed in order to verify that the normal flow is working as expected:
- [✔️ ] Log In/Log Out
- [✔️ ] Backpack and wearables in world
- [✔️ ] Emotes in world and in backpack
- [ ✔️ ] Teleport with map/coordinates/Jump In
- [ ✔️ ] Chat and multiplayer
- [ ✔️ ] Profile card
- [✔️ ] Settings
Evidence:
20260723-0856-39.5123728.mp4
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Description
What does this PR change?
Fix #9438
This PR adds the credits topup functionality in the client. It will be enabled only in .zone env and it's all behind feature flag.
Credits topup allow users to buy more credits from the + icon in the credits balance or when buying an item and not having enough credits.
Test Instructions
IMPORTANT NOTE: the UI is temporary, being the feature disabled we don't care about the UI aspect at the moment, as it's awaiting the design, do not report UI related bugs.
Test Steps
Check that nothing changes in .org environment
Check that the new feature works in .zone environment
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.