improvement: Added customClient option in SignAndSendTransaction & Added detection of denial of transaction#268
improvement: Added customClient option in SignAndSendTransaction & Added detection of denial of transaction#268memxor wants to merge 2 commits intomagicblock-labs:mainfrom
Conversation
WalkthroughAdded a UserApprovedTransaction event and triggers across wallet adapters; extended WalletBase.SignAndSendTransaction to accept an optional IRpcClient for submitting transactions. Signing logic otherwise unchanged. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Runtime/codebase/DeepLinkWallets/PhantomDeepLink.cs`:
- Around line 305-311: On the error branch inside PhantomDeepLink (where it
currently calls SolanaWalletAdapter.TriggerUserApprovedTransaction(false) and
returns), also complete the pending TaskCompletionSource so awaiters don't hang:
call _signedTransactionTaskCompletionSource.TrySetException(new
Exception("Transaction signing cancelled")) and then
_signedTransactionTaskCompletionSource.TrySetResult(null) (matching the WebGL
behavior) before returning; keep the existing
TriggerUserApprovedTransaction(false) call intact.
- Around line 328-334: The error path in PhantomDeepLink.cs never completes the
TaskCompletionSource used by SignMessage/ParseSuccessfullySignedTransaction,
causing awaiters to hang; modify the error branch (where it currently calls
SolanaWalletAdapter.TriggerUserApprovedTransaction(false) and logs the error) to
complete _signedMessageTaskCompletionSource (use TrySetException(new
Exception(errorMessage)) or TrySetResult(null)/false as appropriate for the
expected result type) so callers are released, and use TrySet... methods to
avoid race InvalidOperationExceptions; update the same pattern for
ParseSuccessfullySignedTransaction if present.
In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`:
- Around line 128-129: The SignMessage method is missing the cross-platform
event trigger; after the call to StartAndExecute completes in SignMessage (same
spot where StartAndExecute is used in the transaction flow), call
SolanaWalletAdapter.TriggerUserApprovedTransaction(result.WasSuccessful) to
mirror the behavior in the transaction path. Add this trigger immediately after
StartAndExecute returns (before the subsequent if (!result.WasSuccessful)
branch) so message signing fires the same UserApprovedTransaction event as the
transaction flow.
🪄 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: 6b2c1303-dc89-4b60-920a-4da8bce9415f
📒 Files selected for processing (4)
Runtime/codebase/DeepLinkWallets/PhantomDeepLink.csRuntime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.csRuntime/codebase/SolanaWalletAdapter.csRuntime/codebase/SolanaWalletAdapterWebGL/SolanaWalletAdapterWebGL.cs
| if (!string.IsNullOrEmpty(errorMessage) || string.IsNullOrEmpty(data)) | ||
| { | ||
| SolanaWalletAdapter.TriggerUserApprovedTransaction(false); | ||
| Debug.LogError($"Deeplink error: Error: {errorMessage} + Data: {data}"); | ||
| return; | ||
| } | ||
| else SolanaWalletAdapter.TriggerUserApprovedTransaction(true); |
There was a problem hiding this comment.
Critical: TaskCompletionSource never completed on error path - callers will hang indefinitely.
The new trigger code is placed in an error path that returns early without completing _signedTransactionTaskCompletionSource. Any code awaiting _SignTransaction will hang forever when the user declines.
Compare with the WebGL implementation which properly handles this:
_signedTransactionTaskCompletionSource.TrySetException(new Exception("Transaction signing cancelled"));
_signedTransactionTaskCompletionSource.TrySetResult(null);Proposed fix
if (!string.IsNullOrEmpty(errorMessage) || string.IsNullOrEmpty(data))
{
SolanaWalletAdapter.TriggerUserApprovedTransaction(false);
+ _signedTransactionTaskCompletionSource?.TrySetException(new Exception($"Deeplink error: {errorMessage}"));
+ _signedTransactionTaskCompletionSource?.TrySetResult(null);
Debug.LogError($"Deeplink error: Error: {errorMessage} + Data: {data}");
return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Runtime/codebase/DeepLinkWallets/PhantomDeepLink.cs` around lines 305 - 311,
On the error branch inside PhantomDeepLink (where it currently calls
SolanaWalletAdapter.TriggerUserApprovedTransaction(false) and returns), also
complete the pending TaskCompletionSource so awaiters don't hang: call
_signedTransactionTaskCompletionSource.TrySetException(new
Exception("Transaction signing cancelled")) and then
_signedTransactionTaskCompletionSource.TrySetResult(null) (matching the WebGL
behavior) before returning; keep the existing
TriggerUserApprovedTransaction(false) call intact.
| if (!string.IsNullOrEmpty(errorMessage) || string.IsNullOrEmpty(data)) | ||
| { | ||
| SolanaWalletAdapter.TriggerUserApprovedTransaction(false); | ||
| Debug.LogError($"Deeplink error: Error: {errorMessage} + Data: {data}"); | ||
| return; | ||
| } | ||
| else SolanaWalletAdapter.TriggerUserApprovedTransaction(true); |
There was a problem hiding this comment.
Critical: TaskCompletionSource never completed on error path - callers will hang indefinitely.
Same issue as ParseSuccessfullySignedTransaction - the _signedMessageTaskCompletionSource is never completed when there's an error, causing callers awaiting SignMessage to hang forever.
Proposed fix
if (!string.IsNullOrEmpty(errorMessage) || string.IsNullOrEmpty(data))
{
SolanaWalletAdapter.TriggerUserApprovedTransaction(false);
+ _signedMessageTaskCompletionSource?.TrySetException(new Exception($"Deeplink error: {errorMessage}"));
+ _signedMessageTaskCompletionSource?.TrySetResult(null);
Debug.LogError($"Deeplink error: Error: {errorMessage} + Data: {data}");
return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Runtime/codebase/DeepLinkWallets/PhantomDeepLink.cs` around lines 328 - 334,
The error path in PhantomDeepLink.cs never completes the TaskCompletionSource
used by SignMessage/ParseSuccessfullySignedTransaction, causing awaiters to
hang; modify the error branch (where it currently calls
SolanaWalletAdapter.TriggerUserApprovedTransaction(false) and logs the error) to
complete _signedMessageTaskCompletionSource (use TrySetException(new
Exception(errorMessage)) or TrySetResult(null)/false as appropriate for the
expected result type) so callers are released, and use TrySet... methods to
avoid race InvalidOperationExceptions; update the same pattern for
ParseSuccessfullySignedTransaction if present.
|
|
||
| SolanaWalletAdapter.TriggerUserApprovedTransaction(result.WasSuccessful); |
There was a problem hiding this comment.
LGTM - but missing trigger in SignMessage method.
The trigger placement here is correct, firing after StartAndExecute completes with the appropriate success status.
However, the SignMessage method (lines 147-189) in this file does not trigger UserApprovedTransaction, whereas the WebGL and PhantomDeepLink implementations do trigger the event for message signing. This creates inconsistent cross-platform behavior.
Proposed fix to add trigger in SignMessage
}
);
+ SolanaWalletAdapter.TriggerUserApprovedTransaction(result.WasSuccessful);
if (!result.WasSuccessful)
{
Debug.LogError(result.Error.Message);Add this after line 181, before the if (!result.WasSuccessful) check.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs` around lines
128 - 129, The SignMessage method is missing the cross-platform event trigger;
after the call to StartAndExecute completes in SignMessage (same spot where
StartAndExecute is used in the transaction flow), call
SolanaWalletAdapter.TriggerUserApprovedTransaction(result.WasSuccessful) to
mirror the behavior in the transaction path. Add this trigger immediately after
StartAndExecute returns (before the subsequent if (!result.WasSuccessful)
branch) so message signing fires the same UserApprovedTransaction event as the
transaction flow.
|
@Kuldotha can you please check this? |
memxor
changed the title
|
Hey @memxor @lopeselio, i noticed we are both doing open heart surgery on SolanaMobileWalletAdapter.cs right now, my pr #269 is an active fix for issue #267, which heavily rewrites the MWA auth/signing flow to handle session caching and silent reconnects cause we are modifying the exact same try/catch blocks in _SignAllTransactions and SignTransaction, these two PRs are going to cause a pretty nasty merge conflict for the maintainers since #269 contains the heavier structural changes, would you be open to me just pulling your TriggerUserApprovedTransaction event logic directly into my pr? it perfectly complements the ux fixes im adding, and it saves @Kuldotha from having to untangle a rebase conflict. happy to add you as a coauthor on the commit for the event triggers so you get the contribution credit lmk what you think! |
|
Hey @JoshhSandhu sorry we have gone through your code, and we have seen that you've major changes in there. We have done very minor changes in this PR, so we have decided not to sync. And as it goes for the session caching, we have already fixed that in a previous PR of ours. Thanks for understanding. |
| bool skipPreflight = false, | ||
| Commitment commitment = Commitment.Confirmed) | ||
| Commitment commitment = Commitment.Confirmed, | ||
| IRpcClient customClient = null) |
There was a problem hiding this comment.
This is not the place to pass a custom rpc. Like I mentioned in TG, this is unnecessary as well if you create an additional wallet for the ER connection.
|
@memxor We disagree with commit 1 and don't think this should be in the SDK. Just make a separate wallet for the ER. About commit 2, isn't the issue that we just return the error and never set a result to the TCS? Your change looks like it works around this issue, and wouldn't be necessary if the TCS would be handled properly. Or am I missing something? If so, this also doesn't fix the issue for people who aren't specifically using the event but are waiting for the TCS, they will still be left hanging. |
4 participants
| Status | Type | Core Change |
| Ready | Feature | No |
Commit 1 —
customClientonSignAndSendTransaction(e.g.d07eab0)WalletBase.SignAndSendTransactionnow accepts an optionalIRpcClient customClient. When provided, submission uses that client; otherwise behavior is unchanged (ActiveRpcClient).Commit 2 — Detect transaction decline (e.g.
0727202)This commit builds on the
UserApprovedTransaction/TriggerUserApprovedTransaction(bool)by signaling declines and failures, not only successful approvals.Behavior
data), adapters callTriggerUserApprovedTransaction(false)so listeners can treat it as declined or failed in the same frame as success paths usetrue.Files involved in commit 2
Runtime/codebase/DeepLinkWallets/PhantomDeepLink.cs— decline/error paths for transaction and message signing callbacks.Runtime/codebase/SolanaWalletAdapterWebGL/SolanaWalletAdapterWebGL.cs— matching behavior where WebGL reports cancel/error.Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs— transaction path (and any message-signing parity if included).Runtime/codebase/SolanaWalletAdapter.cs— shared event/API if touched.Why it matters
Deploy / integration notes
UserApprovedTransactionshould expectfalsenot only for “declined” but for any failed or incomplete signing callback where the adapter triggers it (treat as non-success).customClientfor send).