solana-mobile / mobile-wallet-adapter

Other
245 stars 101 forks source link

If you trigger an association activity while the last one is still concluding, it will silently fail #94

Closed steveluscher closed 1 year ago

steveluscher commented 2 years ago

Problem

Naturally, the correct thing to do when you have multiple operations to perform with a wallet is to:

  1. Open a session through association
  2. Invoke n API methods
  3. Close the session

Despite this, we know that people – for one reason or another – are going to write apps that execute that full loop multiple times, with one call coming on the heels of the next.

I discovered a bug with doing so when testing the React Native SDK. I called two @solana/wallet-adapter methods in a row and observed the second one being silently dropped. This appears to be because, if we start an activity before the first one has fully animated back to the original app, that activity will be ignored.

Repro

Make the following modification:

diff --git a/android/fakedapp/src/main/java/com/solana/mobilewalletadapter/fakedapp/MainViewModel.kt b/android/fakedapp/src/main/java/com/solana/mobilewalletadapter/fakedapp/MainViewModel.kt
index 8ffb5a6..1e286f5 100644
--- a/android/fakedapp/src/main/java/com/solana/mobilewalletadapter/fakedapp/MainViewModel.kt
+++ b/android/fakedapp/src/main/java/com/solana/mobilewalletadapter/fakedapp/MainViewModel.kt
@@ -78,9 +78,12 @@ class MainViewModel(application: Application) : AndroidViewModel(application) {
     }

     suspend fun authorizeAndSignTransaction(sender: StartActivityForResultSender) {
+        var authorized = localAssociateAndExecute(sender) { client ->
+            doAuthorize(client)
+        }
+        // delay(1000) <--- add this to give time for the activity to finish, and it works.
         val signedTransactions = localAssociateAndExecute(sender) { client ->
-            val authorized = doAuthorize(client)
-            if (authorized) {
+            if (authorized == true) {
                 val transactions = Array(1) {
                     MemoTransaction.create(uiState.value.publicKeyBase58!!)
                 }

Open the fake dapp and click the ‘combined authorize and sign’ button.

sdlaver commented 2 years ago

Root cause: you can't start activities while not in the foreground. Back to back calls must wait for the originating activity to return to the RESUMED state. This should be elevated to an immediate exception, to catch the programming error.

steveluscher commented 2 years ago

What if, instead, localAssociateAndExecute just didn't resolve until the app returns to the foreground?

steveluscher commented 2 years ago

I guess maybe there are cases where you don't intend to fire off another localAssociateAndExecute but you might like to make use of the result in the background.

steveluscher commented 2 years ago

What about the opposite? Any call to localAssociateAndExecute suspends until the app is foregrounded before starting the activity?

sdlaver commented 2 years ago

What about the opposite? Any call to localAssociateAndExecute suspends until the app is foregrounded before starting the activity?

Yeah, this should be doable. I'll make sure there's a sensible timeout option, in case the app fails to be foregrounded in a timely manner (say, 2 mins).