fix: avoid failure in item price retrieval - #9687
Conversation
🚦 CI Status
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. Warnings not reduced: 13653 => 13653 — remove at least 1 warning to merge. Warnings/errors in files changed by this PR (2)All Unity tests passed ✅
|
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Review — PR #9687: fix: avoid failure in item price retrieval
STEP 2 — Root-cause check
Problem: Guid.NewGuid().GetHashCode() (used in ~7 callers across the codebase to generate JSON-RPC request IDs) can return negative int values. When a negative ID reaches the RPC backend over the websocket transport, it is rejected with error code -32600.
Does the diff fix the cause? Yes — the masking request.id &= int.MaxValue ensures the wire ID is always non-negative. The fix is placed at the transport boundary (RequestEthMethodWithoutSignatureAsync), which is the correct scope since it covers all read-only RPC callers (confirmed: ManaUsdRateReader, PolygonSettlementPoller, and any SDK scene eth_call). The save/restore pattern (callerId → masked wire id → restore on response) preserves caller-visible ID semantics. PASS.
STEP 3 — Design & integration
This diff modifies only one method in an existing class; no new types, systems, or persistent state are introduced. The save/restore pattern adds ~4 lines of complexity to a private method guarded by a mutex (one in-flight request per socket). No lifecycle or ownership concerns apply. PASS.
STEP 4 — Member audit
No public members are added or changed. The modified method is private. No audit issues.
STEP 5 — Line-level review
Two P2 findings — see inline comments.
Finding 1 (P2): The & int.MaxValue masking and the save/restore pattern are correct, but would benefit from a brief inline comment explaining why the wire ID must be non-negative — the RPC backend rejects negative IDs. Without context, a future maintainer might remove it.
Finding 2 (P2): The serialization change from JsonConvert.SerializeObject(request) to new { jsonrpc = "2.0", ... } adds the JSON-RPC 2.0 jsonrpc version field, which the EthApiRequest struct doesn't carry. This is a protocol compliance improvement — correct — but it's a separate behavioral change from the negative-ID fix. A brief comment clarifies intent.
Security review
No security issues found. The masking reduces ID entropy by 1 bit (32→31 bits); collision risk remains negligible (~1-in-2B per pair), and the mutex guarantees single in-flight requests per socket. The anonymous object serialization is equivalent to the struct output minus the [JsonIgnore] field — no new data leakage.
STEP 6 — Complexity
SIMPLE — single file, <10 lines of meaningful change, no ECS/system/async pattern modifications.
STEP 7 — QA assessment
QA_REQUIRED: YES — affects the runtime purchase flow (MANA/USD rate retrieval for marketplace items).
STEP 8 — Non-blocking warnings
None.
STEP 9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Single-method fix in the Dapp Web3 RPC transport — no ECS, async-pattern, or architectural changes.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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:
- [✔️ ] Backpack and wearables in world
- [ ✔️] Emotes in world and in backpack
- [ ✔️] Teleport with map/coordinates/Jump In
- [✔️ ] Chat and multiplayer
- [✔️ ] Profile card
- [✔️ ] Camera
- [ ✔️] Skybox
- [ ✔️] Settings
Evidence:
20260812-0838-32.1241374.mp4
3 participants
Pull Request Description
What does this PR change?
Fix #9688
When trying to buy new items from someone's passport and with a metamask account sometimes there was a failure in the retrieval of the price of the wearable like this:
The problem was random caused by a random request index sometimes being negative, when negative the request for the exchange rate of MANA/USD was failing.
(from editor: [CREDITS_PURCHASE]: MANA/USD rate unavailable for trade 558f37e9-9844-45ae-b7ea-c19bd4aa4b58: RPC eth_call failed: code -32600 unknown command:
{"jsonrpc":"2.0","id":-1409326930,"method":"eth_call","params":[{"to":"0xa40b1d129b8906888720686f3a01921ddf37716f","data":"0x05dec3e4"},"latest"]} )
To avoid changing drastically all dapp request the fix has been applied only at this scope with:
The wire id is masked with & int.MaxValue, so a negative Guid.GetHashCode() id becomes a positive 31-bit one — exactly representable even if a backend parses ids as float64.
The caller's original id is restored on the returned response, so SDK scenes that match responses by id see exactly what they sent.
Test Instructions
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.