Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Onramp Kit and Stripe Integration to allow users to fund their wallet #8

Merged
merged 8 commits into from
Mar 30, 2023

Conversation

ademidun
Copy link
Contributor

@ademidun ademidun commented Mar 29, 2023

Add an option in Wallet Management (previously Transaction Management) that allows users to fund their wallet using fiat payment rails via the Safe {Core} Onramp Kit and Stripe.

Related: safe-global/safe-docs#192

stripe-onramp-kit-demo.mp4

@ademidun ademidun changed the title Add a component to fund wallet: Onramp Kit and Stripe Integration Add Onramp Kit and Stripe Integration to allow users to fund their wallet Mar 29, 2023
@ademidun ademidun marked this pull request as ready for review March 29, 2023 03:50
@ademidun ademidun requested review from dasanra and yagopv March 29, 2023 03:50
@yagopv
Copy link
Member

yagopv commented Mar 29, 2023

Hey!!, I left some comments and suggestions.

As a general comment, I'm not sure if adding the example in the context of a bigger app is better than have it isolated as you can find in the example folder of the onramp-kit. For me the examples should be pretty straightforward and here you have to setup several things (env vars) for reaching the stripe example.

Another things is about code styling as I can see different indentations or blank lines (>1) between instructions sometimes.

As a last comment, we use a lot material UI with custom theming. Is very easy to use and make all our UI demos look and feel the same. Would be great to use it here as well as a future enhancement

src/scenes/Wallet/WalletCreate.tsx Show resolved Hide resolved
src/scenes/Wallet/WalletFund.tsx Outdated Show resolved Hide resolved
src/scenes/Wallet/WalletManage.tsx Outdated Show resolved Hide resolved
src/scenes/Wallet/WalletManage.tsx Outdated Show resolved Hide resolved
src/scenes/Wallet/WalletFund.tsx Outdated Show resolved Hide resolved
@yagopv
Copy link
Member

yagopv commented Mar 29, 2023

The video in the readme is too short? it does not finish the token purchase

@yagopv
Copy link
Member

yagopv commented Mar 29, 2023

⚠️ @ademidun after the hackaton we are going to merge some docs regarding some API change so the way these examples work should be updated

@ademidun
Copy link
Contributor Author

ademidun commented Mar 30, 2023

The video in the readme is too short? it does not finish the token purchase

The goal of the video is not to show the full product flow due to file size. That can already be seen by the screenshots in Onramp Kit.

⚠️ @ademidun after the hackaton we are going to safe-global/safe-docs#190 regarding some API change so the way these examples work should be updated

Thanks for the heads up, we can update when necessary.

@ademidun
Copy link
Contributor Author

Hey!!, I left some comments and suggestions.
As a general comment, I'm not sure if adding the example in the context of a bigger app is better than have it isolated as you can find in the example folder of the onramp-kit. For me the examples should be pretty straightforward and here you have to setup several things (env vars) for reaching the stripe example.

The relevant examples are straightforward as they can be isolated to the specific component as seen here where the tutorial shows how to use it in a specific component. Having it in the context of the larger app allows developers to see how it's all tied together.

Another thing is about code styling as I can see different indentations or blank lines (>1) between instructions sometimes.

Fixed with prettier in 1c583b1

As a last comment, we use a lot material UI with custom theming. Is very easy to use and make all our UI demos look and feel the same. Would be great to use it here as well as a future enhancement.

This is really cool! Thanks for sharing! Something to keep in mind for a future PR.

@ademidun ademidun merged commit 74dd7b5 into main Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants