[DO NOT MERGE - Iceboxed] Add Iframe and Summary card for the KYC #127

gianfra-t · 2024-09-02T13:23:59Z

Available to test UI as offramp is mocked.

Issue #102. Any amount will work and launch the iframe page, which will complete after some seconds mocking the user finishing the process.

Changes

Removes the redirect action for "Start Offramping" button that used to open the KYC site on a different page.
Remove "Confirm" button and replace with only one click on the "Start Offramping" button to start the process.
The KYC is now embedded in an iframe inside the application, which closes and continues to the progress page once the sep24process is completed (response from anchor).

netlify · 2024-09-02T13:24:15Z

✅ Deploy Preview for pendulum-pay ready!

Name	Link
🔨 Latest commit	`f78ae4c`
🔍 Latest deploy log	https://app.netlify.com/sites/pendulum-pay/deploys/66f1ac6b78ae0f0008b53c44
😎 Deploy Preview	https://deploy-preview-127--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.

src/hooks/useMainProcess.ts

gianfra-t · 2024-09-02T15:19:11Z

src/pages/progress/index.tsx

@@ -49,6 +52,7 @@ export const ProgressPage: FC<ProgressPageProps> = ({ setOfframpingPhase, offram

  const main = (
    <main>
+      <SigningBox step={signingPhase} />


Signing now happens in the process page, as it would otherwise not be visible given the iframe screen has already popped up.

gianfra-t · 2024-09-09T19:47:46Z

@pendulum-chain/devs and @pendulum-chain/product I think this is ready for review and final adjustments. Once the UI is fine I'll remove the commented part used for mocking.

ebma · 2024-09-10T10:09:53Z

I didn't look at the code yet but during testing I ran into the 403 error quite quickly.

I entered my credentials in the form of the iframe (name, email, IBAN) and I did that even within a single minute I think. But after clicking the 'Submit' button I encountered the 403.

It doesn't matter how fast or slow I do it, it never works and I always encounter 'Error: 403: Session is not authenticated'.

gianfra-t · 2024-09-10T11:42:18Z

EDIT: Nevermind, I understand what you mean. I didn't test it since I cannot make an offramp anyway, but I can reach the error you also see.

I'm a bit confused @ebma since I can see it always working
But remember that this is mocked so perhaps the wallet is not connected and it is letting you go through anyway, not being able to sign the sep10 challenge?

.

src/components/Iframe/index.tsx

Sharqiewicz · 2024-09-20T12:45:30Z

src/components/Iframe/index.tsx

+}) => {
+  const [cachedSrc, setCachedSrc] = useState<string | null>(null);
+
+  useEffect(() => {


I think we can remove the whole cachedSrc logic. We are not using it anywhere

True, the idea was to save the kyc url since it was being reset, but since the iframe loads only once, we can remove this.

Separated. Also added reusable inner cards.

Sharqiewicz · 2024-09-20T13:07:28Z

src/components/Iframe/index.tsx

+      setCachedSrc(src);
+    }
+  }, [src, cachedSrc]);
+  let main = (


Sharqiewicz · 2024-09-20T13:09:43Z

src/components/Iframe/index.tsx

+    <div className="flex justify-center items-center relative w-full h-[50vh] md:w-[535px] ">
+      <iframe
+        src={src}
+        style={{ border: 'none' }}


can't we just use border-0 class?

Sharqiewicz · 2024-09-20T13:10:15Z

src/components/Iframe/index.tsx

@@ -0,0 +1,51 @@
+import React, { useState, useEffect }  from 'react';


we use preact not react, please import preact/compat

Sharqiewicz · 2024-09-20T13:13:34Z

src/components/SummaryCard/index.tsx

+
+    return (
+
+      <div className="bg-white p-4 rounded-lg shadow-lg w-full">


It is quite big component, wdyt about breaking it down into smaller parts?

You're right. I wanted to get first some feedback on the alignment with the requirements but I agree it needs breaking down for readability.

Sharqiewicz · 2024-09-20T13:14:40Z

src/pages/swap/index.tsx

@@ -36,6 +37,7 @@ export const SwapPage = () => {
  const [isQuoteSubmitted, setIsQuoteSubmitted] = useState(false);
  const formRef = useRef<HTMLDivElement | null>(null);
  const [api, setApi] = useState<ApiPromise | null>(null);
+  const [isIframVisible, setIframVisible] = useState(false);


it is unused

gianfra-t · 2024-09-20T17:11:19Z

@pendulum-chain/devs now we need to wait for Mykobo to implement the cookies fix in production and we can test this with a full offramp.

UPDATE: this has been unblocked.

…n the iframe is already open

ebma

I added minor changes to improve the styling.
And I added code to prevent the content in the summary card to fluctuate. Because I noticed that the quote changed during the time I had the iframe open (over the course of a couple minutes). Let me know what you think.

Besides that, it looks good to me 👍 can confirm the iframe works, and it also recovers properly when the user enters a wrong amount on Mykobo's UI. In that case the toast error is thrown and when the user clicks on the 'Start offramping' button again, a fresh URL is shown in the iframe and the user can continue without the 403 error.

I think this is good to go once the mock changes are reverted.

Sharqiewicz · 2024-09-23T15:39:24Z

src/components/SummaryCard/UpperSummaryCard/index.tsx

+      <span className="text-md md:text-xl font-medium text-blue-800">{amount}</span>
+      <div className="flex items-center mr-2">
+        <img src={icon} className="w-4 md:w-6 h-4 md:h-6" />
+        <span className="ml-1 text-blue-800">{symbol}</span>


We use everywhere text-blue-700

to avoid such confusion I think we should make a class like

vortex-text-blue { @apply .text-blue-700 }

vadaynujra · 2024-09-23T15:48:18Z

I tested twice just now and got the same error, after filling up the first page with the anchor. It might not be related to the iframe implementation but something to do with the anchor or my profile with them?

gianfra-t · 2024-09-23T16:28:58Z

@ebma thanks for the improvements. I never thought about the live price change, that would have been very confusing. I will remove the commented guards.

@vadaynujra It looks like it is related. What browser are you using? ~~Do you remember if your wallet connected?~~ Since I haven't experienced this issue since Mykobo changed their back-end.

TorstenStueber

I tested and it worked well. Great job.

My main remark is about the size of the iframe. I personally don't like that I need to scroll a small box. I didn't test on mobile but can imagine that I would like it even less there.

Another important point is that the SigningBox it not shown anymore when the user signs the Polygon transactions. However, instead of moving the signing box to the IframeComponent, I think it now makes sense to already move to progress screen immediately when SEP 24 is completed and move the SigningBox to the progress screen as well (otherwise I find it slightly confusing UX that after finishing SEP24 I am still shown the iframe for quite some time).

TorstenStueber · 2024-09-23T19:02:11Z

src/services/offrampingFlow.ts

@@ -125,7 +125,7 @@ export interface ExecutionContext {
  setSigningPhase: (n: SigningPhase) => void;
 }

-const OFFRAMPING_STATE_LOCAL_STORAGE_KEY = 'offrampingState';
+export const OFFRAMPING_STATE_LOCAL_STORAGE_KEY = 'offrampingState';


Why is this exported now? It is not used outside this file.

TorstenStueber · 2024-09-23T19:04:11Z

src/services/anchor/index.ts

-  if (config.test.mockSep24) {
-    return { url: 'https://www.example.com', id: '1234' };
-  }
+  // if (config.test.mockSep24) {


Please remove, not comment out.

TorstenStueber · 2024-09-23T19:04:22Z

src/services/anchor/index.ts

@@ -227,11 +227,13 @@ export async function sep24Second(
  const { sep24Url } = tomlValues;

  if (config.test.mockSep24) {
+    // sleep 15 seconds, mock user completion of KYC
+    await new Promise((resolve) => setTimeout(resolve, 150000));


This is actually 150 seconds, not 15 seconds.

TorstenStueber · 2024-09-23T19:05:14Z

src/services/anchor/index.ts

    return {
      amount: sessionParams.offrampAmount,
      memo: 'MYK1722323689',
      memoType: 'text',
-      offrampingAccount: (await fetchSigningServiceAccountId()).stellar.public,
+      offrampingAccount: 'GSAPDFASJFPASFOKNASOFKNAS',


Why this change? The previous setting is better in my opinion as it sends the funds back to the funding account I use locally.

TorstenStueber · 2024-09-23T19:16:03Z

src/pages/swap/index.tsx

-            pending={isInitiating || offrampingStarted || offrampingState !== undefined}
-          />
-        )}
+        <SwapSubmitButton


There is a breaking change here. The button text has been changed in the mean time, can you please change accordingly?

(also the logic for the pending state)

TorstenStueber · 2024-09-23T19:22:58Z

src/pages/swap/index.tsx

@@ -25,6 +25,7 @@ import { SuccessPage } from '../success';
 import { FailurePage } from '../failure';
 import { useInputTokenBalance } from '../../hooks/useInputTokenBalance';
 import { UserBalance } from '../../components/UserBalance';
+import { IframeComponent, IframeProps } from '../../components/Iframe';


The IframeProps import not used.

TorstenStueber · 2024-09-23T19:32:27Z

src/components/Iframe/index.tsx

+        <h2 className="text-md mb-4 mt-4">{subtitle}</h2>  
+      </div>
+    </div>
+    <div className="flex justify-center items-center relative w-full h-[50vh] md:w-[535px] ">


I remember that we discussed once that we want to reduce double scroll and the that the design solution was to prevent scrolling of the outer screen by making the iframe always fit into the screen (i.e., scrolling only happens within the iframe).

We then discussed the other option: make the iframe large enough that it will not scroll and only the outer screen is scrolling.

I actually remember that we decided to use that latter option, am I wrong (I don't find notes about this).

Same for the width of the iframe: it feels right now like looking through a peephole and I don't think that this is such a good UX.

TorstenStueber · 2024-09-23T19:36:04Z

src/components/SummaryCard/index.tsx

+
+  const receiveAmount = calculateTotalReceive(toAmount.toString(), OUTPUT_TOKEN_CONFIG[assetOut]);
+
+  const approximateExchangeRate = roundDownToSignificantDecimals(toAmount.div(fromAmount), 2).toString();


Wouldn't it be better to also use 4 decimals as on the details box on the main page? E.g., reuse the ExchangeRate component.

TorstenStueber · 2024-09-23T19:38:07Z

src/components/SummaryCard/index.tsx

+
+  const receiveAmount = calculateTotalReceive(toAmount.toString(), OUTPUT_TOKEN_CONFIG[assetOut]);
+
+  const approximateExchangeRate = roundDownToSignificantDecimals(toAmount.div(fromAmount), 2).toString();


Please use toFixed(2, 0) instead of toString() for every number stringification of amounts in this component. (see #171)

TorstenStueber · 2024-09-23T19:41:13Z

src/components/SummaryCard/index.tsx

+          <LowerSummaryCard
+            label="Offramp fees"
+            amount={offrampFees}
+            startIcon={<LocalGasStationIcon className="text-blue-700" fontSize="small" />}


I argue that we should remove the gas station icon: #102 (comment)

vadaynujra · 2024-09-24T11:29:47Z

@gianfra-t I used Google Chrome

vadaynujra · 2024-09-24T11:43:45Z

Tried again and got the same error. Attached the console logs. I also tried the flow using production to ensure there wasn't any issue with my details and didn't get the 403 error there.

gianfra-t · 2024-09-24T12:43:45Z

@TorstenStueber I will address your comments but regarding this one:

instead of moving the signing box to the IframeComponent, I think it now makes sense to already move to progress screen immediately when SEP 24 is completed and move the SigningBox to the progress screen as well

I agree and in fact the solution is quite quick, we just need to delete this condition, the includes one.

That said, what was the rationale behind signing on the main screen (even before the iframe)? Because if it is such that the user sees the values when it is signing, then we should stay here (and potentially remove the Iframe and show a message).

TorstenStueber · 2024-09-24T15:01:45Z

Yes, good question. I don't know what the reasoning was. @prayagd @vadaynujra can you give the reasoning?

(I guess it was because of the numbers, as you said. Would it not make sense to show your SummaryCard then also on the progress screen? It is nice to see how much is currently offramping).

TorstenStueber · 2024-09-26T02:25:42Z

What shall we do with this PR? Close it?

gianfra-t · 2024-09-26T14:53:41Z

What shall we do with this PR? Close it?

I changed the title but we can also close it, we can always reopen it.

TorstenStueber · 2024-09-26T15:29:09Z

I would also suggest to just close it instead of keeping it open with the warning in the title.

ebma · 2024-09-26T16:17:41Z

@gianfra-t before closing it, can you leave a comment with a brief summary that explains why we cannot proceed with this change? Namely pointing out the restrictions with the browser settings that interferes with the cookies in the iframe etc.

TorstenStueber · 2024-11-04T09:29:11Z

@gianfra-t let's close this PR? We can still reopen later if we want to go back to the iframe.

gianfra-t · 2024-11-04T12:30:04Z

Closing this. We could later use the UI perhaps.

Just to recap, the reason this didn't work is because the anchor (Mykobo) is using cookies for CSRF protection, and these are being blocked when using the iframe solution (the URL of the iframe is different to Vortex). Specifically, this happens in browsers with stricter policies, which would make the solution not viable for many users.

gianfra-t added 2 commits September 2, 2024 09:41

adding iframe flow

d345c90

move signing modal to progress screem, mock responses to test UI

93f3bf6

gianfra-t added 2 commits September 2, 2024 10:48

show failure screen on initial process

a2f475b

make mobile iframe larger

6172629

gianfra-t commented Sep 2, 2024

View reviewed changes

src/hooks/useMainProcess.ts Outdated Show resolved Hide resolved

gianfra-t commented Sep 2, 2024

View reviewed changes

gianfra-t added 3 commits September 2, 2024 13:07

adjust size for large screen

cddc8ca

delay mock 15 seconds only

171066d

remove second confirmation button

9d5fc4e

gianfra-t changed the title ~~Add Iframe for the KYC~~ [DO NOT MERGE - TESTING UI]. Add Iframe for the KYC Sep 3, 2024

gianfra-t requested a review from Sharqiewicz September 3, 2024 08:25

don't store mocked data in sheet

dc6f6d0

gianfra-t changed the title ~~[DO NOT MERGE - TESTING UI]. Add Iframe for the KYC~~ [DO NOT MERGE - MISSING CARD DESIGN]. Add Iframe for the KYC Sep 5, 2024

gianfra-t added 4 commits September 5, 2024 08:44

first card iteration

bda9638

adjusting summary card

f253afc

refinements to card design, element props

52d2749

fix iframe height to 50%

bc019e6

gianfra-t changed the title ~~[DO NOT MERGE - MISSING CARD DESIGN]. Add Iframe for the KYC~~ Add Iframe and Summary card for the KYC Sep 9, 2024

gianfra-t added 2 commits September 9, 2024 16:37

fix missing eur icon

1ef6395

fix asset text for large screens

24afd1e

ebma linked an issue Sep 10, 2024 that may be closed by this pull request

Implement the SEP24 flow in an iframe #102

Open

Sharqiewicz requested changes Sep 20, 2024

View reviewed changes

gianfra-t added 4 commits September 20, 2024 11:12

adressing review improvements

0080740

breaking summary card into inner cards

942a397

solving merge conflicts

3d52961

readd iframe to swap page

9647226

gianfra-t added 2 commits September 20, 2024 12:19

add remarked section to summary card:

e7635cd

fixing lower summary card issue

4ced67a

ebma added 3 commits September 23, 2024 16:39

Adjust styling of summary cards

88e5887

Add gas icon

fc92db8

Commit to form values to avoid fluctuations of the exchange rates whe…

8b08240

…n the iframe is already open

ebma approved these changes Sep 23, 2024

View reviewed changes

Sharqiewicz reviewed Sep 23, 2024

View reviewed changes

gianfra-t added 2 commits September 23, 2024 13:32

remove mocked values

37304c5

remove mock sep 24 second response

f78ae4c

TorstenStueber reviewed Sep 23, 2024

View reviewed changes

ebma mentioned this pull request Sep 25, 2024

Remove the gas icon from fee section #175

Closed

gianfra-t changed the title ~~Add Iframe and Summary card for the KYC~~ [DO NOT MERGE - Iceboxed] Add Iframe and Summary card for the KYC Sep 26, 2024

gianfra-t closed this Nov 4, 2024

		@@ -0,0 +1,51 @@
		import React, { useState, useEffect } from 'react';


		return (

		<div className="bg-white p-4 rounded-lg shadow-lg w-full">


		const receiveAmount = calculateTotalReceive(toAmount.toString(), OUTPUT_TOKEN_CONFIG[assetOut]);

		const approximateExchangeRate = roundDownToSignificantDecimals(toAmount.div(fromAmount), 2).toString();

[DO NOT MERGE - Iceboxed] Add Iframe and Summary card for the KYC #127

[DO NOT MERGE - Iceboxed] Add Iframe and Summary card for the KYC #127

Conversation

gianfra-t commented Sep 2, 2024 • edited Loading

Changes

netlify bot commented Sep 2, 2024 • edited Loading

✅ Deploy Preview for pendulum-pay ready!

Choose a reason for hiding this comment

gianfra-t commented Sep 9, 2024

ebma commented Sep 10, 2024 • edited Loading

gianfra-t commented Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gianfra-t Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gianfra-t commented Sep 20, 2024 • edited Loading

ebma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadaynujra commented Sep 23, 2024 • edited Loading

gianfra-t commented Sep 23, 2024 • edited Loading

TorstenStueber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadaynujra commented Sep 24, 2024

vadaynujra commented Sep 24, 2024

gianfra-t commented Sep 24, 2024 • edited Loading

TorstenStueber commented Sep 24, 2024

TorstenStueber commented Sep 26, 2024

gianfra-t commented Sep 26, 2024 • edited Loading

TorstenStueber commented Sep 26, 2024

ebma commented Sep 26, 2024

TorstenStueber commented Nov 4, 2024

gianfra-t commented Nov 4, 2024

gianfra-t commented Sep 2, 2024 •

edited

Loading

netlify bot commented Sep 2, 2024 •

edited

Loading

ebma commented Sep 10, 2024 •

edited

Loading

gianfra-t commented Sep 10, 2024 •

edited

Loading

gianfra-t Sep 20, 2024 •

edited

Loading

gianfra-t commented Sep 20, 2024 •

edited

Loading

vadaynujra commented Sep 23, 2024 •

edited

Loading

gianfra-t commented Sep 23, 2024 •

edited

Loading

gianfra-t commented Sep 24, 2024 •

edited

Loading

gianfra-t commented Sep 26, 2024 •

edited

Loading