Auth 2.0 SIWS (Sign In With Solana)#278
Open
mstevens843 wants to merge 4 commits into
Open
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 (14)
WalkthroughThis PR extends the Solana Mobile Stack with new authorization and transaction signing workflows. It introduces SIWS (Sign In With Solana) support, capability discovery via Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 10
🤖 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/JsonRpc20Client.cs`:
- Line 23: PendingRequests is not thread-safe; introduce a private backing field
(e.g., int _pendingRequests) and update the PendingRequests getter to return
that field, then replace all ++/-- operations on PendingRequests (in SendRequest
and in the ContinueWith callbacks referenced) with Interlocked.Increment(ref
_pendingRequests) and Interlocked.Decrement(ref _pendingRequests) respectively
so all updates are atomic and safe across thread-pool callbacks.
In `@Runtime/codebase/SolanaMobileStack/JsonRpcClient/JsonRequest.cs`:
- Around line 149-151: The MinContextSlot property is typed as int? but Solana's
min_context_slot is a u64; change the property declaration on JsonRequest
(property MinContextSlot) from int? to ulong? so it can represent the full
0..2^64-1 range, keep the existing JsonProperty and RequiredMember attributes
unchanged, and run/adjust any callers or deserialization code that assume an int
to accept ulong? (or cast safely) to prevent overflow during JSON parsing.
In
`@Runtime/codebase/SolanaMobileStack/JsonRpcClient/Responses/AuthorizationResult.cs`:
- Around line 46-49: The attributes marking optional response properties as
required must be removed or replaced: remove the [RequiredMember] on
AuthorizationResult.SignInResult (property SignInResult) since it can be null
for wallets without SIWS, and likewise remove or change [RequiredMember] on
AuthorizationResultAccounts properties Chains, Features, and Icon so they are
treated as optional; if your project has an [OptionalMember] or
nullable-handling attribute, use that instead and ensure the types remain
nullable to match the fallback logic in SolanaMobileWalletAdapter._Login().
In
`@Runtime/codebase/SolanaMobileStack/JsonRpcClient/Responses/SignAndSendResult.cs`:
- Around line 16-19: The SignaturesBytes accessor currently returns null when
there are no signatures; update the SignaturesBytes property in
SignAndSendResult to return an empty List<byte[]> instead of null so callers can
safely iterate without null checks—use Signatures (or Signatures?.Count) to
decide whether to Select(Convert.FromBase64String) into a List<byte[]> or return
new List<byte[]>() when empty or null, preserving the [RequiredMember] attribute
and the property's type.
In `@Runtime/codebase/SolanaMobileStack/LocalAssociationScenario.cs`:
- Around line 240-254: StartResponseTimeout currently awaits Task.Delay and then
checks _closed which leaves a race where a response can arrive between delay
completion and the _closed check causing confusing logs; modify
StartResponseTimeout to accept/use a CancellationToken from a
CancellationTokenSource that you create when starting the timeout and cancel in
the response path (e.g., in HandleEncryptedSessionPayload or wherever you set
results) so the delay is aborted as soon as a response is processed; reference
StartResponseTimeout, HandleEncryptedSessionPayload, _closed and use a
CancellationTokenSource (e.g., _responseCts) that is canceled on successful
response handling and passed to Task.Delay to avoid running the timeout action
after a response.
- Around line 178-182: The catch block currently calls CloseAssociation(null)
which causes _startAssociationTaskCompletionSource.SetResult(null) and leads to
a NullReferenceException when the caller checks result.WasSuccessful; change the
catch to call CloseAssociation with a non-null failure result object (e.g., an
AssociationResult/StartAssociationResult instance indicating failure and
containing the exception info) so
_startAssociationTaskCompletionSource.SetResult receives a valid error result;
update CloseAssociation usage in LocalAssociationScenario.cs and ensure the
failure result includes WasSuccessful=false and the exception message/stack so
SolanaMobileWalletAdapter._Login() can safely inspect result.WasSuccessful.
In `@Runtime/codebase/SolanaMobileStack/MobileWalletAdapterClient.cs`:
- Around line 66-73: Extract the duplicated URI validation into a private helper
on MobileWalletAdapterClient (e.g., ValidateUri or ValidateIdentityAndIconUris)
and replace the two inline checks in the constructor/initializer and in
PrepareAuthRequest with calls to that helper; the helper should accept a Uri and
a mode/enum or bool to indicate expected absolute vs relative (use names
identityUri and iconUri to preserve intent), throw the same ArgumentException
messages when validation fails, and update both the constructor code path and
PrepareAuthRequest to call this single helper to remove duplication.
- Line 96: The call to SendRequest is missing the methodName argument causing
logs to show "Unknown"; update the three call
sites—SendRequest<AuthorizationResult>(request) in the authorize path, the
SendRequest call in SignAndSendTransactions, and the SendRequest call in
GetCapabilities—to pass the appropriate methodName strings (e.g., "authorize",
"signAndSendTransactions", "getCapabilities") as the first parameter so
SendRequest logs and telemetry contain the correct method context.
In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`:
- Around line 92-119: The two async lambdas added via actions.Add share and
mutate the captured variable authorization (and read it later for siws
fallback), which is unsafe with the current Action<IAdapterOperations> async
pattern; fix by combining both steps into a single async delegate so the
Authorize result is read synchronously after awaiting the authorize call:
replace the two separate actions.Add(...) delegates with one async delegate that
calls client.Authorize (assigning authorization), then immediately checks
authorization.SignInResult and authorization.PublicKey and, if needed,
constructs siwsMessageText and calls client.SignMessages to set siwsFallbackSig;
alternatively, migrate the actions collection to use Func<IAdapterOperations,
Task> so captured mutations are reliably visible across awaits (preferred
long-term).
- Around line 30-35: ClusterToChain currently lacks a mapping for "localnet",
causing a wrong fallback to "solana:mainnet"; update the code so
RpcCluster.LocalNet is handled specially rather than using the static
ClusterToChain fallback: add logic (e.g., a ResolveLocalnetChainId method called
where ClusterToChain is read or where the fallback on line 83 occurs) that
queries the local RPC node for its genesis hash (or uses the default
test-validator genesis hash if you choose a hardcoded fallback) and returns the
chain id formatted as "solana:<first-32-chars-of-genesis-hash>"; ensure
ClusterToChain usage or the code path that reads it (references to
ClusterToChain and RpcCluster.LocalNet) uses this resolved value for localnet so
SIWS and wallet authorization receive the correct chain identifier.
🪄 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: f2bf6a7d-1cdb-4d3f-a5fe-9c23d89b4ed8
📒 Files selected for processing (14)
Runtime/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/JsonRpcClient/Responses/SignAndSendResult.csRuntime/codebase/SolanaMobileStack/JsonRpcClient/Responses/SignAndSendResult.cs.metaRuntime/codebase/SolanaMobileStack/JsonRpcClient/Responses/SignInResult.csRuntime/codebase/SolanaMobileStack/JsonRpcClient/Responses/SignInResult.cs.metaRuntime/codebase/SolanaMobileStack/LocalAssociationScenario.csRuntime/codebase/SolanaMobileStack/MobileWalletAdapterClient.csRuntime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.csRuntime/codebase/SolanaWalletAdapter.cs
Adds MWA 2.0 authorize support with Sign In With Solana (SIWS), enabling one-shot connect + prove ownership. Also adds signAndSendTransactions and getCapabilities client methods per the MWA 2.0 spec. SDK plumbing: - New Authorize overload with chain, features, addresses, authToken, signInPayload - New SignAndSendTransactions method with options (commitment, skipPreflight) - New GetCapabilities method (non-privileged) - New response models: SignInResult, SignAndSendResult, CapabilitiesResult - Extended AuthorizationResult with accounts[].chains/features/icon + sign_in_result - Extended JsonRequest with Chain, Features, SignInPayload, SignAndSendOptions Adapter-level SIWS: - SolanaMobileWalletAdapterOptions: added siwsDomain, siwsStatement - SolanaMobileWalletAdapter._Login(): SIWS path when siwsDomain is set, legacy path otherwise - SolanaMobileWalletAdapter.LastSignInResult: stores SIWS result from authorize - SolanaWalletAdapter.LastSignInResult: delegation to internal adapter Tested on Solana Seeker with Phantom and Backpack via example app. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…The React Native SDK's signIn() method does: 1. Authorize with sign_in_payload 2. If wallet doesn't return sign_in_result → fallback to sign_messages with a CAIP-122 message 3. Parse the signed payload: last 64 bytes = signature, rest = signed message
- thread-safe PendingRequests counter via Interlocked over a backing field - remove [RequiredMember] from optional MWA response fields (SignInResult, Chains, Features, Icon) and add NullValueHandling.Ignore so JSON serialization matches their wallet-optional nature - SignaturesBytes returns an empty list instead of null when there are no signatures - pass methodName to SendRequest at every call site for clearer debug logs - map localnet cluster to default solana-test-validator genesis hash to avoid silent fall-through to solana:mainnet during local development - combine the SIWS authorize and CAIP-122 fallback into a single delegate so the captured `authorization` mutation is reliably visible to the fallback step (avoids cross-lambda capture issues with the async-void Action<IAdapterOperations> dispatch pattern)
mstevens843
force-pushed
the
feat/authorize-mwa-2.0
branch
from
0ef117f to
af06468
Compare
Contributor
Author
|
Hey, pushed an update on this one. Rebased onto current main and addressed the CodeRabbit feedback in one followup commit. Rebase notes:
CodeRabbit fixes in af06468:
Ready for another look when you have time. |
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.
NOTE: This PR adds SIWS support to the MWA authorize flow, matching the React Native SDK's signIn() behavior. Wallets that don't support SIWS natively get a fallback via sign_messages. Phantom and Solflare have wallet-side issues with the fallback path (details below). The React Native SDK hits the same behavior from these wallets.
Problem
The MWA 2.0 spec defines
sign_in_payloadas part of the authorize request, letting apps authenticate users and verify wallet ownership in a single step. The Unity SDK had no support for this.Developers had to authorize first, then manually construct and sign a SIWS message in a separate session. That doesn't match what the React Native SDK offers out of the box.
Solution
Added SIWS support to
_Login()inSolanaMobileWalletAdapterusing a two-action approach that matches the React Native SDK.Action 1 sends
authorizewithsign_in_payloadcontaining domain, statement, and URI. Wallets that support SIWS natively (like Backpack) returnsign_in_resultdirectly in the authorize response. One prompt, done.Action 2 is the fallback. If the wallet doesn't return
sign_in_result, we construct a CAIP-122 SIWS message and send it viasign_messages. This covers wallets like Jupiter that supportsign_messagesbut don't handlesign_in_payloadin authorize. The signed payload is parsed correctly per the React Native reference: last 64 bytes are the ed25519 signature, everything before is the signed message.LastSignInResultis exposed on bothSolanaMobileWalletAdapterandSolanaWalletAdapterso apps can read the SIWS result after login.When
siwsDomainis not set, the flow falls back to plain authorize with no SIWS. No behavior change for existing users.Test Results
Tested on Solana Seeker (Android 15):
sign_in_resultreturned, single prompt, works perfectlysign_messagesworks, two prompts (connect + sign), SIWS verifiedsign_messagesas a second RPC in the same session. This is a wallet-side limitation, not an SDK issue.Reply already submittedinonActivityResult). Unrelated to this PR.Other Changes
IsV1SessionfromMobileWalletAdapterSession(no longer needed)&v=1from the association intent URLMobileWalletAdapterClient(extracted helper methods, removed excessive logging)SignInPayloadproperty toSignInPayloadDatainJsonRequestParamsto avoid naming collision[Preserve]constructor fromSignInResultDeploy Notes
No new dependencies. No new scripts. Backwards compatible. SIWS is opt-in via
siwsDomainandsiwsStatementonSolanaMobileWalletAdapterOptions.Summary by CodeRabbit
New Features
Improvements