feat(mwa): add injectable auth token cache abstraction#284
Open
JoshhSandhu wants to merge 3 commits into
Open
Conversation
Issue magicblock-labs#271 needs pluggable auth token persistence so games can use Keystore/EncryptedSharedPreferences instead of hardcoded PlayerPrefs. Default cache keeps key `solana_sdk.mwa.auth_token`, preserving PR magicblock-labs#269 sessions with no migration while enabling custom secure backends. Refs magicblock-labs#271
|
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 an IMwaAuthCache abstraction with a default PlayerPrefsAuthCache, injects it into wallet adapters for auth-token persistence, updates adapter logic to use the cache, and adds EditMode tests and Unity .meta files for the new assets. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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: 3
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)
130-138:⚠️ Potential issue | 🟠 MajorReset in-memory token when cached credentials are invalidated.
At Line 130-133 and Line 136-138, persisted credentials are cleared but
_authTokenis not. A stale in-memory token can survive this branch and drive later calls downReauthorize(...)incorrectly.Suggested fix
// Reauthorize failed or returned empty token - clear cached credentials + _authToken = null; PlayerPrefs.DeleteKey(PrefKeyPublicKey); PlayerPrefs.Save(); await _authCache.Clear(); } else if (!pk.IsNullOrEmpty()) { + _authToken = null; PlayerPrefs.DeleteKey(PrefKeyPublicKey); PlayerPrefs.Save(); }🤖 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 130 - 138, When clearing persisted credentials in SolanaMobileWalletAdapter (the branches that call PlayerPrefs.DeleteKey(PrefKeyPublicKey); PlayerPrefs.Save(); and await _authCache.Clear()), also reset the in-memory token by setting _authToken = null (or equivalent cleared state) so a stale in-memory token cannot survive and trigger Reauthorize(...); update both the branch that awaits _authCache.Clear() and the else branch that only deletes PlayerPrefs to clear _authToken alongside existing cleanup calls.
🤖 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/MwaAuthCache/IMwaAuthCache.cs.meta`:
- Around line 3-11: The .cs.meta file for IMwaAuthCache includes an unnecessary
MonoImporter block; edit IMwaAuthCache.cs.meta to remove the entire MonoImporter
section and leave only the minimal metadata fields (fileFormatVersion and guid)
to match the repository convention for C# .cs.meta files (i.e., ensure only
fileFormatVersion and guid keys remain, removing externalObjects,
serializedVersion, defaultReferences, executionOrder, icon, userData,
assetBundleName, and assetBundleVariant entries).
In
`@Runtime/codebase/SolanaMobileStack/MwaAuthCache/PlayerPrefsAuthCache.cs.meta`:
- Around line 3-11: The .cs.meta contains an unnecessary MonoImporter block;
replace it with the repo's minimal C# meta format by removing the MonoImporter
section and leaving only fileFormatVersion and guid entries (ensure the existing
guid value is preserved and include the correct fileFormatVersion key), i.e.,
trim everything referencing
MonoImporter/externalObjects/serializedVersion/defaultReferences/executionOrder/icon/userData/assetBundleName/assetBundleVariant
and keep only the minimal metadata keys fileFormatVersion and guid.
In `@Tests/EditMode/MwaAuthCache/PlayerPrefsAuthCacheTests.cs.meta`:
- Around line 3-11: Replace the verbose MonoImporter block in the meta file with
the minimal convention: remove the entire MonoImporter section (externalObjects,
serializedVersion, defaultReferences, executionOrder, icon, userData,
assetBundleName, assetBundleVariant) and leave only fileFormatVersion and guid
entries (preserve the existing guid value and set fileFormatVersion to the repo
standard). Ensure no other keys remain so the .cs.meta matches the minimal C#
meta format used elsewhere.
---
Outside diff comments:
In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`:
- Around line 130-138: When clearing persisted credentials in
SolanaMobileWalletAdapter (the branches that call
PlayerPrefs.DeleteKey(PrefKeyPublicKey); PlayerPrefs.Save(); and await
_authCache.Clear()), also reset the in-memory token by setting _authToken = null
(or equivalent cleared state) so a stale in-memory token cannot survive and
trigger Reauthorize(...); update both the branch that awaits _authCache.Clear()
and the else branch that only deletes PlayerPrefs to clear _authToken alongside
existing cleanup calls.
🪄 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: ee2c5f80-edbe-4f8c-bbf8-47b9515bfed9
📒 Files selected for processing (10)
Runtime/codebase/SolanaMobileStack/MwaAuthCache.metaRuntime/codebase/SolanaMobileStack/MwaAuthCache/IMwaAuthCache.csRuntime/codebase/SolanaMobileStack/MwaAuthCache/IMwaAuthCache.cs.metaRuntime/codebase/SolanaMobileStack/MwaAuthCache/PlayerPrefsAuthCache.csRuntime/codebase/SolanaMobileStack/MwaAuthCache/PlayerPrefsAuthCache.cs.metaRuntime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.csRuntime/codebase/SolanaWalletAdapter.csTests/EditMode/MwaAuthCache.metaTests/EditMode/MwaAuthCache/PlayerPrefsAuthCacheTests.csTests/EditMode/MwaAuthCache/PlayerPrefsAuthCacheTests.cs.meta
Stale _authToken could survive prefs/cache wipe and push the next login down Reauthorize() with a known-bad value. Trim new C# .cs.meta to repo minimal format (fileFormatVersion + guid). Refs magicblock-labs#271
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 264-271: Logout() currently calls
_authCache.Clear().GetAwaiter().GetResult() synchronously which can throw and
abort the logout flow (preventing DisconnectWallet() from reaching
OnWalletDisconnected); wrap the synchronous cache clear call in a try/catch that
catches Exception, logs the failure (including exception details) and continues
so Logout() completes; apply the same guarded pattern to the other synchronous
cache clear usage at the location referenced (lines ~305-306) to ensure
DisconnectWallet()/OnWalletDisconnected always runs even if
IMwaAuthCache.Clear() fails.
🪄 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: 9cb9c8f7-eed2-420d-8aad-ec77b4a95f2f
📒 Files selected for processing (4)
Runtime/codebase/SolanaMobileStack/MwaAuthCache/IMwaAuthCache.cs.metaRuntime/codebase/SolanaMobileStack/MwaAuthCache/PlayerPrefsAuthCache.cs.metaRuntime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.csTests/EditMode/MwaAuthCache/PlayerPrefsAuthCacheTests.cs.meta
Custom cache backends can throw on clear. Log and continue so DisconnectWallet still invokes OnWalletDisconnected. Refs magicblock-labs#271
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Contributor
Author
|
@Kuldotha Lmk if you want me to walk you through this one when you have a chance to review. CodeRabbit checks are all green! |
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
PR #269 fixed the auth-token lifecycle but left token persistence hardcoded
to
PlayerPrefs. PlayerPrefs is plaintext app storage on Android: a localdevice compromise, an
adb backup, or a rooted-device dump can extract thebearer token and replay the session against the wallet.
Issues #271 and #272 (duplicate trackers, same ask) request the SDK make
this layer pluggable so developers shipping production games can inject
platform-secure storage (Android Keystore, EncryptedSharedPreferences, iOS
Keychain) without touching SDK internals.
There is no abstraction today.
SolanaMobileWalletAdapterwrites the tokendirectly to a hardcoded
PlayerPrefskey, and every read/write/delete sitein
_Login,_SignAllTransactions,SignMessage,Logout, andDisconnectWalletdoes the same thing inline.Solution
Introduce a narrow
IMwaAuthCacheinterface (Get/Set/Clear,auth-token only) plus a default
PlayerPrefsAuthCacheimplementation thatpreserves the existing storage key
solana_sdk.mwa.auth_token. Bothadapters accept the cache as an optional constructor argument that defaults
to the PlayerPrefs implementation.
Why a 3-method interface, token only:
backend to encrypt them adds work for no security benefit.
and harder to ship a buggy one.
Why keep
solana_sdk.mwa.auth_tokenas the default key:mainafter PR Fix/mwa keep connection alive auth token restore #269 keep their session onupgrade; no migration code, no silent logout.
(
PlayerPrefsAuthCache.DefaultKey);SolanaMobileWalletAdapter.PrefKeyAuthTokenaliases it as a compile-time constant chain.
Backward compatibility:
Logout()staysoverride void, theWalletBaseoverride contract isunchanged. The cache
Clear()call is awaited synchronously inside it.IMwaAuthCache authCache = nullparameter at the end with default
null. Every existing call site(including
Web3.LoginWalletAdapter()) compiles unchanged.MigrateLegacyPrefKeys()from PR Fix/mwa keep connection alive auth token restore #269 is left untouched: pre-Fix/mwa keep connection alive auth token restore #269 installsupgrading directly to this PR still get migrated from the legacy
pk/authTokenkeys.Optional per-identity scoping:
new PlayerPrefsAuthCache("phantom")writes tosolana_sdk.mwa.auth_token.phantom. Useful for apps that connect tomultiple wallet identities. Default (no scope) is backward compatible.
Before & After Screenshots
BEFORE: Token persistence hardcoded to
PlayerPrefsAFTER: Adapter routes through
IMwaAuthCache, default impl preserves PR #269 behavior, custom impls plug in via constructor:IMWA_authCashe.3.mp4
Custom cache injection example (lives in PR body, not in SDK):
Other changes (e.g. bug fixes, small refactors)
"solana_sdk.mwa.auth_token"now lives only inPlayerPrefsAuthCache.DefaultKey.SolanaMobileWalletAdapter.PrefKeyAuthTokenis now
private const string PrefKeyAuthToken = PlayerPrefsAuthCache.DefaultKey;,preserving the named constant pattern requested on PR Fix/mwa keep connection alive auth token restore #269 while removing
the duplicated literal.
await _authCache.Get()/await _authCache.Set(...)/await _authCache.Clear())inside the existing
asyncmethods.Logout()is the only sync entrypoint; it uses
.GetAwaiter().GetResult()onClear()to keep theWalletBaseoverride contract.[Preserve]attribute onPlayerPrefsAuthCacheso IL2CPP code strippingdoes not remove it in AOT builds.
IAdapterOperations(still 6 methods),MigrateLegacyPrefKeysbody /signature / access untouched,
PrefKeyPublicKeyandPrefKeyAuthTokenconstants still declared. All four Feat(test)/mwa keep connection alive tests #283 reflection tests stay green.
Deploy Notes
Runtime change. No new external dependencies. No Unity Editor / build
pipeline changes. No new package references in
EditMode.asmdef.New scripts:
Runtime/codebase/SolanaMobileStack/MwaAuthCache/IMwaAuthCache.cs: 3-method interface (Get/Set/Clear) for auth-token persistence.Runtime/codebase/SolanaMobileStack/MwaAuthCache/PlayerPrefsAuthCache.cs: defaultIMwaAuthCacheimplementation backed byPlayerPrefs. Optional scope arg on the second constructor for per-identity bucketing.Tests/EditMode/MwaAuthCache/PlayerPrefsAuthCacheTests.cs: ~14 EditMode tests covering round-trip, no-op on empty/null input, idempotent clear, scope isolation, backward-compat with the PR Fix/mwa keep connection alive auth token restore #269 key, and reflection guards on both adapter constructors.Modified scripts:
Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs: routes every auth-token read/write through_authCache, accepts optionalIMwaAuthCachector argument, retainsMigrateLegacyPrefKeysand bothPrefKey*constants for PR Feat(test)/mwa keep connection alive tests #283 compatibility.Runtime/codebase/SolanaWalletAdapter.cs: forwards the optional cache to the AndroidSolanaMobileWalletAdapter. WebGL and iOS branches ignore the parameter (those platforms do not use MWA bearer tokens).New dependencies:
Tested on Android (Solana Seeker via Mobile Wallet Adapter) in CrossyRoad: https://github.com/JoshhSandhu/CrossyRoad
Closes #271
Closes #272
Summary by CodeRabbit