Check sep24 interactive URL

gianfra-t commented 2 weeks ago

Related to #128.

We realize this will not fix the issue the user may see from waiting too much to click the second confirmation button.

This changes will refresh the sep24 url every 20 seconds until the user actually clicks on the button.

netlify[bot] commented 2 weeks ago

Deploy Preview for pendulum-pay ready!

Name	Link
Latest commit	833197c39c66984b14dfd8f9a550a448a16e307c
Latest deploy log	https://app.netlify.com/sites/pendulum-pay/deploys/66f6a9d0cf011c0008518948
Deploy Preview	https://deploy-preview-155--pendulum-pay.netlify.app
Preview on mobile	Toggle 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 2 weeks ago

@TorstenStueber is this more or less what you had in mind for #128 ? The sep24Url is not really a redirect, yet this works since by not sending the cookies the anchor will not consider it the one-time token expired.

gianfra-t commented 1 week ago

@ebma there is no redirect, and it is expected that it will fail the way you see now. Because AFAIK it works as follows:

They return from the first part of the sep the url. This url does not provide a redirect, but is the one the user needs to open on the new window.
Given the problem we have with the iframe, we could identify that just doing a GET to this url will not send the necessary cookies that are used to log-in the session. If it did, then doing this pre-fetching will invalidate the session entirely (the one-time token use policy they have in place)
But since the session is not validated with the cookies, it also will expire in 1 minute.

I thought this ticket was to check that the url itself returned a 200 (aka. that it was ready to go).

ebma commented 1 week ago

Okay. Maybe what's described in #128 is just not feasible. But the extra check you introduced wouldn't help as it's always done right after we receive the URL and at that time it's never invalid, no?

gianfra-t commented 1 week ago

What was described on the issue was that sometimes the sep24 url will return the 403 response immediately. I personally never saw this and I think this was an artifact of waiting. If the second is the case, then this is a bit useless 😆.

TorstenStueber commented 1 week ago

Yes, I was testing this last week when @gianfra-t worked on the iframe ticket and we discussed possible solutions.

I realized that my assumptions were wrong or outdated. The ticket description does not actually make sense any longer. Sorry that I did not immediately update the ticket.

There is just no redirect, it always either returns a proper website (i.e., 200 response) or the 403 response. So far the only hint we have for the latter case to happen is if the user waits too long between generating the link and clicking on the button. I just checked the sep24 url: it contains a JWT that expires after exactly 30 seconds.

gianfra-t commented 1 week ago

Okay then! What if we change this PR such that we remove the second button and the window is opened as soon as the url is resolved?

TorstenStueber commented 1 week ago

The remaining question is: is this change here still good to have or could it be harmful.

Could it be harmful that we fetch the url already from the frontend? It probably does not "invalidate" the url (what I mean is, if you open the url a second time in the same browser – in top level tabs – then the second time would return a 403, irrespective of the 30s time out).

TorstenStueber commented 1 week ago

@gianfra-t this seems to make sense and we did something similar in the past. However, I changed it for an a element that the user needs to actively click on due to feedback from other team members as mobile devices blocked the programmatic opening of new tabs.

Maybe there is a programmatic way to open a tab also on mobile devices, we could check.

gianfra-t commented 1 week ago

I wasn't aware of that. Makes sense. Then what if we put a little spinner/counter on the second button + we disable the second button once > 30 seconds passed?

Once disabled, the user is presented with the first button again and sep24First is also some again.

TorstenStueber commented 1 week ago

Good idea. I would actually make it simpler: recreate the sep24url automatically every, say, 20 seconds and replace the link in the a element. This would not require user interaction.

The very first button press makes sense, though, as we need to wait for the user to finish entering the input amount.

gianfra-t commented 1 week ago

Okay we can do that second option that's better, and recreate the link every 20 seconds 👍. I'll modify this PR.

gianfra-t commented 1 week ago

@pendulum-chain/devs changed the scope of the PR as workaround for the 403 issue.

ebma commented 1 week ago

I tested it locally and can confirm that it works. I find the code to be a little difficult to read so I started refactoring it a bit. I'm not done yet but will push my changes tomorrow. Just FYI.

gianfra-t commented 1 week ago

Anything I can help with the refactor ?

ebma commented 1 week ago

I moved it directly into an interval which makes us get rid of the extra reset function and is easier to follow IMO. Let me know what you think. Also, I changed it so that the ephemeral secret is passed into the constructInitialState() function. Before that, we were apparently using two different random keypairs, one used in sep10 authentication and the other for the actual ramping process (?) Seemed like a bug to me.

TorstenStueber commented 1 week ago

Oh, great find @ebma. We indeed used a completely different key for sep10. This should not have worked at all but somehow the anchor did possibly not check that properly. Good to have it fixed.

gianfra-t commented 1 week ago

Amazing that the anchor was working even with two different stellar accounts. I tried to look for an explanation because it is hard to believe. On the other hand, if you accept an offramp from any random account as long as the IBAN has been KYC'ed, then it doesn't really matter if you authenticate with a new account and continue the process with another new account, since neither of them will be mapped to the user so far.

ebma commented 1 week ago

It actually does work although you are right that we continue to create new sep24Second promises that will never resolve.

We can refactor it the way you proposed @TorstenStueber. In light of the new developments regarding the iframe solution, I am hesitant to do that. Either we scratch the changes here altogether (except for the ephemeral secret fix) or we merge it the way it is right now and remove it soon after the iframe is working?

TorstenStueber commented 1 week ago

Let's wait for the answer of Mykobo and whether they can enable the iframe solution on production soon. Then I agree that we just scratch the changes here apart from the ephemeral secret.

gianfra-t commented 3 days ago

Since the iframe won't work for now, I implemented a few changes regarding the discussion above to separate the sep24Second process. Only tested slightly.

gianfra-t commented 1 day ago

@pendulum-chain/devs on top of the changes I also realized that if the sep24 or constructInitialState failed, we ended up with a never ending disabled Confirming button. So I added these changes.

Question is, do we want to show some sort of error and disable the button temporarily?

gianfra-t commented 23 hours ago

@TorstenStueber it merged well. I imagine that the behavior of the label "Confirming" now that we refresh the sep24 first process is that upon first successful response, we keep "Enter details".

TorstenStueber commented 23 hours ago

I tested, works correctly.

pendulum-chain / vortex