chore: new creditsflow refinements - #9535
Merged
Merged
Conversation
Contributor
|
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. |
Contributor
|
Warnings count reduced: 13974 => 13949 Warnings/errors in files changed by this PR (82) |
davidejensen
marked this pull request as ready for review
July 31, 2026 16:08
decentraland-bot
self-requested a review
July 31, 2026 16:08
decentraland-bot
left a comment
decentraland-bot
left a comment
Contributor
There was a problem hiding this comment.
STEP 2 — Root-cause check
PASS. The PR fixes intermittent quote failures for USD-pegged items by removing their unnecessary dependency on the MANA/USD oracle at quote time. The oracle rate was never needed for the credit price of USD-pegged items — the price is exact from the on-chain amount. The rate is only needed at purchase time to determine how much MANA the trade draws. This addresses the root cause (misplaced oracle dependency), not a symptom.
STEP 3 — Design & integration
PASS. ManaUsdRateReader is a service class injected by the DI container, not an ECS system — its persistent caches (aggregator addresses, decimals, rates, in-flight requests) are appropriate here.
Owner search: The rate reader is created and held by the credits purchase plugin via the DI container. No existing system or facade managed rate caching before. The caching belongs here.
Deferred rate resolution: USD-pegged items now resolve the MANA draw at purchase time instead of quote time. This is architecturally sound — the USD price is exact, and the MANA draw benefits from a fresh rate. The PrefetchAsync cache warm-up is a good optimization for reducing latency at the confirm click.
Teardown / consumption trace:
GetCreditsButton2.onClick.AddListener(line 47) → matched byRemoveListenerinDispose()(line 62) ✓PrefetchAsync().Forget()— safe:PrefetchAsynccatches all exceptions internally before returning ✓FetchIntoAsync.Forget()— safe: the method handles all exceptions via try/catch and forwards them through theUniTaskCompletionSource✓CancellationTokenSource(FETCH_TIMEOUT)— disposed viausing✓inFlightByMarketplaceentry — removed infinallyblock ofFetchIntoAsync✓
STEP 4 — Member audit
PrefetchAsync(ManaUsdRateReader.cs:121): 1 consumer (CreditsPurchaseService.QuoteInternalAsync). Distinct fromReadAsyncin semantics — it swallows exceptions and logs warnings, providing a safe fire-and-forget entry point. Justified as a separate public method.GetCreditsButton2(CreditsPanelView.cs:16): 1 consumer (CreditsPanelController). Non-descriptive name — see P2 finding below.
STEP 5 — Line-level findings
See inline comments. Two P2 findings, no P0 or P1.
Verified non-issues:
- ManaUsdRateReader cache race: Both the cache write (
rateByMarketplace[key] = ...) and in-flight removal (inFlightByMarketplace.Remove(...)) happen underlock(gate). The cache is populated beforeTrySetResult, and the in-flight entry is removed infinally. A concurrent reader arriving between cache-write and in-flight-removal gets a cache hit (correct). No race. - Double
ResolvingListingstate: Intentional — the first fires fromQuoteAsync(harmless, UI ignores it when not inPurchasingstate), the second fromPurchaseInternalAsyncwhen resolving the rate for USD-pegged trades (!quote.IsLiveRatePrice). Test correctly captures this. received[0]access at purchase time:trade.received[0]is validated at quote time (Length == 0check), andquote.Tradeis an immutable struct. Safe.EthApiResponse.idnull deserialization:[JsonProperty(NullValueHandling.Ignore)]onlong idcorrectly defaults JSONnullto0without throwingJsonSerializationException. Verified by new unit test.- DappWeb3EthereumApi
response.id == 0fallback: The mutex guarantees one in-flight request per socket, so cross-request confusion is not possible. Theid == 0check correctly matches null-id error frames from servers that echo"id": nullfor unparseable requests.
STEP 6 — Complexity
COMPLEX — touches 7+ meaningful files across TTL-cached rate reading with thread-safe request coalescing, async flow restructuring (deferred rate resolution), and RPC error handling across two Web3 API implementations (DappWeb3, ThirdWeb). 312 lines of new test coverage.
STEP 7 — QA assessment
QA_REQUIRED: YES — changes affect the runtime credits purchase flow (quote pricing, purchase-time rate resolution), purchase modal UI (spinner state, failure display, copy changes), credits panel UI (new button), and RPC communication error handling. All user-facing.
STEP 8 — Non-blocking warnings
None. Main scene not modified.
Security review
No security issues found. The lock-based synchronization is correct. CancellationToken propagation is improved across ThirdWeb RPC calls. JSON-RPC error handling is appropriate. The ThirdWebMetaTxService callback hardcodes CancellationToken.None but this is a pre-existing limitation of the callback type (Func<EthApiRequest, int, UniTask<EthApiResponse>>) and does not affect the credits purchase flow (which uses CreditsManagerMetaTxRelayer with proper CT threading).
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Multi-file changes spanning TTL-cached rate reading with thread-safe request coalescing, deferred rate resolution in the purchase flow, and RPC error handling across DappWeb3 and ThirdWeb implementations.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
Contributor
|
🔍 Jarvis reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging. |
Contributor
|
All Unity tests passed ✅
|
lorenzo-ranciaffi
approved these changes
Aug 3, 2026
lorenzo-ranciaffi
left a comment
lorenzo-ranciaffi
left a comment
Contributor
There was a problem hiding this comment.
I don't like the IReshowController approach, I think this could be tackled in a different manner (e.g. working with a stack of open UIs to walk backward) but that's a major change.
Since this needs to be merged asap, let's create a ticket to refactor this so we can tackle it later
davidejensen
enabled auto-merge (squash)
August 3, 2026 13:12
davidejensen
disabled auto-merge
August 3, 2026 15:23
Ludmilafantaniella
approved these changes
Aug 3, 2026
Ludmilafantaniella
left a comment
Ludmilafantaniella
left a comment
Contributor
There was a problem hiding this comment.
Approved ✅
Verified on both Windows and Mac:
- Equip/unequip visual bug after closing Explore panel — fixed
- "Go to backpack" button navigation — fixed
- Price-unavailable modal text overflow — fixed
- Packs not reappearing after buying from another tab — fixed
Known limitation (not blocking): if you start a credits purchase, close it with the X, and then try to buy more, it continues the previous transaction; if the browser tab is closed you can get stuck. Confirmed as a separate corner case to be handled in a follow-up PR.
9535-evi.mp4
✅Smoke test performed:
- ✔️ 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
- ✔️ Camera
- ✔️ Skybox
davidejensen
enabled auto-merge (squash)
August 3, 2026 15:45
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 #9534
This PR applies some visual fixes and improvements for the credits flow and fixes the sometimes failing lsiting fetch by ensuring a correct display of items price.
Test Instructions
Prerequisites
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.