Add SIWE login async task and improve OAuth browser flow #37
Conversation
Introduces UAsyncTaskThirdwebLoginWithSiwe for high-level SIWE login handling, including browser widget orchestration and authentication flow. Refactors OAuth browser and external browser classes to support SIWE, corrects parameter order and provider string casing, and improves resource cleanup and thread safety. Increases HTTP request timeout to 15 seconds and updates relevant delegate signatures and logic for SIWE payload/signature handling.
|
Caution Review failedThe pull request is closed. WalkthroughAdds a new UE async task for SIWE in-app login, standardizes SIWE callback parameter order (Payload, Signature), centralizes authentication cleanup in the external browser, increases a network timeout, and updates the Siwe provider string and related formatting. Changes
Sequence Diagram(s)sequenceDiagram
participant Blueprint
participant AsyncTask as UAsyncTaskThirdwebLoginWithSiwe
participant BrowserWidget as UThirdwebOAuthBrowserUserWidget
participant ExternalBrowser as UThirdwebOAuthExternalBrowser
participant HTTPServer as HTTP Server
Blueprint->>AsyncTask: LoginWithSiwe(WorldContext, Wallet)
AsyncTask->>BrowserWidget: Create & register widget
AsyncTask->>ExternalBrowser: Authenticate(Link, WalletSource)
ExternalBrowser->>ExternalBrowser: CleanupAuthentication()
ExternalBrowser->>ExternalBrowser: Reset state, bind route, start listener
ExternalBrowser->>BrowserWidget: Show browser UI
Note over BrowserWidget,HTTPServer: User completes SIWE flow
HTTPServer->>ExternalBrowser: Callback (Payload, Signature)
ExternalBrowser->>ExternalBrowser: HandleSuccess -> OnSiweComplete(Payload, Signature)
ExternalBrowser->>ExternalBrowser: CleanupAuthentication()
ExternalBrowser->>BrowserWidget: OnSiweComplete(Payload, Signature)
BrowserWidget->>AsyncTask: HandleSiweComplete(Payload, Signature)
AsyncTask->>Blueprint: Success.Broadcast(Payload, Signature)
AsyncTask->>BrowserWidget: Remove widget
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (4)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (12)
Source/Thirdweb/Private/ThirdwebUtils.cpp (1)
519-519: Timeout increase: OK, but consider consistency/configurability.15s seems fine for auth telemetry. Consider centralizing timeouts (e.g., a constant or runtime setting) to keep them consistent with Storage::Download/CreateEngineRequest (30s) and ease future tuning.
Is 15s intentional vs. 30s used elsewhere? If yes, a brief comment would help future maintainers.
Source/Thirdweb/Private/Wallets/ThirdwebInAppWalletHandle.cpp (1)
161-161: Provider string "Siwe": confirm backend expectation.Looks intentional. Verify that create_ecosystem_wallet expects "Siwe" (not "SIWE") to avoid Unknown/Invalid provider responses.
No need for a function‑local static here; make it a local const to avoid static storage.
- static FString Provider = TEXT("Siwe"); + const FString Provider = TEXT("Siwe");Source/Thirdweb/Public/Browser/ThirdwebOAuthBrowserUserWidget.h (2)
49-52: Dead config: bAuthenticateOnConstruct is unused.Either honor it on Construct (when Wallet is valid) or remove to avoid confusion.
87-89: Android handlers: consider UFUNCTION if blueprint/UI wiring is needed.If these must be callable/bindable in BP, annotate with UFUNCTION; otherwise, leave as is.
Source/Thirdweb/Private/Browser/ThirdwebOAuthBrowserUserWidget.cpp (3)
62-67: Lifecycle: avoid callingConditionalBeginDestroy()on owned UObject.Let GC handle the subobject and explicitly clean it up; don’t self-destroy from owner’s
BeginDestroy.void UThirdwebOAuthBrowserUserWidget::BeginDestroy() { - if (ExternalBrowser) { - ExternalBrowser->ConditionalBeginDestroy(); - } + if (ExternalBrowser) { + ExternalBrowser->CleanupAuthentication(); + ExternalBrowser = nullptr; + } Super::BeginDestroy(); }
198-217: Macro style nit.Use
||in preprocessor conditionals for readability.-#if PLATFORM_IOS | PLATFORM_ANDROID +#if PLATFORM_IOS || PLATFORM_ANDROID
19-23: Hardcoded URLs: centralize/config.Consider moving
BackendUrlPrefixandDummyUrlto settings to avoid compile-time constants.Source/Thirdweb/Public/AsyncTasks/Wallets/InApp/AsyncTaskThirdwebLoginWithSiwe.h (1)
20-25: Factory/Activate: add null checks.Defensive: ensure
Browseris valid before use and bail withFailed.Broadcast.Source/Thirdweb/Private/Browser/ThirdwebOAuthExternalBrowser.cpp (4)
110-113: Global StopAllListeners can impact other systems.Avoid stopping all listeners; unbind your route is usually sufficient. If needed, track “started by us” and stop only then.
118-133: Query parsing: normalize/validate.Guard against missing keys with
Request.QueryParams.Contains(...)beforeFindRef, andUrlDecodeboth payload and (if needed) signature.
74-80: Case-insensitive compare nit.Prefer
Url.TrimStartAndEnd().Equals(TEXT("SIWE"), ESearchCase::IgnoreCase)for clarity.
45-46: AuthEvent appears unused.It’s created/triggered/returned but never awaited. Remove to simplify, or add a Wait path if intended.
Also applies to: 139-140, 176-190
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
Source/Thirdweb/Private/AsyncTasks/Wallets/InApp/AsyncTaskThirdwebLoginWithSiwe.cpp(1 hunks)Source/Thirdweb/Private/Browser/ThirdwebOAuthBrowserUserWidget.cpp(1 hunks)Source/Thirdweb/Private/Browser/ThirdwebOAuthExternalBrowser.cpp(1 hunks)Source/Thirdweb/Private/ThirdwebUtils.cpp(1 hunks)Source/Thirdweb/Private/Wallets/ThirdwebInAppWalletHandle.cpp(2 hunks)Source/Thirdweb/Public/AsyncTasks/Wallets/InApp/AsyncTaskThirdwebLoginWithSiwe.h(1 hunks)Source/Thirdweb/Public/Browser/ThirdwebOAuthBrowserUserWidget.h(4 hunks)Source/Thirdweb/Public/Browser/ThirdwebOAuthExternalBrowser.h(2 hunks)Source/Thirdweb/Public/Wallets/ThirdwebInAppWalletHandle.h(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
Source/Thirdweb/Private/AsyncTasks/Wallets/InApp/AsyncTaskThirdwebLoginWithSiwe.cpp (1)
Source/Thirdweb/Private/Browser/ThirdwebOAuthBrowserUserWidget.cpp (2)
HandleSiweComplete(161-176)HandleSiweComplete(161-162)
Source/Thirdweb/Private/Browser/ThirdwebOAuthBrowserUserWidget.cpp (4)
Source/Thirdweb/Private/Browser/ThirdwebOAuthExternalBrowser.cpp (2)
HandleError(165-174)HandleError(165-165)Source/Thirdweb/Private/AsyncTasks/Wallets/InApp/AsyncTaskThirdwebLoginWithSiwe.cpp (2)
HandleSiweComplete(33-50)HandleSiweComplete(33-34)Source/Thirdweb/Private/ThirdwebRuntimeSettings.cpp (2)
GetAppUri(239-250)GetAppUri(239-239)Source/Thirdweb/Private/Browser/Android/ThirdwebAndroidJNI.cpp (2)
CallJniStaticVoidMethod(17-25)CallJniStaticVoidMethod(17-17)
Source/Thirdweb/Public/AsyncTasks/Wallets/InApp/AsyncTaskThirdwebLoginWithSiwe.h (5)
Source/Thirdweb/Public/Browser/ThirdwebOAuthBrowserUserWidget.h (1)
UThirdwebOAuthBrowserUserWidget(11-93)Source/Thirdweb/Public/AsyncTasks/Wallets/InApp/AsyncTaskThirdwebInAppBase.h (1)
UAsyncTaskThirdwebInAppBase(16-23)Source/Thirdweb/Public/Wallets/ThirdwebInAppWalletHandle.h (3)
void(77-81)FInAppWalletHandle(14-388)FString(345-352)Source/Thirdweb/Private/AsyncTasks/Wallets/InApp/AsyncTaskThirdwebLoginWithSiwe.cpp (8)
Activate(12-17)Activate(12-12)LoginWithSiwe(19-31)LoginWithSiwe(19-20)HandleSiweComplete(33-50)HandleSiweComplete(33-34)HandleFailed(52-68)HandleFailed(52-52)Source/Thirdweb/Private/Browser/ThirdwebOAuthBrowserUserWidget.cpp (2)
HandleSiweComplete(161-176)HandleSiweComplete(161-162)
Source/Thirdweb/Public/Browser/ThirdwebOAuthBrowserUserWidget.h (3)
Source/Thirdweb/Private/Browser/Android/ThirdwebAndroidJNI.cpp (2)
void(82-96)void(98-109)Source/Thirdweb/Private/AsyncTasks/Wallets/InApp/AsyncTaskThirdwebLoginWithSiwe.cpp (2)
HandleSiweComplete(33-50)HandleSiweComplete(33-34)Source/Thirdweb/Private/Browser/ThirdwebOAuthBrowserUserWidget.cpp (6)
HandleSiweComplete(161-176)HandleSiweComplete(161-162)HandleDeepLink(183-187)HandleDeepLink(183-183)HandleCustomTabsDismissed(189-195)HandleCustomTabsDismissed(189-190)
Source/Thirdweb/Public/Browser/ThirdwebOAuthExternalBrowser.h (2)
Source/Thirdweb/Private/Browser/ThirdwebOAuthBrowserUserWidget.cpp (6)
Authenticate(77-124)Authenticate(77-78)BeginDestroy(62-67)BeginDestroy(62-62)HandleError(178-180)HandleError(178-178)Source/Thirdweb/Private/Browser/ThirdwebOAuthExternalBrowser.cpp (14)
Authenticate(21-85)Authenticate(21-21)Tick(87-104)Tick(87-87)BeginDestroy(106-116)BeginDestroy(106-106)CallbackRequestHandler(118-148)CallbackRequestHandler(118-119)HandleSuccess(150-163)HandleSuccess(150-150)HandleError(165-174)HandleError(165-165)CleanupAuthentication(176-190)CleanupAuthentication(176-176)
Source/Thirdweb/Private/Browser/ThirdwebOAuthExternalBrowser.cpp (2)
Source/Thirdweb/Private/Browser/ThirdwebOAuthBrowserUserWidget.cpp (6)
Authenticate(77-124)Authenticate(77-78)HandleError(178-180)HandleError(178-178)BeginDestroy(62-67)BeginDestroy(62-62)Source/Thirdweb/Private/ThirdwebRuntimeSettings.cpp (2)
GetExternalAuthRedirectUri(139-149)GetExternalAuthRedirectUri(139-139)
🪛 Clang (14.0.6)
Source/Thirdweb/Private/AsyncTasks/Wallets/InApp/AsyncTaskThirdwebLoginWithSiwe.cpp
[error] 3-3: 'AsyncTasks/Wallets/InApp/AsyncTaskThirdwebLoginWithSiwe.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (12)
Source/Thirdweb/Public/Wallets/ThirdwebInAppWalletHandle.h (1)
334-335: SIWE source string casing change.Changing to "Siwe" looks consistent with upstream usage. Please confirm the Rust APIs that consume provider/source strings are case-sensitive and expect exactly "Siwe".
Source/Thirdweb/Private/Wallets/ThirdwebInAppWalletHandle.cpp (1)
601-603: LinkSiwe argument order (Payload, Signature) aligns with SignInWithEthereum.This matches ecosystem_wallet_sign_in_with_siwe(id, payload, signature). Good change. Please double‑check all other callers of ecosystem_wallet_link_account follow the same schema positions per auth type.
Source/Thirdweb/Public/Browser/ThirdwebOAuthExternalBrowser.h (2)
35-35: Good addition: central cleanup helper.Centralizes teardown and reduces leaks/dup cleanup in lifecycle. LGTM.
24-26: Public API shape unchanged; formatting/style tweaks only.Signature/typedef spacing changes are benign. Enum compact form is fine. No ABI/behavioral impact.
Also applies to: 38-44, 46-46
Source/Thirdweb/Private/AsyncTasks/Wallets/InApp/AsyncTaskThirdwebLoginWithSiwe.cpp (3)
33-50: Thread dispatch and teardown sequencing look good.Broadcast on game thread, then remove widget and finalize task. This is the right order for BP listeners and UI. LGTM.
Ensure you remove delegate bindings (RemoveDynamic) on success/failure if the widget can outlive the task in edge cases.
Also applies to: 52-68
3-3: The header file is present at the correct location. The include path"AsyncTasks/Wallets/InApp/AsyncTaskThirdwebLoginWithSiwe.h"is valid—Unreal Engine automatically adds the module'sPublicdirectory to the include path, so this will resolve correctly toSource/Thirdweb/Public/AsyncTasks/Wallets/InApp/AsyncTaskThirdwebLoginWithSiwe.h.The original review comment's claim of a blocking compiler error is incorrect.
Likely an incorrect or invalid review comment.
12-17: The review comment is partially incorrect; the header markers are already present, but null-safety guards remain missing.The codebase already has the header-level changes the review suggests:
Browseris already markedUPROPERTY(Transient)in the header- Both
HandleSiweComplete()andHandleFailed()are already markedUFUNCTION()in the headerHowever, the null-safety concerns are valid and unaddressed. The code lacks guards:
Activate()(line 13–16): derefs Browser without null checkLoginWithSiwe()(line 26–28): assigns CreateWidget result without validationHandleSiweComplete()(line 37): calls Browser->RemoveFromParent() without guardHandleFailed()(line 54): calls Browser->RemoveFromParent() without guardAdd null checks on Browser before dereferencing in Activate(), HandleSiweComplete(), and HandleFailed(), and validate the CreateWidget result in LoginWithSiwe().
Likely an incorrect or invalid review comment.
Source/Thirdweb/Public/Browser/ThirdwebOAuthBrowserUserWidget.h (1)
21-24: SIWE parameter order normalized — good.Payload-first ordering is consistent with the new async task and .cpp broadcast. No issues.
Also applies to: 81-83
Source/Thirdweb/Private/Browser/ThirdwebOAuthBrowserUserWidget.cpp (1)
24-46: Delegate binding is fine.Binding to
thisis OK since ExternalBrowser’s Outer is the widget; lifetimes align.Source/Thirdweb/Public/AsyncTasks/Wallets/InApp/AsyncTaskThirdwebLoginWithSiwe.h (1)
27-31: Delegate shape matches widget — good.Source/Thirdweb/Private/Browser/ThirdwebOAuthExternalBrowser.cpp (2)
150-163: Delegate firing then cleanup — good.Success/Error trigger then cleanup is sound and aligns with widget flow.
Also applies to: 165-174
40-44: Incorrect analysis of HTTP router binding parameter.The review comment misunderstands the
GetHttpRouter()API. The second parameter (bFailOnBindFailure) controls whether bind failures are fatal errors—it does not control the binding address. FHttpServerModule defaults to bind to "localhost" (127.0.0.1), not 0.0.0.0.The code is already secure: line 79 explicitly uses
http://localhost:8789/callbackas the redirect URL, confirming loopback-only binding. Changingtruetofalsewould suppress bind failures silently, which is counterproductive for error handling—the opposite of a security benefit.No changes needed.
Likely an incorrect or invalid review comment.
Introduces UAsyncTaskThirdwebLoginWithSiwe for high-level SIWE login handling, including browser widget orchestration and authentication flow. Refactors OAuth browser and external browser classes to support SIWE, corrects parameter order and provider string casing, and improves resource cleanup and thread safety. Increases HTTP request timeout to 15 seconds and updates relevant delegate signatures and logic for SIWE payload/signature handling.
Closes BLD-445
Summary by CodeRabbit
New Features
Improvements