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

feat: argent discount #1034

Merged
merged 8 commits into from
Feb 12, 2025
Merged

feat: argent discount #1034

merged 8 commits into from
Feb 12, 2025

Conversation

Marchand-Nicolas
Copy link
Collaborator

@Marchand-Nicolas Marchand-Nicolas commented Feb 9, 2025

close: #1014

Summary by CodeRabbit

  • New Features

    • Launched an enhanced discount experience with dynamic screens that adjust based on wallet connectivity.
    • Introduced a new promotional discount offer, providing detailed savings information for domain purchases.
    • Added new state management for wallet connection status and screen navigation.
    • Enhanced the RegisterDiscount component with improved state initialization and validation checks.
    • Added a new constant for the Argent discount offer, encapsulating promotional details.
  • Bug Fixes

    • Improved validations and error handling during discount registrations for a smoother user journey.
  • Style

    • Refined layout and spacing on discount screens for a cleaner, more polished visual presentation.
    • Introduced a new CSS class for better layout management.

@Marchand-Nicolas Marchand-Nicolas added the 🚧 In progress do not merge Pull Request in progress, please do not merge label Feb 9, 2025
Copy link

vercel bot commented Feb 9, 2025

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

Name Status Preview Comments Updated (UTC)
app-starknet-id ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 11, 2025 6:19pm
sepolia-app-starknet-id ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 11, 2025 6:19pm

Copy link
Contributor

coderabbitai bot commented Feb 9, 2025

Walkthrough

This update adjusts multiple discount-related components and refines Argent wallet integration. The DiscountOfferScreen now references updated styling. The RegisterDiscount component refactors salt initialization, introduces a metadataHash check, and tweaks formatting. The useAllowanceCheck hook adds a conditional check to prevent undefined errors. The Argent page is enhanced with new state variables, navigation functions, and conditional rendering based on wallet status. Additionally, new CSS rules are added, and a constant encapsulating Argent discount details is introduced.

Changes

File(s) Change Summary
components/discount/discountOfferScreen.tsx,
components/discount/registerDiscount.tsx
DiscountOfferScreen now uses styles.wrapperScreen instead of homeStyles.wrapperScreen. RegisterDiscount refactors salt initialization by storing the generated salt in a variable, adds a check for metadataHash, and updates formatting for the disabled button message.
hooks/useAllowanceCheck.tsx Adds a conditional check for erc20AllowanceRes before calling setCurrentAllowance to avoid potential undefined errors.
pages/argent.tsx Introduces new state variables (searchResult, screen, isArgent), adds goBack and handleSetScreen functions, and uses useEffect hooks to manage discount expiry and wallet connection status for conditional rendering.
styles/discount.module.css Adds a new .screen class with layout properties and updates the .wrapperScreen height calculation.
utils/discounts/argent.ts Introduces a new constant argentDiscount containing detailed properties of the Argent discount offer.

Sequence Diagram(s)

sequenceDiagram
    participant U as User
    participant AP as ArgentPage
    participant LS as LocalStorage
    participant WS as Wallet Checker

    U->>AP: Load Argent page
    AP->>LS: Check discount expiry
    LS-->>AP: Return expiry status
    AP->>AP: Set screen based on expiry
    AP->>WS: Verify wallet connection (is Argent?)
    WS-->>AP: Return wallet status
    AP->>U: Render appropriate screen (prompt, discount offer, or registration)
Loading

Assessment against linked issues

Objective Addressed Explanation
Argent discount page requirements (#1014): Create discount for Argent Metal Card clients, including usage limits and reserved letters. Usage limit (1200 times) and letter reservation (5 letters for one year) requirements are not implemented.

Suggested labels

🔥 Ready for review

Suggested reviewers

  • fricoben

📜 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 1b3b92b and da884bf.

📒 Files selected for processing (1)
  • utils/discounts/argent.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • utils/discounts/argent.ts

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: 2

🔭 Outside diff range comments (1)
components/discount/registerDiscount.tsx (1)

52-413: Consider breaking down the large component.

The component is handling multiple responsibilities including:

  • Domain registration
  • Sales tax calculation
  • Auto-renewal
  • Swiss residency
  • Payment processing

Consider splitting it into smaller, focused components for better maintainability.

Example structure:

// components/discount/register/
├── PaymentSection.tsx       // Currency selection & price display
├── SalesTaxCalculator.tsx   // Swiss VAT logic
├── AutoRenewalSection.tsx   // Renewal checkbox & logic
├── RegistrationForm.tsx     // Main form & validation
└── RegisterDiscount.tsx     // Main component (orchestrator)
🧹 Nitpick comments (2)
pages/argent.tsx (1)

15-16: Consider using an enum for screen management.

Using a number for screen state makes the code less maintainable. Consider using an enum to make the code more readable and type-safe.

+enum Screen {
+  ENDED,
+  OFFER,
+  REGISTER
+}
+
-const [screen, setScreen] = useState<number>(1);
+const [screen, setScreen] = useState<Screen>(Screen.OFFER);
components/discount/registerDiscount.tsx (1)

320-328: Extract sales tax calculation logic.

The sales tax calculation logic should be moved to a separate utility function or custom hook for better reusability and testing.

+// utils/salesTax.ts
+export function calculateSalesTax(price: bigint, isSwissResident: boolean) {
+  if (isSwissResident) {
+    return {
+      rate: swissVatRate,
+      amount: applyRateToBigInt(price, swissVatRate)
+    };
+  }
+  return {
+    rate: 0,
+    amount: BigInt(0)
+  };
+}

 useEffect(() => {
-  if (isSwissResident) {
-    setSalesTaxRate(swissVatRate);
-    setSalesTaxAmount(applyRateToBigInt(price, swissVatRate));
-  } else {
-    setSalesTaxRate(0);
-    setSalesTaxAmount(BigInt(0));
-  }
+  const { rate, amount } = calculateSalesTax(price, isSwissResident);
+  setSalesTaxRate(rate);
+  setSalesTaxAmount(amount);
 }, [isSwissResident, price]);
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between dc0a63a and bdc0a14.

📒 Files selected for processing (6)
  • components/discount/discountOfferScreen.tsx (1 hunks)
  • components/discount/registerDiscount.tsx (4 hunks)
  • hooks/useAllowanceCheck.tsx (1 hunks)
  • pages/argent.tsx (1 hunks)
  • styles/discount.module.css (1 hunks)
  • utils/discounts/argent.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • components/discount/discountOfferScreen.tsx
🔇 Additional comments (5)
styles/discount.module.css (2)

1-7: New .screen class added to support updated layout.
The new CSS class is well defined with properties (min-height, position, margin-top, overflow, padding-top) that support the intended structure for the discount screen. Ensure that its usage in the component consistently matches the design specifications.


10-14: Adjusted height for .wrapperScreen to align with revised layout spacing.
Changing the height calculation to "calc(82vh - 2rem)" appears intended to accommodate the new spacing introduced by the .screen class. Please verify that this modification produces the expected visual alignment and responsiveness across different viewport sizes.

hooks/useAllowanceCheck.tsx (1)

37-38: LGTM! Good defensive programming.

The added null check for erc20AllowanceRes before accessing its properties prevents potential runtime errors.

pages/argent.tsx (1)

29-35: LGTM! Good wallet connection handling.

The wallet connection check is well implemented with proper state management and user feedback.

components/discount/registerDiscount.tsx (1)

172-172: LGTM! Good defensive check for metadataHash.

The added check prevents constructing calls with undefined metadataHash, improving reliability.

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

♻️ Duplicate comments (1)
pages/argent.tsx (1)

41-49: ⚠️ Potential issue

Security concern: Sensitive referral data stored in plain text

The referral data—including the sponsor address—is stored without encryption, validation, or cleanup.

Consider:

  1. Encrypting the data before storage
  2. Adding validation when reading
  3. Implementing an expiry cleanup mechanism
 const handleSetScreen = (screen: number) => {
   const referralData = {
     sponsor: argentDiscount.sponsor,
     expiry: new Date().getTime() + 7 * 24 * 60 * 60 * 1000,
+    version: '1',  // Add version for future compatibility
   };
 
-  localStorage.setItem("referralData", JSON.stringify(referralData));
+  // Encrypt data before storage
+  const encryptedData = encryptData(JSON.stringify(referralData));
+  localStorage.setItem("referralData", encryptedData);
   setScreen(screen);
 };
🧹 Nitpick comments (3)
pages/argent.tsx (3)

20-27: Enhance expiry check robustness.

The expiry check could be more robust by:

  1. Using a constant for timestamp comparison
  2. Adding error handling for invalid expiry dates
 useEffect(() => {
   const currentDate = new Date();
   const timestamp = currentDate.getTime();
+  const MILLISECONDS_PER_SECOND = 1000;
+  const expiryTimestamp = Number(argentDiscount.expiry) * MILLISECONDS_PER_SECOND;
 
-  if (timestamp >= argentDiscount.expiry) {
+  if (!isNaN(expiryTimestamp) && timestamp >= expiryTimestamp) {
     setScreen(0);
   }
 }, []);

37-39: Add bounds check to prevent invalid navigation.

The goBack function should prevent navigation below screen 0.

 function goBack() {
-  setScreen(screen - 1);
+  setScreen(Math.max(0, screen - 1));
 }

95-98: Add validation for environment variable.

The mailing list environment variable could be undefined, which might cause issues.

 mailGroups={[
-  process.env.NEXT_PUBLIC_MAILING_LIST_GROUP ?? "",
+  process.env.NEXT_PUBLIC_MAILING_LIST_GROUP || 
+    throw new Error("NEXT_PUBLIC_MAILING_LIST_GROUP is not defined"),
   argentDiscount.discountMailGroupId,
 ]}
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between bdc0a14 and 06700c9.

📒 Files selected for processing (2)
  • pages/argent.tsx (1 hunks)
  • styles/discount.module.css (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • styles/discount.module.css

Copy link
Contributor

@fricoben fricoben left a comment

Choose a reason for hiding this comment

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

Some changes, you can test on sepolia with discountId=3 btw

pages/argent.tsx Outdated
customMessage={argentDiscount.offer.customMessage}
goBack={goBack}
priceInEth={BigInt(argentDiscount.offer.price)}
mailGroups={[
Copy link
Contributor

Choose a reason for hiding this comment

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

Mail don't exist anymore you can delete everything related to that (in the discount & component also)

name: "Argent 1$ domain",
image: "/argent/argentdiscount.webp",
expiry: 1740494381000, // timestamp in ms
discountMailGroupId: "124587870775149633",
Copy link
Contributor

Choose a reason for hiding this comment

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

same to delete

offer: {
duration: 16,
customMessage: "3 months (with Argent discount)",
discountId: "0x617267656e745f6f6e655f646f6c6c6172",
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you try this one mainnet for a year ? Maybe it works I don't remember the lenght of the discount

@fricoben fricoben added ❌ Change request Change requested from reviewer and removed 🚧 In progress do not merge Pull Request in progress, please do not merge labels Feb 11, 2025
@vercel vercel bot requested a deployment to Preview – app-starknet-id February 11, 2025 15:43 Abandoned
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 (3)
pages/argent.tsx (3)

14-19: Consider deriving initial screen state from argentDiscount expiry.

Instead of setting initial screen state to 1 and then immediately checking expiry in useEffect, consider deriving it during initialization:

-  const [screen, setScreen] = useState<number>(1);
+  const [screen, setScreen] = useState<number>(() => {
+    return Date.now() >= argentDiscount.expiry ? 0 : 1;
+  });

20-35: Enhance expiry monitoring for real-time updates.

The current implementation only checks expiry once when the component mounts. Consider implementing continuous monitoring to update the screen state if the discount expires during the session.

  useEffect(() => {
-   const currentDate = new Date();
-   const timestamp = currentDate.getTime();
-
-   if (timestamp >= argentDiscount.expiry) {
-     setScreen(0);
-   }
-  }, []);
+   const checkExpiry = () => {
+     if (Date.now() >= argentDiscount.expiry) {
+       setScreen(0);
+     }
+   };
+   
+   checkExpiry();
+   const interval = setInterval(checkExpiry, 60000); // Check every minute
+   
+   return () => clearInterval(interval);
+  }, []);

52-100: Add loading states and error handling for wallet connection.

The current implementation doesn't handle loading states or potential errors during wallet connection. Consider:

  1. Adding loading indicators during wallet connection
  2. Handling potential connection errors
  3. Providing feedback when switching screens
  {!isArgent || !account ? (
    <div className="flex flex-col items-center justify-center h-[82vh]">
      <div className="mb-10">
        <ArgentIcon width="150px" color="#FF875B" />
      </div>
      <div className={styles.title}>Connect Argent wallet</div>
      <div className={styles.description}>
        To access this discount, you need to connect an Argent wallet.
      </div>
+     {connector.isLoading && <div>Connecting...</div>}
+     {connector.error && (
+       <div className="text-red-500">
+         Failed to connect: {connector.error.message}
+       </div>
+     )}
      {!account ? (
        <div className="flex items-center mt-3">
          <ConnectButton />
        </div>
      ) : null}
    </div>
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 06700c9 and d524096.

📒 Files selected for processing (3)
  • components/discount/registerDiscount.tsx (4 hunks)
  • pages/argent.tsx (1 hunks)
  • utils/discounts/argent.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • utils/discounts/argent.ts
  • components/discount/registerDiscount.tsx
🔇 Additional comments (2)
pages/argent.tsx (2)

1-11: Add missing type import for SearchResult.

The SearchResult type is used in the state declaration but not imported.


41-49: Security concern: Sensitive referral data stored in plain text.

The referral data containing sponsor address is stored without encryption or validation.

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 (1)
utils/discounts/argent.ts (1)

9-9: Consider extracting the duplicated image path.

The image path is duplicated. Consider extracting it to a constant to improve maintainability.

 export const argentDiscount = {
+  image: "/argent/argentdiscount.webp",
   offer: {
     duration: 365,
     customMessage: "-50% (with Argent discount)",
     discountId: "0x617267656e745f6f6e655f646f6c6c6172",
     price: "288493150684927",
     desc: "Get your domain for half the price. Don't miss out on this exclusive opportunity only available with your Argent wallet!",
     title: { desc: "Mint your domain", catch: "-50% discount" },
-    image: "/argent/argentdiscount.webp",
+    image: argentDiscount.image,
   },
   name: "Argent -50% domain",
-  image: "/argent/argentdiscount.webp",
   expiry: 1770207266000, // timestamp in ms
   sponsor: "0x64d28d1d1d53a0b5de12e3678699bc9ba32c1cb19ce1c048578581ebb7f8396",
 };

Also applies to: 12-12

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between d524096 and 1e2e192.

📒 Files selected for processing (1)
  • utils/discounts/argent.ts (1 hunks)
🔇 Additional comments (3)
utils/discounts/argent.ts (3)

3-6: Consider moving configuration to environment variables.

The discount configuration contains sensitive data like discountId, price, expiry, and sponsor address that should be managed through environment variables rather than being hardcoded.

Also applies to: 13-14


4-4: LGTM! Clear and consistent messaging.

The discount messaging is clear and consistent across all text elements, properly communicating the 50% discount offer.

Also applies to: 7-8


3-3: Verify duration and expiry settings.

Please confirm:

  1. Has the 365-day duration been tested on mainnet? (Previous review raised this question)
  2. Is the expiry date of June 2026 aligned with business requirements?

Let's check if there are any existing discounts with similar duration:

Also applies to: 13-13

❌ Verification inconclusive

🏁 Script executed:

#!/bin/bash
# Search for other discount configurations with duration or expiry settings
rg -i "duration.*365|expiry.*2026" --type ts

Length of output: 1446


Mainnet Testing and Business Requirements Confirmation

The 365-day duration configuration in argent.ts is consistent with similar implementations in other discount modules (e.g., in Evergreen and checkout components). However, we did not find explicit evidence within the codebase confirming that the 365-day duration has been specifically tested on mainnet. Additionally, while the expiry date (set to approximately June 2026) appears to be hardcoded as intended, it isn’t clear from the repository whether this date has been reviewed against current business requirements.

  • Action Items:
    • Confirm via deployment or QA logs that the 365-day discount setting has been fully validated on mainnet.
    • Verify with the business or product team that the expiry date (June 2026) aligns with the intended discount lifecycle and requirements.

Copy link
Contributor

@fricoben fricoben left a comment

Choose a reason for hiding this comment

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

lgtm

@fricoben fricoben merged commit fad429f into testnet Feb 12, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
❌ Change request Change requested from reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Argent discount page
2 participants