pendulum-chain / vortex

1 stars 0 forks source link

Fix for connection closed while waiting for receipt response #159

Closed gianfra-t closed 1 week ago

gianfra-t commented 1 week ago

157 and #160.

Following changes to the flow:

Recoverable errors retry

netlify[bot] commented 1 week ago

Deploy Preview for pendulum-pay ready!

Name Link
Latest commit c5a4fd7fc9379c234b59028cdc63e54245662e01
Latest deploy log https://app.netlify.com/sites/pendulum-pay/deploys/66eb82a91cf26a00084e60ce
Deploy Preview https://deploy-preview-159--pendulum-pay.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

gianfra-t commented 1 week ago

@pendulum-chain/devs ready for review and testing IMO. If the fix is urgent, we can later add a persistent cache on the backend (also low effort).

gianfra-t commented 1 week ago

I was assuming that the sendTransaction function would fail if sent twice (given that the hash is removed from the map on the contract). In that case I think it is a bit of a race condition right? If the UI reloads while sendTransaction is still waiting on the backend, then the second call to this endpoint will fail without the cache.

Again I was with the working assumption that sendTransaction throws an error when the contract reverts, and in fact when I try with some random values this is the case.

ebma commented 1 week ago

If we don't want to rely on the transaction cache on the webserver because it could also crash due to some unforeseen issue, we could also put the received transaction hash in the local storage to the other data we store for the offramping process. Then, when the frontend reloads, it could first check if it already received a transaction hash once and wait for the transaction receipt of that instead of hitting the endpoint again.

gianfra-t commented 1 week ago

@pendulum-chain/devs As I am testing the refresh solution, I realize that it is a bit bad how it looks when the page refreshes alone. Do we want to continue with this anyway?

ebma commented 1 week ago

Bad in what sense? Like, as a user you feel like it's buggy and the refresh is unintended? If you have a bad feeling as a user, how about we give a heads-up warning that the page is going to refresh in e.g. 10 seconds or whatever we choose? We could show this as a toast notification with the change I added in https://github.com/pendulum-chain/vortex/pull/154. Or we show a count-down in the progress-page that counts down the seconds until we perform the refresh.

gianfra-t commented 1 week ago

I think we should either add some UI element, or force refresh the Polkadot.js API on the background, which is the only instance we keep right? I lean for the second one to avoid doing more UI changes, but not strong opinion.

TorstenStueber commented 1 week ago

Some addendum to my message above. I tested to submit a transaction to the receiver contract for an id that is invalid (for example because there was already some succcessful transaction executing the XCM in the meantime).

And indeed, the sendTransaction already throws an error. This happens despite the transaction not being executed in the block yet but because the gas estimation algorithm that is happening before the transaction is submitted would already error as the transaction reverts:

Details: VM Exception while processing transaction: revert Hash invalid
Version: 2.21.3
    at [...]
  cause: EstimateGasExecutionError: An internal error was received.

I think we can just ignore this error (in absence of other bugs this just means that the funds are already on Pendulum) and instead also additionally check whether some amount of USDCaxl already arrived on the Pendulum ephemeral account and – if this is the case – switch to the next phase.

TorstenStueber commented 1 week ago

@gianfra-t could we maybe just get rid of the failure phase altogether? Instead of switching to that failure phase, stay in the current phase and just have a boolean hook state isFailure in, e.g., useMainProcess. This boolean state is false at start and when it is true, then show the failure screen.

This way, even when 10 minutes elapsed and we show the failure screen, one could still try to recover from the error by just reloading the page. This could address the problem that @ebma mentions above ("We also need to consider that the user might just close the during the processing and then reopen the page more than 10 minutes in the future.").

gianfra-t commented 1 week ago

@TorstenStueber I don't understand how that solution we would distinguish how to show or not show the failure page. Would it be time based? It is a bit confusing if the user loads the site, is shown the failure page and yet the app still is doing operations. What do you think of this idea here?

At some point we need to have some criteria (time based, count based or type of error) to show or not show the failure screen and "give up" on the process.

TorstenStueber commented 1 week ago

At some point we need to have some criteria (time based, count based or type of error) to show or not show the failure screen and "give up" on the process.

@gianfra-t there is no perfect solution. Either we leave it to the user to decide by showing both options Continue and Restart (as I originally proposed) or we do the Continue automatically in the background and then have to decide ourselves when to display the Restart option (e.g., the failure screen) – as we decided today.

The time based criterion is surely the easiest one. However, I somehow think that we should give the user to option to try to recover – so I figured the proposal in my last message would allow for that.

TorstenStueber commented 1 week ago

I took over the PR of @gianfra-t and added some more failure handling code.

I made many different tests and fixed some more issues I found.

I am adding a couple of comments to explain my changes. Here is a summary:

Screenshot 2024-09-18 at 19 05 43

TorstenStueber commented 1 week ago

I created an extra PR to address all review comments.