feat(MWA): add sign_and_send_transactions#279
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 (1)
WalkthroughAdds sign-and-send transaction support: new interface method, request options and result models, client adapter RPC call, and wallet adapter overrides implementing authorization, context-slot lookup, transaction submission, and signature conversion. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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: 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 193-200: The code silently swallows exceptions from
ActiveRpcClient.GetLatestBlockHashAsync and proceeds to call
sign_and_send_transactions without min_context_slot; change this to surface
failures so we don't send incompatible requests. Replace the empty catch around
the GetLatestBlockHashAsync call (the contextSlot lookup) with logic that either
rethrows the exception or returns an explicit error/result to the caller (so the
caller of the method that constructs the sign_and_send_transactions request will
not proceed), and ensure any logging uses a clear message including the
exception. Locate the block using ActiveRpcClient.GetLatestBlockHashAsync and
the contextSlot variable and make the method fail-fast instead of continuing
when that call fails so min_context_slot is never omitted.
🪄 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: 2d282995-d3c6-40bf-8cbe-91f22b5a22d6
📒 Files selected for processing (7)
Runtime/codebase/SolanaMobileStack/Interfaces/IAdapterOperations.csRuntime/codebase/SolanaMobileStack/JsonRpcClient/JsonRequest.csRuntime/codebase/SolanaMobileStack/JsonRpcClient/Responses/SignAndSendResult.csRuntime/codebase/SolanaMobileStack/JsonRpcClient/Responses/SignAndSendResult.cs.metaRuntime/codebase/SolanaMobileStack/MobileWalletAdapterClient.csRuntime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.csRuntime/codebase/SolanaWalletAdapter.cs
…ctions Previously the GetLatestBlockHashAsync exception was swallowed and the request proceeded without min_context_slot, which is the exact gap this PR is meant to close for Phantom compatibility. Surface the error instead so the caller does not silently send a request the wallet will reject.
mstevens843
force-pushed
the
feat/sign-and-send-txs
branch
from
b6f1a3b to
591ea8a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 295-306: In SignAndSendTransaction, the catch that currently
rethrows after failing to fetch the latest block hash (catch around
ActiveRpcClient.GetLatestBlockHashAsync and variable contextSlot) should not
rethrow; instead log the error and leave contextSlot as null so the method
continues and returns a RequestResult<string> failure consistently like the
later failure path; remove the "throw" and ensure the catch only logs (e.g.,
Debug.LogError) and preserves/sets contextSlot = null; apply the same
non-throwing change to any similar catch blocks in this method (the later
failure handling region around the RequestResult<string> return).
- Around line 277-283: The code dereferences authorization and unconditionally
assigns _authToken after checking result.WasSuccessful, which can overwrite a
valid persisted token when authorization is null or contains no AuthToken;
update the post-success logic in the method handling result/res so that you
first guard that authorization is not null and authorization.AuthToken is not
null/empty before assigning _authToken, only set _authToken when a real token
exists, and preserve the existing token otherwise (do not clear or overwrite
it); reference the variables/methods result, authorization, _authToken, and the
returned res to find and adjust the logic accordingly.
🪄 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: 2b6a2001-22b8-430d-9da6-425cc39a9d1b
📒 Files selected for processing (7)
Runtime/codebase/SolanaMobileStack/Interfaces/IAdapterOperations.csRuntime/codebase/SolanaMobileStack/JsonRpcClient/JsonRequest.csRuntime/codebase/SolanaMobileStack/JsonRpcClient/Responses/SignAndSendResult.csRuntime/codebase/SolanaMobileStack/JsonRpcClient/Responses/SignAndSendResult.cs.metaRuntime/codebase/SolanaMobileStack/MobileWalletAdapterClient.csRuntime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.csRuntime/codebase/SolanaWalletAdapter.cs
- guard _SignAndSendAllTransactions against null authorization and avoid wiping a valid session token when the wallet returns no fresh token - return a failure RequestResult<string> on context-slot fetch failure instead of throwing, matching the rest of SignAndSendTransaction
Contributor
Author
|
Pushed another round of fixes to address the new CodeRabbit feedback after the rebase.
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 the MWA 2.0
sign_and_send_transactionsmethod to the Unity SDK. The wallet signs the transaction and submits it to the network in one step, returning the signature. All testedwallets work except Backpack, which crashes due to a deserialization bug on their side.
Problem
The MWA 2.0 spec defines
sign_and_send_transactionsas a native wallet method that lets the wallet handle both signing and network submission. The Unity SDK only hadsign_transactions, which meant developers had to sign through the wallet, get the signed transaction back, then submit it themselves via RPC. The React Native SDK already supportssignAndSendTransactionsout of the box.Solution
Added
sign_and_send_transactionssupport across the full stack:SignAndSendOptionsclass inJsonRequest.cswith all spec fields (min_context_slot,commitment,skip_preflight,max_retries,wait_for_commitment_to_send_next_transaction)SignAndSendResultresponse model with base64 signature decodingSignAndSendTransactions()on the client and interfaceSignAndSendTransaction()override onSolanaMobileWalletAdapterthat opens a session, does auth/reauth, then callssign_and_send_transactionsSignAndSendTransaction()delegate onSolanaWalletAdapterThe override automatically fetches
minContextSlotfrom the latest blockhash before sending to the wallet. This is needed because Phantom has a known bug (solana-mobile/mobile-wallet-adapter#1146) where it opens but never shows the approval dialog ifminContextSlotis not set in theoptions. The spec says it's optional, but Phantom requires it. Fetching it automatically means developers don't have to think about it.
The returned signature is converted from base64 to base58 so it matches what Solana explorers and RPC expect.
Test Results
Tested on Solana Seeker (Android 15):
All wallets work except Backpack. Jupiter, Solflare, Phantom, and Seed Vault all successfully sign, submit, and return valid base58 transaction signatures.
Phantom originally failed with the same symptom described in issue #1146. It would open the wallet but never show an approval prompt. After adding the automatic
minContextSlotfetch, Phantom works correctly and shows the approval dialog as expected.Backpack crashes inside its own Kotlin code with
JsonDecodingException: Class discriminator was missinginSolanaMobileWalletAdapterWalletLibModule. This is a Backpack deserialization bug. The transaction payload we send is identical to what Jupiter and Solflare receive and handle fine. There are no open issues or workarounds for this on Backpack's side.Deploy Notes
No new dependencies. No new scripts. Non-breaking addition. The existing
sign_transactionspath is unchanged.Summary by CodeRabbit
New Features
Behavior Changes