pendulum-chain / vortex

1 stars 0 forks source link

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

Open gianfra-t opened 3 weeks ago

gianfra-t commented 3 weeks ago

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

netlify[bot] commented 3 weeks ago

Deploy Preview for pendulum-pay ready!

Name Link
Latest commit f78ae4cb3b67e52ae8d3dc27ecd290a7c108b230
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...

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

@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 commented 2 weeks ago

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

image

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 commented 2 weeks ago

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?

image.

gianfra-t commented 1 week ago

@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.

vadaynujra commented 4 days ago

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? image

gianfra-t commented 4 days ago

@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.

vadaynujra commented 4 days ago

@gianfra-t I used Google Chrome

vadaynujra commented 4 days ago

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. image

gianfra-t commented 4 days ago

@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 commented 3 days ago

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 commented 2 days ago

What shall we do with this PR? Close it?

gianfra-t commented 1 day ago

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 commented 1 day ago

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

ebma commented 1 day ago

@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.