feat: register referral attribution from the launcher referrer argument - #9488
feat: register referral attribution from the launcher referrer argument#9488braianj wants to merge 9 commits into
Conversation
decentraland-bot
added
the
ext-contribution
|
Build failed! Check the logs to see what went wrong. |
|
Warnings not reduced: 14075 => 14076 — remove at least one warning to merge. Warnings/errors in files changed by this PR (41) |
|
Slack notification sent to #explorer-ext-contributions for external review. |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
STEP 2 — Root-cause check
PASS. This PR adds a new feature (referral attribution from a launcher argument), not a bug fix. The diff correctly implements two referral consumption paths: URL construction for wallet sign-ups and API registration for new-account onboarding.
STEP 3 — Design & integration
PASS with notes.
Owner search — RegisterReferralAsync: The referral API call runs inside LobbyForNewAccountAuthState.PublishNewProfileAsync, which executes only when a new account completes onboarding. The lifecycle owner for new-account finalization is LobbyForNewAccountAuthState itself (it owns the profile publish, newsletter subscription, and transition to the world). Placing the referral registration here is correct — no other existing owner manages this lifecycle step. Files checked: AuthenticationScreenController.cs (state machine wiring), LobbyForExistingAccountAuthState (existing-account path — correctly excluded), InitAuthState, LoginSelectionAuthState.
Owner search — DeepLinkSignInUrl: Extracted from inline URL construction in DappDeepLinkAuthenticator.LoginAsync. The extraction is justified: it makes URL-building logic independently testable without instantiating the full authenticator. The split pays for itself in test coverage (CLAUDE.md §11 "Extracting when you should merge").
Owner search — ReferrerArg: Shared static utility consumed by both DeepLinkSignInUrl.Build and LobbyForNewAccountAuthState. Two independent consumers → extraction justified.
Referrer flow: The referrer enters via appArgs at two composition-root sites (BootstrapContainer.CreateWeb3Dependencies and Web3AuthenticationPlugin.InitializeAsync), feeding two separate dependency chains. This is noted as P2 below.
Teardown / consumption trace: No subscriptions, event hookups, connections, or persistent resources introduced. RegisterReferralAsync is awaited inline. No teardown needed.
STEP 4 — Member audit
DeepLinkSignInUrl.Build()— public static, 1 production caller (DappDeepLinkAuthenticator.LoginAsync), 4 test callers. Independently tested. Acceptable single-caller extraction.ReferrerArg.Normalize()— public static, 2 production callers. Appropriate.AppArgsFlags.REFERRER— public const, 3 usage sites. Appropriate.DecentralandUrl.ReferralProgress— enum member, 2 usage sites. Appropriate.
No single-use predicates, absent-equals-false, or redundant-guard issues.
STEP 5 — Findings
See inline comments below for suggestion blocks.
| # | Sev | Location | Issue |
|---|---|---|---|
| 1 | P1 | PR settings | PR targets main instead of dev (repo default branch is dev) |
| 2 | P1 | LobbyForNewAccountAuthState.cs:346–382 |
Combined referral timeout up to 10s blocks user with no feedback |
| 3 | P2 | DappDeepLinkAuthenticator.cs:57 |
Raw referrer stored; inconsistent with LobbyForNewAccountAuthState which normalizes at construction |
[P1] PR targets main instead of dev. This repo's default branch is dev (origin/HEAD → origin/dev). Per the branch model, feature PRs should target dev. Please retarget this PR before merge.
Security review: No security issues found. Input validation via ReferrerArg.Normalize (^0x[a-fA-F0-9]{40}$) is strict and correctly prevents injection in both URL and JSON interpolation paths. API requests use signed fetch. No secrets or sensitive data in the diff.
Git conventions (ADR-6): PASS. Title follows feat: ... format. Branch follows feat/... convention.
STEP 6 — Complexity
COMPLEX — Modifies authentication/onboarding flow, adds signed-fetch web requests during new-account finalization, touches DI wiring in BootstrapContainer and Web3AuthenticationPlugin, adds a new DecentralandUrl enum member, and spans 17 files across multiple assemblies.
STEP 7 — QA assessment
QA_REQUIRED: YES — Changes affect the runtime authentication/onboarding flow. New-account registration now includes a server round-trip (POST + PATCH) between profile creation and entering the world. Wallet sign-ups get a modified browser URL. Both paths need manual verification.
STEP 8 — Non-blocking warnings
None.
REVIEW_RESULT: FAIL ❌
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies auth onboarding flow, adds signed-fetch web requests, touches DI wiring in BootstrapContainer/Web3AuthenticationPlugin, and spans 17 files across multiple assemblies.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by braianj via Slack
lorux0
left a comment
lorux0
left a comment
There was a problem hiding this comment.
Great work! Added small comments
|
Tests: 24459 passed, 0 failed ✅ |
3 participants
Consumes the
--referrerlaunch argument the launcher passes and uses it to register the referral for new accounts.referrerapp arg (AppArgsFlags.REFERRER) plus a validatedReferrerArgnormalizer (0x+ 40 hex, lowercased).DeepLinkSignInUrl(extracted fromDappDeepLinkAuthenticatorso it is unit-testable) appends the referrer to the signature web app URL, so wallet users carry attribution into the web setup flow.LobbyForNewAccountAuthState— which runs only for new accounts — registers the referral against social-api with a signed fetch, covering the email/OTP onboarding that never opens the browser. Time-boxed and best-effort: it never blocks or fails onboarding.Please note before merging:
LoggedInstatus transition happens earlier when the lobby is entered, so registration is not strictly ordered ahead of theLOGGED_INanalytics event. With an idempotent create on the backend and the multi-day finalization rule, the worst case is one login day not counted rather than a lost referral; closing the ordering gap fully would mean moving the status transition, which is left out of scope here.