fix: allow emailOTP only for epic store - #9555
Conversation
|
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. |
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — #9555 fix: allow emailOTP only for epic store
STEP 2 — Root-cause check
PASS. This is an acknowledged mitigation for #9554. Epic builds cannot receive decentraland:// deeplink callbacks, so wallet/social login is completely blocked. Hiding those options and forcing email OTP is a valid interim fix. The PR description explicitly states this is "not final solution" — acceptable.
STEP 3 — Design & integration
PASS. No new long-lived units introduced. The otherLoginMethodsEnabled flag flows through the existing controller → state → view pattern, which is the correct seam for this change. The installSource string is already injected into the controller; deriving isEpicBuild from it is the natural place. The LoginSelectionAuthState stores the flag and passes it to the view on Enter(int) — this correctly covers all entry points (Enter(int) called from Enter(ErrorType) and directly from the controller).
Owner search: N/A — no new lifecycle-managing units.
STEP 4 — Member audit
| Member | File | Consumers | Verdict |
|---|---|---|---|
OtherLoginContainer |
LoginSelectionAuthView |
Show() (1) |
Serialized Unity reference — appropriate |
ContinueWithTextContainer |
LoginSelectionAuthView |
Show() (1) |
Serialized Unity reference — appropriate |
otherLoginMethodsEnabled |
LoginSelectionAuthState |
Enter(int) → view.Show() (1) |
State-machine pass-through — appropriate |
EPIC_STORE_INSTALL_SOURCE |
AuthenticationScreenController |
OnViewInstantiated() (1) |
Named constant — appropriate |
STEP 5 — Line-level findings
See inline comments. Two P2 findings:
- P2 — Null-forgiving operator (
!) onviewInstancewithout justifying comment - P2 —
ToggleOptionsPanelExpansion()bypasses theotherLoginMethodsEnabledguard onSetOptionsPanelVisibility
Security review
No security issues found. The change restricts available login methods (does not add new attack surface). The installSource comparison uses a build-time constant. No secrets, no injection vectors, no auth bypass.
STEP 6 — Complexity
COMPLEX — modifies login method availability in the authentication flow; touches 4 files including auth screen controller and state machine.
STEP 7 — QA assessment
QA_REQUIRED: YES — changes the login screen UI and available authentication methods for Epic builds. Needs manual verification on both Epic and non-Epic builds.
STEP 8 — Non-blocking warnings
None. Main scene not modified.
STEP 9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies login method availability in the authentication screen flow, gating UI and state by install source.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
|
🔍 Jarvis reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging. |
decentraland-bot
left a comment
There was a problem hiding this comment.
Automated review
STEP 1 — Load context & set scope
Loaded CLAUDE.md, docs/README.md, docs/web3-authentication.md, docs/mvc.md, and docs/code-style-guidelines.md. Checked out PR #9555 locally and reviewed the changed auth UI/controller files plus related auth/provider wiring.
STEP 2 — Root-cause check
The immediate Epic blocker is that web3/social login depends on the decentraland:// callback, which Epic installs do not own. This PR mitigates the production blocker by exposing only Email OTP for Epic builds while leaving normal launcher builds unchanged. For that scoped mitigation, the diff addresses the immediate user-facing cause; the protocol-handler/web3 callback gap remains a separate follow-up.
STEP 3 — Design & integration
No new long-lived system/plugin/service/controller is introduced. The lifecycle owner for this UI remains Web3AuthenticationPlugin → AuthenticationScreenController, and state transitions remain in the existing auth FSM. The view changes stay inside LoginSelectionAuthView, consistent with docs/mvc.md view responsibilities.
Teardown/consumption trace: no new subscriptions, callbacks, native resources, or buffers were added. Existing AddListener/Submitted += hooks in LoginSelectionAuthState still have matching cleanup in Exit() via RemoveAllListeners() and Submitted -= OTPLogin.
STEP 4 — Member audit
New/changed public members:
LoginSelectionAuthView.OtherLoginContainer: 1 consumer (Show) — serialized Unity reference, acceptable view wiring.LoginSelectionAuthView.ContinueWithTextContainer: 1 consumer (Show) — serialized Unity reference, acceptable view wiring.LoginSelectionAuthView.Show(int, bool, bool): 1 consumer (LoginSelectionAuthState.Enter(int)) — existing view API extended to receive login-method visibility.
No single-use derived predicate or redundant accessor issue found.
STEP 5 — Line-level review
Found one low-risk robustness issue below. Security pass: no security issues found.
STEP 6 — Complexity assessment
COMPLEX: this touches runtime authentication UI and login-method gating, which is user-facing and auth-sensitive even though the code diff is small.
STEP 7 — QA assessment
QA_REQUIRED: YES, because this changes runtime authentication UI behavior and must be verified on Epic and non-Epic builds.
STEP 8 — Non-blocking warnings
No main scene changes. CI is currently partially pending; approval-gate checks are failing only because QA/DEV approvals are still required.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Runtime authentication UI and login-method gating are changed for Epic builds.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by Ignacio Mazzara (<@U9ETM8CJH>) via Slack
|
Warnings not reduced: 14004 => 14062 — remove at least 59 warnings to merge. Warnings/errors in files changed by this PR (12) |
anicalbano
left a comment
There was a problem hiding this comment.
✔️ Tested on Windows and macOS
✔️ Email OTP
✔️ Metamask
✔️ Google
✔️ New user
What does this PR change?
Fixes #9554
Works as mitigation change, not final solution.
Test instructions
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.