pendulum-chain / vortex

https://app.vortexfinance.co/
1 stars 1 forks source link

Bugfix: workaround for erroneuous sep24 url resolution #128

Closed TorstenStueber closed 1 month ago

TorstenStueber commented 2 months ago

The sep24 url will either return a 301/302 redirect to the actual url the user should be redirected to or a 403 response. Usually the first response will be returned for the first request to the url and the 403 for all subsequent requests. We already have a mechanism in place that allows the user to only call the url once. For the iframe solution this shouldn't be a problem either.

It seems that sometimes the sep24 url will return the 403 response immediately, even on the first call. We could circumvent this by:

TODO Implement the circumvention strategy.

TorstenStueber commented 2 months ago

Hey team! Please add your planning poker estimate with Zenhub @bogdanS98 @b-yap @ebma @gianfra-t @Sharqiewicz

prayagd commented 2 months ago

@TorstenStueber we dont estimate bugs, moving this to ready

gianfra-t commented 2 months ago

Do we understand why this happens? I don't remember experiencing this before. Could it also be that it is a UI bug that called the URL twice?

ebma commented 2 months ago

I encountered this issue as well in the past. At that time I assumed it was because the SEP24 URL was 'stale' ie. I waited too long to click on the button to access the anchor UI. I think this happens because we prepare the SEP24 stuff in advance and the user can take an arbitrary amount of time to access the KYC page later.

gianfra-t commented 2 months ago

But if that is the case, now the iframe will load immediately as soon as the url is defined, and it will solve this issue. Maybe we can deploy this new solution and check if users encounter this error again. Perhaps we could add more logs in the console to be sure what is going on, should it happen again.

ebma commented 2 months ago

I also think we should wait and see if this still pops up when we show the iframe. Maybe we can save the efforts and this strategies described in the description are not needed anymore.

ebma commented 2 months ago

@TorstenStueber do you think this is very urgent and we need to address this right away or can it wait until we implemented the iframe in https://github.com/pendulum-chain/vortex/issues/102?

TorstenStueber commented 2 months ago

It's okay to wait (although I don't see this solution as a lot of effort, just a few lines of code in one file).

gianfra-t commented 2 months ago

True that it is only a few lines of code, but at least in this case I would like to understand why the anchor would send an initial failed response, otherwise we end up with much patch logic that may not be necessary.

gianfra-t commented 2 months ago

I will look at this since the iframe solution is stuck.

TorstenStueber commented 1 month ago

My assumptions for this ticket were wrong, see here: https://github.com/pendulum-chain/vortex/pull/155#issuecomment-2355668223

vadaynujra commented 1 month ago

Can be closed as not required in light of the iframe solution @TorstenStueber ?

gianfra-t commented 1 month ago

If the iframe solution works well, then yes this will not be needed.