Fix/mwa keep connection alive auth token restore#269
Open
JoshhSandhu wants to merge 22 commits intomagicblock-labs:mainfrom
Open
Fix/mwa keep connection alive auth token restore#269JoshhSandhu wants to merge 22 commits intomagicblock-labs:mainfrom
JoshhSandhu wants to merge 22 commits intomagicblock-labs:mainfrom
Conversation
_authToken was never set to null on Logout(), allowing reauthorize attempts with a revoked token within the same session. authToken PlayerPrefs key was also never cleared, leaving a stale token on disk after logout. Fixes part of magicblock-labs#267
…ve login When keepConnectionAlive = true, _Login() previously restored Account from PlayerPrefs["pk"] without verifying authorization state this caused _authToken to be null on process restart, triggering an unexpected Authorize() popup mid-transaction instead of at login time. Fix: attempt Reauthorize() with cached authToken before returning cached Account. On failure, clear both cached keys and fall through to full Authorize() flow, also persist authToken alongside pk on successful Authorize so the token survives process restart. Fixes magicblock-labs#267
Added two missing MWA spec operations to the adapter interface: - Deauthorize(string authToken): revokes wallet-side auth token. Takes authToken only -- no identity fields per MWA spec. Returns non-generic Task (no payload on success). - GetCapabilities(): queries wallet feature limits and supported transaction versions. Returns new CapabilitiesResult model with MaxTransactionsPerRequest, MaxMessagesPerRequest, SupportedTransactionVersions, and SupportsCloneAuthorization. CapabilitiesResult.cs added alongside existing response models following same Preserve and JsonProperty conventions. Related to magicblock-labs#267
Added Deauthorize(string authToken) and PrepareDeauthorizeRequest() to MobileWalletAdapterClient. The deauthorize RPC sends only auth_token on the wire, no identity fields, matching the MWA spec, result is discarded as the RPC returns no payload on success. Related to magicblock-labs#267
Added GetCapabilities() and PrepareGetCapabilitiesRequest() to MobileWalletAdapterClient. Queries wallet feature limits via get_capabilities RPC. Takes no parameters per MWA spec. Returns CapabilitiesResult with MaxTransactionsPerRequest, MaxMessagesPerRequest, SupportedTransactionVersions, and SupportsCloneAuthorization. Related to magicblock-labs#267
…vents Added OnWalletDisconnected and OnWalletReconnected events and two new public async methods to SolanaMobileWalletAdapter: DisconnectWallet(): captures auth token from memory with PlayerPrefs fallback, sends deauthorize RPC, then calls Logout() to clear all local state event fires after cleanup deauthorize failure is logged but never blocks local logout. ReconnectWallet(): attempts silent reconnect via Login() which uses the cached token reauthorize path event fires after Login() succeeds. Errors are logged without re-throwing. Related to magicblock-labs#267
…to SolanaWalletAdapter Exposed DisconnectWallet() and ReconnectWallet() on the cross-platform SolanaWalletAdapter wrapper. Both methods cast _internalWallet to SolanaMobileWalletAdapter and delegate. NotImplementedException thrown for non-mobile adapter targets. Related to magicblock-labs#267
After process restart, _authToken is null in memory even when a valid token exists in PlayerPrefs from a successful reauthorize. This caused _SignAllTransactions() and SignMessage() to fall into the Authorize() branch unnecessarily, triggering an unexpected wallet popup mid-session. Fix: read authToken from PlayerPrefs at the top of both signing methods before the Authorize/Reauthorize branch is evaluated. Fixes magicblock-labs#267
LocalAssociationScenario does not implement IDisposable in the current codebase. using var causes a compile error against main. Changed to plain var with TODO comments on lines 63 and 196. Will be updated to using var after PR magicblock-labs#260 merges, which adds IDisposable to LocalAssociationScenario. Related to magicblock-labs#267
…e path Two fixes in the keepConnectionAlive reauthorize guard: 1. Added null check on reauth result before assigning to _authToken. A null reauth or empty AuthToken would silently set _authToken to null, re-introducing the mid-transaction Authorize() popup from Issue magicblock-labs#267. 2. Persist the (possibly rotated) token back to PlayerPrefs after a successful reauthorize. Wallets that rotate auth tokens on every reauthorize call would otherwise force a full Authorize() popup on every cold start despite keepConnectionAlive = true. Fixes magicblock-labs#267
M-1: wrap DisconnectWallet() StartAndExecute in try/catch so Logout() and OnWalletDisconnected always fire even on transport exceptions. Deauthorize is best-effort, local cleanup is guaranteed. M-2: guard OnWalletReconnected behind null check on Login() return. Prevents false-positive reconnect event when Login() returns null. M-4: add GetCapabilities() to SolanaMobileWalletAdapter and forward through SolanaWalletAdapter using same cast-and-delegate pattern. M-5: add OnWalletDisconnected and OnWalletReconnected events to SolanaWalletAdapter. Constructor wires them to mobile adapter events under UNITY_ANDROID guard. Related to magicblock-labs#267
…ilities() parameters - Added a warning log for failed Deauthorize() RPC responses to improve error visibility. - Changed Params in GetCapabilities() to use a new JsonRequest.JsonRequestParams() object for better clarity and adherence to expected structure.
…auth-token-restore
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds Deauthorize and GetCapabilities RPC surface and CapabilitiesResult model; extends MobileWalletAdapterClient, SolanaMobileWalletAdapter, and SolanaWalletAdapter to persist and use Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Runtime/codebase/SolanaMobileStack/Interfaces/IAdapterOperations.cs`:
- Around line 15-19: Add the explicit public modifier to the interface members
in IAdapterOperations for consistency with existing members: change the
Deauthorize and GetCapabilities declarations to include the public keyword
(i.e., make the signatures public Task Deauthorize(string authToken); and public
Task<CapabilitiesResult> GetCapabilities();) so they match the style of other
methods in the IAdapterOperations interface.
In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`:
- Around line 228-246: ReconnectWallet currently swallows exceptions (logs
e.Message) so callers can't tell failure from success; update ReconnectWallet to
rethrow the caught exception after logging (use throw;) so callers of
ReconnectWallet can handle failures, and ensure any callers of ReconnectWallet
handle the propagated exception; reference: ReconnectWallet, Login,
OnWalletReconnected.
- Around line 63-93: The async lambda passed to StartAndExecute uses
Action<IAdapterOperations> which makes it effectively async void and can swallow
exceptions; change the delegate type to Func<IAdapterOperations, Task> (update
LocalAssociationScenario.StartAndExecute and ExecuteNextAction to accept and
await IEnumerable<Func<IAdapterOperations, Task>>) and replace
Action<IAdapterOperations> with Func<IAdapterOperations, Task> where you build
the reauthorizeScenario actions so the async client => { ... } returns a Task,
ensuring exceptions from Reauthorize propagate and are awaited before checking
reauthorizeResult.WasSuccessful and updating _authToken.
In `@Runtime/codebase/SolanaWalletAdapter.cs`:
- Around line 112-120: Add an XML doc comment to the GetCapabilities method that
documents the possible null return and when it occurs: explain that
GetCapabilities returns a CapabilitiesResult for SolanaMobileWalletAdapter,
throws NotImplementedException when _internalWallet is non-null but unsupported,
and returns null when _internalWallet is null or when running on platforms that
don't support mobile adapters; reference GetCapabilities, _internalWallet,
SolanaMobileWalletAdapter, and CapabilitiesResult in the summary/remarks so
callers know to handle the null case.
- Around line 88-110: Both DisconnectWallet and ReconnectWallet currently
silently return when _internalWallet is null, causing inconsistent behavior with
other methods; update both methods (DisconnectWallet and ReconnectWallet) to
handle the null case consistently by throwing a NotImplementedException (or at
minimum logging a warning) when _internalWallet is null, while retaining the
existing cast to SolanaMobileWalletAdapter and the await/return flow when the
cast succeeds so behavior matches _Login and other adapter methods.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 70b132c7-d3b8-4dbf-9bca-b5ca330a04f2
📒 Files selected for processing (6)
Runtime/codebase/SolanaMobileStack/Interfaces/IAdapterOperations.csRuntime/codebase/SolanaMobileStack/JsonRpcClient/Responses/CapabilitiesResult.csRuntime/codebase/SolanaMobileStack/JsonRpcClient/Responses/CapabilitiesResult.cs.metaRuntime/codebase/SolanaMobileStack/MobileWalletAdapterClient.csRuntime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.csRuntime/codebase/SolanaWalletAdapter.cs
- Add public modifier to Deauthorize and GetCapabilities in IAdapterOperations - Rethrow exception in ReconnectWallet so callers can detect failure - Add comment for null wallet case in DisconnectWallet and ReconnectWallet - Add XML doc comment to GetCapabilities explaining null return case
There was a problem hiding this comment.
♻️ Duplicate comments (2)
Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs (1)
250-270: 🧹 Nitpick | 🔵 TrivialGetCapabilities may return null on silent lambda failure.
Due to the async void lambda pattern, if
client.GetCapabilities()throws an exception, it will be swallowed,result.WasSuccessfulmay still be true (depending on how the scenario handles async exceptions), andcapabilitieswill remainnull. Callers should be aware of this edge case.This is part of the broader architectural concern deferred to PR
#260per retrieved learnings, so no immediate action is required. However, consider adding a null check with a warning before returning:♻️ Optional defensive check
if (!result.WasSuccessful) { Debug.LogError(result.Error.Message); throw new Exception(result.Error.Message); } + if (capabilities == null) + { + Debug.LogWarning("[MWA] GetCapabilities succeeded but returned null"); + } return capabilities;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs` around lines 250 - 270, GetCapabilities can return null if the async lambda passed to LocalAssociationScenario.StartAndExecute swallows exceptions (e.g., client.GetCapabilities() fails); add a defensive null-check after the scenario completes: if capabilities is null, log a descriptive error via Debug.LogError and throw an exception (or return a failed CapabilitiesResult) so callers never receive null. Locate the GetCapabilities method, the localAssociationScenario/StartAndExecute call and the client.GetCapabilities() invocation to implement the check and error handling.Runtime/codebase/SolanaWalletAdapter.cs (1)
101-111: 🧹 Nitpick | 🔵 TrivialReconnectWallet silently returns when no wallet is configured.
Unlike
DisconnectWalletwhich has an explanatory comment,ReconnectWalletsilently returns when_internalWalletis null. For a reconnect operation, callers might reasonably expect an exception or a return value indicating failure.Consider either adding a comment explaining the silent return, or throwing to be consistent with
_Login:♻️ Option 1: Add explanatory comment
if (_internalWallet != null) throw new NotImplementedException(); + // No internal wallet configured - nothing to reconnect }♻️ Option 2: Throw for consistency with _Login
if (_internalWallet != null) throw new NotImplementedException(); + throw new InvalidOperationException("No wallet configured to reconnect"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Runtime/codebase/SolanaWalletAdapter.cs` around lines 101 - 111, The ReconnectWallet method currently silently returns when _internalWallet is null; change it to be explicit like _Login by throwing an InvalidOperationException (or a more specific exception) when _internalWallet is null, and include a brief message such as "No wallet configured to reconnect" to aid callers; update the method handling in ReconnectWallet (and optionally add a short comment above it mirroring DisconnectWallet's style if you prefer the explanatory-comment approach) so callers receive a clear failure instead of a silent return.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`:
- Around line 250-270: GetCapabilities can return null if the async lambda
passed to LocalAssociationScenario.StartAndExecute swallows exceptions (e.g.,
client.GetCapabilities() fails); add a defensive null-check after the scenario
completes: if capabilities is null, log a descriptive error via Debug.LogError
and throw an exception (or return a failed CapabilitiesResult) so callers never
receive null. Locate the GetCapabilities method, the
localAssociationScenario/StartAndExecute call and the client.GetCapabilities()
invocation to implement the check and error handling.
In `@Runtime/codebase/SolanaWalletAdapter.cs`:
- Around line 101-111: The ReconnectWallet method currently silently returns
when _internalWallet is null; change it to be explicit like _Login by throwing
an InvalidOperationException (or a more specific exception) when _internalWallet
is null, and include a brief message such as "No wallet configured to reconnect"
to aid callers; update the method handling in ReconnectWallet (and optionally
add a short comment above it mirroring DisconnectWallet's style if you prefer
the explanatory-comment approach) so callers receive a clear failure instead of
a silent return.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 66ea8686-94c9-408a-a3c5-9f7fb360bcfe
📒 Files selected for processing (3)
Runtime/codebase/SolanaMobileStack/Interfaces/IAdapterOperations.csRuntime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.csRuntime/codebase/SolanaWalletAdapter.cs
- Add null check warning in GetCapabilities when result is null - Add explanatory comment in ReconnectWallet null wallet case
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs (1)
278-310:⚠️ Potential issue | 🔴 Critical
SignMessage()can dereferenceAccountafter a cold start.With the new token restore, this method can now run before
Login(). In that pathAccountis still null, so Line 309 can throw before the request is sent. Use the public key returned inauthorization(or the cachedpk) when the in-memoryAccountis unavailable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs` around lines 278 - 310, SignMessage() can dereference Account after a cold start; modify the SignMessages call in the SignMessage method so it uses the public key from the authorization result (authorization.PublicKey) or a cached public-key fallback instead of directly using Account.PublicKey.KeyBytes when Account is null. Concretely: inside the async client => { signedMessages = await client.SignMessages(...) } block, compute the address bytes as Account?.PublicKey?.KeyBytes ?? authorization?.PublicKey?.KeyBytes ?? cachedPublicKeyBytes (or similar cached pk field) and pass that into the addresses list so the call never dereferences a null Account.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`:
- Around line 140-141: The code restores _authToken from PlayerPrefs
unconditionally and only updates the in-memory _authToken after successful
Authorize()/Reauthorize(), which breaks the intended
_walletOptions.keepConnectionAlive behavior; change the restore so
PlayerPrefs.GetString("authToken") is only used when
_walletOptions.keepConnectionAlive is true (gate the restore behind that flag),
and after successful Authorize() and Reauthorize() persist the refreshed auth
token and PublicKey back to PlayerPrefs (PlayerPrefs.SetString("authToken", ...)
and SetString("publicKey", ...) or remove them when keepConnectionAlive is
false) so both memory and persistent state remain consistent. Apply the same
gating and persistence fixes where _authToken/PublicKey are read or written
(refer to usages around the existing _authToken read, and the Authorize(),
Reauthorize() methods and the code regions you flagged at 178-179, 278-279,
319-320).
- Around line 60-88: The code currently constructs Account using the cached pk
even after a successful Reauthorize; instead, use the Reauthorize result's
public key (reauth.PublicKey) as the source of truth: when
reauthorizeResult.WasSuccessful and reauth is non-null and reauth.PublicKey is
not empty, persist that public key back to the same PlayerPrefs key you
originally read pk from and construct/return new Account using reauth.PublicKey
(rather than pk); update any local variables that represent the cached key so
subsequent logic uses reauth.PublicKey.
---
Outside diff comments:
In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`:
- Around line 278-310: SignMessage() can dereference Account after a cold start;
modify the SignMessages call in the SignMessage method so it uses the public key
from the authorization result (authorization.PublicKey) or a cached public-key
fallback instead of directly using Account.PublicKey.KeyBytes when Account is
null. Concretely: inside the async client => { signedMessages = await
client.SignMessages(...) } block, compute the address bytes as
Account?.PublicKey?.KeyBytes ?? authorization?.PublicKey?.KeyBytes ??
cachedPublicKeyBytes (or similar cached pk field) and pass that into the
addresses list so the call never dereferences a null Account.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fac9c4e3-a6c7-4b9d-b820-c687ec938948
📒 Files selected for processing (2)
Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.csRuntime/codebase/SolanaWalletAdapter.cs
Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs
Outdated
Show resolved
Hide resolved
Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs
Outdated
Show resolved
Hide resolved
- Use reauth.PublicKey as source of truth after successful reauthorize instead of cached PlayerPrefs pk. Falls back to cached pk only when wallet returns no key. - Gate authToken PlayerPrefs restore and persist behind keepConnectionAlive flag in _SignAllTransactions() and SignMessage(). Persists rotated token after every successful signing operation. - Guard Account.PublicKey.KeyBytes in SignMessage() against null Account on cold start using cached pk from PlayerPrefs as fallback. Throws clear exception if neither is available.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`:
- Around line 291-294: The code captures cachedPk from Account/PublicKey or
PlayerPrefs before the wallet authorization, which can mismatch the authorized
session; update the signing flow to use the authorized public key
(authorization.PublicKey) when present instead of the pre-captured cachedPk —
move or perform the signing logic inside the same callback/lambda where
authorization (from Authorize() or Reauthorize()) is assigned, or after
authorization completes, and fall back to Account?.PublicKey or PlayerPrefs only
if authorization.PublicKey is null; ensure you update references to cachedPk
used in the signer method to read authorization.PublicKey when available to keep
the signed message consistent with the active authorized account.
- Around line 279-283: In SolanaMobileWalletAdapter.GetCapabilities, instead of
logging a warning and returning null when capabilities is null after a
successful RPC (result.WasSuccessful == true), throw an exception (e.g.,
InvalidOperationException) with a clear message that the RPC completed but
returned no capabilities; include any available context (method name, maybe
result metadata) in the exception message so callers cannot mistakenly treat a
null as success and will be forced to handle the unexpected empty response.
- Around line 85-93: When reauthorizeResult.WasSuccessful is true you still must
verify that _authToken was actually populated; update the block handling
reauthorizeResult (check _authToken for null/empty after reauthorization) and
only persist PlayerPrefs and return the new Account (using resolvedKey from
reauthPublicKey or pk) if _authToken is non-empty. If _authToken is empty, treat
the reauthorization as failed (clear any stale PlayerPrefs authToken if present,
do not return the Account, and surface the failure path — e.g., return a
failure/null result or throw) so subsequent calls will invoke the proper
authorization flow instead of assuming success.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 50817458-72c4-4192-a71f-057694889658
📒 Files selected for processing (1)
Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs
Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs
Outdated
Show resolved
Hide resolved
- Treat reauthorize as failure when WasSuccessful but token is empty Clear cached keys and fall through to full Authorize flow - Throw exception in GetCapabilities when RPC succeeds but returns null data instead of silently returning null to caller
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs (1)
336-346:⚠️ Potential issue | 🟡 MinorPotential null dereference if
authorizationis not populated.Same issue as in
_SignAllTransactions: if the first lambda'sAuthorize/Reauthorizecall fails silently,authorizationcould remain null at line 341.🛡️ Proposed defensive check
if (!result.WasSuccessful) { Debug.LogError(result.Error.Message); throw new Exception(result.Error.Message); } + if (authorization == null) + { + throw new Exception("[MWA] SignMessage: authorization was not populated"); + } _authToken = authorization.AuthToken;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs` around lines 336 - 346, authorization may be null even when result.WasSuccessful is true, so add a defensive null-check before reading authorization.AuthToken in the code after the authorization flow (and mirror the same fix in _SignAllTransactions): if authorization is null or authorization.AuthToken is null/empty, log a clear error via Debug.LogError (include context like "authorization is null after Authorize/Reauthorize") and throw a meaningful Exception instead of dereferencing; otherwise assign _authToken and continue with the existing PlayerPrefs logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`:
- Around line 85-104: The reauthorization-success branch checks
reauthorizeResult.WasSuccessful and then deletes PlayerPrefs keys when
_authToken is empty, but execution then falls through to the final cleanup that
deletes the same keys again; modify the control flow in the block that checks
_authToken (inside the reauthorizeResult.WasSuccessful branch in
SolanaMobileWalletAdapter.cs) so cleanup happens only once—either by returning
immediately after performing the deletion for the empty-token case or by
restructuring the logic to have a single shared cleanup path after the
reauthorizeResult handling; reference the reauthorizeResult.WasSuccessful check,
the _authToken variable, the reauthPublicKey/pk resolution that returns new
Account(...), and PlayerPrefs.DeleteKey/PlayerPrefs.Save to locate and update
the code.
- Around line 190-196: The code assumes authorization is non-null when setting
_authToken and persisting it; add a defensive null check for the local variable
authorization before using it (e.g., inside the method that currently references
authorization, check if authorization == null), and if null either throw a clear
exception or return an appropriate empty result (e.g., an empty Transaction[]),
avoiding the NullReferenceException; ensure the logic that persists _authToken
(when _walletOptions.keepConnectionAlive) and the subsequent use of
res.SignedPayloads/Transaction.Deserialize only runs when authorization is
non-null.
---
Outside diff comments:
In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`:
- Around line 336-346: authorization may be null even when result.WasSuccessful
is true, so add a defensive null-check before reading authorization.AuthToken in
the code after the authorization flow (and mirror the same fix in
_SignAllTransactions): if authorization is null or authorization.AuthToken is
null/empty, log a clear error via Debug.LogError (include context like
"authorization is null after Authorize/Reauthorize") and throw a meaningful
Exception instead of dereferencing; otherwise assign _authToken and continue
with the existing PlayerPrefs logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 723be0b8-31e1-421a-b7f1-a3bb366910b9
📒 Files selected for processing (1)
Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs
- Remove redundant PlayerPrefs deletion in empty-token reauthorize failure path, replace with fall-through comment - Add null guard on authorization object after StartAndExecute in _Login(), _SignAllTransactions(), and SignMessage() to prevent silent NullReferenceException from async void lambda edge case
Contributor
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs (2)
94-97:⚠️ Potential issue | 🟠 MajorPersist the reauthorized public key, not just the token.
This branch returns
resolvedKeybut only writesauthTokenback to disk. If the wallet reauthorizes a different account,PlayerPrefs["pk"]stays stale and a later cold-startSignMessage()can still target the old address. SaveresolvedKeyalongside the refreshed token, and mirror that refresh after successful auth in the signing methods.Based on learnings:
SignMessage()intentionally usescachedPkfromAccount?.PublicKeyorPlayerPrefs.GetString("pk")under the currentAction<IAdapterOperations>pattern, so stale persisted keys remain on the active signing path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs` around lines 94 - 97, The branch that handles reauthorization currently persists only the refreshed _authToken via PlayerPrefs.SetString("authToken", _authToken) but returns resolvedKey; update this block to also persist the resolvedKey (e.g., PlayerPrefs.SetString("pk", resolvedKey) and PlayerPrefs.Save()) so PlayerPrefs["pk"] is not stale, and ensure after successful auth the in-memory Account/PublicKey (and any cachedPk used by SignMessage) is updated to the new PublicKey so signing methods (SignMessage and any code reading Account?.PublicKey or PlayerPrefs.GetString("pk")) see the refreshed key immediately.
197-202:⚠️ Potential issue | 🟠 MajorDon't replace a valid session with an empty
AuthToken.
_Login()already treats a successfulReauthorize()with no token as failure, but both signing paths still assign and persistauthorization.AuthTokenunconditionally. If the wallet reports success without a token here, the current sign works and the next request falls back toAuthorize()again, reintroducing the mid-session prompt this PR is fixing. Only overwrite/persist the token when it is non-empty; otherwise keep the previous token or surface the failure explicitly.Also applies to: 352-356
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs` around lines 197 - 202, The code is overwriting and persisting _authToken with authorization.AuthToken even when that token is empty; change the signing-path assignments (the block that sets _authToken and calls PlayerPrefs.SetString when _walletOptions.keepConnectionAlive is true) to only update and persist the token if authorization.AuthToken is not null/empty—leave the existing _authToken unchanged (or surface the failure) when the returned AuthToken is empty; apply the same conditional check to the other occurrence around the code referenced at lines 352-356 so both signing flows follow the same non-empty-token guard and avoid replacing a valid session with an empty token.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`:
- Around line 139-141: The code is storing the wallet bearer token in
PlayerPrefs (PlayerPrefs.SetString("authToken", _authToken)), which is insecure;
replace use of PlayerPrefs for the auth token with platform-secure storage:
persist _authToken into an encrypted/keystore-backed storage (e.g., Android
Keystore / EncryptedSharedPreferences or an existing secure-storage
abstraction), and update load/clear logic used by Reauthorize() and
Deauthorize() to read/erase from that secure storage instead of PlayerPrefs
(leave publicKey in PlayerPrefs if acceptable). Ensure the new storage API
encrypts at rest and is used for Set/Get/Delete of the auth token across the
SolanaMobileWalletAdapter class.
- Around line 193-203: Guard the signing result before dereferencing: in
SignAllTransactions (and the similar SignTransaction(s) path around the 348-358
range) check that the local result object (res) and its payload collection
(res.SignedPayloads / signedMessages) are not null before calling .Select(...)
or Transaction.Deserialize; if they are null, handle gracefully (e.g. return an
empty Transaction[] or throw a clear exception) and ensure the failure is
propagated (set the operation result as failed) so swallowed exceptions in the
async Action<IAdapterOperations> callback can't lead to NullReferenceException.
---
Duplicate comments:
In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`:
- Around line 94-97: The branch that handles reauthorization currently persists
only the refreshed _authToken via PlayerPrefs.SetString("authToken", _authToken)
but returns resolvedKey; update this block to also persist the resolvedKey
(e.g., PlayerPrefs.SetString("pk", resolvedKey) and PlayerPrefs.Save()) so
PlayerPrefs["pk"] is not stale, and ensure after successful auth the in-memory
Account/PublicKey (and any cachedPk used by SignMessage) is updated to the new
PublicKey so signing methods (SignMessage and any code reading
Account?.PublicKey or PlayerPrefs.GetString("pk")) see the refreshed key
immediately.
- Around line 197-202: The code is overwriting and persisting _authToken with
authorization.AuthToken even when that token is empty; change the signing-path
assignments (the block that sets _authToken and calls PlayerPrefs.SetString when
_walletOptions.keepConnectionAlive is true) to only update and persist the token
if authorization.AuthToken is not null/empty—leave the existing _authToken
unchanged (or surface the failure) when the returned AuthToken is empty; apply
the same conditional check to the other occurrence around the code referenced at
lines 352-356 so both signing flows follow the same non-empty-token guard and
avoid replacing a valid session with an empty token.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cfe3add6-7df9-400a-9c61-3418bc483eb0
📒 Files selected for processing (1)
Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs
Comment on lines
139
to
+141
| PlayerPrefs.SetString("pk", publicKey.ToString()); | ||
| PlayerPrefs.SetString("authToken", _authToken); | ||
| PlayerPrefs.Save(); |
There was a problem hiding this comment.
Avoid storing the wallet bearer token in PlayerPrefs.
authToken is now the credential that drives silent Reauthorize() / Deauthorize(). PlayerPrefs is plain app storage on Android, so a local compromise or backup extraction can replay a wallet session. Please move this to secure storage (Keystore / EncryptedSharedPreferences, or an equivalent abstraction).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs` around lines
139 - 141, The code is storing the wallet bearer token in PlayerPrefs
(PlayerPrefs.SetString("authToken", _authToken)), which is insecure; replace use
of PlayerPrefs for the auth token with platform-secure storage: persist
_authToken into an encrypted/keystore-backed storage (e.g., Android Keystore /
EncryptedSharedPreferences or an existing secure-storage abstraction), and
update load/clear logic used by Reauthorize() and Deauthorize() to read/erase
from that secure storage instead of PlayerPrefs (leave publicKey in PlayerPrefs
if acceptable). Ensure the new storage API encrypts at rest and is used for
Set/Get/Delete of the auth token across the SolanaMobileWalletAdapter class.
Contributor
Author
There was a problem hiding this comment.
Acknowledged. Secure storage is the correct long-term solution and is planned via the IMwaAuthCache abstraction. The current PlayerPrefs implementation is the default fallback intentionally designed to be replaceable. Developers can inject a secure storage backend by implementing IMwaAuthCache. Tracked for a follow-up PR.
There was a problem hiding this comment.
@JoshhSandhu, understood — the IMwaAuthCache injection pattern is a clean approach; keeping PlayerPrefs as the default fallback while letting consumers swap in an EncryptedSharedPreferences-backed or Keystore-backed implementation is a solid design.
Would you like me to open a GitHub issue to formally track the IMwaAuthCache secure-storage implementation so it doesn't get lost between PRs?
✏️ Learnings added
Learnt from: JoshhSandhu
Repo: magicblock-labs/Solana.Unity-SDK PR: 269
File: Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs:139-141
Timestamp: 2026-03-29T12:58:53.254Z
Learning: In `Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`, storing `_authToken` in `PlayerPrefs` is an intentional default fallback. The long-term plan is to introduce an `IMwaAuthCache` abstraction that developers can implement to inject platform-secure storage (e.g., Android Keystore / EncryptedSharedPreferences). Do not raise the PlayerPrefs-for-authToken pattern as a blocking issue; note it only as a follow-up improvement tracked via the IMwaAuthCache abstraction.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: JoshhSandhu
Repo: magicblock-labs/Solana.Unity-SDK PR: 269
File: Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs:291-294
Timestamp: 2026-03-29T01:23:38.676Z
Learning: In SolanaMobileStack C# code, if you have multiple lambdas where the first lambda mutates/sets values captured from the outer scope and the second lambda is run later, do not rely on those captured mutations being visible across lambdas when using the current `async void Action<IAdapterOperations>` pattern. Treat cross-lambda variable sharing via mutated closures as unsafe. Prefer using explicit data flow (e.g., compute the needed values up front and store them in variables that won’t be mutated later, or redesign the pipeline to use `Func<IAdapterOperations, Task>`/pass required values as parameters) so the correct PublicKey/value used for signing is unambiguous.
Contributor
Author
There was a problem hiding this comment.
Yes please go ahead and open that issue.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
gh issue create \
--repo magicblock-labs/Solana.Unity-SDK \
--title "feat(MWA): Introduce IMwaAuthCache abstraction for secure auth token storage" \
--assignee "JoshhSandhu" \
--body "## Summary
Currently, \`SolanaMobileWalletAdapter\` stores the MWA \`authToken\` in \`PlayerPrefs\` as a default fallback. While functional, \`PlayerPrefs\` is plain unencrypted app storage on Android, meaning a local compromise or backup extraction could expose the bearer token and allow session replay.
The agreed long-term solution is to introduce an \`IMwaAuthCache\` abstraction that developers can implement to inject platform-secure storage backends (e.g., Android Keystore / EncryptedSharedPreferences).
## Proposed Work
- Define an \`IMwaAuthCache\` interface with \`Set\`, \`Get\`, and \`Delete\` methods for the auth token.
- Update \`SolanaMobileWalletAdapter\` to use \`IMwaAuthCache\` for all \`_authToken\` read/write/clear operations across:
- \`_Login()\`
- \`_SignAllTransactions()\`
- \`SignMessage()\`
- \`Logout()\`
- \`DisconnectWallet()\`
- \`ReconnectWallet()\`
- Provide a default \`PlayerPrefsAuthCache : IMwaAuthCache\` implementation as the fallback (preserving current behaviour).
- Document how developers can supply a secure \`EncryptedSharedPreferences\`-backed or Keystore-backed implementation via constructor injection.
## References
- PR where this was discussed: magicblock-labs/Solana.Unity-SDK#269
- Review comment: https://github.com/magicblock-labs/Solana.Unity-SDK/pull/269#discussion_r3006148255
- Requested by: `@JoshhSandhu`
"
Length of output: 76
🏁 Script executed:
#!/bin/bash
gh issue create \
--repo magicblock-labs/Solana.Unity-SDK \
--title "feat(MWA): Introduce IMwaAuthCache abstraction for secure auth token storage" \
--body "## Summary
Currently, \`SolanaMobileWalletAdapter\` stores the MWA \`authToken\` in \`PlayerPrefs\` as a default fallback. While functional, \`PlayerPrefs\` is plain unencrypted app storage on Android, meaning a local compromise or backup extraction could expose the bearer token and allow session replay.
The agreed long-term solution is to introduce an \`IMwaAuthCache\` abstraction that developers can implement to inject platform-secure storage backends (e.g., Android Keystore / EncryptedSharedPreferences).
## Proposed Work
- Define an \`IMwaAuthCache\` interface with \`Set\`, \`Get\`, and \`Delete\` methods for the auth token.
- Update \`SolanaMobileWalletAdapter\` to use \`IMwaAuthCache\` for all \`_authToken\` read/write/clear operations across:
- \`_Login()\`
- \`_SignAllTransactions()\`
- \`SignMessage()\`
- \`Logout()\`
- \`DisconnectWallet()\`
- \`ReconnectWallet()\`
- Provide a default \`PlayerPrefsAuthCache : IMwaAuthCache\` implementation as the fallback (preserving current behaviour).
- Document how developers can supply a secure \`EncryptedSharedPreferences\`-backed or Keystore-backed implementation via constructor injection.
## References
- PR where this was discussed: magicblock-labs/Solana.Unity-SDK#269
- Review comment: https://github.com/magicblock-labs/Solana.Unity-SDK/pull/269#discussion_r3006148255
- Requested by: `@JoshhSandhu`
"
Length of output: 62
@JoshhSandhu, the issue has been created: #272 — feat(MWA): Introduce IMwaAuthCache abstraction for secure auth token storage.
It covers:
- Defining the
IMwaAuthCacheinterface (Set/Get/Delete) - Updating all
_authTokenread/write/clear sites inSolanaMobileWalletAdapterto use it - Providing a default
PlayerPrefsAuthCache : IMwaAuthCachefallback to preserve current behaviour - Documenting secure backend injection (Android Keystore / EncryptedSharedPreferences)
Note: The bot integration doesn't have permission to set assignees on this repository, so you may want to assign it to yourself manually on the issue page.
🧠 Learnings used
Learnt from: JoshhSandhu
Repo: magicblock-labs/Solana.Unity-SDK PR: 269
File: Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs:139-141
Timestamp: 2026-03-29T12:58:55.890Z
Learning: In `Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`, storing `_authToken` in `PlayerPrefs` is an intentional default fallback. The long-term plan is to introduce an `IMwaAuthCache` abstraction that developers can implement to inject platform-secure storage (e.g., Android Keystore / EncryptedSharedPreferences). Do not raise the PlayerPrefs-for-authToken pattern as a blocking issue; note it only as a follow-up improvement tracked via the IMwaAuthCache abstraction.
Learnt from: JoshhSandhu
Repo: magicblock-labs/Solana.Unity-SDK PR: 269
File: Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs:291-294
Timestamp: 2026-03-29T01:23:38.676Z
Learning: In SolanaMobileStack C# code, if you have multiple lambdas where the first lambda mutates/sets values captured from the outer scope and the second lambda is run later, do not rely on those captured mutations being visible across lambdas when using the current `async void Action<IAdapterOperations>` pattern. Treat cross-lambda variable sharing via mutated closures as unsafe. Prefer using explicit data flow (e.g., compute the needed values up front and store them in variables that won’t be mutated later, or redesign the pipeline to use `Func<IAdapterOperations, Task>`/pass required values as parameters) so the correct PublicKey/value used for signing is unambiguous.
Learnt from: JoshhSandhu
Repo: magicblock-labs/Solana.Unity-SDK PR: 269
File: Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs:63-93
Timestamp: 2026-03-29T00:04:25.932Z
Learning: In `magicblock-labs/Solana.Unity-SDK`, the `async client => { ... }` lambdas passed to `LocalAssociationScenario.StartAndExecute()` use `Action<IAdapterOperations>` (effectively async void), which can swallow exceptions. This is a known architectural issue intentionally left unchanged to avoid conflicting with PR `#260`, which is rewriting `LocalAssociationScenario.ExecuteNextAction()` and is expected to migrate the delegate type to `Func<IAdapterOperations, Task>`.
Added null checks for res in _SignAllTransactions() and signedMessages in SignMessage() to prevent NullReferenceException when the signing callback fails silently due to the async void Action pattern.
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Contributor
Author
This was referenced Apr 4, 2026
This was referenced Apr 7, 2026
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
1 participant
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.
Problem
While building Unity Android games on Solana (CrossyRoad and SolRacer),
I found that after an explicit logout, the wallet silently reconnected
on the next app launch without showing an authorization prompt. The
player tapped Logout but the game behaved as if they never did.
Debugging traced this to three gaps in SolanaMobileWalletAdapter:
Logout() never set _authToken = null. Within the same session,
signing operations after logout would attempt Reauthorize() with
a token the user intended to revoke.
keepConnectionAlive = true restored Account from PlayerPrefs["pk"]
without verifying the auth session. On process restart, _authToken
was null, so the first SignTransaction() call triggered a full
Authorize() popup during gameplay instead of at login time.
IAdapterOperations had no Deauthorize() method. The wallet app was
never notified of logout. Auth tokens remained valid indefinitely.
Solution
Lifecycle fixes (closes #267):
cached Account. Fall through to full Authorize() if token is invalid
or missing.
and SignMessage() to handle cold start correctly
New RPC operations (MWA spec):
CapabilitiesResult with MaxTransactionsPerRequest,
MaxMessagesPerRequest, SupportedTransactionVersions,
SupportsCloneAuthorization
New lifecycle API:
for cold start, sends Deauthorize RPC, clears all auth state.
Deauthorize is best-effort, local cleanup always runs.
fires OnWalletReconnected on success
SolanaMobileWalletAdapter and SolanaWalletAdapter
Before & After Screenshots
BEFORE -> wallet silently reconnects after explicit logout:
Open app
Connect wallet -- Seeker authorization prompt appears -- Approve
Close app
Reopen app -- No prompt -- wallet silently reconnected
Logout -- token never actually revoked on wallet side
AFTER -> auth cache correctly invalidated on logout:
AfterChanges.1.mp4
Open app
Connect wallet -- Seeker authorization prompt appears -- Approve
Close app without logout
Reopen app -- Silent reauthorize -- Connected instantly -- No popup
Logout -- Deauthorize RPC sent to Seeker -- cache cleared
Reopen app -- Seeker authorization prompt appears -- Fresh approval required
Tested on Android (Seeker wallet via Mobile Wallet Adapter) in:
Other changes
following same Preserve and JsonProperty conventions
local logout
signatures or public method signatures changed
Summary by CodeRabbit
New Features
Chores / Behavior