WalletConnect / WalletConnectKotlinV2

WalletConnect Kotlin SDK v2
Apache License 2.0
232 stars 80 forks source link

Incorrect Order of Transaction Requests with Multiple Active Sessions in WalletConnect #1268

Open let-it-snow opened 10 months ago

let-it-snow commented 10 months ago

We have encountered changes to the way transaction requests are received when there are multiple active sessions in WalletConnect. Please research this issue.

Issue Description: If there are multiple active sessions, the order in which transaction requests are received in the application is incorrect. The Request from the first service received in the fun onSessionRequest() method is repeated until it is rejected or accepted, regardless of the other Request from the other service. The second request is displayed only after the first one has been processed.

override fun onSessionRequest(sessionRequest: Sign.Model.SessionRequest, verifyContext: Sign.Model.VerifyContext) {

}

This leads to inconsistency in the logic of the Android and iOS platforms. Previously, the logic for receiving Requests was identical for both platforms.

Issue Reproducibility: The problem is reproduced in WalletConnect BOM 1.20.0 and several previous versions.

Preconditions: There were 2 active WalletConnect sessions created in two different services (Service 1 and Service 2).

Steps to reproduce:

  1. Create a transaction request from Service 1 - this transaction request (sessionRequest: Sign.Model.SessionRequest) will be received on the mobile device.
  2. Create a transaction request from Service 2.

Intermediate result: The device re-receives the transaction request from Service 1 instead of Service 2.

  1. Sign or reject the transaction request from Service 1.

Result: The device receives a transaction request from Service 2. Changing the logic for receiving requests introduces discrepancies into the operation of the Stellar logic and makes signing a transaction impossible.

Expected result: The transaction request from Service 2 is received immediately in the second step above, regardless of whether the transaction request from Service 1 is confirmed or rejected in the first step.

Additional Information: We were unable to determine exactly when this request processing deviation occurred. However, this behavior change creates challenges on the iOS platform to maintain logical consistency between the Android and iOS platforms. There is a problem with changing the logic for receiving requests. It causes issues with the operation of the Stellar logic and makes it impossible to sign a transaction. Furthermore, the current change in the logic for receiving incoming transactions has led to a critical issue. When we receive a transaction for signing, it comes with an XDR that needs to be signed, along with a sequence number for each transaction. However, because transactions are processed sequentially instead of immediately upon creation, all transactions received during the signing process have the same sequence number. This creates an error that makes it impossible to sign transactions properly. It is essential to address this issue as it affects the logical consistency between platforms and significantly impacts the functionality of the Stellar logic. Please let us know if you need any further clarification or assistance. Thank you

jakubuid commented 9 months ago

Hello @let-it-snow, thank you for bringing this up. You're right saying that the pending requests are stored internally in the queue. It prevents users from having un-responded requests. It means that whenever a user receives a requests and this requests us not approved or rejected it'll be emitted from the SDK whenever the next request is sent. So as you described, users will see firstly the first un-responded request, after approving or rejecting the next one will be immediately emitted.

This logic was supposed to be implemented on the iOS SDK as well. The fact that there is feature parity miss match, causes the inconsistency in the SDKs logic. Apologies for that.

On my side, I'll forward this to Swift team to investigate further and add missing behaviour.

From your message, it's not fully clear to me if this logic causes users not being able to connect or sign. Can you please provide the way how I can reproduce your issue? I can reproduce the queue logic since it was implemented on purpose, but I can't understand the case of your users flow.

wayne-law commented 9 months ago

@jakubuid Hello, I am a wallet developer, and I am perplexed by your implementation. Under your internal queue mechanism, I consistently receive the first unprocessed request. Sometimes, when users interact with a dApp on their mobile browser, they initiate two transactions. Upon returning to our wallet for processing, due to this implementation, we can only display the first transaction. Users may mistakenly process the displayed first transaction as the second one, potentially causing them losses.

jakubuid commented 9 months ago

@wayne-law The second request should be emitted when the first one is responded to. Can you share the reproduction steps? I'd like to test it on my side.

let-it-snow commented 8 months ago

Hi @jakubuid,

Sorry for the delay in my response.

From your message, it's not fully clear to me if this logic causes users not being able to connect or sign. Can you please provide the way how I can reproduce your issue? I can reproduce the queue logic since it was implemented on purpose, but I can't understand the case of your users flow.

On the Stellar network, every new transaction is given a sequence number when it is created. This sequence number increases by 1 for each subsequent transaction. However, if the first transaction is not signed and another transaction is initiated, the new transaction will have the same sequence number as the unsigned one.

Implemented on your side behavior leads to a situation when only one transaction can be signed and completed successfully. Any attempt to sign a second transaction will result in an error that prevents its completion.

To address the issue, we propose receiving transactions at the moment of their creation without queuing them. This would allow rejecting transactions created before the currently active transaction was signed.

As an example here, let's use the WalletConnect in the LOBSTR mobile app:

Preconditions: Two active WalletConnect sessions have been created in the LOBSTR for two different services (e.g. StellarTerm and Aquarius).

Steps to reproduce the issue:

  1. Create a transaction request on StellarTerm. The request sessionRequest: Sign.Model.SessionRequest will be received by the LOBSTR app, containing the transaction XDR along with its sequence number.

  2. Create a transaction request from Aquarius. As a result, the transaction XDR will be created with the same Sequence number as for the StellarTerm service.

Result:

We believe that the issue has been fully described, and we've proposed a viable solution for it.

Thanks.

jakubuid commented 8 months ago

Implemented on your side behavior leads to a situation when only one transaction can be signed and completed successfully. Any attempt to sign a second transaction will result in an error that prevents its completion. SDK allows to sign of an indefinite number of transactions. Once a given transaction is approved/rejected the next one in the queue is emitted.

The iOS team is working on adding a queue for pending requests so that both platforms will have the same functionality.

The transaction numbering logic on Stellar network shouldn't be affected by the queue logic. Would a feature where the queue logic is disabled help here? Can the numbering logic be changed in a way to increase the index every-time when transaction is created?

let-it-snow commented 8 months ago

Hi @jakubuid,

We believe adding a feature that disables the queue logic would be a good solution that meets our needs. Disabling this feature would allow us to manage the process of creating transactions more flexibly.

Thanks for your cooperation.