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 MoonPay onramp option #4027

Conversation

EnricoBarbieri1997
Copy link
Collaborator

What is the purpose of the change:

Add MoonPay as an option to buy assets with FIAT

Linear Task

https://linear.app/osmosis/issue/FE-1270/review-moonpay-proposal-and-consider-implementation

Brief Changelog

  • Add MoonPay brandin
  • Add MoonPay Localization

Testing and Verifying

This change has not been tested locally, because some functionalities are available on production environment only

Copy link

vercel bot commented Jan 6, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
osmosis-frontend ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 10, 2025 0:13am
4 Skipped Deployments
Name Status Preview Comments Updated (UTC)
osmosis-frontend-datadog ⬜️ Ignored (Inspect) Visit Preview Jan 10, 2025 0:13am
osmosis-frontend-dev ⬜️ Ignored (Inspect) Visit Preview Jan 10, 2025 0:13am
osmosis-frontend-edgenet ⬜️ Ignored (Inspect) Jan 10, 2025 0:13am
osmosis-testnet ⬜️ Ignored (Inspect) Visit Preview Jan 10, 2025 0:13am

Copy link
Contributor

coderabbitai bot commented Jan 6, 2025

Warning

There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure.

🔧 eslint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

packages/web/integrations/moonpay/index.tsx

Oops! Something went wrong! :(

ESLint: 8.50.0

ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct.

The config "next/core-web-vitals" was referenced from the config file in "/packages/web/.eslintrc.json".

If you still have problems, please stop by https://eslint.org/chat/help to chat with the team.

Walkthrough

This pull request introduces integration for MoonPay, a fiat on-ramp service, into the web application. The changes encompass adding environment variable placeholders for MoonPay keys, creating a new React component for the MoonPay widget, updating fiat ramp configurations, and implementing a server-side API endpoint for URL signature generation. The implementation spans multiple files, covering frontend integration, API handling, and configuration setup.

Changes

File Change Summary
packages/web/.env Added commented placeholders for MoonPay private and public keys
packages/web/integrations/fiat-ramps.ts Added "moonpay" to FiatRampKey and FiatRampDisplayInfos
packages/web/integrations/moonpay/index.tsx New Moonpay React component with URL signature generation function
packages/web/integrations/moonpay/types.ts Added MoonpaySignUrlResponse interface
packages/web/modals/fiat-on-ramp-selection.tsx Added MoonPay option to fiat ramp selection
packages/web/modals/fiat-ramps.tsx Integrated Moonpay component into fiat ramps modal
packages/web/pages/_app.tsx Added MoonPayProvider with dynamic import
packages/web/pages/api/integrations/moonpay/sign-url.ts New API endpoint for MoonPay URL signature generation
packages/web/pages/components.tsx Added "moonpay-logo" icon
packages/web/hooks/bridge.tsx Added onRequestBack function to handle back navigation in the modal
packages/web/modals/base.tsx Introduced hideDefaultBackButton property to control visibility of the back button

Sequence Diagram

sequenceDiagram
    participant User
    participant FiatRampsModal
    participant Moonpay
    participant API
    participant MoonPayWidget

    User->>FiatRampsModal: Select MoonPay
    FiatRampsModal->>Moonpay: Render Moonpay component
    Moonpay->>API: Request URL signature
    API-->>Moonpay: Return signed URL
    Moonpay->>MoonPayWidget: Initialize with signed URL
    MoonPayWidget->>User: Display purchase interface
Loading

Possibly related PRs

Suggested reviewers

  • JoseRFelix
  • DavideSegullo

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4be0212 and ee2efe1.

📒 Files selected for processing (1)
  • packages/web/integrations/moonpay/index.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/web/integrations/moonpay/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
  • GitHub Check: Analyze (javascript)
  • GitHub Check: test (20.x, web)
  • GitHub Check: wait-for-deployment
  • GitHub Check: Summary

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🧹 Nitpick comments (3)
packages/web/pages/api/integrations/moonpay/sign-url.ts (1)

18-18: Consider caching MoonPay instance.

Creating a new MoonPay instance on every request is inefficient.

Consider initializing the MoonPay instance once and reusing it:

// At the top of the file
const moonPay = process.env.MOONPAY_PRIVATE_KEY 
  ? new MoonPay(process.env.MOONPAY_PRIVATE_KEY)
  : null;

// In the handler
if (!moonPay) {
  return res.status(500).send({ error: "Moonpay private key not set" });
}
packages/web/integrations/moonpay/index.tsx (1)

29-31: Add prop types validation.

Consider adding runtime prop types validation.

import PropTypes from 'prop-types';

Moonpay.propTypes = {
  assetKey: PropTypes.string.isRequired,
  isOpen: PropTypes.bool.isRequired,
  onRequestClose: PropTypes.func.isRequired,
};
packages/web/.env (1)

57-59: Enhance environment variable documentation and security.

  1. Add comments explaining the purpose and format of each key
  2. Document the required steps to obtain these keys
  3. Consider adding validation patterns

Add documentation:

 # Moonpay public and private keys
-# MOONPAY_PRIVATE_KEY=
-# NEXT_PUBLIC_MOONPAY_PUBLIC_KEY=
+# MOONPAY_PRIVATE_KEY= # Required for URL signing. Never commit the actual key!
+# NEXT_PUBLIC_MOONPAY_PUBLIC_KEY= # Required for widget initialization. Format: pk_live_* or pk_test_*
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7b03c95 and 3a97141.

⛔ Files ignored due to path filters (21)
  • packages/web/localizations/de.json is excluded by !**/*.json
  • packages/web/localizations/en.json is excluded by !**/*.json
  • packages/web/localizations/es.json is excluded by !**/*.json
  • packages/web/localizations/fa.json is excluded by !**/*.json
  • packages/web/localizations/fr.json is excluded by !**/*.json
  • packages/web/localizations/gu.json is excluded by !**/*.json
  • packages/web/localizations/hi.json is excluded by !**/*.json
  • packages/web/localizations/ja.json is excluded by !**/*.json
  • packages/web/localizations/ko.json is excluded by !**/*.json
  • packages/web/localizations/pl.json is excluded by !**/*.json
  • packages/web/localizations/pt-br.json is excluded by !**/*.json
  • packages/web/localizations/ro.json is excluded by !**/*.json
  • packages/web/localizations/ru.json is excluded by !**/*.json
  • packages/web/localizations/tr.json is excluded by !**/*.json
  • packages/web/localizations/zh-cn.json is excluded by !**/*.json
  • packages/web/localizations/zh-hk.json is excluded by !**/*.json
  • packages/web/localizations/zh-tw.json is excluded by !**/*.json
  • packages/web/package.json is excluded by !**/*.json
  • packages/web/public/icons/sprite.svg is excluded by !**/*.svg, !**/*.svg
  • packages/web/public/logos/moonpay.svg is excluded by !**/*.svg, !**/*.svg
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock, !**/*.lock
📒 Files selected for processing (9)
  • packages/web/.env (1 hunks)
  • packages/web/integrations/fiat-ramps.ts (2 hunks)
  • packages/web/integrations/moonpay/index.tsx (1 hunks)
  • packages/web/integrations/moonpay/types.ts (1 hunks)
  • packages/web/modals/fiat-on-ramp-selection.tsx (1 hunks)
  • packages/web/modals/fiat-ramps.tsx (2 hunks)
  • packages/web/pages/_app.tsx (2 hunks)
  • packages/web/pages/api/integrations/moonpay/sign-url.ts (1 hunks)
  • packages/web/pages/components.tsx (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • packages/web/pages/components.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: Analyze (javascript)
  • GitHub Check: Summary
  • GitHub Check: wait-for-deployment
🔇 Additional comments (5)
packages/web/integrations/moonpay/types.ts (1)

1-3: LGTM! Well-defined type with appropriate immutability.

The interface correctly defines the API response type with a readonly signature property.

packages/web/integrations/fiat-ramps.ts (1)

19-24: Verify MoonPay logo asset existence.

The configuration looks good, but we should verify the existence of the MoonPay logo asset.

✅ Verification successful

MoonPay logo asset verified successfully

The logo file exists at the correct location: packages/web/public/logos/moonpay.svg, matching the configured path /logos/moonpay.svg.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify MoonPay logo asset exists
fd -t f "moonpay.svg" public/logos/

Length of output: 136


Script:

#!/bin/bash
# First, let's find all SVG files to understand where logos are stored
fd -e svg

# Then specifically look for moonpay.svg anywhere in the repository
fd "moonpay.svg"

Length of output: 6816

packages/web/modals/fiat-ramps.tsx (1)

6-6: LGTM! Clean integration of MoonPay component.

The changes follow the established pattern of the modal's implementation, maintaining consistency with other fiat ramp integrations.

Also applies to: 46-47

packages/web/pages/_app.tsx (2)

75-78: LGTM! Correct SSR configuration.

The dynamic import with ssr: false is properly configured to avoid SSR-related issues with the MoonPay widget.


92-93: Security: Review debug mode configuration.

Debug mode is enabled in development, which could potentially expose sensitive information.

Consider:

  1. Adding a separate environment variable for debug mode
  2. Documenting what information is exposed in debug mode
  3. Ensuring debug logs are properly sanitized

Comment on lines +11 to +13
if (req.method !== "POST") {
return res.status(405).send({ error: "Method not allowed" });
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add rate limiting to prevent abuse.

The API endpoint should implement rate limiting to prevent potential abuse.

Consider using a rate limiting middleware like express-rate-limit or implementing a custom solution.

Comment on lines +34 to +37
return res
.status(500)
.send({ error: "An unexpected error occurred. Please try again." });
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Use generic error messages for security.

Avoid exposing internal error details in production.

-      .send({ error: "An unexpected error occurred. Please try again." });
+      .send({ error: "Request failed" });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return res
.status(500)
.send({ error: "An unexpected error occurred. Please try again." });
}
return res
.status(500)
.send({ error: "Request failed" });
}

Comment on lines +19 to +23
const { url: moonPayUrl } = await req.body;

if (!moonPayUrl) {
return res.status(400).send({ error: "Moonpay url not provided" });
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Validate MoonPay URL to prevent security issues.

The endpoint accepts any URL without validation. This could lead to security issues if malicious URLs are provided.

Add URL validation:

  const { url: moonPayUrl } = await req.body;

  if (!moonPayUrl) {
    return res.status(400).send({ error: "Moonpay url not provided" });
  }
+ if (!moonPayUrl.startsWith('https://buy.moonpay.com/')) {
+   return res.status(400).send({ error: "Invalid Moonpay URL" });
+ }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const { url: moonPayUrl } = await req.body;
if (!moonPayUrl) {
return res.status(400).send({ error: "Moonpay url not provided" });
}
const { url: moonPayUrl } = await req.body;
if (!moonPayUrl) {
return res.status(400).send({ error: "Moonpay url not provided" });
}
if (!moonPayUrl.startsWith('https://buy.moonpay.com/')) {
return res.status(400).send({ error: "Invalid Moonpay URL" });
}


const account = accountStore.getWallet(chainStore.osmosis.chainId);

let walletAddress = account?.address;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Initialize wallet address with empty string.

Using undefined for walletAddress prop could cause issues.

- let walletAddress = account?.address;
+ let walletAddress = account?.address ?? '';
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let walletAddress = account?.address;
let walletAddress = account?.address ?? '';

Comment on lines +15 to +27
async function generateMoonpayUrlSignature(url: string): Promise<string> {
return (
await apiClient<MoonpaySignUrlResponse>(
"/api/integrations/moonpay/sign-url",
{
method: "POST",
data: {
url,
},
}
)
).signature;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling to URL signature generation.

The function should handle API errors gracefully.

 async function generateMoonpayUrlSignature(url: string): Promise<string> {
+  try {
     return (
       await apiClient<MoonpaySignUrlResponse>(
         "/api/integrations/moonpay/sign-url",
         {
           method: "POST",
           data: {
             url,
           },
         }
       )
     ).signature;
+  } catch (error) {
+    console.error('Failed to generate MoonPay URL signature:', error);
+    throw new Error('Failed to generate MoonPay URL signature');
+  }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async function generateMoonpayUrlSignature(url: string): Promise<string> {
return (
await apiClient<MoonpaySignUrlResponse>(
"/api/integrations/moonpay/sign-url",
{
method: "POST",
data: {
url,
},
}
)
).signature;
}
async function generateMoonpayUrlSignature(url: string): Promise<string> {
try {
return (
await apiClient<MoonpaySignUrlResponse>(
"/api/integrations/moonpay/sign-url",
{
method: "POST",
data: {
url,
},
}
)
).signature;
} catch (error) {
console.error('Failed to generate MoonPay URL signature:', error);
throw new Error('Failed to generate MoonPay URL signature');
}
}

Comment on lines 19 to 23
{
...FiatRampDisplayInfos.moonpay,
initialAsset: "OSMO",
subtitle: t("components.fiatOnrampSelection.moonpay"),
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Translation key mismatch needs to be corrected

The translation key components.fiatOnrampSelection.moonpay doesn't match the actual key in the translation files. The correct key should be components.fiatOnrampSelection.moonpaySubtitle as evidenced by the presence of moonpaySubtitle in all language files.

Regarding the placement of MoonPay as the first option, this appears intentional as it supports both OSMO and USDC, similar to other primary options.

🔗 Analysis chain

Verify translation key and consider option placement.

While the implementation is correct, please ensure:

  1. The translation key "components.fiatOnrampSelection.moonpay" exists in all language files
  2. Consider if placing MoonPay as the first option aligns with the intended user experience
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the translation key exists in language files
rg -l "components.fiatOnrampSelection.moonpay" localizations/

Length of output: 116


Script:

#!/bin/bash
# Find translation-related files
fd -e json -e yaml -e yml . | grep -i "lang\|local\|i18n\|translat"

# Alternative search for the translation key in all files
rg "components\.fiatOnrampSelection\.moonpay" --type json --type yaml

Length of output: 777


Script:

#!/bin/bash
# Check English translation file first
cat packages/web/localizations/en.json | grep -A 2 -B 2 "moonpay"

# Check all translation files for the key
for file in packages/web/localizations/*.json; do
    echo "=== $file ==="
    cat "$file" | grep -A 1 "moonpay"
done

Length of output: 5346

Comment on lines +91 to +94
<MoonPayProvider
apiKey={process.env.NEXT_PUBLIC_MOONPAY_PUBLIC_KEY ?? ""}
debug={process.env.NODE_ENV === "development"}
>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling for missing API key.

The current implementation might cause runtime issues if the environment variable is not set.

Consider adding validation:

-            <MoonPayProvider
-              apiKey={process.env.NEXT_PUBLIC_MOONPAY_PUBLIC_KEY ?? ""}
-              debug={process.env.NODE_ENV === "development"}
-            >
+            {process.env.NEXT_PUBLIC_MOONPAY_PUBLIC_KEY ? (
+              <MoonPayProvider
+                apiKey={process.env.NEXT_PUBLIC_MOONPAY_PUBLIC_KEY}
+                debug={process.env.NODE_ENV === "development"}
+              >
+                {/* ... children ... */}
+              </MoonPayProvider>
+            ) : (
+              <ErrorBoundary fallback={<ErrorFallback />}>
+                {/* ... children ... */}
+              </ErrorBoundary>
+            )}

Committable suggestion skipped: line range outside the PR's diff.



# Moonpay public and private keys
# MOONPAY_PRIVATE_KEY=
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

Security: Critical - .env file is tracked in git

The security concern is valid and even more critical than initially noted:

  1. The packages/web/.env file is currently tracked in git
  2. No .env patterns in .gitignore to protect this file
  3. Contains sensitive configuration (MOONPAY_PRIVATE_KEY)

Required actions:

  • Add packages/web/.env to .gitignore
  • Remove the file from git tracking using git rm --cached packages/web/.env
  • Document the required environment variables in a .env.example file
  • Consider using a secret management service for production deployments
🔗 Analysis chain

Security: Protect private key.

The private key should be handled with extra care:

  1. Add it to .gitignore
  2. Document the secure way to provide it in production
  3. Consider using a secret management service
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify .gitignore contains the private key pattern
grep "MOONPAY_PRIVATE_KEY" .gitignore || echo "Warning: MOONPAY_PRIVATE_KEY not found in .gitignore"

Length of output: 152


Script:

#!/bin/bash
# Check if .env is properly gitignored
grep -E "\.env|\.env\." .gitignore || echo "Warning: .env patterns not found in .gitignore"

# Look for documentation about environment setup
fd -e md -x grep -l -i "environment|env|secret|moonpay" {} \;

# Check for potential secret management patterns
rg -i "secret|vault|keymanagement|process\.env" -g "!*.{lock,md}" -g "!{dist,build}/*"

Length of output: 104298


Script:

#!/bin/bash
# Check for .env patterns in .gitignore that would cover packages/web/.env
grep -E "^\.env|^packages/web/\.env|^\*\.env" .gitignore || echo "Warning: No matching .env patterns found"

# Look for environment setup documentation
fd -e md -x grep -l -i "environment|env setup|configuration" {} \;

# Check if .env file is tracked
git ls-files packages/web/.env || echo "packages/web/.env is not tracked"

Length of output: 272

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
packages/web/modals/base.tsx (1)

70-85: Remove unnecessary Fragment.

The Fragment wrapper is redundant as it contains only conditional rendering of a single child component.

-          <>
-            {!hideDefaultBackButton && (
-              <IconButton
-                aria-label="Back"
-                className="top-9.5 absolute left-8 z-50 cursor-pointer py-0 text-osmoverse-400 md:left-7 md:top-7"
-                icon={
-                  <Icon
-                    id={backIcon ?? "chevron-left"}
-                    width={18}
-                    height={18}
-                  />
-                }
-                onClick={onRequestBack}
-              />
-            )}
-          </>
+          !hideDefaultBackButton && (
+            <IconButton
+              aria-label="Back"
+              className="top-9.5 absolute left-8 z-50 cursor-pointer py-0 text-osmoverse-400 md:left-7 md:top-7"
+              icon={
+                <Icon
+                  id={backIcon ?? "chevron-left"}
+                  width={18}
+                  height={18}
+                />
+              }
+              onClick={onRequestBack}
+            />
+          )
🧰 Tools
🪛 Biome (1.9.4)

[error] 70-85: Avoid using unnecessary Fragment.

A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment

(lint/complexity/noUselessFragments)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ef804ff and 36bbbb4.

⛔ Files ignored due to path filters (18)
  • packages/web/localizations/de.json is excluded by !**/*.json
  • packages/web/localizations/en.json is excluded by !**/*.json
  • packages/web/localizations/es.json is excluded by !**/*.json
  • packages/web/localizations/fa.json is excluded by !**/*.json
  • packages/web/localizations/fr.json is excluded by !**/*.json
  • packages/web/localizations/gu.json is excluded by !**/*.json
  • packages/web/localizations/hi.json is excluded by !**/*.json
  • packages/web/localizations/ja.json is excluded by !**/*.json
  • packages/web/localizations/ko.json is excluded by !**/*.json
  • packages/web/localizations/pl.json is excluded by !**/*.json
  • packages/web/localizations/pt-br.json is excluded by !**/*.json
  • packages/web/localizations/ro.json is excluded by !**/*.json
  • packages/web/localizations/ru.json is excluded by !**/*.json
  • packages/web/localizations/tr.json is excluded by !**/*.json
  • packages/web/localizations/zh-cn.json is excluded by !**/*.json
  • packages/web/localizations/zh-hk.json is excluded by !**/*.json
  • packages/web/localizations/zh-tw.json is excluded by !**/*.json
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock, !**/*.lock
📒 Files selected for processing (5)
  • packages/web/hooks/bridge.tsx (1 hunks)
  • packages/web/integrations/moonpay/index.tsx (1 hunks)
  • packages/web/modals/base.tsx (3 hunks)
  • packages/web/modals/fiat-on-ramp-selection.tsx (2 hunks)
  • packages/web/modals/fiat-ramps.tsx (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/web/modals/fiat-on-ramp-selection.tsx
  • packages/web/integrations/moonpay/index.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/web/modals/base.tsx

[error] 70-85: Avoid using unnecessary Fragment.

A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment

(lint/complexity/noUselessFragments)

⏰ Context from checks skipped due to timeout of 90000ms (9)
  • GitHub Check: production
  • GitHub Check: test (20.x, tx)
  • GitHub Check: test (20.x, bridge)
  • GitHub Check: test (20.x, stores)
  • GitHub Check: test (20.x, server)
  • GitHub Check: Analyze (javascript)
  • GitHub Check: wait-for-deployment
  • GitHub Check: test (20.x, web)
  • GitHub Check: Summary
🔇 Additional comments (5)
packages/web/modals/base.tsx (1)

26-26: LGTM! Good addition of the hideDefaultBackButton prop.

The new prop enhances the flexibility of the modal component by allowing control over the back button visibility.

packages/web/hooks/bridge.tsx (1)

407-410: LGTM! Clean implementation of the back navigation.

The back handler correctly manages state by clearing the fiat ramp params and showing the selection modal.

packages/web/modals/fiat-ramps.tsx (3)

13-19: LGTM! Well-structured provider mapping.

The FIAT_RAMPS_PRETTY_NAMES constant provides a clean mapping of provider keys to display names.


53-68: LGTM! Enhanced modal header implementation.

The custom header provides consistent navigation with both back and close buttons, improving the user experience.


73-74: LGTM! Clean MoonPay integration.

The MoonPay case follows the established pattern of other providers.

<IconButton
icon={<Icon id="arrow-left-thin" />}
aria-label="Back"
className="size-12 text-wosmongton-200 hover:text-osmoverse-100 md:size-8"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

The size-12 class needs to be replaced with w-12 h-12

The size-{number} utility was introduced in Tailwind 3.3.0, but this project uses version 3.2.4. Use w-12 h-12 instead to set both width and height to the same value.

🔗 Analysis chain

Verify the size-12 Tailwind class.

The size-12 class appears to be a custom class. Ensure it's properly defined in your Tailwind configuration.

Also applies to: 64-64

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if size-12 is defined in Tailwind config
fd -e js -e ts "tailwind.config" -x cat {} | grep -A 10 -B 10 "size-"

Length of output: 69


Script:

#!/bin/bash
# Check Tailwind version in package.json files
fd package.json -x cat {} | grep -A 5 -B 5 "tailwindcss"

Length of output: 320


Script:

#!/bin/bash
# Look for custom CSS files and Tailwind config files in various locations
fd -e css -e scss -x cat {} | grep -A 5 -B 5 "size-"

# Also check for any Tailwind config files we might have missed
find . -type f -exec grep -l "tailwind" {} \; | grep -i "config"

Length of output: 69814


Script:

#!/bin/bash
# Look for Tailwind plugin configurations
fd -e js -e ts -e json -x grep -l "tailwind.*plugin" {} \; 
fd -e js -e ts -e json -x grep -l "addUtilities" {} \;

Length of output: 121

@fabryscript fabryscript self-assigned this Jan 10, 2025
@fabryscript fabryscript changed the title Enrico/fe-1270-review-moonpay-proposal-and-consider-implementation Add MoonPay onramp option Jan 10, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
packages/web/modals/fiat-on-ramp-selection.tsx (2)

72-103: Consider extracting the MoonPay UI into a separate component.

The conditional rendering creates a complex nested structure. Consider extracting this into a separate component for better maintainability.

+ const MoonPayOption: FC<{ displayName: string }> = ({ displayName }) => (
+   <div className="ml-5 flex flex-col text-left gap-1">
+     {/* ... rest of the MoonPay-specific JSX ... */}
+   </div>
+ );

112-125: Consider moving provider configuration to a separate file.

The MOONPAY_SUPPORTED_PROVIDERS constant would be better placed in a configuration file:

  1. Move to a dedicated config file (e.g., fiat-ramp-config.ts)
  2. Consider making the paths configurable per environment

Example structure:

// fiat-ramp-config.ts
export const PAYMENT_PROVIDERS = {
  moonpay: {
    supported: [
      {
        id: "apple-pay",
        icon: process.env.PUBLIC_URL + "/payment-methods/apple-pay.svg",
      },
      // ...
    ],
  },
};
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 36bbbb4 and 4be0212.

⛔ Files ignored due to path filters (20)
  • packages/web/localizations/de.json is excluded by !**/*.json
  • packages/web/localizations/en.json is excluded by !**/*.json
  • packages/web/localizations/es.json is excluded by !**/*.json
  • packages/web/localizations/fa.json is excluded by !**/*.json
  • packages/web/localizations/fr.json is excluded by !**/*.json
  • packages/web/localizations/gu.json is excluded by !**/*.json
  • packages/web/localizations/hi.json is excluded by !**/*.json
  • packages/web/localizations/ja.json is excluded by !**/*.json
  • packages/web/localizations/ko.json is excluded by !**/*.json
  • packages/web/localizations/pl.json is excluded by !**/*.json
  • packages/web/localizations/pt-br.json is excluded by !**/*.json
  • packages/web/localizations/ro.json is excluded by !**/*.json
  • packages/web/localizations/ru.json is excluded by !**/*.json
  • packages/web/localizations/tr.json is excluded by !**/*.json
  • packages/web/localizations/zh-cn.json is excluded by !**/*.json
  • packages/web/localizations/zh-hk.json is excluded by !**/*.json
  • packages/web/localizations/zh-tw.json is excluded by !**/*.json
  • packages/web/public/payment-methods/apple-pay.svg is excluded by !**/*.svg, !**/*.svg
  • packages/web/public/payment-methods/mastercard.svg is excluded by !**/*.svg, !**/*.svg
  • packages/web/public/payment-methods/visa.svg is excluded by !**/*.svg, !**/*.svg
📒 Files selected for processing (1)
  • packages/web/modals/fiat-on-ramp-selection.tsx (4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: wait-for-deployment
  • GitHub Check: Summary
🔇 Additional comments (3)
packages/web/modals/fiat-on-ramp-selection.tsx (3)

20-24: LGTM! MoonPay integration looks good.

The implementation correctly adds MoonPay as a fiat ramp option with appropriate translation key and OSMO as the initial asset.


52-53: Verify navigation flow with hidden back button.

The addition of hideDefaultBackButton removes the default back navigation. Please ensure there's an alternative way for users to navigate back from this modal.

Run this script to check for alternative navigation methods:

Also applies to: 56-56

✅ Verification successful

Navigation flow is properly implemented despite hidden back button

The modal implements proper navigation through:

  • Modal close handler via onRequestClose
  • Button click handlers that properly close the modal
  • Parent component's toggleFiatOnRampSelection handler
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for navigation handlers in parent components
rg -B 5 -A 5 "FiatOnrampSelectionModal" --type typescript

Length of output: 92


Script:

#!/bin/bash
# Search for the modal component and its usage with proper extensions
rg -B 5 -A 5 "FiatOnrampSelectionModal" -g "*.{ts,tsx,jsx}"

# Also check the modal component file itself for close/navigation handlers
cat "packages/web/modals/fiat-on-ramp-selection.tsx"

Length of output: 6443


84-95: Verify image assets and improve accessibility.

The payment provider icons implementation needs attention:

  1. Ensure all image assets exist in the public directory
  2. Consider adding aria-label for the container
  3. The negative transform might cause layout issues on small screens

Run this script to verify image assets:

✅ Verification successful

Image assets verified, accessibility improvements still needed

The payment provider icons are correctly placed in packages/web/public/payment-methods/, but two concerns remain:

  • Add an aria-label to the icon container for better accessibility
  • Review the negative transform on small screens as it might cause layout issues
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if payment method images exist
for method in "apple-pay" "mastercard" "visa"; do
  if [ ! -f "public/payment-methods/${method}.svg" ]; then
    echo "Missing: ${method}.svg"
  fi
done

Length of output: 458


Script:

#!/bin/bash
# Find MOONPAY_SUPPORTED_PROVIDERS definition
ast-grep --pattern 'const MOONPAY_SUPPORTED_PROVIDERS = $_'

# Also search with ripgrep for any other references or definitions
rg "MOONPAY_SUPPORTED_PROVIDERS" -A 5

Length of output: 1992


Script:

#!/bin/bash
# Search for payment method related files across the repository
fd -t f -e svg -e png -e jpg "apple-pay|mastercard|visa" 

# Also check common asset directories explicitly
find . -type f \( -path "*/assets/*" -o -path "*/public/*" -o -path "*/static/*" -o -path "*/images/*" \) -exec file {} \; | grep -i "image"

Length of output: 33590

@EnricoBarbieri1997 EnricoBarbieri1997 merged commit 4d91181 into stage Jan 24, 2025
36 checks passed
@EnricoBarbieri1997 EnricoBarbieri1997 deleted the enrico/fe-1270-review-moonpay-proposal-and-consider-implementation branch January 24, 2025 14:31
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.

3 participants