feat(mwa-v2): complete MWA v2 rewrite with docs#285
Open
Zurcusa wants to merge 24 commits into
Open
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (9)
WalkthroughAdds Mobile Wallet Adapter v2: async/cancellable CAIP-2 chain APIs, raw JSON‑RPC path and JsonRpcException mapping, strict authorization parser, IAuthorizationCache + PlayerPrefs default, adapter refactor with concurrency gate, session endianness fix, extensive EditMode tests/fixtures, documentation, and repo/.meta updates. ChangesMobile Wallet Adapter v2 refactor and ecosystem updates
Estimated code review effort Possibly related issues
Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 37
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/mwa-cache-guide.md`:
- Around line 50-52: The heading "Example: In-Memory Cache" is misleading
because DemoAuthorizationCache uses PlayerPrefs.GetString/SetString
(disk-backed); update the doc by either renaming the heading to clarify it's a
PlayerPrefs-backed cache (suggested: "Example: PlayerPrefs-backed Authorization
Cache" or "Example: Custom-Key PlayerPrefs Cache") and add a short note that
DemoAuthorizationCache persists to PlayerPrefs, or replace the example body with
a true in-memory implementation that mirrors InMemoryCache (as shown in
CustomCacheInjectionTest.cs) which stores state in a private field; ensure you
reference DemoAuthorizationCache and/or InMemoryCache in the text so readers
know which implementation the heading refers to.
In `@docs/mwa-method-reference.md`:
- Around line 158-159: SignAndSendTransaction currently returns a base64
signature but must return a base58-encoded transaction ID; update the code path
in SolanaMobileWalletAdapter.cs that reads success.Signatures[0] so it encodes
that signature in base58 (not base64) before populating and returning
RequestResult<string> (the value used as result.Result by SwapScreen.cs,
ReceiveScreen.cs, NftMint.cs and RPC/ConfirmTransaction calls). Locate the
conversion around success.Signatures[0] in the SignAndSendTransaction method and
replace the base64 conversion with the library/utility call that converts the
raw signature bytes to a base58 string so explorers and ConfirmTransaction
receive a proper Solana txid.
In `@docs/mwa-quick-start.md`:
- Around line 20-23: The markdown block starting with the fenced code block that
contains the JSON line for "com.solana.unity_sdk" lacks a blank line before the
opening ```; edit the docs/mwa-quick-start.md content around that paragraph and
insert a single blank line immediately before the fenced code block so the code
fence is separated from the preceding prose (satisfying MD031).
In `@docs/sign-and-send-investigation.md`:
- Around line 21-154: The markdown violates MD022/MD031/MD040: add a blank line
before and after each heading (e.g., "### Backpack (fails)", "### Phantom
(works)", etc.), ensure fenced log blocks have blank lines separating them from
surrounding text, and annotate each fenced block with a language tag such as
"text" (e.g., change ``` to ```text) so all code fences are language-tagged and
spacing around headings/fences is normalized.
In `@Packages/BouncyCastle.Cryptography.dll.meta`:
- Around line 19-24: The BouncyCastle.Cryptography.dll plugin is disabled for
the Unity Editor (the metadata shows "enabled: 0" under the Editor platform),
which prevents EditMode tests from running; to fix, edit the
Packages/BouncyCastle.Cryptography.dll.meta entry for the Editor platform and
set enabled to 1 (enable the Editor platform for the
BouncyCastle.Cryptography.dll plugin), keeping existing settings (e.g.,
DefaultValueInitialized) intact so Editor tests like
Tests/EditMode/Crypto/EcdsaSignaturesTests.cs can load the assembly.
In `@Runtime/codebase/SolanaMobileStack/AuthorizationHelpers.cs`:
- Around line 17-33: PrimaryAccountPublicKeyBytes currently calls
Convert.FromBase64String on primary.Address which can throw
ArgumentNullException for null/empty values and bypass the intended
InvalidAuthorizationException contract; add a guard at the start of
PrimaryAccountPublicKeyBytes (after retrieving primary via PrimaryAccount())
that checks string.IsNullOrWhiteSpace(primary.Address) and if true throws
InvalidAuthorizationException with an appropriate message like
"accounts[0].address is null or empty", then proceed to
Convert.FromBase64String; keep the existing FormatException catch and the
subsequent length check against PublicKeyByteLength unchanged.
In `@Runtime/codebase/SolanaMobileStack/AuthorizationRecord.cs`:
- Line 8: AuthorizationRecord.SchemaVersion currently initializes to 1 while
SolanaMobileWalletAdapter.ExpectedSchemaVersion is 2; update the default
initializer on the SchemaVersion property in the AuthorizationRecord class to =
2 so deserialized records without an explicit field match the adapter's expected
schema version and avoid unnecessary cache invalidation.
In `@Runtime/codebase/SolanaMobileStack/DeauthorizeResult.cs`:
- Around line 5-18: The base discriminated-union class DeauthorizeResult is
missing a private constructor, allowing external assemblies to derive new
variants; add a private parameterless constructor to DeauthorizeResult (e.g.,
private DeauthorizeResult() {}) inside the DeauthorizeResult class definition so
only the declared nested sealed subclasses (FullyRevoked, LocalOnly, Failed) can
exist and prevent external subclassing.
In `@Runtime/codebase/SolanaMobileStack/Exceptions/JsonRpcException.cs.meta`:
- Around line 3-11: The .meta file contains an extraneous MonoImporter block;
update JsonRpcException.cs.meta to remove the entire MonoImporter: { ... }
section and leave only the minimal required keys (fileFormatVersion and guid) so
the file matches project convention and other C# Unity .cs.meta files.
In
`@Runtime/codebase/SolanaMobileStack/Exceptions/OperationInFlightException.cs.meta`:
- Around line 3-11: The .meta file contains an unnecessary MonoImporter block;
open
Runtime/codebase/SolanaMobileStack/Exceptions/OperationInFlightException.cs.meta
and remove the entire MonoImporter section so the file only includes the minimal
Unity meta entries (fileFormatVersion and guid) per project convention, leaving
no other keys or blocks.
In `@Runtime/codebase/SolanaMobileStack/JsonRpc20Client.cs`:
- Around line 109-112: The response envelope's "result" indexer can be null,
which leads task.SetResult(envelope["result"]) to set a null into the
Task<JToken> (causing downstream NREs); update the code in JsonRpc20Client.cs
(the block that calls task.SetResult) to detect when envelope["result"] is null
and instead pass JValue.CreateNull() into task.SetResult so the returned
Task<JToken> is a non-null JToken; ensure this change is applied where the
method returns Task<JToken> and in any related path that sets the task result.
- Around line 39-50: The Debug.Log in JsonRpc20Client.SendRequestRaw leaks
sensitive wire messages in production; wrap or remove the UnityEngine.Debug.Log
call so it only runs in non-production builds (e.g., gate it behind a
preprocessor symbol such as UNITY_EDITOR or DEBUG) or remove logging of full
message entirely, keeping the rest of SendRequestRaw (message serialization,
_messageSender.Send, TaskCompletionSource and RegisterRawListener) unchanged;
ensure the symbol check surrounds only the Debug.Log line so sensitive payloads
(auth tokens, addresses) are never written to logcat in release builds.
In `@Runtime/codebase/SolanaMobileStack/JsonRpcClient/JsonRequest.cs`:
- Around line 78-79: The duplicate SendOptions property
([JsonProperty("sendOptions")] public JObject SendOptions) is incorrect per MWA
v2 (the correct wire key is "options" and is already represented by the Options
property); remove the SendOptions property declaration from JsonRequest.cs so
the code uses the existing Options property for send transaction options and
avoid the unused/incorrect "sendOptions" mapping.
In `@Runtime/codebase/SolanaMobileStack/JsonRpcErrorCodes.cs`:
- Around line 1-11: The JsonRpcErrorCodes class is currently declared in the
global namespace; wrap it in the explicit namespace
Solana.Unity.SolanaMobileStack to match other MWA types and avoid naming
conflicts. Locate the public static class JsonRpcErrorCodes and add the
namespace declaration so the class (and its constants like AuthorizationFailed,
InvalidPayloads, NotSigned, NotSubmitted, TooManyPayloads, ChainNotSupported)
are declared inside Solana.Unity.SolanaMobileStack, and remove the ReSharper
disable CheckNamespace directive if no longer needed.
In `@Runtime/codebase/SolanaMobileStack/MobileWalletAdapterClient.cs`:
- Around line 151-164: The JsonRequest being built for
RpcMethodNames.SignAndSendTransactions incorrectly sets both
JsonRequest.JsonRequestParams.Options and SendOptions to the same wireOptions,
producing duplicate "options" and "sendOptions" fields; change the builder in
MobileWalletAdapterClient.SignAndSendTransactions (the JsonRequest creation
before calling SendRequestRaw) so only Options is assigned (or set SendOptions =
null / omit it) to ensure the outgoing JSON contains a single options field per
the MWA v2 spec.
In `@Runtime/codebase/SolanaMobileStack/PlayerPrefsAuthorizationCache.cs`:
- Line 16: The private field _warnedThisSession in PlayerPrefsAuthorizationCache
is currently an instance field but must be static to enforce "once per session"
behavior and to match the test reset logic; change its declaration to "private
static bool _warnedThisSession" so the warning gate is shared across all
instances and the existing test teardown that uses BindingFlags.Static |
BindingFlags.NonPublic will correctly reset it.
- Around line 14-27: The constructor for PlayerPrefsAuthorizationCache currently
defines LegacyPkKey but never removes it from PlayerPrefs, causing the
LegacyPk_DeletedOnConstruction test to fail; update the constructors (the
parameterless PlayerPrefsAuthorizationCache() and the
PlayerPrefsAuthorizationCache(string scope) flow) to call
PlayerPrefs.DeleteKey(LegacyPkKey) and then PlayerPrefs.Save() during
construction so the legacy "pk" entry is removed and persisted when an instance
of PlayerPrefsAuthorizationCache is created.
In `@Runtime/codebase/SolanaMobileStack/ReconnectResult.cs`:
- Around line 6-19: Add a private parameterless constructor to the
ReconnectResult base class to seal the inheritance hierarchy so external
assemblies cannot derive from it; keep the nested sealed subclasses
SilentSuccess, NoCachedSession, and Failed as-is so they can still instantiate
the base. Modify the ReconnectResult type by adding a private ReconnectResult()
{ } constructor to prevent external subclassing and preserve exhaustive pattern
matching.
In `@Runtime/codebase/SolanaMobileStack/SendOptions.cs.meta`:
- Around line 3-11: The MonoImporter block in SendOptions.cs.meta is not allowed
by project convention; open SendOptions.cs.meta, remove the entire MonoImporter:
... block (the keys externalObjects, serializedVersion, defaultReferences,
executionOrder, icon, userData, assetBundleName, assetBundleVariant) so the file
contains only the required fileFormatVersion and guid entries, then save the
cleaned .meta file (ensuring guid is preserved).
In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`:
- Around line 456-459: The AuthorizationFailed fast-path in
SignAndSendTransactionsInternal and ReconnectInternal currently sets _authToken
= null (and clears cache in one place) but omits the same logout/disconnect
steps done in DeauthorizeInternal/DisconnectInternal; update the
AuthorizationFailed case in both SignAndSendTransactionsInternal and
ReconnectInternal to, after clearing cache and setting _authToken = null, call
LogoutSuppressed() and invoke OnWalletDisconnected?() so WalletBase.Account and
PublicKey are cleared and listeners notified, mirroring
DeauthorizeInternal/DisconnectInternal behavior.
- Around line 723-729: The SIWS fallback extraction does not validate
signedBytes length and can throw when signedBytes.Length < 64; in the code
handling siwsFallbackSig and SignedPayloadsBytes (use symbols siwsFallbackSig,
SignedPayloadsBytes, signedBytes, sigBytes, msgBytes) add a defensive check that
signedBytes != null and signedBytes.Length >= 64 before allocating
msgBytes/sigBytes and copying; if the check fails, log/return a clear error (or
skip this payload) instead of proceeding to create negative-length arrays so the
method fails gracefully.
- Around line 431-435: In the lambdas inside SignAndSendTransactionsInternal,
LoginWithSignInInternal (fallback), and CloneAuthorizationInternal you are
calling AuthorizeAsync but discarding its return value, which loses rotated auth
tokens; change each lambda to capture the returned authorization (e.g., var
authorization = await client.AuthorizeAsync(...)), update _authToken =
authorization.AuthToken ?? _authToken, and then pass the updated token
explicitly into subsequent calls (e.g.,
SignMessagesAsync/CloneAuthorizationAsync) instead of relying on mutated
closures — follow the pattern used in _SignAllTransactions where authorization
is awaited and _authToken is refreshed.
- Around line 381-385: The code in SignAndSendTransaction is returning
success.Signatures[0] as base64 which is wrong for Solana RPCs; replace
Convert.ToBase64String(success.Signatures[0]) with a base58 encoding of the raw
64-byte signature (e.g., use a Base58 encoder such as Org.BouncyCastle's Base58
utility), add the corresponding using (e.g., using
Org.BouncyCastle.Utilities.Encoders; or the appropriate Base58 class), and
return that base58 string so callers can pass the result directly to RPC methods
like ConfirmTransaction/GetSignatureStatuses.
In `@Tests/EditMode/Crypto/MobileWalletAdapterSessionTests.cs.meta`:
- Around line 3-11: Remove the extraneous MonoImporter block from the
Tests/EditMode/Crypto/MobileWalletAdapterSessionTests.cs.meta file so it only
contains the minimal .cs.meta fields used by this repo (fileFormatVersion and
guid); locate the "MonoImporter:" section in the shown diff and delete that
entire block (including its nested keys externalObjects, serializedVersion,
defaultReferences, executionOrder, icon, userData, assetBundleName,
assetBundleVariant) leaving only the minimal metadata to prevent metadata drift.
In `@Tests/EditMode/JsonRpc/ResponseModelTests.cs.meta`:
- Around line 3-11: The .meta file contains an extra MonoImporter block that
violates the project's minimal .cs.meta convention; remove the entire
MonoImporter section (the keys starting with "MonoImporter:" and its nested
fields like externalObjects, serializedVersion, defaultReferences,
executionOrder, icon, userData, assetBundleName, assetBundleVariant) from
ResponseModelTests.cs.meta and ensure the file contains only the canonical keys
fileFormatVersion and guid with their values to match other C# Unity .cs.meta
files in the repo.
In `@Tests/EditMode/Mocks/MockMessageSender.cs.meta`:
- Around line 3-11: The .meta file contains an unnecessary MonoImporter block;
remove the entire MonoImporter: { ... } section from
Tests/EditMode/Mocks/MockMessageSender.cs.meta and reduce the file to the
repository convention: only include fileFormatVersion and guid entries,
preserving the existing guid value if present and removing defaultReferences,
executionOrder, icon, userData, assetBundleName, assetBundleVariant and any
other MonoImporter keys so the file matches the minimal .cs.meta format used
across the repo.
In `@Tests/EditMode/MwaAdapter/ConcurrencyTests.cs`:
- Around line 26-31: The test uses FormatterServices.GetUninitializedObject
which bypasses field initializers so the inline _gate initializer never runs;
update the test (which creates a SolanaMobileWalletAdapter via
GetUninitializedObject) to explicitly initialize the _gate before asserting by
using the same pattern as ReconnectTests/DisconnectTests: obtain the gate field
(field/gateField) by reflection and call gateField.SetValue(adapter, new
System.Threading.SemaphoreSlim(1,1)) (or construct via a real constructor path)
and then assert that the gate is non-null and CurrentCount == 1.
In `@Tests/EditMode/MwaAdapter/ConcurrencyTests.cs.meta`:
- Around line 3-11: Remove the entire MonoImporter block (including keys like
externalObjects, serializedVersion, defaultReferences, executionOrder, icon,
userData, assetBundleName, assetBundleVariant) from the .meta file and leave
only the minimal Unity metadata fields fileFormatVersion and guid so the file
matches the project convention; ensure the resulting ConcurrencyTests.cs.meta
contains only fileFormatVersion and guid entries and nothing else.
In `@Tests/EditMode/MwaAdapter/DeauthorizeTests.cs.meta`:
- Around line 3-11: The .meta contains an unnecessary MonoImporter block; update
DeauthorizeTests.cs.meta to follow project convention by removing the entire
MonoImporter section and leaving only the minimal required keys:
fileFormatVersion and guid (ensure those two remain and no other entries like
defaultReferences, executionOrder, icon, userData, assetBundleName, or
assetBundleVariant are present).
In `@Tests/EditMode/MwaAdapter/DisconnectTests.cs`:
- Around line 27-31: The Guard() method currently asserts CacheField and
DisconnectMethod but misses asserting AuthTokenField; add an assertion for
AuthTokenField (similar to the existing ones) inside Guard() so that
AuthTokenField is verified not-null with a clear message (e.g., "AuthTokenField
not found") to fail fast if the reflected field is renamed or missing; locate
the Guard() method and add the assert alongside CacheField and DisconnectMethod
checks referencing AuthTokenField.
In `@Tests/EditMode/MwaAdapter/DisconnectTests.cs.meta`:
- Around line 3-11: Remove the entire MonoImporter block from the .cs.meta
content so the file only contains the minimal metadata (keep only
fileFormatVersion and guid entries); locate the MonoImporter mapping in the
DisconnectTests.cs.meta content and delete that mapping (the keys starting with
"MonoImporter:" and its nested fields) to match the repository convention of
minimal C# .meta files.
In `@Tests/EditMode/MwaAdapter/ReconnectTests.cs`:
- Around line 24-36: The Guard() setup currently asserts CacheField and
ReconnectMethod but misses verifying the private _gate field used in
CreateAdapter(); add an assertion in Guard() that the reflection lookup for
_gate (the same code using typeof(SolanaMobileWalletAdapter).GetField("_gate",
BindingFlags.Instance | BindingFlags.NonPublic)) returns a non-null FieldInfo
with a clear message like "_gate not found" so reflection drift fails fast; keep
CreateAdapter() unchanged but reference the same field lookup (gateField) used
to set the SemaphoreSlim to ensure consistency.
In `@Tests/EditMode/MwaCache/CustomCacheInjectionTest.cs`:
- Around line 1-2: Replace calls to the obsolete
FormatterServices.GetUninitializedObject(Type) with
RuntimeHelpers.GetUninitializedObject(Type) throughout the test (e.g., in
CustomCacheInjectionTest where FormatterServices.GetUninitializedObject is
used); add the using directive for System.Runtime.CompilerServices if missing
and update all occurrences (including the ones noted at lines 31-32) to call
RuntimeHelpers.GetUninitializedObject(Type) instead of
FormatterServices.GetUninitializedObject(Type).
In `@Tests/EditMode/MwaCache/PlayerPrefsAuthorizationCacheTests.cs`:
- Around line 21-23: The test attempts to reset the private field
_warnedThisSession on PlayerPrefsAuthorizationCache using GetField with
BindingFlags.Static which returns null because the field is an instance field;
update the reflection call to use BindingFlags.Instance | BindingFlags.NonPublic
and call SetValue with the specific PlayerPrefsAuthorizationCache instance used
in the test (or, if the implementation makes _warnedThisSession static, switch
to BindingFlags.Static and keep null as the instance argument) so the field is
actually reset before assertions.
In `@Tests/EditMode/MwaCache/PlayerPrefsAuthorizationCacheTests.cs.meta`:
- Around line 3-11: The .meta file contains an unwanted MonoImporter block;
remove the entire MonoImporter section so the file only includes the required
minimal keys (fileFormatVersion and guid) following the project's convention;
ensure no other keys like defaultReferences, executionOrder, icon, userData,
assetBundleName, or assetBundleVariant remain and that the metadata for
PlayerPrefsAuthorizationCacheTests.cs only has fileFormatVersion and guid.
In `@Tests/EditMode/MwaClient/DeauthorizeRequestWireTest.cs`:
- Around line 29-30: Replace the hard-coded method name in the assertion with
the constant to keep tests in sync: update the assertion that currently checks
for "\"method\":\"deauthorize\"" to instead use RpcMethodNames.Deauthorize
(e.g., build the expected substring dynamically from RpcMethodNames.Deauthorize)
so the test references the same symbol used to construct the request
(RpcMethodNames.Deauthorize) while leaving the auth_token assertion as-is.
In `@Tests/EditMode/MwaParser/ParserVerbosityTests.cs.meta`:
- Around line 3-11: Remove the MonoImporter block from the
Tests/EditMode/MwaParser/ParserVerbosityTests.cs.meta file so the .cs.meta
adheres to project convention; keep only the minimal metadata fields
(fileFormatVersion and guid) in the file, deleting the entire MonoImporter: {
... } section (the long block containing externalObjects, serializedVersion,
defaultReferences, executionOrder, icon, userData, assetBundleName,
assetBundleVariant) so the metadata is minimal and consistent with other C#
.meta files.
🪄 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: b27193e2-874e-4bf7-bd4c-e50e003289e7
📒 Files selected for processing (120)
.gitignorePackages/BouncyCastle.Cryptography.dll.metaRuntime/codebase/SolanaMobileStack/AccountInfo.csRuntime/codebase/SolanaMobileStack/AccountInfo.cs.metaRuntime/codebase/SolanaMobileStack/AuthorizationHelpers.csRuntime/codebase/SolanaMobileStack/AuthorizationHelpers.cs.metaRuntime/codebase/SolanaMobileStack/AuthorizationRecord.csRuntime/codebase/SolanaMobileStack/AuthorizationRecord.cs.metaRuntime/codebase/SolanaMobileStack/AuthorizationResponseParser.csRuntime/codebase/SolanaMobileStack/AuthorizationResponseParser.cs.metaRuntime/codebase/SolanaMobileStack/DeauthorizeResult.csRuntime/codebase/SolanaMobileStack/DeauthorizeResult.cs.metaRuntime/codebase/SolanaMobileStack/Exceptions.metaRuntime/codebase/SolanaMobileStack/Exceptions/InvalidAuthorizationException.csRuntime/codebase/SolanaMobileStack/Exceptions/InvalidAuthorizationException.cs.metaRuntime/codebase/SolanaMobileStack/Exceptions/JsonRpcException.csRuntime/codebase/SolanaMobileStack/Exceptions/JsonRpcException.cs.metaRuntime/codebase/SolanaMobileStack/Exceptions/OperationInFlightException.csRuntime/codebase/SolanaMobileStack/Exceptions/OperationInFlightException.cs.metaRuntime/codebase/SolanaMobileStack/IAuthorizationCache.csRuntime/codebase/SolanaMobileStack/IAuthorizationCache.cs.metaRuntime/codebase/SolanaMobileStack/ITransactionPayload.csRuntime/codebase/SolanaMobileStack/ITransactionPayload.cs.metaRuntime/codebase/SolanaMobileStack/Interfaces/IAdapterOperations.csRuntime/codebase/SolanaMobileStack/JsonRpc20Client.csRuntime/codebase/SolanaMobileStack/JsonRpcClient/JsonRequest.csRuntime/codebase/SolanaMobileStack/JsonRpcClient/Responses/AuthorizationResult.csRuntime/codebase/SolanaMobileStack/JsonRpcClient/Responses/CapabilitiesResult.csRuntime/codebase/SolanaMobileStack/JsonRpcClient/Responses/CapabilitiesResult.cs.metaRuntime/codebase/SolanaMobileStack/JsonRpcErrorCodes.csRuntime/codebase/SolanaMobileStack/JsonRpcErrorCodes.cs.metaRuntime/codebase/SolanaMobileStack/LegacyTransactionPayload.csRuntime/codebase/SolanaMobileStack/LegacyTransactionPayload.cs.metaRuntime/codebase/SolanaMobileStack/LogVerbosity.csRuntime/codebase/SolanaMobileStack/LogVerbosity.cs.metaRuntime/codebase/SolanaMobileStack/MobileWalletAdapterClient.csRuntime/codebase/SolanaMobileStack/MobileWalletAdapterSession.csRuntime/codebase/SolanaMobileStack/PlayerPrefsAuthorizationCache.csRuntime/codebase/SolanaMobileStack/PlayerPrefsAuthorizationCache.cs.metaRuntime/codebase/SolanaMobileStack/ReconnectResult.csRuntime/codebase/SolanaMobileStack/ReconnectResult.cs.metaRuntime/codebase/SolanaMobileStack/RpcMethodNames.csRuntime/codebase/SolanaMobileStack/RpcMethodNames.cs.metaRuntime/codebase/SolanaMobileStack/SendOptions.csRuntime/codebase/SolanaMobileStack/SendOptions.cs.metaRuntime/codebase/SolanaMobileStack/SignAndSendTxResult.csRuntime/codebase/SolanaMobileStack/SignAndSendTxResult.cs.metaRuntime/codebase/SolanaMobileStack/SignInPayload.csRuntime/codebase/SolanaMobileStack/SignInPayload.cs.metaRuntime/codebase/SolanaMobileStack/SignInResult.csRuntime/codebase/SolanaMobileStack/SignInResult.cs.metaRuntime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.csRuntime/codebase/SolanaWalletAdapter.csTests/EditMode/Crypto/EcdsaSignaturesTests.csTests/EditMode/Crypto/EcdsaSignaturesTests.cs.metaTests/EditMode/Crypto/MobileWalletAdapterSessionTests.csTests/EditMode/Crypto/MobileWalletAdapterSessionTests.cs.metaTests/EditMode/EditMode.asmdefTests/EditMode/JsonRpc/ResponseModelTests.csTests/EditMode/JsonRpc/ResponseModelTests.cs.metaTests/EditMode/Mocks/MockMessageSender.csTests/EditMode/Mocks/MockMessageSender.cs.metaTests/EditMode/MwaAdapter.metaTests/EditMode/MwaAdapter/ConcurrencyTests.csTests/EditMode/MwaAdapter/ConcurrencyTests.cs.metaTests/EditMode/MwaAdapter/DeauthorizeTests.csTests/EditMode/MwaAdapter/DeauthorizeTests.cs.metaTests/EditMode/MwaAdapter/DisconnectTests.csTests/EditMode/MwaAdapter/DisconnectTests.cs.metaTests/EditMode/MwaAdapter/ReconnectTests.csTests/EditMode/MwaAdapter/ReconnectTests.cs.metaTests/EditMode/MwaAdapter/SignAndSendTests.csTests/EditMode/MwaAdapter/SignAndSendTests.cs.metaTests/EditMode/MwaCache.metaTests/EditMode/MwaCache/CustomCacheInjectionTest.csTests/EditMode/MwaCache/CustomCacheInjectionTest.cs.metaTests/EditMode/MwaCache/PlayerPrefsAuthorizationCacheTests.csTests/EditMode/MwaCache/PlayerPrefsAuthorizationCacheTests.cs.metaTests/EditMode/MwaCache/SchemaVersionValidationTests.csTests/EditMode/MwaCache/SchemaVersionValidationTests.cs.metaTests/EditMode/MwaClient/AuthorizeRequestWireTests.csTests/EditMode/MwaClient/AuthorizeRequestWireTests.cs.metaTests/EditMode/MwaClient/DeauthorizeRequestWireTest.csTests/EditMode/MwaClient/DeauthorizeRequestWireTest.cs.metaTests/EditMode/MwaClient/LoginShapeCompatibilityTest.csTests/EditMode/MwaClient/LoginShapeCompatibilityTest.cs.metaTests/EditMode/MwaClient/MobileWalletAdapterClientTests.csTests/EditMode/MwaClient/MobileWalletAdapterClientTests.cs.metaTests/EditMode/MwaParser.metaTests/EditMode/MwaParser/AuthorizationResponseParserTests.csTests/EditMode/MwaParser/AuthorizationResponseParserTests.cs.metaTests/EditMode/MwaParser/Fixtures.metaTests/EditMode/MwaParser/Fixtures/authorize-v2-bad-base64.jsonTests/EditMode/MwaParser/Fixtures/authorize-v2-bad-base64.json.metaTests/EditMode/MwaParser/Fixtures/authorize-v2-empty-accounts.jsonTests/EditMode/MwaParser/Fixtures/authorize-v2-empty-accounts.json.metaTests/EditMode/MwaParser/Fixtures/authorize-v2-empty-authtoken.jsonTests/EditMode/MwaParser/Fixtures/authorize-v2-empty-authtoken.json.metaTests/EditMode/MwaParser/Fixtures/authorize-v2-full.jsonTests/EditMode/MwaParser/Fixtures/authorize-v2-full.json.metaTests/EditMode/MwaParser/Fixtures/authorize-v2-minimal.jsonTests/EditMode/MwaParser/Fixtures/authorize-v2-minimal.json.metaTests/EditMode/MwaParser/Fixtures/authorize-v2-oversize-accounts.jsonTests/EditMode/MwaParser/Fixtures/authorize-v2-oversize-accounts.json.metaTests/EditMode/MwaParser/Fixtures/authorize-v2-oversize-chains.jsonTests/EditMode/MwaParser/Fixtures/authorize-v2-oversize-chains.json.metaTests/EditMode/MwaParser/Fixtures/authorize-v2-oversize-display.jsonTests/EditMode/MwaParser/Fixtures/authorize-v2-oversize-display.json.metaTests/EditMode/MwaParser/Fixtures/authorize-v2-oversize-icon.jsonTests/EditMode/MwaParser/Fixtures/authorize-v2-oversize-icon.json.metaTests/EditMode/MwaParser/Fixtures/authorize-v2-wrong-length.jsonTests/EditMode/MwaParser/Fixtures/authorize-v2-wrong-length.json.metaTests/EditMode/MwaParser/ParserVerbosityTests.csTests/EditMode/MwaParser/ParserVerbosityTests.cs.metadocs.metadocs/mwa-cache-guide.mddocs/mwa-method-reference.mddocs/mwa-migration-v1-to-v2.mddocs/mwa-quick-start.mddocs/sign-and-send-investigation.md
💤 Files with no reviewable changes (4)
- Tests/EditMode/Crypto/EcdsaSignaturesTests.cs
- Tests/EditMode/Crypto/MobileWalletAdapterSessionTests.cs
- Tests/EditMode/JsonRpc/ResponseModelTests.cs
- Tests/EditMode/Mocks/MockMessageSender.cs
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs (2)
457-460:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear wallet state on auth revocation, not just the token.
These revocation paths clear
_authTokenand cache, but they skipLogoutSuppressed()andOnWalletDisconnected.WalletBase.Account/PublicKeytherefore stays populated after the session has already been revoked.🐛 Proposed fix
try { await _cache.ClearAsync(); } catch (Exception ex) { Debug.LogWarning($"[MWA] Cache clear failed after auth revocation: {ex.Message}"); } _authToken = null; + LogoutSuppressed(); + OnWalletDisconnected?.Invoke(); return new SignAndSendTxResult.AuthRevoked();try { await _cache.ClearAsync(); } catch (Exception ex) { Debug.LogWarning($"[MWA] Cache clear failed after auth revocation: {ex.Message}"); } _authToken = null; + LogoutSuppressed(); + OnWalletDisconnected?.Invoke(); return new ReconnectResult.NoCachedSession();Also applies to: 571-575
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs` around lines 457 - 460, The AuthorizationFailed handling only clears _authToken and cache but leaves wallet state populated; update the JsonRpcErrorCodes.AuthorizationFailed case in the SignAndSendTx flow (and the similar block around the other occurrence) to call LogoutSuppressed() and OnWalletDisconnected() after clearing cache and clearing _authToken so WalletBase.Account/PublicKey are reset; ensure you maintain the existing try/catch around _cache.ClearAsync() and keep returning new SignAndSendTxResult.AuthRevoked() after invoking those two methods.
432-437:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersist rotated auth tokens from these reauthorization calls.
These lambdas await
AuthorizeAsyncand discard the returnedAuthorizationResult. If the wallet rotates the auth token during reauthorization,_authTokenstays stale;CloneAuthorizationInternalthen immediately uses that stale token, and the sign-and-send / SIWS fallback paths never persist the refreshed session.🐛 Proposed fix pattern
- async client => - { - await client.AuthorizeAsync( - _identityUri, _iconRelativeUri, - _walletOptions.name, chain, _authToken, CancellationToken.None); - }, + async client => + { + var reauth = await client.AuthorizeAsync( + _identityUri, _iconRelativeUri, + _walletOptions.name, chain, _authToken, CancellationToken.None); + _authToken = reauth?.AuthToken ?? _authToken; + },Based on learnings: cross-lambda variable sharing via mutated closures is unsafe in the current
async void Action<IAdapterOperations>pattern; pass the refreshed token explicitly.Also applies to: 699-703, 768-777
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs` around lines 432 - 437, The lambda passed to the Action<IAdapterOperations> invokes AuthorizeAsync but discards the returned AuthorizationResult so rotated tokens aren’t persisted; update the call sites (the async lambda wrapping AuthorizeAsync at the shown block and the similar blocks around CloneAuthorizationInternal usage) to capture the AuthorizationResult, extract the refreshed token from it, and pass that token explicitly into CloneAuthorizationInternal (or into subsequent sign/send flows) instead of relying on the mutated closure _authToken; refactor the async void Action<IAdapterOperations> usage so the refreshed token is threaded out of the lambda (e.g., return or callback the AuthorizationResult) and persist the new token back to the field where appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Runtime/codebase/SolanaMobileStack/JsonRpc20Client.cs`:
- Around line 47-50: The raw response listeners (RegisterRawListener /
ReceiverRaw handling TaskCompletionSource<JToken>) currently complete any
pending task on the next envelope without checking envelope["id"], causing
overlapping requests to complete the wrong TaskCompletionSource and possible
exceptions when a late message hits an already-completed task; update the
listener logic so it reads the JSON-RPC "id" from the incoming envelope and only
Call SetResult/SetException on the TaskCompletionSource whose id matches the
request's id, and immediately unregister the listener as soon as a matching
envelope is processed (do not rely on a delayed ContinueWith removal); apply the
same id-check/unregister pattern to the other raw-listener sites identified (the
blocks around lines 67–71 and 98–114) to ensure each TaskCompletionSource is
correlated to its JSON-RPC id before completing.
In `@Runtime/codebase/SolanaMobileStack/MobileWalletAdapterClient.cs`:
- Around line 127-129: The current guard only checks the array but allows null
or empty entries; in MobileWalletAdapterClient (where base64Payloads is used)
validate each entry in base64Payloads and throw an ArgumentException (or
ArgumentNullException for null elements) referencing nameof(base64Payloads) if
any element is null or string.IsNullOrWhiteSpace, before
ct.ThrowIfCancellationRequested(); iterate the array, check each element and
include the index in the exception message to aid debugging so no null/empty
payload is serialized and sent.
- Around line 227-228: The code currently returns null when the JSON response
from SendRequestRaw lacks "auth_token", which can silently treat a malformed
clone_authorization response as success; update the method that calls
SendRequestRaw so it checks raw?["auth_token"] and, if missing or null, throws a
protocol-level exception (e.g., InvalidOperationException or a custom
ProtocolException) with a clear message like "Malformed clone_authorization
response: missing auth_token" instead of returning null, ensuring callers cannot
overwrite valid cached auth state with a null token.
In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`:
- Around line 184-190: ReloadAuthTokenFromCacheIfNeeded and the SignMessage
public-key fallback are reading raw cache entries and may reuse
schema-mismatched or wrong-chain records; instead, call and use
LoadValidCachedRecordAsync to validate cache records before assigning _authToken
or using cached data. Update ReloadAuthTokenFromCacheIfNeeded to await
LoadValidCachedRecordAsync() and set _authToken only from its validated result,
and change the SignMessage fallback path to retrieve the validated record via
LoadValidCachedRecordAsync (or factor out the validation step) rather than
calling _cache.GetAsync() directly.
- Around line 581-590: ReconnectInternal currently assumes authorization is
non-null after a successful result; add a null check for the authorization
variable before dereferencing it (before using authorization.AuthToken,
CacheAuthorizationAsync(authorization), and new
PublicKey(authorization.PrimaryAccountPublicKeyBytes())). If authorization is
null, return a ReconnectResult.Failed with an Error describing the missing
payload (e.g., "Authorization payload missing") so the method returns a
structured failure instead of throwing NullReferenceException.
- Around line 88-92: The constructor currently dereferences solanaWalletOptions
directly for identity/icon URIs causing a null crash; normalize the options
first by assigning _walletOptions = solanaWalletOptions ?? new
SolanaWalletOptions() (or another sensible default) and then use
_walletOptions.identityUri and _walletOptions.iconUri when creating _identityUri
and _iconRelativeUri, ensuring you also handle null/empty URI strings before new
Uri(...) so defaults are used consistently for _cache, _verbosity, and later
uses of _walletOptions.name; update the code that sets _cache, _verbosity,
_identityUri, and _iconRelativeUri to reference the normalized _walletOptions
and guard Uri construction.
In `@Tests/EditMode/MwaAdapter/ReconnectTests.cs`:
- Around line 31-37: CreateAdapter uses FormatterServices.GetUninitializedObject
which leaves important fields null so Reconnect() runs on a partially
initialized SolanaMobileWalletAdapter; instead either instantiate the adapter
via its real constructor or explicitly set all fields Reconnect reads (set
_walletOptions with a non-null name, _identityUri, _iconRelativeUri, and the
base RpcCluster property) after using GetUninitializedObject, and ensure any
background gating/state the constructor would set is replicated (you already set
_gate and CacheField but must also populate _walletOptions, _identityUri,
_iconRelativeUri and base RpcCluster) so Reconnect() won't hit
NullReferenceExceptions and tests validate reconnect semantics rather than
masking failures.
---
Duplicate comments:
In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`:
- Around line 457-460: The AuthorizationFailed handling only clears _authToken
and cache but leaves wallet state populated; update the
JsonRpcErrorCodes.AuthorizationFailed case in the SignAndSendTx flow (and the
similar block around the other occurrence) to call LogoutSuppressed() and
OnWalletDisconnected() after clearing cache and clearing _authToken so
WalletBase.Account/PublicKey are reset; ensure you maintain the existing
try/catch around _cache.ClearAsync() and keep returning new
SignAndSendTxResult.AuthRevoked() after invoking those two methods.
- Around line 432-437: The lambda passed to the Action<IAdapterOperations>
invokes AuthorizeAsync but discards the returned AuthorizationResult so rotated
tokens aren’t persisted; update the call sites (the async lambda wrapping
AuthorizeAsync at the shown block and the similar blocks around
CloneAuthorizationInternal usage) to capture the AuthorizationResult, extract
the refreshed token from it, and pass that token explicitly into
CloneAuthorizationInternal (or into subsequent sign/send flows) instead of
relying on the mutated closure _authToken; refactor the async void
Action<IAdapterOperations> usage so the refreshed token is threaded out of the
lambda (e.g., return or callback the AuthorizationResult) and persist the new
token back to the field where appropriate.
🪄 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: 76081900-05d6-47a9-ae4e-26fcb7e7c49b
📒 Files selected for processing (14)
Packages/BouncyCastle.Cryptography.dll.metaRuntime/codebase/SolanaMobileStack/AuthorizationRecord.csRuntime/codebase/SolanaMobileStack/JsonRpc20Client.csRuntime/codebase/SolanaMobileStack/JsonRpcClient/JsonRequest.csRuntime/codebase/SolanaMobileStack/JsonRpcErrorCodes.csRuntime/codebase/SolanaMobileStack/JsonRpcErrorCodes.cs.metaRuntime/codebase/SolanaMobileStack/MobileWalletAdapterClient.csRuntime/codebase/SolanaMobileStack/PlayerPrefsAuthorizationCache.csRuntime/codebase/SolanaMobileStack/RpcMethodNames.cs.metaRuntime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.csTests/EditMode/MwaAdapter/ConcurrencyTests.csTests/EditMode/MwaAdapter/DeauthorizeTests.csTests/EditMode/MwaAdapter/ReconnectTests.csTests/EditMode/MwaCache/PlayerPrefsAuthorizationCacheTests.cs
Zurcusa
force-pushed
the
pr/6-mwa-docs
branch
3 times, most recently
from
67e50a7 to
8863367
Compare
- Guard null/empty address in PrimaryAccountPublicKeyBytes - Private constructors on DeauthorizeResult and ReconnectResult - Null-result check in JsonRpc20Client.ReceiverRaw - Cache auth token refresh in sign_and_send authorize action - LogoutSuppressed + OnWalletDisconnected on auth revocation paths - Fix base64 → base58 typo in method reference
- Guard null/empty address in PrimaryAccountPublicKeyBytes - Private constructors on DeauthorizeResult and ReconnectResult - Null-result check in JsonRpc20Client.ReceiverRaw - Cache auth token refresh in sign_and_send authorize action - LogoutSuppressed + OnWalletDisconnected on auth revocation paths - Fix base64 → base58 typo in method reference
- Return base58-encoded signatures instead of base64 (Solana RPC compat) - Remove duplicate sendOptions wire field (MWA v2 spec uses "options" only) - Gate Debug.Log wire messages behind UNITY_EDITOR || MWA_VERBOSE_WIRE - Remove unused LegacyPkKey constant from PlayerPrefsAuthorizationCache - Make _warnedThisSession static for once-per-session semantics - Add SIWS fallback length guard for signed payloads < 64 bytes - Enable BouncyCastle for Editor platform (fixes EditMode test loading) - Add namespace to JsonRpcErrorCodes - Fix SchemaVersion default to match ExpectedSchemaVersion (2) - Fix DeauthorizeTests missing _gate reflection init (4 test failures) - Fix ConcurrencyTests assertion for GetUninitializedObject compatibility - Fix ReconnectTests platform guard for non-Android EditMode runs - Remove invalid LegacyPk_DeletedOnConstruction test
- DeauthorizeRequestWireTest: use RpcMethodNames.Deauthorize in assertion instead of hardcoded string - ReconnectTests: promote _gate lookup to static field with OneTimeSetUp guard so reflection drift fails fast with a clear message
LoginWithSignInInternal SIWS fallback was the last AuthorizeAsync call site that discarded the return value, losing token rotations.
- Null-safe constructor: default solanaWalletOptions to prevent NPE - Validate payload elements are non-null before sending to wallet - Treat missing auth_token in clone_authorization as protocol error - Use LoadValidCachedRecordAsync in ReloadAuthTokenFromCacheIfNeeded to enforce schema/chain validation on cached records - Guard null authorization in ReconnectInternal success path - Remove unused using in ConcurrencyTests
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 9
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/MobileWalletAdapterSession.cs (1)
162-183:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate frame length first and only commit
Rxafter auth succeeds.A short payload now fails via raw indexing/
ArraySegmentbefore this method can reject it cleanly, and_mSeqNumberRxis advanced beforeDoFinal()verifies the GCM tag. One tampered frame with the expected next counter can therefore permanently desync the session and cause the real next message to be rejected.💡 Suggested fix
public byte[] DecryptSessionPayload(byte[] payload) { if (_encryptionKey == null) { const string e = "Cannot decrypt, no session key has been established"; Debug.LogError(e); throw new InvalidOperationException(e); } + + if (payload == null || payload.Length < SeqNumLengthBytes + AesIvLengthBytes + AesTagLengthBytes) + { + const string e = "Encrypted payload is too short"; + Debug.LogError(e); + throw new InvalidOperationException(e); + } var seqNumRaw = new ArraySegment<byte>(payload, 0, SeqNumLengthBytes).ToArray(); uint seqNum = (uint)(payload[0] << 24 | payload[1] << 16 | payload[2] << 8 | payload[3]); if (seqNum != _mSeqNumberRx + 1) { const string e = "Encrypted messages has invalid sequence number"; Debug.LogError(e); throw new InvalidOperationException(e); } - _mSeqNumberRx = (int)seqNum; try { var keyParam = new KeyParameter(_encryptionKey); var iv = new ArraySegment<byte>(payload, SeqNumLengthBytes, AesIvLengthBytes).ToArray(); @@ var toDecipher = new ArraySegment<byte>( payload, SeqNumLengthBytes + AesIvLengthBytes, payload.Length - SeqNumLengthBytes - AesIvLengthBytes).ToArray(); var decipherText = new byte[cipher.GetOutputSize(toDecipher.Length)]; int len = cipher.ProcessBytes(payload, SeqNumLengthBytes + AesIvLengthBytes, toDecipher.Length, decipherText, 0); cipher.DoFinal(decipherText, len); + _mSeqNumberRx = (int)seqNum; return decipherText; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Runtime/codebase/SolanaMobileStack/MobileWalletAdapterSession.cs` around lines 162 - 183, Validate the incoming payload length before any indexing (ensure payload.Length >= SeqNumLengthBytes + AesIvLengthBytes + AesTagLengthBytes and enough ciphertext) in MobileWalletAdapterSession so ArraySegment/byte indexing cannot throw; compute seqNum using the safe seqNumRaw span rather than raw indexing, but do NOT assign to _mSeqNumberRx until after GCM authentication succeeds (after cipher.DoFinal completes without exception). Also fix the cipher.ProcessBytes call to pass the ciphertext buffer (toDecipher) and correct lengths, and only update _mSeqNumberRx once ProcessBytes/DoFinal have completed successfully.
♻️ Duplicate comments (1)
Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs (1)
817-822:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the validated cache path for the
SignMessagepublic-key fallback.This reintroduces the earlier cache-validation problem: a schema-mismatched or wrong-chain record can still supply
cachedPkhere even thoughReloadAuthTokenFromCacheIfNeeded()now filters those records out. Please go throughLoadValidCachedRecordAsync()instead of_cache.GetAsync().💡 Minimal fix
string cachedPk = Account?.PublicKey?.ToString(); if (string.IsNullOrEmpty(cachedPk)) { - var record = await _cache.GetAsync(); + var record = await LoadValidCachedRecordAsync(); cachedPk = record?.AccountAddress; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs` around lines 817 - 822, In SignMessage, the fallback path obtains cachedPk from _cache.GetAsync(), which can return schema/chain-mismatched records; instead call LoadValidCachedRecordAsync() (the same validated loader used by ReloadAuthTokenFromCacheIfNeeded()) and use its returned record?.AccountAddress for cachedPk so only validated cache entries are used for the public-key fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/mwa-method-reference.md`:
- Line 3: The doc states "Methods marked with a lock require exclusive access"
but no methods show a lock; update the `SolanaMobileWalletAdapter` method list
to either remove the "marked with a lock" phrase and state that all auth/signing
methods are gated by the `SemaphoreSlim` concurrency gate, or add a consistent
lock indicator (e.g., "🔒") next to each gated method heading (at minimum:
Login, LoginWithSignIn, Reconnect, Deauthorize, CloneAuthorization and all
signing methods) so readers can immediately see which methods throw
OperationInFlightException when called concurrently.
In `@Runtime/codebase/SolanaMobileStack/AuthorizationResponseParser.cs`:
- Around line 60-129: The loop currently only validates the primary address but
leaves non-primary AccountInfo.Address values unchecked; update the address
handling in the accounts construction loop (where AccountInfo is created) to run
the same base64 extraction and 32-byte length check used for PrimaryAccount() —
i.e., call ExtractBase64Payload and EstimateBase64DecodedLength for each
entry["address"], log and add to droppedFields when the decoded length exceeds
the expected byte size, and either set Address to null/empty or skip adding that
AccountInfo (consistent with existing drop behavior) so downstream code never
receives unvalidated raw address strings.
In `@Runtime/codebase/SolanaMobileStack/DeauthorizeResult.cs`:
- Around line 11-19: The LocalOnly and Failed result variants expose mutable
properties; change WalletPackage on LocalOnly and Error on Failed to be
immutable by adding constructors that accept the respective values and making
the properties get-only (constructor-initialized) instead of having public
setters; update any instantiation sites to pass the values via the new
constructors and remove code that mutates these properties after creation.
In `@Runtime/codebase/SolanaMobileStack/ReconnectResult.cs`:
- Around line 10-20: SilentSuccess and Failed currently allow creation without
required payloads (Account and Error respectively); change both classes to
require non-null constructor parameters (e.g., SilentSuccess(Account account)
and Failed(Exception error)), make the properties get-only (public Account
Account { get; } and public Exception Error { get; }) and validate against null
(throw ArgumentNullException) to enforce the non-null invariant at construction
time.
In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`:
- Around line 336-348: The semaphore only protects parts of the state; update
the methods _Login, _SignAllTransactions, SignMessage(byte[]), and
GetCapabilities() to acquire the same gate via TryAcquireGate before touching
shared state (_authToken, Account, the auth cache) and always ReleaseGate in a
finally block so Disconnect()/Deauthorize() cannot concurrently clear/recreate
state during those operations; ensure you set _currentOperation appropriately
(use the existing MwaOperation enum/values) and handle the case where
TryAcquireGate returns false by failing fast or returning an appropriate error
result.
- Around line 700-706: The code caches the auth session and returns a new
Account tuple on successful LoginWithSignIn but never sets the adapter's own
Account (and thus PublicKey) property; after creating the new Account (via new
PublicKey(authorization.PrimaryAccountPublicKeyBytes()) and new Account(...))
assign it to the adapter's Account property (and update any related PublicKey
field if present) before returning, ensuring the adapter's state reflects the
successful sign-in (update symbols: _authToken, CacheAuthorizationAsync,
PublicKey(...), Account(...), and the adapter's Account/PublicKey properties).
- Around line 403-416: The code recreates a SendOptions instance when populating
MinContextSlot and accidentally omits the WaitForCommitmentToSendNextTransaction
flag; update the branch that builds the new SendOptions (inside the
GetLatestBlockHashAsync result handling) to copy
WaitForCommitmentToSendNextTransaction from the original options into the new
SendOptions so callers that set WaitForCommitmentToSendNextTransaction are not
changed when MinContextSlot is backfilled.
In `@Runtime/codebase/SolanaWalletAdapter.cs`:
- Around line 31-34: The code currently ignores authCache when
options.solanaMobileWalletAdapterOptions is null, causing
SolanaMobileWalletAdapter to default to PlayerPrefsAuthorizationCache; to fix,
ensure you create a default SolanaMobileWalletAdapterOptions instance when
options.solanaMobileWalletAdapterOptions is null before assigning Cache: replace
or precede the use of options.solanaMobileWalletAdapterOptions with a non-null
mwaOptions (e.g., instantiate new SolanaMobileWalletAdapterOptions if null),
then apply mwaOptions.Cache ??= authCache, and finally pass that mwaOptions into
the SolanaMobileWalletAdapter constructor used to set _internalWallet.
In `@Tests/EditMode/MwaParser/AuthorizationResponseParserTests.cs`:
- Around line 99-129: Add unit tests that cover the untested branches in
AuthorizationResponseParser.Parse: create three new tests mirroring the existing
patterns using fixtures authorize-v2-oversize-features.json,
authorize-v2-oversize-wallet-icon.json, and authorize-v2-with-signin.json; for
features assert result.Accounts[0].Features.Length == 32 (features[] > 32 must
be truncated), for wallet_icon assert result.Accounts[0].WalletIcon is null
(wallet_icon > 64 KiB post-decode must be dropped), and for sign-in assert that
the parsed SignInResult object (result.SignInResult or
result.Accounts[0].SignInResult depending on model) contains the expected mapped
fields to validate the sign_in_result mapping into SignInResult. Ensure test
names follow the existing naming convention (e.g., V2OversizeFeatures_Truncates,
V2OversizeWalletIcon_DropsField, V2WithSignIn_ParsesSignInResult) and use
AuthorizationResponseParser.Parse to load the fixture.
---
Outside diff comments:
In `@Runtime/codebase/SolanaMobileStack/MobileWalletAdapterSession.cs`:
- Around line 162-183: Validate the incoming payload length before any indexing
(ensure payload.Length >= SeqNumLengthBytes + AesIvLengthBytes +
AesTagLengthBytes and enough ciphertext) in MobileWalletAdapterSession so
ArraySegment/byte indexing cannot throw; compute seqNum using the safe seqNumRaw
span rather than raw indexing, but do NOT assign to _mSeqNumberRx until after
GCM authentication succeeds (after cipher.DoFinal completes without exception).
Also fix the cipher.ProcessBytes call to pass the ciphertext buffer (toDecipher)
and correct lengths, and only update _mSeqNumberRx once ProcessBytes/DoFinal
have completed successfully.
---
Duplicate comments:
In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`:
- Around line 817-822: In SignMessage, the fallback path obtains cachedPk from
_cache.GetAsync(), which can return schema/chain-mismatched records; instead
call LoadValidCachedRecordAsync() (the same validated loader used by
ReloadAuthTokenFromCacheIfNeeded()) and use its returned record?.AccountAddress
for cachedPk so only validated cache entries are used for the public-key
fallback.
🪄 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: c932c445-6e8d-41d6-a73b-1c225f39d98a
📒 Files selected for processing (120)
.gitignorePackages/BouncyCastle.Cryptography.dll.metaRuntime/codebase/SolanaMobileStack/AccountInfo.csRuntime/codebase/SolanaMobileStack/AccountInfo.cs.metaRuntime/codebase/SolanaMobileStack/AuthorizationHelpers.csRuntime/codebase/SolanaMobileStack/AuthorizationHelpers.cs.metaRuntime/codebase/SolanaMobileStack/AuthorizationRecord.csRuntime/codebase/SolanaMobileStack/AuthorizationRecord.cs.metaRuntime/codebase/SolanaMobileStack/AuthorizationResponseParser.csRuntime/codebase/SolanaMobileStack/AuthorizationResponseParser.cs.metaRuntime/codebase/SolanaMobileStack/DeauthorizeResult.csRuntime/codebase/SolanaMobileStack/DeauthorizeResult.cs.metaRuntime/codebase/SolanaMobileStack/Exceptions.metaRuntime/codebase/SolanaMobileStack/Exceptions/InvalidAuthorizationException.csRuntime/codebase/SolanaMobileStack/Exceptions/InvalidAuthorizationException.cs.metaRuntime/codebase/SolanaMobileStack/Exceptions/JsonRpcException.csRuntime/codebase/SolanaMobileStack/Exceptions/JsonRpcException.cs.metaRuntime/codebase/SolanaMobileStack/Exceptions/OperationInFlightException.csRuntime/codebase/SolanaMobileStack/Exceptions/OperationInFlightException.cs.metaRuntime/codebase/SolanaMobileStack/IAuthorizationCache.csRuntime/codebase/SolanaMobileStack/IAuthorizationCache.cs.metaRuntime/codebase/SolanaMobileStack/ITransactionPayload.csRuntime/codebase/SolanaMobileStack/ITransactionPayload.cs.metaRuntime/codebase/SolanaMobileStack/Interfaces/IAdapterOperations.csRuntime/codebase/SolanaMobileStack/JsonRpc20Client.csRuntime/codebase/SolanaMobileStack/JsonRpcClient/JsonRequest.csRuntime/codebase/SolanaMobileStack/JsonRpcClient/Responses/AuthorizationResult.csRuntime/codebase/SolanaMobileStack/JsonRpcClient/Responses/CapabilitiesResult.csRuntime/codebase/SolanaMobileStack/JsonRpcClient/Responses/CapabilitiesResult.cs.metaRuntime/codebase/SolanaMobileStack/JsonRpcErrorCodes.csRuntime/codebase/SolanaMobileStack/JsonRpcErrorCodes.cs.metaRuntime/codebase/SolanaMobileStack/LegacyTransactionPayload.csRuntime/codebase/SolanaMobileStack/LegacyTransactionPayload.cs.metaRuntime/codebase/SolanaMobileStack/LogVerbosity.csRuntime/codebase/SolanaMobileStack/LogVerbosity.cs.metaRuntime/codebase/SolanaMobileStack/MobileWalletAdapterClient.csRuntime/codebase/SolanaMobileStack/MobileWalletAdapterSession.csRuntime/codebase/SolanaMobileStack/PlayerPrefsAuthorizationCache.csRuntime/codebase/SolanaMobileStack/PlayerPrefsAuthorizationCache.cs.metaRuntime/codebase/SolanaMobileStack/ReconnectResult.csRuntime/codebase/SolanaMobileStack/ReconnectResult.cs.metaRuntime/codebase/SolanaMobileStack/RpcMethodNames.csRuntime/codebase/SolanaMobileStack/RpcMethodNames.cs.metaRuntime/codebase/SolanaMobileStack/SendOptions.csRuntime/codebase/SolanaMobileStack/SendOptions.cs.metaRuntime/codebase/SolanaMobileStack/SignAndSendTxResult.csRuntime/codebase/SolanaMobileStack/SignAndSendTxResult.cs.metaRuntime/codebase/SolanaMobileStack/SignInPayload.csRuntime/codebase/SolanaMobileStack/SignInPayload.cs.metaRuntime/codebase/SolanaMobileStack/SignInResult.csRuntime/codebase/SolanaMobileStack/SignInResult.cs.metaRuntime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.csRuntime/codebase/SolanaWalletAdapter.csTests/EditMode/Crypto/EcdsaSignaturesTests.csTests/EditMode/Crypto/EcdsaSignaturesTests.cs.metaTests/EditMode/Crypto/MobileWalletAdapterSessionTests.csTests/EditMode/Crypto/MobileWalletAdapterSessionTests.cs.metaTests/EditMode/EditMode.asmdefTests/EditMode/JsonRpc/ResponseModelTests.csTests/EditMode/JsonRpc/ResponseModelTests.cs.metaTests/EditMode/Mocks/MockMessageSender.csTests/EditMode/Mocks/MockMessageSender.cs.metaTests/EditMode/MwaAdapter.metaTests/EditMode/MwaAdapter/ConcurrencyTests.csTests/EditMode/MwaAdapter/ConcurrencyTests.cs.metaTests/EditMode/MwaAdapter/DeauthorizeTests.csTests/EditMode/MwaAdapter/DeauthorizeTests.cs.metaTests/EditMode/MwaAdapter/DisconnectTests.csTests/EditMode/MwaAdapter/DisconnectTests.cs.metaTests/EditMode/MwaAdapter/ReconnectTests.csTests/EditMode/MwaAdapter/ReconnectTests.cs.metaTests/EditMode/MwaAdapter/SignAndSendTests.csTests/EditMode/MwaAdapter/SignAndSendTests.cs.metaTests/EditMode/MwaCache.metaTests/EditMode/MwaCache/CustomCacheInjectionTest.csTests/EditMode/MwaCache/CustomCacheInjectionTest.cs.metaTests/EditMode/MwaCache/PlayerPrefsAuthorizationCacheTests.csTests/EditMode/MwaCache/PlayerPrefsAuthorizationCacheTests.cs.metaTests/EditMode/MwaCache/SchemaVersionValidationTests.csTests/EditMode/MwaCache/SchemaVersionValidationTests.cs.metaTests/EditMode/MwaClient/AuthorizeRequestWireTests.csTests/EditMode/MwaClient/AuthorizeRequestWireTests.cs.metaTests/EditMode/MwaClient/DeauthorizeRequestWireTest.csTests/EditMode/MwaClient/DeauthorizeRequestWireTest.cs.metaTests/EditMode/MwaClient/LoginShapeCompatibilityTest.csTests/EditMode/MwaClient/LoginShapeCompatibilityTest.cs.metaTests/EditMode/MwaClient/MobileWalletAdapterClientTests.csTests/EditMode/MwaClient/MobileWalletAdapterClientTests.cs.metaTests/EditMode/MwaParser.metaTests/EditMode/MwaParser/AuthorizationResponseParserTests.csTests/EditMode/MwaParser/AuthorizationResponseParserTests.cs.metaTests/EditMode/MwaParser/Fixtures.metaTests/EditMode/MwaParser/Fixtures/authorize-v2-bad-base64.jsonTests/EditMode/MwaParser/Fixtures/authorize-v2-bad-base64.json.metaTests/EditMode/MwaParser/Fixtures/authorize-v2-empty-accounts.jsonTests/EditMode/MwaParser/Fixtures/authorize-v2-empty-accounts.json.metaTests/EditMode/MwaParser/Fixtures/authorize-v2-empty-authtoken.jsonTests/EditMode/MwaParser/Fixtures/authorize-v2-empty-authtoken.json.metaTests/EditMode/MwaParser/Fixtures/authorize-v2-full.jsonTests/EditMode/MwaParser/Fixtures/authorize-v2-full.json.metaTests/EditMode/MwaParser/Fixtures/authorize-v2-minimal.jsonTests/EditMode/MwaParser/Fixtures/authorize-v2-minimal.json.metaTests/EditMode/MwaParser/Fixtures/authorize-v2-oversize-accounts.jsonTests/EditMode/MwaParser/Fixtures/authorize-v2-oversize-accounts.json.metaTests/EditMode/MwaParser/Fixtures/authorize-v2-oversize-chains.jsonTests/EditMode/MwaParser/Fixtures/authorize-v2-oversize-chains.json.metaTests/EditMode/MwaParser/Fixtures/authorize-v2-oversize-display.jsonTests/EditMode/MwaParser/Fixtures/authorize-v2-oversize-display.json.metaTests/EditMode/MwaParser/Fixtures/authorize-v2-oversize-icon.jsonTests/EditMode/MwaParser/Fixtures/authorize-v2-oversize-icon.json.metaTests/EditMode/MwaParser/Fixtures/authorize-v2-wrong-length.jsonTests/EditMode/MwaParser/Fixtures/authorize-v2-wrong-length.json.metaTests/EditMode/MwaParser/ParserVerbosityTests.csTests/EditMode/MwaParser/ParserVerbosityTests.cs.metadocs.metadocs/mwa-cache-guide.mddocs/mwa-method-reference.mddocs/mwa-migration-v1-to-v2.mddocs/mwa-quick-start.mddocs/sign-and-send-investigation.md
💤 Files with no reviewable changes (4)
- Tests/EditMode/JsonRpc/ResponseModelTests.cs
- Tests/EditMode/Mocks/MockMessageSender.cs
- Tests/EditMode/Crypto/EcdsaSignaturesTests.cs
- Tests/EditMode/Crypto/MobileWalletAdapterSessionTests.cs
Semaphore gate: - Wrap _Login, _SignAllTransactions, SignMessage, GetCapabilities in TryAcquireGate/ReleaseGate to prevent concurrent state corruption Bug fixes: - Preserve WaitForCommitmentToSendNextTransaction when backfilling MinContextSlot - Set WalletBase.Account on LoginWithSignIn success - Default null mwaOptions in SolanaWalletAdapter so cache injection works Upstream PR magicblock-labs#283 test compat: - Update IAdapterOperationsContractTests for v2 AuthorizeAsync API - Rewrite SolanaMobileWalletAdapterPrefsTests for v2 cache-based migration - Fix MobileWalletAdapterClientLifecycleTests Authorize → AuthorizeAsync
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Author
|
@JoshhSandhu, not sure if you're the right guy to tag for a check. |
Author
|
Maybe @Kuldotha is the correct person to tag. |
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.
Overview
Complete rewrite of the Mobile Wallet Adapter (MWA) implementation from v1 to v2. 120 files changed, ~4200 additions across types, wire protocol, client, adapter, cache, tests, and documentation.
Individual PRs (for granular review)
PlayerPrefsAuthorizationCache, response parser with size caps, 9 test suitesSolanaMobileWalletAdapterrewrite: session lifecycle, SIWS, concurrency gate, silent reconnect, 5 test suitesWhat Changed
Core Types (PR #3)
AccountInfo,AuthorizationRecord,SignInPayload,SignInResult,SendOptionsIAuthorizationCacheinterface (#272)DeauthorizeResult,ReconnectResult,SignAndSendTxResultJsonRpcException,OperationInFlightException,TransportExceptionWire Protocol + Client (PR #4)
chainidentifiers and backward-compatclusterfieldAuthorizeAsyncwith SIWS,DeauthorizeAsync,SignAndSendTransactionsAsync,CloneAuthorizationAsyncAuthorizationResponseParserwith permissive parsing and size caps for untrusted wallet dataPlayerPrefsAuthorizationCachewith scoped keys, schema versioning, corrupt-JSON recoveryAdapter (PR #5)
Disconnect(),Reconnect()(4-state result),Deauthorize()(tri-state),GetCapabilities(),LoginWithSignIn(SignInPayload),SignAndSendTransactions(Transaction[], SendOptions)SemaphoreSlim(1,1)prevents overlapping wallet calls"pk"/"authToken"and PR#269 keys to new cacheDocumentation (PR #6)
mwa-quick-start.md— install, configure, first connectionmwa-method-reference.md— all 13 public methods with signatures and result variantsmwa-cache-guide.md— IAuthorizationCache interface, default/custom implementationsmwa-migration-v1-to-v2.md— v1 → v2 migration guidesign-and-send-investigation.md— Backpack failure analysisKnown Issue: Backpack
sign_and_send_transactionssign_and_send_transactionsfails with Backpack wallet. The transaction is successfully created, signed, and submitted on-chain — the issue is only in receiving the response back in the dApp. Backpack's WebSocket server crashes during the RPC submission phase, corrupting the local association before the response can be delivered. The dApp's pending request hangs indefinitely. This is a Backpack-specific issue — Phantom and Solflare handle the same flow correctly, surviving 8–12 seconds of Android activity STOP without any WebSocket errors.sign_transactionsworks with all wallets including Backpack (no RPC submission involved). Full investigation:docs/sign-and-send-investigation.mdPre-existing
LocalAssociationScenarioIssuesThese issues exist in the upstream
LocalAssociationScenario.csand are outside the scope of this rewrite:returninExecuteNextAction(line 134-135): Falls through to_actions.Dequeue()on an empty queue afterCloseAssociation().OnClosereconnect loop (line 46-50): No delay, no retry limit, no close-awareness. Thread pool storm when the wallet server is gone.TimeSpan.FromSeconds(9000)= 2.5-hour timeout instead of 9 seconds.scenarioResultaccess:scenarioResult.WasSuccessfulthrows if scenario returns null.Breaking Changes
IAdapterOperations.Authorize()andReauthorize()removed — useAuthorizeAsync()AuthorizationResult.PublicKeyandAccountLabelremoved — useAuthorizationHelpers.PrimaryAccountPublicKeyBytes()andPrimaryAccount().LabelDemo App
https://github.com/Zurcusa/unity-solana-mwa-example
Testing Matrix
Tested on Pixel 7 emulator and Solana Seeker (real device).
Wallets Tested
Functionality Coverage
Summary by CodeRabbit
New Features
Documentation
Tests