Allow for swap possibility before offramping. #33
Conversation
✅ Deploy Preview for pendulum-pay ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
gianfra-t
changed the title
|
@pendulum-chain/devs the token config was modified to be consistent with a nabla instance on Foucoco. This allows to test a swap between |
|
@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?) |
|
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
force-pushed
the
add-swap-option
branch
from
0f84f34 to
81a22b0
Compare
There was a problem hiding this comment.
Some remarks:
- As a user, I feel overwhelmed by having both buttons shown at once. Let's only show the second button after I clicked on the first one.
When I click on the button to enter the bank details, my session already expired. We should probably delay the initialization of the session with the anchor so that this doesn't happen. Maybe only do it once I click on the button and before opening the new tab.
I can still edit the input fields of the swap form, even after starting the offramp. This doesn't make sense and we should try to disable these fields.
A low-hanging fruit for improving UX would be to move the button to 'Swap to other asset before offramp' above the first input field and below the 'Offramp asset' heading. We could also use tab components.
There was a problem hiding this comment.
The code structure is not great but that's too be expected in this prototype and good enough I think. I would appreciate it if we could move some of the components out of the 'InputKeys' directory as it's not obvious why all swap-related components are in that directory. Maybe just move the respective ones into an extra directory 'Nabla'.
src/services/nabla.ts
Outdated
| // https://github.com/pendulum-chain/portal/blob/c164e5b083e751e4c748edecc8560746e80a5be0/src/hooks/nabla/useErc20TokenApproval.ts#L70 | ||
| // TODO is it really necessary, how much is "safe"? | ||
| // Alternatively we can call again until reflected. | ||
| await new Promise((resolve) => setTimeout(resolve, 5000)); |
There was a problem hiding this comment.
I'm not 100% sure but I think in the portal this was a limitation for correct rendering in the UI components. But since we do all in one go, I assume we can get away without waiting at all. Have you tried if it also works without waiting @gianfra-t?
There was a problem hiding this comment.
I haven't, I could test once more in Foucoco using no delay. In theory it should since we are expecting for confirmation in the previous await,
There was a problem hiding this comment.
It seems to work fine without the extra delay.
There was a problem hiding this comment.
It should work fine as you don't have a need to read and show the current allowance. If you did that, then you might still request the old allowance. But since the next thing you do is to submit another transaction, then this would already be up to date as it happens on chain.
@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. |
|
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. |
|
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. |
src/components/InputKeys/index.tsx
Outdated
| {isSubmitted && ( | ||
| <div className="offramp-started"> | ||
| Offramp started for asset - {selectedAsset?.toUpperCase()} | ||
| <input type="radio" name="my_tabs_2" id="swap" onClick={() => setActiveTab("swap")} role="tab" class="tab" aria-label="USDT offramp" /> |
There was a problem hiding this comment.
'Direct offramp' vs 'USDT offramp' doesn't seem like the best naming to me. Maybe we call the 'USDT offramp' something like 'Routed offramp' instead to indicate that something happens in between? Also not that great of a name.
There was a problem hiding this comment.
True, the person may immediately think it involves USDT offramping. What about "pay with USDT"? Routed offramp makes sense but seems a bit technical.
src/components/InputKeys/index.tsx
Outdated
| </div> | ||
| <div role="tablist" class="tabs tabs-lifted"> | ||
| <FormProvider {...form}> | ||
| <input type="radio" name="my_tabs_2" id="direct" onClick={() => setActiveTab("direct")} role="tab" class="tab" aria-label="Direct Offramp" /> |
There was a problem hiding this comment.
Even though you specify a default value for activeTab, it doesn't work for me when I open the app.
I assume it's because you are using these radio type inputs though it could also be because of the wantsSwap prop. Either way, I would prefer if we either simplify this by using the daisy-ui tab components, usage see here, or the tailwind classes, see here.
The components should be fairly easy to use with
<Tabs variant="lifted" size="md">
<Tab active={activeTab === "swap"}>USDT Offramp</Tab>
<Tab active={activeTab === "direct"}>Direct Offramp</Tab>
</Tabs>
or we could even keep using the radio-type input by using this.
There was a problem hiding this comment.
I pushed a change for the tabs here. It is simplified and I think it works. I only see a small "flicker" when changing tabs that I don't like much.
Also set the default to be swap.
There was a problem hiding this comment.
Nice 👍 we don't need the checks for wantsSwap in l. 283 and l. 322 though, right? As it's related to the active tab and the content is now only rendered when active. Maybe this reduces the flicker.
Generally, I think we don't need to put wantsSwap into an extra state as we can simply derive it from the active tab with const wantsSwap = activeTab === 'swap'
There was a problem hiding this comment.
Regarding the first part, we do. At least with the last tab "implementation" we did. I faced the issue that the form was not being properly handled because both From components were being rendered. Only that one was hidden.
But I could try your suggestion to reduce the extra state, although I don't think it will reduce the flicker since this was not present in the previous tab but was also doing the checks.
There was a problem hiding this comment.
Can you try if we still need it with the RadioTabs from daisyUI? Judging from the code in their examples I thought it would work automatically.
There was a problem hiding this comment.
Yup, seems to work the same. It must be different to the previous tabs we used.
There was a problem hiding this comment.
I will have to rollback on this one because the second From component does indeed interfere. It gets buggy when trying to fetch swap quote.
Maybe we should add some timer to the |
|
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. |
|
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. |
At least with mykobo, reloading the page throws an error regarding one-time token use.
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:
|
Oh, indeed. I just checked myself. I think that this cannot be intended and should be changed on Mykobo at some point.
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. |
|
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 |
|
I don't think that you can outsmart the browser here. Happy testing! |
|
Seems like it's possible! There is very little delay. |
|
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. |
* initial wip * intermediate step * intermediate 2 * finishing UI, testing contract calls for swap * . * replace big Number library * clean App.css, fix balance checks * remove non offrampable assets from token selector in to form * further cleaning .css * re-add amount checks, comments * lint errors * mainnet config for nabla instance * modify local storage key to avoid interference with portal app * show swap quote result even if balance is not enough * copy token icons to dist * fix input too small/large bug * move nabla related component * remove allowance delay * disable input form after offramp has started * WIP sep token expiration * useEffect to set offrampable asset, types fixes, formatting * fix effective exchange rate calculation * fix icons not showing in production * tabs to switch offramp modes * improve tab selection * set BRL limit to 1 * modification of tabs component * remove useState for wants swap variable * format, remove render condition inside radio tab * remove commented code * send anchor the amount to offramp * perform first step sep24 upon click * formatting * remove top level icons * set stellar url for signer service to mainnet
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)
From.tsx,To.tsx,AmountSelector.tsx, etc are added to makes this possible.TOKEN_CONFIGto handle nabla related data.BigNumberlibrary back tobig.js. This library allows handle decimal numbers as well, which makes the integration with existingportalcomponents much easier.Testing
Swap was tested using a Nabla instance in Foucoco, with the corresponding temporary modifications to
TOKEN_CONFIG.