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.

Sign up for GitHub

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

Jump to bottom

fix(trading): show offers by redirect url params #17566

Merged
merged 1 commit into from
Mar 20, 2025
Merged

fix(trading): show offers by redirect url params #17566

merged 1 commit into from
Mar 20, 2025

Conversation

FreeWall
Copy link
Member

@FreeWall FreeWall commented Mar 11, 2025 •
edited
Loading

@FreeWall FreeWall added bug Something isn't working as expected +Invity Related to Invity project labels Mar 11, 2025
@FreeWall FreeWall requested a review from adderpositive March 11, 2025 12:53
adderpositive
adderpositive approved these changes Mar 11, 2025
View reviewed changes
Copy link
Contributor

@adderpositive adderpositive left a comment

Choose a reason for hiding this comment

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

Looks good! 👌

@FreeWall FreeWall force-pushed the fix/trading-redirect-offers branch 3 times, most recently from 6195bac to 421101d Compare March 12, 2025 11:04
@FreeWall FreeWall marked this pull request as ready for review March 12, 2025 11:27
@FreeWall FreeWall requested a review from tomasklim March 12, 2025 11:28
@coderabbitai coderabbitai
Copy link

coderabbitai bot commented Mar 12, 2025

Walkthrough

The changes introduce new hooks for handling form defaults during redirected trading operations. Specifically, the useTradingBuyForm and useTradingSellForm hooks now utilize newly added hooks—useTradingBuyFormRedirectValues and useTradingSellFormRedirectValues—to determine default values when a form is accessed via a redirect. The modifications include incorporating a paymentMethod property into the data structures and URL hash generation for both buy and sell functionalities. Interface definitions in the redirect logic have been updated and renamed to distinguish between buy and sell operations. Additionally, utility functions and their corresponding tests have been adjusted to expect the inclusion of paymentMethod in quote request URLs, ensuring that the generated links correctly reflect the updated structure.

📜 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 8a3168c and 421101d.

📒 Files selected for processing (12)
  • packages/suite/src/hooks/wallet/trading/form/useTradingBuyForm.tsx (4 hunks)
  • packages/suite/src/hooks/wallet/trading/form/useTradingBuyFormRedirectValues.tsx (1 hunks)
  • packages/suite/src/hooks/wallet/trading/form/useTradingSellForm.ts (4 hunks)
  • packages/suite/src/hooks/wallet/trading/form/useTradingSellFormRedirectValues.tsx (1 hunks)
  • packages/suite/src/hooks/wallet/useTradingRedirect.ts (11 hunks)
  • packages/suite/src/utils/wallet/trading/__fixtures__/buyUtils.ts (2 hunks)
  • packages/suite/src/utils/wallet/trading/__fixtures__/sellUtils.ts (2 hunks)
  • packages/suite/src/utils/wallet/trading/__tests__/buyUtils.test.ts (1 hunks)
  • packages/suite/src/utils/wallet/trading/__tests__/sellUtils.test.ts (2 hunks)
  • packages/suite/src/utils/wallet/trading/buyUtils.ts (1 hunks)
  • packages/suite/src/utils/wallet/trading/sellUtils.ts (1 hunks)
  • packages/suite/src/views/wallet/trading/redirect/TradingRedirect.tsx (5 hunks)
🔇 Additional comments (44)
packages/suite/src/utils/wallet/trading/__fixtures__/buyUtils.ts (2)

11-11: Good addition of paymentMethod property

The addition of the paymentMethod property to QUOTE_REQUEST_FIAT is consistent with the changes to support payment method in URL parameters.

20-20: Good addition of paymentMethod property

The addition of the paymentMethod property to QUOTE_REQUEST_CRYPTO properly maintains consistency across quote request objects.

packages/suite/src/hooks/wallet/trading/form/useTradingSellForm.ts (4)

44-44: Good import of the new redirect values hook

Correctly importing the newly created hook to handle form values from URL parameters.

126-126: Good implementation of redirect values handling

The hook is properly initialized with the required parameters to extract form values from URL parameters when the form is accessed via redirect.

156-156: Good update to form's default values priority

The form initialization now correctly prioritizes values from URL parameters (redirectValues) over draft values, which helps maintain the expected behavior when users are redirected with specific parameters.

359-361: Good update to include paymentMethod in quote link

The createQuoteLink function call now correctly includes the payment method, ensuring that generated URLs will preserve this information when users are redirected.

packages/suite/src/utils/wallet/trading/__fixtures__/sellUtils.ts (2)

11-11: Good addition of paymentMethod property

The addition of the paymentMethod property to QUOTE_REQUEST_FIAT maintains consistency with the changes across the trading functionality.

20-20: Good addition of paymentMethod property

The addition of the paymentMethod property to QUOTE_REQUEST_CRYPTO completes the consistent implementation across both buy and sell fixtures.

packages/suite/src/hooks/wallet/trading/form/useTradingBuyForm.tsx (5)

39-39: Good import of the new redirect values hook

Correctly importing the newly created hook for handling buy form values from URL parameters.

85-85: Good implementation of redirect values handling

The hook is properly initialized to extract form values from URL parameters when redirected.

106-107: Good update to form's default values priority

The form now correctly prioritizes values from URL redirects, maintaining the expected behavior when links with specific parameters are used.

110-112: Good update to previousValues reference

The condition to initialize previousValues now correctly checks for isFromRedirect to avoid using stale draft data when the form is accessed via a redirect URL.

163-166: Good update to include paymentMethod in quote link

The createQuoteLink function call now correctly includes the payment method in the generated URL, maintaining consistency in the redirect URL structure.

packages/suite/src/utils/wallet/trading/__tests__/buyUtils.test.ts (2)

20-20: Update to expected URL in test for fiat quote request

The test has been updated to expect the payment method (creditCard) as part of the generated URL. This matches the implementation changes in the createQuoteLink function.

24-24: Update to expected URL in test for crypto quote request

The test now expects the payment method (creditCard) to be included in the generated URL for crypto quote requests as well, ensuring consistency with the updated implementation.

packages/suite/src/utils/wallet/trading/buyUtils.ts (2)

14-14: Payment method added to crypto quote URL hash

The paymentMethod parameter is now included in the URL hash for crypto quote requests, enabling redirect URLs to specify which payment method should be pre-selected in the offers page.

16-16: Payment method added to fiat quote URL hash

Similar to the crypto quote change, the paymentMethod parameter is now included in the URL hash for fiat quote requests, maintaining consistency between both types of quotes.

packages/suite/src/hooks/wallet/trading/form/useTradingBuyFormRedirectValues.tsx (3)

1-6: New hook for handling redirect values in buy form

This hook implementation properly imports the necessary dependencies for working with the buy form and trading parameters.

7-12: Hook function signature with proper types

The hook takes appropriate parameters to determine if values are from a redirect and includes the quote request data. The return type is correctly specified as potentially returning null when not applicable.

13-25: Logic for extracting form values from quote request

The implementation properly extracts values from the quote request when appropriate conditions are met. The payment method is correctly handled with a conditional check to ensure it exists before attempting to use it.

I particularly like the early return pattern using the ternary operator, which makes the code clean and easy to follow.

packages/suite/src/utils/wallet/trading/sellUtils.ts (2)

73-73: Payment method added to crypto sell quote URL hash

The paymentMethod parameter is now included in the URL hash for crypto-based sell quotes, enabling redirects to pre-select the payment method in the sell offers page.

75-75: Payment method added to fiat sell quote URL hash

Similarly, the paymentMethod parameter is now included in the URL hash for fiat-based sell quotes, maintaining consistency between both types of quotes.

packages/suite/src/hooks/wallet/trading/form/useTradingSellFormRedirectValues.tsx (1)

16-65: Well-structured hook implementation for handling redirect values.

The implementation correctly manages the logic for setting default values based on URL parameters for the trading sell form. The hook properly leverages useMemo to optimize for performance when computing derived values.

A couple of observations:

  • The hook properly handles the newly added paymentMethod parameter, formatting it for use in the form
  • Good validation with the null return when preconditions aren't met
packages/suite/src/utils/wallet/trading/__tests__/sellUtils.test.ts (3)

104-105: Tests updated to include the payment method parameter.

The test case has been properly updated to include the "creditCard" payment method in the URL. This aligns with the implementation changes in the sell utils.

109-110: Tests updated to include the payment method parameter.

The test case has been properly updated to include the "creditCard" payment method in the URL. This aligns with the implementation changes in the sell utils.

119-120: Tests updated to include the payment method parameter.

The test case has been properly updated to include the "creditCard" payment method in the URL for the scenario that includes orderId. This aligns with the implementation changes in the sell utils.

packages/suite/src/views/wallet/trading/redirect/TradingRedirect.tsx (8)

3-3: Import updated to include payment method types.

The import has been correctly updated to include the payment method types that will be used in the redirect logic.

25-30: Renamed function references to be more explicit.

Good change to rename the functions to explicitly differentiate between buy and sell operations. This improves code clarity and maintainability.

39-45: Route type updated to include sell-detail.

This change properly expands the route types to include 'sell-detail', which is necessary for the enhanced redirect functionality.

51-59: Buy offers redirect updated to include payment method.

The redirect logic now correctly includes the payment method parameter from the URL, properly typed as BuyCryptoPaymentMethod.

62-64: Buy detail redirect handler added.

New handler added for the 'detail' route type, which calls redirectToBuyDetail. This is a necessary addition to support the expanded functionality.

67-74: Fee index updated to accommodate payment method parameter.

The fee index has been correctly incremented from 9 to 10 to account for the new payment method parameter. The orderId extraction has also been updated accordingly.

82-82: Sell offers redirect updated to include payment method.

The payment method parameter is now correctly included in the redirect to sell offers, typed as SellCryptoPaymentMethod.

104-106: Updated dependency array with renamed functions.

The useEffect dependency array has been properly updated to include the renamed redirect functions.

packages/suite/src/hooks/wallet/useTradingRedirect.ts (10)

2-2: Import updated to include BuyCryptoPaymentMethod.

The import has been correctly updated to include the BuyCryptoPaymentMethod type.

6-6: Import updated to include SellCryptoPaymentMethod.

The import has been correctly updated to include the SellCryptoPaymentMethod type.

20-30: Interface renamed and updated to include payment method.

The interface has been correctly renamed from OfferRedirectParams to BuyOfferRedirectParams for clarity, and updated to include the paymentMethod property.

41-41: SellOfferRedirectParams updated to include payment method.

The SellOfferRedirectParams interface has been correctly updated to include the paymentMethod property of type SellCryptoPaymentMethod.

71-106: Function renamed and updated to include payment method.

The function has been correctly renamed from redirectToOffers to redirectToBuyOffers for clarity, and updated to include the paymentMethod in the commonParams.

118-118: Payment method extraction added to redirectToSellOffers.

The payment method parameter is now correctly extracted from the params object.

125-125: Common params updated to include payment method.

The commonParams object in redirectToSellOffers now correctly includes the paymentMethod property.

150-150: Route selection logic updated.

The logic to determine the route (either 'wallet-trading-sell-confirm' or 'wallet-trading-sell-offers') based on the presence of orderId is correctly implemented.

191-191: Function renamed for clarity.

The function has been correctly renamed from redirectToDetail to redirectToBuyDetail for clarity and consistency.

207-208: Return object updated with renamed functions.

The return object of the useTradingRedirect hook has been correctly updated to include the renamed functions.

✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ 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.
  • @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.

@tomasklim
Copy link
Member

Does it work with passphrase wallets?

@FreeWall
Copy link
Member Author

Does it work with passphrase wallets?

Yes, remember wallet is enabled before redirect

@tomasklim
Copy link
Member

What if there is more remembered wallets, does it redirect to correct one?

@FreeWall
Copy link
Member Author

What if there is more remembered wallets, does it redirect to correct one?

Yes, I just tested it with two different passphrase wallets.

@FreeWall FreeWall force-pushed the fix/trading-redirect-offers branch from 421101d to 197b007 Compare March 20, 2025 11:47
@tomasklim tomasklim merged commit 533e190 into develop Mar 20, 2025
34 checks passed
@tomasklim tomasklim deleted the fix/trading-redirect-offers branch March 20, 2025 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adderpositive adderpositive adderpositive approved these changes

@tomasklim tomasklim Awaiting requested review from tomasklim

Assignees
No one assigned
Labels
bug Something isn't working as expected +Invity Related to Invity project
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Trading - sell can't continue after redirected back to suite
3 participants
@FreeWall @tomasklim @adderpositive