pendulum-chain / vortex

1 stars 0 forks source link

Allow for swap possibility before offramping. #33

Closed gianfra-t closed 3 months ago

gianfra-t commented 4 months ago

Closes: #25

General Description

Adds the option to swap an initial asset for another such that the off-ramp can be performed on the second one.

The interface of the portal is largely used and adapted to interact with the swap before submitting of the off-ramp.

Code changes (most critical)

Testing

Swap was tested using a Nabla instance in Foucoco, with the corresponding temporary modifications to TOKEN_CONFIG.

netlify[bot] commented 4 months ago

Deploy Preview for pendulum-pay ready!

Name Link
Latest commit 7ff9af487d67bbc990184fac1919ba3f83c28a95
Latest deploy log https://app.netlify.com/sites/pendulum-pay/deploys/665e14135572260008546f8f
Deploy Preview https://deploy-preview-33--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 4 months ago

@pendulum-chain/devs the token config was modified to be consistent with a nabla instance on Foucoco.

This allows to test a swap between XCM: 0 and XCM: 3, here mocked to be USDT and EURC respectively. This needs to be changed when testing is done.

TorstenStueber commented 4 months ago

@gianfra-t What is the reason that this version is now using testnet? Can we not change it back to mainnet (and using the Pendulum deployment of Nabla?)

gianfra-t commented 4 months ago

Yes @TorstenStueber we can, I left it pointing to testnet in case during review someone wanted to test it without the need for "real" tokens. Should I change it to mainnet and skip this?

gianfra-t commented 4 months ago

@ebma thanks for the remarks. I managed to correct some of them but I think the session expiration issue will not be as simple to fix. The problem is that the token is already validated as soon as the user is presented with the option to open the external window. This happens when the component loads for the first time. The immediate previous step on the offramp opration is the creation of this token here.

I tried on this commit to generate a new token when the user has indeed opened the window, print it manually and check if it would work as well, but this is not the case. Even more the original token is invalidated once it is used.

The only option I see is to perform the first step of the sep24 also when the user clicks the button. This will probably add a bit of delay.

In which scenario did you encounter this problem? The new added changes should not have affected this. I will add that I also encountered this issue once. I waited for a few minutes to open the window while debugging.

gianfra-t commented 4 months ago

@TorstenStueber @ebma I realize that the button to switch between swap and non-swap is not very intuitive. Should we go with a top level tab instead like @ebma suggested in the previous comment?

ebma commented 4 months ago

I find it acceptable to allow the case for a direct offramp without a swap (that you support right now), but the simplest would be to let the user decide initially by choosing the action via a button and in the case of a direct offramp to just behave in the previous prototype.

@gianfra-t how about we use top-level tabs to switch between your new UI for swapping (using Nabla) but always starting with USDT, and the old/existing offramp for directly offramping with BRL/EUR. For the swap, I think we can keep the swap form as is and always set USDT in the first selector, not allowing the user to change away from it.

gianfra-t commented 4 months ago

Seems good! It shouldn't be a problem to just fix USDT for the swap tab, I think the logic in our component can also be reused.

gianfra-t commented 4 months ago

I pushed a version with tab selection. We can later tweak how it looks, if needed. For the swap option, USDT will always be selected.

ebma commented 4 months ago

I managed to correct some of them but I think the session expiration issue will not be as simple to fix. The problem is that the token is already validated as soon as the user is presented with the option to open the external window. This happens when the component loads for the first time. The immediate previous step on the offramp opration is the creation of this token here.

Maybe we should add some timer to the Sep24 component that makes it periodically try to restore the session token? Though I can see some logic here that at least according to the comment already tries to refresh the token. Seems like this didn't work reliably.

gianfra-t commented 4 months ago

Exactly, at least that method didn't work to refresh the token. Yes, the token is obtained again, but useless with the url provided by the anchor. I imagine they map the session request with the initiating token since it is a one-time only token.

TorstenStueber commented 4 months ago

Regarding the token expiry: I just checked and it seems that the url generated through SEP-24 contains a JWT that is only valid for 30 seconds. That means that we need to immediately open the new tab once we initiate SEP-24.

The SEP-10 auth token is valid for 24 hours and I am pretty sure that it will not be invalidated after one use (?) I think we don't need to worry about that one.

Why do we have any need to do the SEP-24 initialization early? Let's just do it once we are ready to open the anchor UI.

Let's also be civilized and don't constantly create a new token. I am pretty sure that the anchor needs to add a new entry in their database for every initialization request we do. So let's not spam that one.

gianfra-t commented 4 months ago

it will not be invalidated after one use

At least with mykobo, reloading the page throws an error regarding one-time token use.

Why do we have any need to do the SEP-24 initialization early

We are not doing it early in that sense, it's only done right before the user is presented with the button to open the external window. Now, if once the window is open the token won't expire, then it may be better to either:

TorstenStueber commented 4 months ago

At least with mykobo, reloading the page throws an error regarding one-time token use.

Oh, indeed. I just checked myself. I think that this cannot be intended and should be changed on Mykobo at some point.

it's only done right before the user is presented with the button to open the external window.

Ah, right. Either one of the solutions you suggest would generally be correct. However, there might be a permission problem of the browser: to keep the user safe, JS code is not allowed to just open a new tab. To my knowledge this can only be done inside the handler of a click event and in that handler this would need to happen synchronously (that means we cannot first await some I/O event).

Let's test that. If my assumption is right, then we need to implement a workaround for this (not really required for the prototype). The only one that comes to my mind is a quite a heavy hack: open a tab that is linking to a custom GET endpoint of our backend. This url would take the SEP-10 token as a query parameter. The GET request would then execute SEP-24 on the backend and return a 303 response with a redirect to the anchor url.

If we don't find a simpler workaround, let's just ignore this issue for the prototype.

gianfra-t commented 4 months ago

I will test initiating the Sep24 upon click, but I also think that awaiting the process will not be possible before opening the window. Maybe .then could work.

TorstenStueber commented 4 months ago

I don't think that you can outsmart the browser here. Happy testing!

gianfra-t commented 4 months ago

Seems like it's possible! There is very little delay.

ebma commented 4 months ago

I think all flagged issues are solved for now then? Let's fix the formatting and get rid of commented-out code blocks so we can finalize the review.

@pendulum-chain/product please have a brief look at the deploy preview and let us know if there is anything wrong that you'd like us to change right away. Otherwise we can merge this to staging and start handing it out for testing.