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

ref: Reactor register/pricing/discounts #874

Merged
merged 8 commits into from
Sep 3, 2024
Merged

Conversation

fricoben
Copy link
Contributor

@fricoben fricoben commented Aug 21, 2024

Description

  • Adding pricing variables related in bigint everywhere before displaying it
  • Change duration in days everywhere it makes sense
  • Take into consideration old approval while registering, if the approval is not infinite we add up the approval (one 10Y approval per domain)

My E2E

  • /Register in both currencies
    • USD display bug
    • Sales tax discount bug calculation
    • Altcoin buy duration bug
  • /Renew in both currencies
  • /gift
  • /subscription in both currencies
  • /freerenewal in both currencies

Summary by CodeRabbit

  • New Features

    • Enhanced clarity in duration handling by renaming properties from duration to durationInDays or durationInYears across multiple components.
    • Introduced bigint type for monetary values, improving precision in financial calculations.
    • Added new functions for pricing calculations, including getDisplayablePrice and getApprovalAmount.
    • Introduced a Notification component for user feedback on token quote retrieval failures.
    • Added a custom hook useCurrencyManagement for managing currency selection and fetching token quotes.
  • Bug Fixes

    • Corrected import paths and consolidated props in components for better maintainability.
  • Documentation

    • Updated type definitions to reflect changes in property names and types for better clarity.
  • Refactor

    • Streamlined state management and logic in various components to enhance readability and performance, including the use of hooks and memoization.
  • Chores

    • Removed unused functions and optimized existing ones for improved functionality and clarity.

Copy link

vercel bot commented Aug 21, 2024

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 Sep 2, 2024 0:19am
sepolia-app-starknet-id ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 2, 2024 0:19am

@fricoben fricoben changed the title ref: first commit of the refactoring ref: Reactor register/pricing/discounts Aug 21, 2024
@fricoben fricoben added the 🚧 In progress do not merge Pull Request in progress, please do not merge label Aug 21, 2024
Copy link
Contributor

coderabbitai bot commented Aug 21, 2024

Walkthrough

The changes across the codebase involve renaming properties for clarity, particularly changing duration to durationInDays or durationInYears. Additionally, monetary values have shifted from string types to bigint for improved precision in financial calculations. Several functions have been renamed or restructured to align with these updates, and some components have consolidated their props into single objects for better management. A new custom hook for currency management has also been introduced.

Changes

Files Change Summary
components/discount/freeRegisterCheckout.tsx, components/discount/freeRegisterSummary.tsx, components/discount/registerDiscount.tsx, components/discount/freeRenewalCheckout.tsx, components/domains/autorenewal.tsx, components/domains/registerCheckboxes.tsx, components/domains/registerSummary.tsx, components/domains/registerV3.tsx, components/domains/renewalV2.tsx, context/FormProvider.tsx Renamed duration to durationInDays or durationInYears across multiple components. Changed monetary types from string to bigint.
components/discount/freeRenewalCheckout.tsx, components/discount/registerDiscount.tsx, components/domains/registerSummary.tsx Consolidated props into single objects for certain components, improving clarity.
components/identities/actions/clickable/clickablePersonhoodIcon.tsx Removed import of constants from starknet.
hooks/useAllowanceCheck.tsx, hooks/useNeedAllowances.tsx Updated return types and state management to reflect changes in allowance handling, introducing AllowanceStatus type.
utils/feltService.ts, utils/subscriptionService.ts Modified functions to accept bigint instead of string for monetary values, enhancing precision in calculations.
pages/argent.tsx Simplified components by removing state management and conditional rendering, directly rendering static screens instead.
utils/constants.ts Updated the value of swissVatRate from 0.077 to 0.081.
utils/discounts/freeRegistration.ts, utils/discounts/freeRenewal.ts Changed price from string to BigInt(0) and renamed duration to durationInDays.
tests/utils/altcoinService.test.js, tests/utils/feltService.test.js Updated tests to utilize BigInt for numerical calculations instead of strings.
types/frontTypes.d.ts Renamed properties in various types to improve clarity and specificity, particularly around monetary values.
hooks/checkout/useCurrencyManagement.tsx Introduced a new custom hook for managing currency selection and fetching token quotes.

Sequence Diagram(s)

sequenceDiagram
    participant A as User
    participant B as Currency Management Hook
    participant C as Token Quote Service

    A->>B: Select currency
    B->>C: Fetch token quote
    C-->>B: Return quote data
    B-->>A: Display quote
Loading

This diagram illustrates the interaction between the user and the new currency management hook, highlighting the process of selecting a currency and fetching the corresponding token quote.


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>.
    • 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 generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @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 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.
Early access features: disabled

We are currently testing the following features in early access:

  • Anthropic claude-3-5-sonnet for code reviews: Anthropic claims that the new Claude model has stronger code understanding and code generation capabilities than their previous models. Note: Our default code review model was also updated late last week. Please compare the quality of the reviews between the two models by toggling the early access feature.

Note:

  • You can enable or disable early access features from the CodeRabbit UI or by updating the CodeRabbit configuration file.
  • Please join our Discord Community to provide feedback and report issues on the discussion post.

@fricoben fricoben added 🍒 Nice to have Nice to have but not a priority 🔥 Ready for review This pull request needs a review and removed 🚧 In progress do not merge Pull Request in progress, please do not merge 🍒 Nice to have Nice to have but not a priority labels Aug 28, 2024
@fricoben fricoben marked this pull request as ready for review August 28, 2024 16:59
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: 3

Outside diff range, codebase verification and nitpick comments (1)
context/FormProvider.tsx (1)

Line range hint 64-73: Fix the inconsistency in clearForm.

The duration should be renamed to durationInYears to match the updated FormState type.

Apply this diff to fix the inconsistency:

 def clearForm = useCallback(() => {
    setFormState((prevState) => ({
      ...prevState,
      selectedDomains: {},
      tokenId: 0,
-     duration: 1,
+     durationInYears: 1,
      isUpselled: false,
      selectedPfp: undefined,
    }));
  }, []);
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 7a801c0 and 7ad9073.

Files selected for processing (36)
  • components/discount/freeRegisterCheckout.tsx (4 hunks)
  • components/discount/freeRegisterSummary.tsx (2 hunks)
  • components/discount/freeRenewalCheckout.tsx (11 hunks)
  • components/discount/registerDiscount.tsx (10 hunks)
  • components/domains/autorenewal.tsx (11 hunks)
  • components/domains/registerCheckboxes.tsx (3 hunks)
  • components/domains/registerSummary.tsx (6 hunks)
  • components/domains/registerV3.tsx (1 hunks)
  • components/domains/renewalV2.tsx (1 hunks)
  • components/domains/steps/checkoutCard.tsx (32 hunks)
  • components/domains/steps/reduceDuration.tsx (2 hunks)
  • components/domains/steps/userInfoForm.tsx (4 hunks)
  • components/identities/actions/clickable/clickablePersonhoodIcon.tsx (1 hunks)
  • context/FormProvider.tsx (8 hunks)
  • hooks/useAllowanceCheck.tsx (2 hunks)
  • hooks/useNeedAllowances.tsx (2 hunks)
  • hooks/useNeedSubscription.tsx (2 hunks)
  • pages/argent.tsx (1 hunks)
  • pages/freerenewal.tsx (2 hunks)
  • pages/gift.tsx (1 hunks)
  • pages/quantumleap.tsx (1 hunks)
  • tests/utils/altcoinService.test.js (4 hunks)
  • tests/utils/feltService.test.js (5 hunks)
  • tests/utils/priceService.test.js (3 hunks)
  • tests/utils/subscriptionService.test.js (1 hunks)
  • types/frontTypes.d.ts (3 hunks)
  • utils/altcoinService.ts (6 hunks)
  • utils/callData/autoRenewalCalls.ts (1 hunks)
  • utils/callData/registrationCalls.ts (8 hunks)
  • utils/constants.ts (1 hunks)
  • utils/discounts/evergreen.ts (2 hunks)
  • utils/discounts/freeRegistration.ts (1 hunks)
  • utils/discounts/freeRenewal.ts (1 hunks)
  • utils/feltService.ts (1 hunks)
  • utils/priceService.ts (3 hunks)
  • utils/subscriptionService.ts (1 hunks)
Files skipped from review due to trivial changes (6)
  • components/domains/renewalV2.tsx
  • components/identities/actions/clickable/clickablePersonhoodIcon.tsx
  • pages/argent.tsx
  • pages/freerenewal.tsx
  • pages/quantumleap.tsx
  • tests/utils/subscriptionService.test.js
Additional context used
Biome
utils/feltService.ts

[error] 30-30: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.

See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.

(lint/suspicious/noGlobalIsNan)

Additional comments not posted (126)
utils/subscriptionService.ts (2)

Line range hint 3-9: LGTM!

The function is correctly implemented.

The code changes are approved.


Line range hint 11-14: LGTM!

The function is correctly implemented.

The code changes are approved.

utils/discounts/freeRenewal.ts (2)

16-16: LGTM!

Renaming duration to durationInDays improves clarity regarding the unit of measurement.

The code changes are approved.


19-19: LGTM!

Changing price from a string to BigInt improves precision in financial calculations.

The code changes are approved.

utils/discounts/freeRegistration.ts (2)

16-16: LGTM!

Renaming duration to durationInDays improves clarity regarding the unit of measurement.

The code changes are approved.


18-18: LGTM!

Changing price from a string to BigInt improves precision in financial calculations.

The code changes are approved.

utils/discounts/evergreen.ts (2)

2-4: LGTM!

The changes to the duration properties improve clarity by explicitly indicating that durations are in days.

The code changes are approved.


15-17: LGTM!

The changes to the duration properties improve clarity by explicitly indicating that durations are in days.

The code changes are approved.

utils/callData/autoRenewalCalls.ts (2)

6-11: LGTM!

The changes improve type safety and simplify the logic by eliminating unnecessary conversions.

The code changes are approved.


18-26: LGTM!

The changes improve type safety and simplify the logic by eliminating unnecessary conversions.

The code changes are approved.

hooks/useAllowanceCheck.tsx (1)

Line range hint 15-44: LGTM!

The changes improve the functionality of the hook by providing more comprehensive information about the allowance status.

The code changes are approved.

components/domains/steps/reduceDuration.tsx (2)

9-9: LGTM!

The change aligns with the new terminology and enhances clarity.

The code changes are approved.


20-20: LGTM!

The change ensures consistency with the updated prop type.

The code changes are approved.

components/discount/freeRegisterSummary.tsx (3)

3-3: LGTM!

The import is necessary for the updated yearly price calculation.

The code changes are approved.


8-8: LGTM!

The change aligns with the new terminology and enhances clarity.

The code changes are approved.


Line range hint 14-31: LGTM!

The changes ensure that the displayed message and price are accurate and user-friendly.

The code changes are approved.

pages/gift.tsx (1)

46-46: LGTM! But verify the consistency of the new property name.

The change enhances clarity by specifying the unit of measurement for the duration. Ensure that the new property name durationInDays is consistent with the rest of the codebase.

The code changes are approved.

Run the following script to verify the consistency of the new property name:

Verification successful

Property Name Consistency Verified

The property name durationInDays has been consistently applied across the codebase, and there are no remaining instances of the old property name duration. This confirms that the update has been correctly implemented.

  • Instances of durationInDays were found, confirming its usage.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the consistency of the new property name `durationInDays`.

# Test: Search for the old property name. Expect: No occurrences of the old property name.
rg --type js --type ts --type tsx -w 'duration='

# Test: Search for the new property name. Expect: Only occurrences of the new property name.
rg --type js --type ts --type tsx -w 'durationInDays='

Length of output: 159


Script:

#!/bin/bash
# Description: Verify the consistency of the new property name `durationInDays`.

# Find all .js, .ts, and .tsx files and search for the old and new property names.
fd -e js -e ts -e tsx -x rg -w 'duration=' {}
fd -e js -e ts -e tsx -x rg -w 'durationInDays=' {}

Length of output: 212

utils/feltService.ts (1)

41-49: LGTM!

The changes improve type safety and streamline the function by reducing unnecessary type checks and conversions.

The code changes are approved.

components/domains/registerV3.tsx (1)

37-37: LGTM! But verify the consistency of the new property name.

The change enhances clarity by specifying the unit of measurement for the duration. Ensure that the new property name durationInYears is consistent with the rest of the codebase.

The code changes are approved.

Run the following script to verify the consistency of the new property name:

Verification successful

The new property name durationInYears is consistently used across the codebase.

The old property name duration is no longer present, and durationInYears is correctly used in several files, indicating that the change has been applied consistently.

  • components/domains/steps/checkoutCard.tsx
  • components/discount/freeRenewalCheckout.tsx
  • components/discount/registerDiscount.tsx
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the consistency of the new property name `durationInYears`.

# Test: Search for the old property name. Expect: No occurrences of the old property name.
rg --type js --type ts --type tsx -w 'duration='

# Test: Search for the new property name. Expect: Only occurrences of the new property name.
rg --type js --type ts --type tsx -w 'durationInYears='

Length of output: 160


Script:

#!/bin/bash
# Description: Verify the consistency of the new property name `durationInYears`.

# Test: Search for the old property name. Expect: No occurrences of the old property name.
rg --type js --type ts --type jsx -w 'duration='

# Test: Search for the new property name. Expect: Only occurrences of the new property name.
rg --type js --type ts --type jsx -w 'durationInYears='

Length of output: 160


Script:

#!/bin/bash
# Description: Verify the consistency of the new property name `durationInYears`.

# Test: Search for the old property name. Expect: No occurrences of the old property name.
rg --type js --type ts -w 'duration='

# Test: Search for the new property name. Expect: Only occurrences of the new property name.
rg --type js --type ts -w 'durationInYears='

Length of output: 347

components/domains/registerCheckboxes.tsx (3)

4-4: LGTM!

The import statement for weiToEth is correctly added.

The code changes are approved.


20-20: LGTM!

The type definition for maxPriceRange is correctly changed from string to bigint.

The code changes are approved.


40-43: LGTM!

The function getHelperText is correctly updated to use weiToEth instead of gweiToEth.

The code changes are approved.

hooks/useNeedAllowances.tsx (3)

10-10: LGTM!

The import statement for isApprovalInfinite is correctly added.

The code changes are approved.


15-20: LGTM!

The initial state for needsAllowances is correctly updated to include currentAllowance.

The code changes are approved.


82-85: LGTM!

The state update for needsAllowances is correctly modified to merge the previous allowances with the new allowances.

The code changes are approved.

utils/priceService.ts (9)

1-1: LGTM!

The import statement for weiToEth is correctly added.

The code changes are approved.


2-2: LGTM!

The import statement for selectedDomainsToArray is correctly added.

The code changes are approved.


12-19: LGTM!

The function getDomainPriceWei is correctly renamed and updated to enhance clarity regarding the expected input.

The code changes are approved.


37-45: LGTM!

The function getManyDomainsPriceWei is correctly renamed and updated to accept a Record<string, boolean> | undefined instead of an array of strings.

The code changes are approved.


58-61: LGTM!

The function getYearlyPriceWei is correctly renamed and updated to return a value in Wei.

The code changes are approved.


66-72: LGTM!

The function getTotalYearlyPrice is correctly updated to handle selected domains in a more flexible manner.

The code changes are approved.


74-76: LGTM!

The function getDisplayablePrice is correctly added to convert a price in Wei to a formatted string.

The code changes are approved.


78-89: LGTM!

The function getApprovalAmount is correctly added to calculate the amount to approve based on various parameters.

The code changes are approved.


91-103: LGTM!

The function isApprovalInfinite is correctly added to determine if a given approval amount is infinite based on defined thresholds.

The code changes are approved.

context/FormProvider.tsx (3)

11-11: LGTM!

The renaming of duration to durationInYears improves clarity.

The code changes are approved.


37-37: LGTM!

The renaming of duration to durationInYears in initialState ensures consistency with the updated FormState type.

The code changes are approved.


60-62: LGTM!

The refactoring to use useCallback improves performance by memoizing the function.

The code changes are approved.

types/frontTypes.d.ts (3)

54-57: LGTM!

The renaming of duration to durationInDays improves clarity, and changing price from string to bigint ensures appropriate handling of large numerical values.

The code changes are approved.


129-130: LGTM!

Replacing the boolean type with AllowanceStatus enhances the structure and clarity of the allowance-related data.

The code changes are approved.


149-151: LGTM!

The renaming of duration, paidDuration, and maxDuration to durationInDays, paidDurationInDays, and maxDurationInDays improves clarity.

The code changes are approved.

tests/utils/feltService.test.js (6)

49-51: LGTM!

The new test case improves coverage by testing the conversion of a number to its hex representation.

The code changes are approved.


55-57: LGTM!

The new test case improves coverage by testing the conversion of 0 to its hex representation.

The code changes are approved.


75-78: LGTM!

The renaming of gweiToEth to weiToEth improves clarity by specifying the unit of the input value. The updated test cases reflect this change appropriately.

The code changes are approved.


88-107: LGTM!

The updated test cases enhance type safety and ensure that the function handles large integers appropriately.

The code changes are approved.


115-136: LGTM!

The reformatting of test cases improves readability without altering the underlying logic.

The code changes are approved.


Line range hint 152-168: LGTM!

The updated test cases improve coverage by testing edge cases and ensuring that the function handles invalid inputs appropriately.

The code changes are approved.

utils/callData/registrationCalls.ts (6)

4-12: LGTM!

The change from string to bigint for the price parameter is appropriate and the conversion to a string for the calldata is correctly implemented.

The code changes are approved.


Line range hint 20-33: LGTM!

The change from durationInYears to durationInDays simplifies the logic and the use of numberToString ensures accurate representation in the calldata.

The code changes are approved.


Line range hint 50-65: LGTM!

The change from durationInYears to durationInDays simplifies the logic and the use of numberToString ensures accurate representation in the calldata.

The code changes are approved.


103-111: LGTM!

The change from string to bigint for the amount parameter is appropriate and the conversion to a string for the calldata is correctly implemented.

The code changes are approved.


Line range hint 117-127: LGTM!

The change from durationInYears to durationInDays simplifies the logic and the direct inclusion in the calldata ensures accurate representation.

The code changes are approved.


Line range hint 137-149: LGTM!

The change from durationInYears to durationInDays simplifies the logic and the direct inclusion in the calldata ensures accurate representation.

The code changes are approved.

utils/constants.ts (1)

8-8: LGTM!

The change in the swissVatRate constant reflects the updated VAT rate and is correctly implemented.

The code changes are approved.

components/domains/registerSummary.tsx (8)

8-14: LGTM!

The imports are correctly updated to include necessary utilities and components.

The code changes are approved.


Line range hint 17-33: LGTM!

The updates to the RegisterSummaryProps type to use bigint for price properties and renaming duration to durationInYears improve clarity and precision.

The code changes are approved.


Line range hint 37-51: LGTM!

The updates to the RegisterSummary component's props and internal logic to accommodate the new types and improve state management are well-implemented.

The code changes are approved.


53-64: LGTM!

The use of useState and useEffect hooks to manage and fetch the ETH to USD price is correctly implemented.

The code changes are approved.


75-101: LGTM!

The logic for calculating the USD registration price and sales tax amount using the new computeUsdRegistrationPrice and computeUsdSalesTaxAmount functions is clear and well-structured.

The code changes are approved.


138-148: LGTM!

The displayTokenPrice function correctly handles the display of prices, including sales tax information for Swiss residents.

The code changes are approved.


151-157: LGTM!

The getCheckoutMessage function correctly formats the checkout message based on the new price properties and duration.

The code changes are approved.


166-174: LGTM!

The rendering logic for the RegisterSummary component, including the conditional display of the USD price, is well-implemented.

The code changes are approved.

components/domains/steps/userInfoForm.tsx (4)

66-66: LGTM!

The function changeDuration correctly updates the form state with durationInYears.

The code changes are approved.


84-85: LGTM!

The function updateFormState correctly updates the form state with durationInYears.

The code changes are approved.

Tools
Biome

[error] 85-85: Unnecessary use of boolean literals in conditional expression.

Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with

(lint/complexity/noUselessTernary)


111-112: LGTM!

The function isDisabled correctly checks the conditions using durationInYears.

The code changes are approved.


157-157: LGTM!

The NumberTextField component correctly reflects the new property name durationInYears.

The code changes are approved.

utils/altcoinService.ts (9)

18-29: LGTM!

The function getDomainPriceAltcoin correctly handles the conversion and calculation using bigint.

The code changes are approved.


35-47: LGTM!

The function getPriceForDuration correctly handles the conversion and calculation using bigint.

The code changes are approved.


98-107: LGTM!

The function getDomainPrice correctly handles the conversion and calculation using bigint.

The code changes are approved.


112-120: LGTM!

The function getYearlyPrice correctly calculates the yearly price using bigint.

The code changes are approved.


122-136: LGTM!

The function getManyDomainsPrice correctly calculates the price for multiple domains using bigint.

The code changes are approved.


138-148: LGTM!

The function getTotalYearlyPrice correctly calculates the total yearly price for multiple domains using bigint.

The code changes are approved.


155-160: LGTM!

The function getAutoRenewAllowance correctly handles the conversion and calculation using bigint.

The code changes are approved.


Line range hint 181-208: LGTM!

The function smartCurrencyChoosing correctly handles the conversion and calculation using bigint.

The code changes are approved.


3-3: LGTM!

The import statement correctly includes getDomainPriceWei.

The code changes are approved.

components/discount/freeRegisterCheckout.tsx (2)

36-36: LGTM!

The function FreeRegisterCheckout correctly reflects the new property name durationInDays.

The code changes are approved.


201-204: LGTM!

The FreeRegisterSummary component correctly reflects the new property name durationInDays.

The code changes are approved.

tests/utils/priceService.test.js (7)

5-6: LGTM!

The function getDomainPriceWei is correctly tested with various domain names and expected prices.

The code changes are approved.

Also applies to: 17-43


6-7: LGTM!

The function getManyDomainsPriceWei is correctly tested with various sets of domains and durations.

The code changes are approved.

Also applies to: 47-81


9-10: LGTM!

The function getTotalYearlyPrice is correctly tested with various sets of selected domains.

The code changes are approved.

Also applies to: 135-152


10-11: LGTM!

The function getYearlyPriceWei is correctly tested with various domain names and expected prices.

The code changes are approved.

Also applies to: 156-172


11-12: LGTM!

The function getApprovalAmount is correctly tested with various sets of inputs.

The code changes are approved.

Also applies to: 175-226


12-13: LGTM!

The function getDisplayablePrice is correctly tested with various price values.

The code changes are approved.

Also applies to: 229-253


13-14: LGTM!

The function isApprovalInfinite is correctly tested with various values.

The code changes are approved.

Also applies to: 256-289

tests/utils/altcoinService.test.js (8)

25-33: LGTM!

The function getDomainPriceAltcoin is correctly tested with various price and quote values.

The code changes are approved.

Also applies to: 37-42, 47-52


Line range hint 59-59: LGTM!

The function getLimitPriceRange is correctly tested with various currency types and price values.

The code changes are approved.


Line range hint 59-59: LGTM!

The function getRenewalPriceETH is correctly tested with various price data and duration values.

The code changes are approved.


140-152: LGTM!

The function getDomainPrice is correctly tested with various domain names and currency types.

The code changes are approved.


Line range hint 161-187: LGTM!

The function getAutoRenewAllowance is correctly tested with various currency types, sales tax rates, and domain prices.

The code changes are approved.


193-217: LGTM!

The function getPriceForDuration is correctly tested with various price and duration values.

The code changes are approved.


193-217: LGTM!

The function fetchAvnuQuoteData is correctly tested with various fetch scenarios.

The code changes are approved.


193-217: LGTM!

The function smartCurrencyChoosing is correctly tested with various token balances and domain prices.

The code changes are approved.

components/domains/autorenewal.tsx (11)

60-63: Update state management for monetary values.

The state variables salesTaxAmount, priceInEth, and price have been updated to use bigint for enhanced precision in financial calculations.

The code changes are approved.


84-84: Rename needsAllowance to allowanceStatus.

The variable needsAllowance has been renamed to allowanceStatus to reflect a clearer understanding of its purpose.

The code changes are approved.


193-199: Add logic for handling token quotes.

The logic for setting quoteData and handling errors has been added to provide user feedback regarding token quotes.

The code changes are approved.


232-233: Update price calculation logic.

The logic for updating priceInEth has been updated to use getTotalYearlyPrice.

The code changes are approved.


257-257: Update sales tax calculation logic.

The logic for setting salesTaxAmount has been updated to use bigint and applyRateToBigInt.

The code changes are approved.


273-285: Update approval amount calculation logic.

The logic for calculating amountToApprove has been updated to use getApprovalAmount.

The code changes are approved.


326-326: Update dependencies for useEffect.

The dependencies for the useEffect hook have been updated to include allowanceStatus.

The code changes are approved.


434-439: Add Notification component for currency errors.

The Notification component has been added to provide user feedback regarding currency errors.

The code changes are approved.


49-49: Add import for Notification component.

The Notification component is imported to provide user feedback. Ensure the component is correctly implemented in ../UI/notification.

Run the following script to verify the component definition:

Verification successful

Notification Component is Correctly Defined

The Notification component is correctly defined in the components/UI/notification.tsx file as a FunctionComponent. This confirms that the import statement in components/domains/autorenewal.tsx is valid and correctly implemented.

  • Location of Definition: components/UI/notification.tsx
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the component definition for `Notification`.

# Test: Search for the component definition. Expect: Only occurrences of the new component definition.
rg --type javascript -A 5 $'const Notification'

Length of output: 83


Script:

#!/bin/bash
# Description: Verify the component definition for `Notification`.

# Test: Search for the component definition. Expect: Only occurrences of the new component definition.
rg --type js -A 5 $'const Notification'

Length of output: 40


Script:

#!/bin/bash
# Description: Verify the component definition for `Notification` in various file types and patterns.

# Test: Search for the component definition using different patterns and file types.
rg --type-add 'jsx:*.{jsx,tsx}' --type jsx -A 5 $'(const|function|class) Notification'

Length of output: 407


19-20: Update imports for getApprovalAmount and getTotalYearlyPrice.

The imports have been updated to reflect the new functions. Ensure these functions are correctly implemented in ../../utils/priceService.

Run the following script to verify the function definitions:

Verification successful

Function Implementations Verified for getApprovalAmount and getTotalYearlyPrice.

The functions are correctly implemented in utils/priceService.ts. Ensure that the imports in components/domains/autorenewal.tsx are from this file.

  • getApprovalAmount is defined in utils/priceService.ts.
  • getTotalYearlyPrice is defined in utils/priceService.ts and utils/altcoinService.ts.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the function definitions for `getApprovalAmount` and `getTotalYearlyPrice`.

# Test: Search for the function definitions. Expect: Only occurrences of the new function definitions.
rg --type python -A 5 $'def getApprovalAmount'
rg --type python -A 5 $'def getTotalYearlyPrice'

Length of output: 159


Script:

#!/bin/bash
# Description: Verify the function definitions for `getApprovalAmount` and `getTotalYearlyPrice` in TypeScript files.

# Test: Search for the function definitions. Expect: Only occurrences of the new function definitions.
rg --type typescript -A 5 $'function getApprovalAmount'
rg --type typescript -A 5 $'function getTotalYearlyPrice'

Length of output: 185


Script:

#!/bin/bash
# Description: Verify the function definitions for `getApprovalAmount` and `getTotalYearlyPrice` in .ts files.

# Test: Search for the function definitions. Expect: Only occurrences of the new function definitions.
rg -t ts -A 5 $'function getApprovalAmount'
rg -t ts -A 5 $'function getTotalYearlyPrice'

Length of output: 1054


46-46: Update import for useNeedAllowances.

The import has been updated to reflect the new hook name. Ensure the hook is correctly implemented in @/hooks/useNeedAllowances.

Run the following script to verify the hook definition:

components/discount/freeRenewalCheckout.tsx (8)

55-55: Consolidate props into offer object.

The props have been consolidated into a single offer object of type Discount, simplifying the component's interface.

The code changes are approved.


67-67: Update state management for salesTaxAmount.

The state variable salesTaxAmount has been updated to use bigint for enhanced precision in financial calculations.

The code changes are approved.


79-81: Update state management for potentialPrice.

The state variable potentialPrice has been updated to use bigint for enhanced precision in financial calculations.

The code changes are approved.


237-242: Update sales tax calculation logic.

The logic for setting salesTaxAmount has been updated to use bigint and applyRateToBigInt.

The code changes are approved.


263-275: Update approval amount calculation logic.

The logic for calculating amountToApprove has been updated to use getApprovalAmount.

The code changes are approved.


399-399: Update prop for RegisterSummary.

The prop priceInEth has been updated to use potentialPrice.

The code changes are approved.


47-47: Update import for getTotalYearlyPrice.

The import has been updated to reflect the new function name. Ensure the function is correctly implemented in ../../utils/altcoinService.

Run the following script to verify the function definition:


21-22: Update imports for getApprovalAmount and getDisplayablePrice.

The imports have been updated to reflect the new functions. Ensure these functions are correctly implemented in ../../utils/priceService.

Run the following script to verify the function definitions:

Verification successful

Verification Successful: Imports and Function Definitions are Correct

The functions getApprovalAmount and getDisplayablePrice are correctly defined in utils/priceService.ts and properly imported in freeRenewalCheckout.tsx. No issues were found with the imports or the function definitions.

  • getApprovalAmount is defined and exported in utils/priceService.ts.
  • getDisplayablePrice is defined and exported in utils/priceService.ts.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the function definitions for `getApprovalAmount` and `getDisplayablePrice`.

# Test: Search for the function definitions. Expect: Only occurrences of the new function definitions.
rg --type python -A 5 $'def getApprovalAmount'
rg --type python -A 5 $'def getDisplayablePrice'

Length of output: 159


Script:

#!/bin/bash
# Description: Verify the function definitions for `getApprovalAmount` and `getDisplayablePrice` in TypeScript files.

# Test: Search for the function definitions in TypeScript files.
ast-grep --lang typescript --pattern 'function getApprovalAmount($_) { $$$ }'
ast-grep --lang typescript --pattern 'function getDisplayablePrice($_) { $$$ }'

Length of output: 161


Script:

#!/bin/bash
# Description: Search for the function names `getApprovalAmount` and `getDisplayablePrice` in TypeScript files to capture different declaration styles.

# Test: Search for the function names in TypeScript files.
rg 'getApprovalAmount' --type ts -A 3
rg 'getDisplayablePrice' --type ts -A 3

Length of output: 5041

components/domains/steps/checkoutCard.tsx (16)

78-84: Update state management for monetary values.

The state variables dailyPriceInEth, discountedPriceInEth, price, discountedPrice, and maxPriceRange have been updated to use bigint for enhanced precision in financial calculations.

The code changes are approved.


87-87: Update state management for salesTaxAmount.

The state variable salesTaxAmount has been updated to use bigint for enhanced precision in financial calculations.

The code changes are approved.


116-116: Add state management for currencyError.

The state variable currencyError has been added to handle errors related to currency fetching.

The code changes are approved.


125-134: Add logic for handling token quotes.

The logic for setting quoteData and handling errors has been added to provide user feedback regarding token quotes.

The code changes are approved.


168-176: Update price calculation logic.

The logic for updating dailyPriceInEth and discountedPriceInEth has been updated to use getManyDomainsPriceWei.

The code changes are approved.


179-193: Update discounted price calculation logic.

The logic for updating discountedPrice has been updated to use bigint and getDomainPriceAltcoin.

The code changes are approved.


Line range hint 237-277: Update sales tax calculation logic.

The logic for setting salesTaxAmount has been updated to use bigint and applyRateToBigInt.

The code changes are approved.


Line range hint 292-307: Update price calculation logic.

The logic for updating price has been updated to use getManyDomainsPriceWei and getDomainPriceAltcoin.

The code changes are approved.


322-322: Update dependencies for useEffect.

The dependencies for the useEffect hook have been updated to include allowanceStatus.

The code changes are approved.


434-445: Update approval amount calculation logic.

The logic for calculating amountToApprove has been updated to use getApprovalAmount.

The code changes are approved.


569-578: Update approval amount calculation logic.

The logic for calculating amountToApprove has been updated to use getApprovalAmount.

The code changes are approved.


Line range hint 794-807: Add ReduceDuration component for handling reduced duration.

The ReduceDuration component has been added to handle cases where the user's balance is insufficient for the selected duration.

The code changes are approved.


Line range hint 819-836: Update prop for RegisterSummary.

The props priceInEth, price, durationInYears, discountedPrice, and discountedPriceInEth have been updated to use the new state variables.

The code changes are approved.


880-885: Add Notification component for currency errors.

The Notification component has been added to provide user feedback regarding currency errors.

The code changes are approved.


62-62: Add import for Notification component.

The Notification component is imported to provide user feedback. Ensure the component is correctly implemented in ../UI/notification.

Run the following script to verify the component definition:

Verification successful

Notification component is correctly implemented and imported.

The Notification component is defined in components/UI/notification.tsx as expected. The import statement in checkoutCard.tsx should function correctly. No further action is required.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the component definition for `Notification`.

# Test: Search for the component definition. Expect: Only occurrences of the new component definition.
rg --type javascript -A 5 $'const Notification'

Length of output: 83


Script:

#!/bin/bash
# Description: Verify the component definition for `Notification`.

# Test: Search for the component definition. Expect: Only occurrences of the new component definition.
rg --type typescript -A 5 $'const Notification'

Length of output: 83


Script:

#!/bin/bash
# Description: Verify the component definition for `Notification`.

# Test: Search for the component definition. Expect: Only occurrences of the new component definition.
rg -A 5 $'const Notification'

Length of output: 350


33-39: Update imports for pricing functions.

The imports have been updated to reflect the new functions. Ensure these functions are correctly implemented in @/utils/altcoinService and @/utils/priceService.

Run the following script to verify the function definitions:

Verification successful

Functions Verified in Updated Imports

The functions getYearlyPrice, smartCurrencyChoosing, getApprovalAmount, and getManyDomainsPriceWei are correctly implemented in the respective files altcoinService.ts and priceService.ts. The search results confirm their presence and correct implementation.

  • Verified Functions:
    • getYearlyPrice in utils/altcoinService.ts and utils/priceService.ts
    • smartCurrencyChoosing in utils/altcoinService.ts
    • getApprovalAmount in utils/priceService.ts
    • getManyDomainsPriceWei in utils/priceService.ts

These functions are correctly imported and implemented, aligning with the changes in the checkoutCard.tsx file.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the function definitions for `getAutoRenewAllowance`, `getDomainPriceAltcoin`, `getPriceForDuration`, `getTokenQuote`, `getYearlyPrice`, `smartCurrencyChoosing`, `getApprovalAmount`, and `getManyDomainsPriceWei`.

# Test: Search for the function definitions. Expect: Only occurrences of the new function definitions.
rg --type python -A 5 $'def getAutoRenewAllowance'
rg --type python -A 5 $'def getDomainPriceAltcoin'
rg --type python -A 5 $'def getPriceForDuration'
rg --type python -A 5 $'def getTokenQuote'
rg --type python -A 5 $'def getYearlyPrice'
rg --type python -A 5 $'def smartCurrencyChoosing'
rg --type python -A 5 $'def getApprovalAmount'
rg --type python -A 5 $'def getManyDomainsPriceWei'

Length of output: 643


Script:

#!/bin/bash
# Description: Verify the function definitions for `getAutoRenewAllowance`, `getDomainPriceAltcoin`, `getPriceForDuration`, `getTokenQuote`, `getYearlyPrice`, `smartCurrencyChoosing`, `getApprovalAmount`, and `getManyDomainsPriceWei`.

# Test: Search for the function definitions. Expect: Only occurrences of the new function definitions.
rg --type js -A 5 $'function getAutoRenewAllowance'
rg --type js -A 5 $'function getDomainPriceAltcoin'
rg --type js -A 5 $'function getPriceForDuration'
rg --type js -A 5 $'function getTokenQuote'
rg --type js -A 5 $'function getYearlyPrice'
rg --type js -A 5 $'function smartCurrencyChoosing'
rg --type js -A 5 $'function getApprovalAmount'
rg --type js -A 5 $'function getManyDomainsPriceWei'

Length of output: 403


Script:

#!/bin/bash
# Description: Verify the function definitions for various formats in JavaScript/TypeScript files.

# Test: Search for the function definitions, including arrow functions and exports.
rg --type js --type ts -A 5 $'const getAutoRenewAllowance ='
rg --type js --type ts -A 5 $'const getDomainPriceAltcoin ='
rg --type js --type ts -A 5 $'const getPriceForDuration ='
rg --type js --type ts -A 5 $'const getTokenQuote ='
rg --type js --type ts -A 5 $'const getYearlyPrice ='
rg --type js --type ts -A 5 $'const smartCurrencyChoosing ='
rg --type js --type ts -A 5 $'const getApprovalAmount ='
rg --type js --type ts -A 5 $'const getManyDomainsPriceWei ='
rg --type js --type ts -A 5 $'export function getAutoRenewAllowance'
rg --type js --type ts -A 5 $'export function getDomainPriceAltcoin'
rg --type js --type ts -A 5 $'export function getPriceForDuration'
rg --type js --type ts -A 5 $'export function getTokenQuote'
rg --type js --type ts -A 5 $'export function getYearlyPrice'
rg --type js --type ts -A 5 $'export function smartCurrencyChoosing'
rg --type js --type ts -A 5 $'export function getApprovalAmount'
rg --type js --type ts -A 5 $'export function getManyDomainsPriceWei'

Length of output: 4026

hooks/useNeedSubscription.tsx Outdated Show resolved Hide resolved
utils/feltService.ts Outdated Show resolved Hide resolved
components/discount/registerDiscount.tsx Show resolved Hide resolved
Copy link
Collaborator

@Marchand-Nicolas Marchand-Nicolas left a comment

Choose a reason for hiding this comment

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

I got a bug on the domain purchase page (because a request fails (maybe would not happen in prod, but anyway fixing it could prevent future problems) I think and the error is not well handled).
Also put some comments for code cleaning.

if (
erc20AllowanceError ||
(erc20AllowanceRes &&
erc20AllowanceRes["remaining"].low !== UINT_128_MAX &&
erc20AllowanceRes["remaining"].high !== UINT_128_MAX)
) {
setNeedsAllowance(true);
setCurrentAllowance(BigInt(erc20AllowanceRes["remaining"].low));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ici après 10 secondes sur la page d'achat de domaine j'ai TypeError: Cannot read properties of undefined (reading 'remaining').

Selon moi cela provient du fait que erc20AllowanceRes n'est pas forcément définit vu que la condition est justement erc20AllowanceError || ....

Je pense que le mieux est de mettre:

if (erc20AllowanceRes)  setCurrentAllowance(BigInt(erc20AllowanceRes["remaining"].low));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Si tu check tu as bien erc20AllowanceRes dans les conditions.

    if (
      erc20AllowanceError ||
      (erc20AllowanceRes &&
        erc20AllowanceRes["remaining"].low !== UINT_128_MAX &&
        erc20AllowanceRes["remaining"].high !== UINT_128_MAX)
    )
    ```

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sauf si erc20AllowanceError est vrai justemebt

Copy link
Collaborator

Choose a reason for hiding this comment

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

Et c’est spécifiquement quand erc20AllowanceError est vrai que erc20AllowanceRes est indéfini et que cela engendre une erreur

components/discount/freeRenewalCheckout.tsx Outdated Show resolved Hide resolved
context/FormProvider.tsx Outdated Show resolved Hide resolved
hooks/useNeedAllowances.tsx Outdated Show resolved Hide resolved
pages/argent.tsx Outdated Show resolved Hide resolved
@Marchand-Nicolas Marchand-Nicolas added ❌ Change request Change requested from reviewer and removed 🔥 Ready for review This pull request needs a review labels Aug 30, 2024
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: 16

Outside diff range, codebase verification and nitpick comments (2)
utils/subscriptionService.ts (1)

18-32: Simplification of Subscription Data Processing

The processSubscriptionData function has been simplified to focus solely on Ethereum subscriptions. This change streamlines the function by removing the need to iterate through various currency types and simplifies the logic significantly.

While this simplification may improve performance and maintainability, it's important to consider the impact on users with altcoin subscriptions. If the system previously supported multiple subscription types, ensure that this change aligns with strategic goals and does not inadvertently exclude important functionalities.

Consider adding additional checks or fallbacks for altcoin subscriptions if they are still relevant to the business model.

components/domains/steps/checkoutCard.tsx (1)

Line range hint 151-223: Complex but necessary useEffect for handling transaction effects.

This useEffect handles the effects of transactions effectively, including tracking analytics events, registering metadata, and managing email subscriptions. The use of external API calls and analytics tracking is well-implemented. Consider adding error handling for the API calls to manage potential failures gracefully and ensure the user is informed of any issues.

Add error handling for the external API calls used in this useEffect to enhance robustness and ensure the user is informed of any issues.

Tools
Biome

[error] 213-213: Unnecessary use of boolean literals in conditional expression.

Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with

(lint/complexity/noUselessTernary)

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 7ad9073 and d7004a4.

Files selected for processing (13)
  • components/discount/freeRenewalCheckout.tsx (12 hunks)
  • components/domains/steps/checkoutCard.tsx (10 hunks)
  • context/FormProvider.tsx (7 hunks)
  • hooks/checkout/useCheckoutState.tsx (1 hunks)
  • hooks/checkout/useCurrencyManagement.tsx (1 hunks)
  • hooks/checkout/usePriceManagement.tsx (1 hunks)
  • hooks/checkout/useRegisterTxPrep.tsx (1 hunks)
  • hooks/checkout/useRenewalTxPrep.tsx (1 hunks)
  • hooks/useNeedAllowances.tsx (2 hunks)
  • pages/argent.tsx (1 hunks)
  • tests/utils/subscriptionService.test.js (2 hunks)
  • utils/feltService.ts (1 hunks)
  • utils/subscriptionService.ts (2 hunks)
Files skipped from review as they are similar to previous changes (2)
  • components/discount/freeRenewalCheckout.tsx
  • hooks/useNeedAllowances.tsx
Additional comments not posted (9)
pages/argent.tsx (1)

1-12: Simplification of the Argent Component

The Argent component has been simplified to directly render the DiscountEndScreen with a fixed message and image. This change removes any dynamic behavior related to user interactions and discount status checks.

If this change is due to the permanent discontinuation of the discount feature, it's appropriate. However, it would be beneficial to add a comment or documentation within the code to clarify this change for future maintainers or developers.

context/FormProvider.tsx (1)

Line range hint 7-117: General approval with a suggestion for dependency array review.

The changes in FormProvider are well-aligned with the PR objectives, particularly the renaming of duration to durationInYears for clarity. The use of useCallback and useMemo is appropriate for optimizing performance. However, ensure that all dependency arrays are correctly specified to avoid issues with stale closures or unnecessary executions.

Consider reviewing the following dependency arrays for correctness:

  • useMemo for metadata handling.
  • useCallback for updateFormState and clearForm.

This review ensures that the functions behave as expected without causing performance issues or bugs due to missing dependencies.

tests/utils/subscriptionService.test.js (2)

Line range hint 4-39: Test suite for getNonSubscribedDomains is comprehensive and well-implemented.

The test cases cover various scenarios effectively, ensuring that the function behaves as expected across different input configurations.


Line range hint 41-69: Test suite for fullIdsToDomains is well-structured and effective.

The test cases adequately validate the function's ability to correctly identify and return Stark root domains, ensuring robust functionality.

hooks/checkout/usePriceManagement.tsx (3)

1-11: Well-structured hook with appropriate imports.

The usePriceManagement hook is well-structured and includes all necessary imports for managing price-related logic in the application. The use of bigint for monetary values aligns with the PR's objectives to handle financial calculations more precisely.


13-41: Appropriate initial state setup and function parameters.

The usePriceManagement function is defined with a comprehensive set of parameters that are essential for managing pricing logic. The initial state setup using bigint for all monetary values is appropriate and aligns with the PR's objectives for precise financial calculations.


200-203: Simple and effective logging for sales tax amount changes.

This useEffect provides a straightforward logging mechanism for tracking changes in the sales tax amount. This is useful for debugging and monitoring purposes. The implementation is simple and effective.

components/domains/steps/checkoutCard.tsx (2)

Line range hint 1-36: Well-structured component with appropriate imports.

The CheckoutCard component is well-structured and includes all necessary imports for managing various aspects of the domain checkout process. The imports are appropriate for the required functionalities, including price calculations, currency management, and transaction preparations.


25-36: Appropriate initial state setup and component props.

The CheckoutCard component is defined with well-structured props to handle different forms and discounts in the domain checkout process. The initial state setup using hooks for managing call data and transaction execution is appropriate and aligns with the component's responsibilities.

utils/feltService.ts Show resolved Hide resolved
utils/feltService.ts Show resolved Hide resolved
Comment on lines 5 to 63
export const useCurrencyManagement = () => {
const [displayedCurrency, setDisplayedCurrency] = useState<CurrencyType>(
CurrencyType.ETH
);
const [quoteData, setQuoteData] = useState<QuoteQueryData | null>(null);
const [currencyError, setCurrencyError] = useState<boolean>(false);
const [hasChosenCurrency, setHasChosenCurrency] = useState<boolean>(false);

const onCurrencySwitch = (currency: CurrencyType) => {
setDisplayedCurrency(currency);
setHasChosenCurrency(true);
};

// we fetch the quote for the currency selected
useEffect(() => {
const fetchQuote = async () => {
if (displayedCurrency === CurrencyType.ETH) {
setQuoteData(null);
return;
}
const data = await getTokenQuote(ERC20Contract[displayedCurrency]);
if (data) {
setQuoteData(data);
setCurrencyError(false);
} else {
setDisplayedCurrency(CurrencyType.ETH);
setCurrencyError(true);
}
};

const scheduleRefetch = () => {
const now = parseInt((new Date().getTime() / 1000).toFixed(0));
const timeLimit = now - 60; // 60 seconds
if (!quoteData || displayedCurrency === CurrencyType.ETH) {
setQuoteData(null);
return;
}

if (quoteData.max_quote_validity <= timeLimit) {
fetchQuote();
}

const timeUntilNextCheck = quoteData.max_quote_validity - timeLimit;
setTimeout(scheduleRefetch, Math.max(15000, timeUntilNextCheck * 100));
};

fetchQuote();
scheduleRefetch();
}, [displayedCurrency, quoteData]);

return {
displayedCurrency,
quoteData,
currencyError,
hasChosenCurrency,
onCurrencySwitch,
setHasChosenCurrency,
setCurrencyError,
};
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 30, 2024

Choose a reason for hiding this comment

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

General approval with a suggestion for improvement.

The useCurrencyManagement hook is well-structured and handles state and side effects appropriately. However, consider clearing the timeout set in scheduleRefetch to prevent potential memory leaks. This can be done by returning a cleanup function from the useEffect that clears the timeout.

Consider adding the following cleanup logic:

useEffect(() => {
  let timeoutId;
  const scheduleRefetch = () => {
    // existing logic
    timeoutId = setTimeout(scheduleRefetch, Math.max(15000, timeUntilNextCheck * 100));
  };

  // existing fetch logic

  return () => clearTimeout(timeoutId);
}, [displayedCurrency, quoteData]);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Rabbit a raison, en effet comme scheduleRefetch s'appelle elle même avec un timeout, si tu ne return pas dans ton useEffect de clearTimeout, alors à chaque reefresh du useEffect tu vas accumuler une boucle qui va tourner en parallèle pour rien. #874 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

@Marchand-Nicolas, merci pour la confirmation! Vous avez tout à fait raison, sans le clearTimeout dans le useEffect, on risque d'accumuler des boucles parallèles inutiles à chaque rafraîchissement du useEffect. C'est une bonne pratique d'ajouter cette fonction de nettoyage pour éviter les fuites de mémoire et les exécutions parallèles non désirées.

hooks/checkout/useCheckoutState.tsx Show resolved Hide resolved
tests/utils/subscriptionService.test.js Show resolved Hide resolved
hooks/checkout/usePriceManagement.tsx Show resolved Hide resolved
hooks/checkout/usePriceManagement.tsx Show resolved Hide resolved
hooks/checkout/usePriceManagement.tsx Show resolved Hide resolved
components/domains/steps/checkoutCard.tsx Show resolved Hide resolved
components/domains/steps/checkoutCard.tsx Show resolved Hide resolved
@Marchand-Nicolas Marchand-Nicolas self-requested a review August 31, 2024 10:26
Copy link
Collaborator

@Marchand-Nicolas Marchand-Nicolas left a comment

Choose a reason for hiding this comment

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

J'ai un bug quand je veux switch de currency, et ya une opti à faire pour éviter de cumuler les appels récursifs lors du fetch des prix selon moi

hooks/checkout/useCurrencyManagement.tsx Show resolved Hide resolved
Comment on lines 5 to 63
export const useCurrencyManagement = () => {
const [displayedCurrency, setDisplayedCurrency] = useState<CurrencyType>(
CurrencyType.ETH
);
const [quoteData, setQuoteData] = useState<QuoteQueryData | null>(null);
const [currencyError, setCurrencyError] = useState<boolean>(false);
const [hasChosenCurrency, setHasChosenCurrency] = useState<boolean>(false);

const onCurrencySwitch = (currency: CurrencyType) => {
setDisplayedCurrency(currency);
setHasChosenCurrency(true);
};

// we fetch the quote for the currency selected
useEffect(() => {
const fetchQuote = async () => {
if (displayedCurrency === CurrencyType.ETH) {
setQuoteData(null);
return;
}
const data = await getTokenQuote(ERC20Contract[displayedCurrency]);
if (data) {
setQuoteData(data);
setCurrencyError(false);
} else {
setDisplayedCurrency(CurrencyType.ETH);
setCurrencyError(true);
}
};

const scheduleRefetch = () => {
const now = parseInt((new Date().getTime() / 1000).toFixed(0));
const timeLimit = now - 60; // 60 seconds
if (!quoteData || displayedCurrency === CurrencyType.ETH) {
setQuoteData(null);
return;
}

if (quoteData.max_quote_validity <= timeLimit) {
fetchQuote();
}

const timeUntilNextCheck = quoteData.max_quote_validity - timeLimit;
setTimeout(scheduleRefetch, Math.max(15000, timeUntilNextCheck * 100));
};

fetchQuote();
scheduleRefetch();
}, [displayedCurrency, quoteData]);

return {
displayedCurrency,
quoteData,
currencyError,
hasChosenCurrency,
onCurrencySwitch,
setHasChosenCurrency,
setCurrencyError,
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rabbit a raison, en effet comme scheduleRefetch s'appelle elle même avec un timeout, si tu ne return pas dans ton useEffect de clearTimeout, alors à chaque reefresh du useEffect tu vas accumuler une boucle qui va tourner en parallèle pour rien. #874 (comment)

@Marchand-Nicolas Marchand-Nicolas added ❌ Change request Change requested from reviewer and removed 🔥 Ready for review This pull request needs a review labels Aug 31, 2024
@fricoben fricoben added 🔥 Ready for review This pull request needs a review and removed ❌ Change request Change requested from reviewer labels Sep 2, 2024
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

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between d7004a4 and 1f15c04.

Files selected for processing (1)
  • hooks/checkout/useCurrencyManagement.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • hooks/checkout/useCurrencyManagement.tsx

Copy link
Member

@Th0rgal Th0rgal left a comment

Choose a reason for hiding this comment

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

on the frontend it lgtm

Copy link
Collaborator

@Marchand-Nicolas Marchand-Nicolas left a comment

Choose a reason for hiding this comment

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

Lgtm code good and working fine

@Th0rgal Th0rgal merged commit 5eb1ee2 into testnet Sep 3, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔥 Ready for review This pull request needs a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants