feat(trading): otc market banner #17202
Conversation
adderpositive
added
feature
packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormOfferOTC.tsx
Outdated
Show resolved
Hide resolved
adderpositive
force-pushed
the
feat/trading-otc
branch
from
e8a26b4 to
5ba38ab
Compare
|
🚀 Expo preview is ready!
|
adderpositive
force-pushed
the
feat/trading-otc
branch
from
5ba38ab to
def4814
Compare
packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormOfferOTC.tsx
Outdated
Show resolved
Hide resolved
| @@ -1420,6 +1420,18 @@ export default defineMessages({ | |||
| defaultMessage: 'View details', | |||
| id: 'TR_TRADING_VIEW_DETAILS', | |||
| }, | |||
| TR_TRADING_OTC_INFO: { | |||
| defaultMessage: 'For purchases over USD (EUR) 50,000, consider our OTC partner:', | |||
There was a problem hiding this comment.
I would choose careful wording like:
For buy and sell orders exceeding USD (EUR) 50,000, consider our OTC partner:
Because the banner will be also for sell.
There was a problem hiding this comment.
Make the amount as a placeholder with value, so it reflects what is coming from backend.
| const ALLOWED_FIAT_CURRENCIES = ['usd', 'eur']; | ||
| const FIAT_AMOUNT_ALLOWANCE = '50001'; |
There was a problem hiding this comment.
I would rather:
- get these values from API
- lets set FIAT_AMOUNT_ALLOWANCE to 50000 and modify comparison operator accordingly. (it looks weird to have it like this - but once it will be over API we could set any value)
packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormOfferOTC.tsx
Outdated
Show resolved
Hide resolved
WalkthroughThis update introduces enhancements to support over-the-counter (OTC) trading functionality across the application. Four new message entries are added to provide users with guidance and action prompts for OTC trading transactions. A new React component, ✨ Finishing Touches
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
suite-common/trading/src/invityAPI.ts (1)
441-451: Fix error logging message.The error logging message '[getInfo]' appears to be a copy-paste error. It should be '[getOTCData]' to maintain consistency with other error logging patterns in this file.
- console.error('[getInfo]', error); + console.error('[getOTCData]', error);packages/suite/src/support/messages.ts (1)
1423-1434: Well-structured messaging for OTC market feature, but consider refining the wording.The addition of these three messages nicely supports the OTC market banner feature mentioned in the PR objectives. You've included dedicated messages for both buying and selling through Mercuryo.
However, I notice that the wording in TR_TRADING_OTC_INFO still primarily emphasizes "purchases" despite LukasRada's earlier feedback suggesting more inclusive language for both buy and sell operations. Consider revising to something like:
- defaultMessage: 'For purchases over USD (EUR) 50,000, consider our OTC partner:', + defaultMessage: 'For buy and sell orders exceeding USD (EUR) 50,000, consider our OTC partner:',This would make it clearer that the OTC service is available for both buying and selling high-value amounts.
packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormOfferOTC.tsx (4)
26-32: Improve numeric comparison logic.The current implementation compares strings numerically with
Number(fiatAmount) < Number(FIAT_AMOUNT_ALLOWANCE). This works but could be improved for clarity sinceFIAT_AMOUNT_ALLOWANCEis already defined as a string.- const FIAT_AMOUNT_ALLOWANCE = '50001'; + const FIAT_AMOUNT_ALLOWANCE = 50000; - Number(fiatAmount) < Number(FIAT_AMOUNT_ALLOWANCE); + Number(fiatAmount) <= FIAT_AMOUNT_ALLOWANCE;
33-56: Consider fetching OTC URL on demand.The current implementation fetches the OTC URL on component render. As suggested in a previous review, consider fetching the URL only when the user clicks on the link to reduce unnecessary API calls.
You could implement this by:
- Moving the
getOtcDatafunction outside theuseEffect- Adding a click handler that calls this function if the URL hasn't been fetched yet
- Showing a loading state while fetching
Example implementation:
const handleLinkClick = async (event: React.MouseEvent) => { if (!otcUrl) { event.preventDefault(); await getOtcData(); // Open URL programmatically after fetching } }; return ( // ... <TrezorLink href={otcUrl || '#'} onClick={handleLinkClick} target="_blank" typographyStyle="hint" > // ... );
41-51: Add URL validation to prevent potential errors.The code creates a URL object without validation. While this is unlikely to cause issues in this specific context, adding validation for the
otcData.apiUrlwould improve robustness.- const apiUrl = new URL(otcData.apiUrl); + try { + const apiUrl = new URL(otcData.apiUrl); + const params = new URLSearchParams({ + widget_id: otcData.idWidget, + otc_id: otcData.idOtcUser, + fiat_amount: fiatAmount, + fiat_currency: currencySelect, + type: context.type, + }); + + apiUrl.search = params.toString(); + setOtcUrl(apiUrl.toString()); + } catch (error) { + console.error('Invalid OTC API URL:', error); + }
62-77: Consider adding accessibility attributes to the link.The link to the OTC market could benefit from additional accessibility attributes such as
aria-labelto provide more context for screen readers.<TrezorLink href={otcUrl} target="_blank" + aria-label={context.type === 'buy' ? 'Open OTC buy page in new tab' : 'Open OTC sell page in new tab'} typographyStyle="hint" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/suite/src/support/messages.ts(1 hunks)packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormOffer.tsx(2 hunks)packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormOfferOTC.tsx(1 hunks)suite-common/trading/src/invityAPI.ts(3 hunks)suite-common/trading/src/types.ts(1 hunks)
🔇 Additional comments (5)
suite-common/trading/src/types.ts (1)
90-94: LGTM! The type definition looks good.Adding the
TradingOTCtype is clean and follows the project's typing conventions with well-named properties.packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormOffer.tsx (2)
31-31: LGTM! Clean import.The import for the new OTC component follows the existing import pattern.
162-162: LGTM! Conditional rendering looks good.The conditional rendering shows the OTC banner only for 'buy' or 'sell' trading types, which aligns with the PR objective of adding an OTC market banner.
suite-common/trading/src/invityAPI.ts (1)
88-89: LGTM! Endpoint property follows convention.The OTC_INFO property is correctly declared as private readonly and follows the naming convention used for other service endpoints.
packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormOfferOTC.tsx (1)
23-24: Consider fetching constants from API instead of hardcoding.The comment indicates these constants will be refactored to be more dynamic in the future. As mentioned in the previous review, consider fetching these values from the API now instead of hardcoding them, which would provide more flexibility.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormOfferOTC.tsx (5)
30-42: Consider adding error handling to API callThe API call to get OTC data doesn't include error handling. If the call fails, users won't know why the OTC offer isn't showing.
const getOtcData = async () => { - const otcData = await invityAPI.getOTCData(); + try { + const otcData = await invityAPI.getOTCData(); + + if (!otcData) return; + + setOtcData(otcData); + } catch (error) { + console.error('Failed to fetch OTC data:', error); + // Optionally add user-facing error handling if appropriate + } - - if (!otcData) return; - - setOtcData(otcData); };
80-88: Add security attributes to external linkWhen opening external links in new tabs, it's a security best practice to include the
rel="noopener noreferrer"attribute to prevent potential exploitation through thewindow.openerAPI.- <TrezorLink href={apiUrl.toString()} target="_blank" typographyStyle="hint"> + <TrezorLink href={apiUrl.toString()} target="_blank" rel="noopener noreferrer" typographyStyle="hint">
30-42: Consider implementing "fetch OTC data on click" approachAs suggested in a previous comment, fetching the OTC URL on click rather than on render would improve performance and reduce unnecessary API calls, especially if many users view the form but few click the link.
This would involve:
- Remove the current useEffect
- Create a click handler that fetches data only when needed
- Show a loading state during the fetch
Example implementation:
- useEffect(() => { - if (!apiKey) return; - - const getOtcData = async () => { - const otcData = await invityAPI.getOTCData(); - - if (!otcData) return; - - setOtcData(otcData); - }; - - getOtcData(); - }, [apiKey]); + const [isLoading, setIsLoading] = useState(false); + + const handleOtcLinkClick = async (e: React.MouseEvent) => { + e.preventDefault(); + if (!apiKey) return; + + setIsLoading(true); + try { + const fetchedOtcData = await invityAPI.getOTCData(); + + if (!fetchedOtcData) { + setIsLoading(false); + return; + } + + setOtcData(fetchedOtcData); + + // Create URL with parameters + const apiUrl = new URL(fetchedOtcData.apiUrl); + const params = new URLSearchParams({ + widget_id: fetchedOtcData.idWidget, + otc_id: fetchedOtcData.idOtcUser, + fiat_amount: fiatAmount, + fiat_currency: currencySelect, + type: context.type, + }); + + apiUrl.search = params.toString(); + + // Open the URL in a new tab + window.open(apiUrl.toString(), '_blank', 'noopener,noreferrer'); + } catch (error) { + console.error('Failed to fetch OTC data:', error); + } finally { + setIsLoading(false); + } + };And update the link in the render function:
- <TrezorLink href={apiUrl.toString()} target="_blank" typographyStyle="hint"> + <TrezorLink onClick={handleOtcLinkClick} href="#" typographyStyle="hint">
44-56: Improve condition readability with early returns patternThe current condition block can be restructured for better readability by using early returns for each check.
- if ( - !otcData || - amountInCrypto || - !isCurrencyAllowed || - !fiatAmount || - Number(fiatAmount) <= Number(otcData.minimumFiat) - ) { - return null; - } + if (!otcData) return null; + if (amountInCrypto) return null; + if (!isCurrencyAllowed) return null; + if (!fiatAmount) return null; + if (Number(fiatAmount) <= Number(otcData.minimumFiat)) return null;Alternatively, if you prefer to keep them together:
- if ( - !otcData || - amountInCrypto || - !isCurrencyAllowed || - !fiatAmount || - Number(fiatAmount) <= Number(otcData.minimumFiat) - ) { + // Don't show OTC banner if any of these conditions are met + if (!otcData || + amountInCrypto || + !isCurrencyAllowed || + !fiatAmount || + Number(fiatAmount) <= Number(otcData.minimumFiat)) { return null; }
53-53: Add safety check for potential NaN comparisonConverting strings to numbers using
Number()could result in NaN if the input is not a valid number. Consider adding a safety check to handle this edge case.- Number(fiatAmount) <= Number(otcData.minimumFiat) + const numFiatAmount = Number(fiatAmount); + const minFiat = Number(otcData.minimumFiat); + isNaN(numFiatAmount) || isNaN(minFiat) || numFiatAmount <= minFiat
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/suite/src/support/messages.ts(1 hunks)packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormOfferOTC.tsx(1 hunks)suite-common/trading/src/invityAPI.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- suite-common/trading/src/invityAPI.ts
- packages/suite/src/support/messages.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: EAS Update
- GitHub Check: prepare_android_test_app
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
| if (!apiKey) return; | ||
|
|
||
| const getOtcData = async () => { | ||
| const otcData = await invityAPI.getOTCData(); |
There was a problem hiding this comment.
fetching the data every time user opens buy/sell form seems a bit like a waste of resources to me
There was a problem hiding this comment.
I agree, I will add it on loading trading section in new refactored trading part? What do you think? 😁
tomasklim
added
the
release
packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormOfferOTC.tsx
Show resolved
Hide resolved
|
|
||
| if ( | ||
| !otcData || | ||
| amountInCrypto || |
There was a problem hiding this comment.
Maybe it would make sense to convert it? so that you can also show it if user wants to buy 1 btc?
adderpositive
force-pushed
the
feat/trading-otc
branch
from
9a5401c to
87df0a5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormOfferOTC.tsx (3)
58-67: Consider implementing lazy loading of OTC URL.As suggested in the past review comments, fetching the OTC URL on click rather than on render could improve performance and address potential security concerns with URL parameters.
Instead of constructing the URL on render, consider implementing a click handler that fetches and constructs the URL only when needed:
const handleOtcLinkClick = async (e: React.MouseEvent) => { e.preventDefault(); // Construct URL parameters only on click const apiUrl = new URL(otcData.apiUrl); const params = new URLSearchParams({ widget_id: otcData.idWidget, otc_id: otcData.idOtcUser, fiat_amount: fiatAmount, fiat_currency: currencySelect, type: context.type, }); apiUrl.search = params.toString(); window.open(apiUrl.toString(), '_blank'); };Then update the TrezorLink component to use this handler instead of the pre-constructed URL.
34-34: Add error handling for the API call.The current implementation doesn't handle errors that might occur during the API call to
getOTCData().Consider adding error handling and potentially a way to communicate to users if something goes wrong:
const getOtcData = async () => { - const otcData = await invityAPI.getOTCData(); + try { + const otcData = await invityAPI.getOTCData(); + + if (!otcData) return; + + setOtcData(otcData); + } catch (error) { + // Consider logging the error or showing a user-friendly message + console.error('Failed to fetch OTC data:', error); + } - - if (!otcData) return; - - setOtcData(otcData); };
50-50: Consider converting crypto amounts to fiat for comparison.As mentioned in the review comments, it might be valuable to convert crypto amounts to fiat to enable showing the OTC option even when users enter crypto values.
If users enter an amount in crypto, consider converting it to fiat for comparison with the minimum threshold instead of simply not showing the banner. This would provide a more consistent experience regardless of how the user inputs the amount.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/suite/src/support/messages.ts(1 hunks)packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormOffer.tsx(2 hunks)packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormOfferOTC.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormOffer.tsx
- packages/suite/src/support/messages.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: EAS Update
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (6)
packages/suite/src/views/wallet/trading/common/TradingForm/TradingFormOfferOTC.tsx (6)
30-42: Consider optimizing data fetching pattern.The current implementation fetches OTC data on every component mount which could lead to unnecessary API calls, especially since this component will render whenever the trading form is opened.
Consider moving this data fetching logic to a higher level component that loads once when the trading section is initialized, as you mentioned in response to previous feedback. This would prevent redundant API calls and improve performance.
44-46: Add null check forotcData.allowedCurrencies.The optional chaining on
otcData?.allowedCurrenciesis good, but there's no guarantee thatallowedCurrenciesis always an array even ifotcDataexists.Add additional type safety to ensure this won't cause runtime errors:
- const isCurrencyAllowed = otcData?.allowedCurrencies?.includes( + const isCurrencyAllowed = Array.isArray(otcData?.allowedCurrencies) && otcData.allowedCurrencies.includes( currencySelect as FiatCurrencyCode, );
48-56: Consider adding a check for excessive amounts.There appears to be a check for minimum fiat amounts, but no check for maximum allowed amounts which might be relevant for OTC trading.
Based on the review comments, there was a suggestion to add a FIAT_AMOUNT_ALLOWANCE. Consider implementing a maximum amount check as well:
if ( !otcData || amountInCrypto || !isCurrencyAllowed || !fiatAmount || - Number(fiatAmount) <= Number(otcData.minimumFiat) + Number(fiatAmount) <= Number(otcData.minimumFiat) || + (otcData.maximumFiat && Number(fiatAmount) > Number(otcData.maximumFiat)) ) { return null; }
69-94: Ensure the banner design matches the specified style.According to the previous review comments, there was a specific style guideline for how the Trezor banner should look.
Verify that the current implementation of the Banner matches the design specification shared in the review comments. If adjustments are needed to match the reference image, consider styling props or additional wrapper elements.
1-15: The imports are well-organized and appropriate for the component's functionality.The imports are logically grouped and include all necessary dependencies for the component.
16-29: Good use of context and state management.The component correctly leverages the trading form context to access form values and determine whether it's in a buy or sell context. The state management with useState is appropriate for storing the OTC data.
Description
Add a banner with a link to the Invity OTC.
Related Issue
Resolve #17198
Screenshots