Closed zed-wong closed 3 months ago
vercel[bot]
commented
3 months ago The latest updates on your projects. Learn more about Vercel for Git ↗︎
| Name | Status | Preview | Comments | Updated (UTC) |
|---|---|---|---|---|
| mr-market | ✅ Ready (Inspect) | Visit Preview | 💬 Add feedback | Apr 3, 2024 6:41am |
railway-app[bot]
commented
3 months ago This PR is being deployed to Railway 🚅
Mr.Market: ◻️ REMOVED
github-actions[bot]
commented
3 months ago PR Description updated to latest commit (https://github.com/Hu-Fi/Mr.Market/commit/b508c377d7fc36bd792ae73501d947afd871dad1)
github-actions[bot]
commented
3 months ago | ⏱️ Estimated effort to review [1-5] | 4, due to the extensive changes across multiple files, including backend and frontend logic, integration of new functionalities, and modifications to existing structures. The PR introduces new features related to order handling and display, which requires careful review of both logic and UI changes. |
| 🧪 Relevant tests | No |
| 🔍 Possible issues | Possible Bug: The `getUserOrders` function in `interface/src/lib/helpers/mixin.ts` does not handle the case where `orders` is undefined or null after fetching. This could lead to runtime errors. |
| Performance Concern: The `setInterval` used in `interface/src/lib/components/spot/history/orderDetail.svelte` for fetching order details might not be cleared if the component is destroyed before finding the order or reaching the timeout. This could lead to memory leaks or unexpected behavior. | |
| Code Duplication: Similar error handling logic is repeated in multiple places within `interface/src/lib/helpers/hufi/spot.ts`. Consider abstracting this into a separate function to improve code maintainability. | |
| Inconsistent Error Handling: The error handling in `interface/src/lib/helpers/hufi/spot.ts` logs errors to the console but does not provide a way to notify the user or handle errors gracefully in the UI. | |
| 🔒 Security concerns | No |
| relevant file | interface/src/lib/helpers/mixin.ts |
| suggestion | Consider adding a check for `orders` being undefined or null in the `getUserOrders` function to prevent potential runtime errors. [important] |
| relevant line | const orders = await getOrdersByUser(user_id); |
| relevant file | interface/src/lib/components/spot/history/orderDetail.svelte |
| suggestion | Ensure to clear the `setInterval` when the component is destroyed or when the order is found or timeout is reached to prevent potential memory leaks. [important] |
| relevant line | var interval = setInterval(() => { |
| relevant file | interface/src/lib/helpers/hufi/spot.ts |
| suggestion | Abstract the repeated error handling logic into a separate function to reduce code duplication and improve maintainability. [medium] |
| relevant line | } catch (error) { |
| relevant file | interface/src/lib/helpers/hufi/spot.ts |
| suggestion | Implement a user-friendly error handling strategy that could involve retry mechanisms, user notifications, or alternative actions in case of failure. [medium] |
| relevant line | console.error('Error fetching orders by user:', error); |
github-actions[bot]
commented
3 months ago | Category | Suggestions |
| Enhancement |
Add response status check before parsing JSON data.___ **Consider checking the response status code to ensure the fetch operation was successfulbefore attempting to parse the JSON data. This can help in handling HTTP errors more gracefully.** [interface/src/lib/helpers/hufi/spot.ts [5-6]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-cf7952a608e6022938e20333e3ac4fa650cee3ec0ee1b9a93b751215f5f15355R5-R6) ```diff const response = await fetch(`${HUFI_BACKEND_URL}/exchange/orders/${userId}`); +if (!response.ok) { + throw new Error(`HTTP error! status: ${response.status}`); +} const data = await response.json(); ``` |
Optimize symbol splitting by doing it once and reusing the result.___ **Sinceevent.symbol.split('/') is used twice to get baseSymbol and targetSymbol, consider splitting it once and reusing the result to improve code efficiency.** [server/src/modules/mixin/listeners/exchange.listener.ts [26-27]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-91e3bad7e30c4409762ddb39bfeccd21a4e656b55ff4f2c169fc2959820973aeR26-R27) ```diff -let baseSymbol = event.symbol.split('/')[0]; -let targetSymbol = event.symbol.split('/')[1]; +const [baseSymbol, targetSymbol] = event.symbol.split('/'); ``` | |
Prevent multiple authentication requests by disabling the button during the authentication process.___ **Consider disabling the button during the authentication process to prevent multiple clicksand potential issues with multiple authentication requests.** [interface/src/lib/components/common/connectWalletBtn.svelte [33-38]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-e4c4aabcf6a4083502cddfbd41e4ba22bff0ecf35ece70283f5c287ad7ed8b78R33-R38) ```diff | |
Use gradient background for button for UI consistency.___ **Consider using Tailwind CSS's utility classes for gradients instead of a solid backgroundcolor for the button to enhance the UI consistency with the rest of the application.** [interface/src/lib/components/dialogs/manageOrder/cancelOrder.svelte [37]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-18a5517d2f5ce3cfd10921a4716e024ae0c56542c27b34852a5b9dcb5e19ba48R37-R37) ```diff - | |
Use page data for dynamic order details.___ **Instead of hardcoding the order details, use the$page.data.order directly to make the component dynamic and capable of displaying different order details based on the page data.** [interface/src/lib/components/spot/history/orderDetail.svelte [7-26]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-f23b2cd24acc1ec41f0fb4400075be6ed4ca2249c023bd60333e00cc92b420acR7-R26) ```diff -$: o = { - orderId: "a22170a7-ec32-4718-80f7-c304959c3e42", - ... -}; +$: o = $page.data.order; ``` | |
Add title attribute to exchange icon for better UX.___ **To enhance user experience, consider adding atitle attribute to the the exchange name. This provides users with additional context when they hover over the icon.** [interface/src/lib/components/spot/history/singleSpotHistory.svelte [34]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-46990d6b58514b9c48cffbfea1af4818ee778f89c86f04c68656b4601132054dR34-R34) ```diff - | |
| Best practice |
Handle potential rejections from
___
**Since |
Ensure type consistency for
___
**Ensure consistency in the type of | |
Ensure loading state is cleared in a finally block.___ **To avoid potential memory leaks or unexpected behavior, ensure to clear the loading statein a finally block within the try-catch structure, ensuring it executes regardless of the promise's outcome.** [interface/src/lib/components/home/oauth/connectMixin.svelte [19-24]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-9900fdef6564837a60597b2cc56579c0a4fd4657bbe885bf665f6f0ea6ab9348R19-R24) ```diff -mixinConnectLoading.set(false); +try { + await AfterMixinOauth(token); +} catch (error) { + console.error(error); +} finally { + mixinConnectLoading.set(false); +} ``` | |
Use strict equality checks to avoid type coercion bugs.___ **Use strict equality checks (=== and !==) instead of loose equality checks (== and !=) to avoid potential type coercion bugs.** [interface/src/lib/components/spot/manage/openOrders.svelte [27]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-5ef626be775a5833a21c46be63819177cd70dab825166b17bfc47e574b2e9f0aR27-R27) ```diff -{#if os.length != 0} +{#if os.length !== 0} ``` | |
Handle default case in switch statement for robustness.___ **It's a good practice to handle the case where$orderFilterMode does not match any of the cases in the switch statement, perhaps by returning all items or a subset based on a default condition.** [interface/src/lib/components/spot/manage/openOrders.svelte [14-19]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-5ef626be775a5833a21c46be63819177cd70dab825166b17bfc47e574b2e9f0aR14-R19) ```diff switch ($orderFilterMode) { case 0: return item.type.toUpperCase().includes("LIMIT"); case 1: return item.type.toUpperCase().includes("MARKET"); + default: + // Handle default case, e.g., return all items or a specific subset + return true; // Example: return all items by default } ``` | |
| Maintainability |
Remove or use the unused
___
**The |
Use a constant for excluded exchanges to improve maintainability.___ **To improve maintainability, consider using a constant for 'BITFINEX' instead of hardcodingit multiple times. This approach makes future changes easier and reduces the risk of typos.** [interface/src/lib/components/dialogs/candle/candlePairSelector.svelte [17-35]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-090902fdcadd148833cc97fe9b2404e2b460d51bfb6fcd743ed98e36038518dfR17-R35) ```diff -{ name: 'all' }, ...SUPPORTED_EXCHANGES.filter((e)=>{return e!='bitfinex'}).map(exchange => ({ name: exchange })) +const EXCLUDED_EXCHANGE = 'BITFINEX'; +{ name: 'all' }, ...SUPPORTED_EXCHANGES.filter((e)=>{return e.toUpperCase() != EXCLUDED_EXCHANGE}).map(exchange => ({ name: exchange })) ... -return item.exchange.toUpperCase() != 'BITFINEX' +return item.exchange.toUpperCase() != EXCLUDED_EXCHANGE ``` | |
Destructure
___
**To improve code readability and maintainability, consider destructuring the | |
Use more descriptive variable names.___ **Consider using a more descriptive variable name instead ofos for the filtered orders. Descriptive names improve code readability and maintainability.** [interface/src/lib/components/spot/manage/openOrders.svelte [11-20]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-5ef626be775a5833a21c46be63819177cd70dab825166b17bfc47e574b2e9f0aR11-R20) ```diff -$: os = $userOrders.filter((item)=>{ +$: filteredOrders = $userOrders.filter((item)=>{ return item.state.includes('EXCHANGE_ORDER_PARTIAL_FILLED') || item.state.includes('ORDER_CREATED') }).filter((item) => { switch ($orderFilterMode) { case 0: return item.type.toUpperCase().includes("LIMIT"); case 1: return item.type.toUpperCase().includes("MARKET"); } }); ``` | |
| Bug |
Await the result of
___
**The |
| Performance |
Optimize data fetching by selecting only necessary fields from the database.___ **Instead of loading all orders withfind and then iterating over them to delete the apiKeyId field, consider using a query to select only the necessary fields. This approach reduces the amount of data transferred and processed.** [server/src/modules/mixin/exchange/exchange.repository.ts [56-60]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-4ae06c682ed3768a469dc4da09182b2825f09801283b0d2111ef9863d044ada1R56-R60) ```diff -let orders = await this.spotOrderRepository.find({ where: { userId } }); -orders = orders.map((order) => { - delete order.apiKeyId; - return order; +const orders = await this.spotOrderRepository.find({ + where: { userId }, + select: ['id', 'userId', 'exchangeName', 'symbol', 'type', 'state', 'amount', 'price'] // Add other necessary fields but exclude 'apiKeyId' }); ``` |
Improve asynchronous operations performance with
___
**Use | |
Combine filter conditions for better performance and readability.___ **Instead of using multiplefilter methods, you can combine the conditions into a single filter to improve performance and readability.**
[interface/src/lib/components/spot/manage/openOrders.svelte [11-20]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-5ef626be775a5833a21c46be63819177cd70dab825166b17bfc47e574b2e9f0aR11-R20)
```diff
-$: os = $userOrders.filter((item)=>{
- return item.state.includes('EXCHANGE_ORDER_PARTIAL_FILLED') || item.state.includes('ORDER_CREATED')
-}).filter((item) => {
+$: filteredOrders = $userOrders.filter((item) => {
+ const isOrderStateValid = item.state.includes('EXCHANGE_ORDER_PARTIAL_FILLED') || item.state.includes('ORDER_CREATED');
+ let isOrderTypeValid = false;
switch ($orderFilterMode) {
case 0:
- return item.type.toUpperCase().includes("LIMIT");
+ isOrderTypeValid = item.type.toUpperCase().includes("LIMIT");
+ break;
case 1:
- return item.type.toUpperCase().includes("MARKET");
+ isOrderTypeValid = item.type.toUpperCase().includes("MARKET");
+ break;
}
+ return isOrderStateValid && isOrderTypeValid;
});
```
| |
| Accessibility |
Improve accessibility by using a button element for interactive components.___ **For better accessibility and semantics, consider using acomponent and ensure it has an appropriate role attribute if it's not inherently a button.**
[interface/src/lib/components/spot/manage/openOrders.svelte [92]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-5ef626be775a5833a21c46be63819177cd70dab825166b17bfc47e574b2e9f0aR92-R92)
```diff
- |
zed-wong
commented
3 months ago /improve
github-actions[bot]
commented
3 months ago | Category | Suggestions |
| Best practice |
Add explicit return types to asynchronous functions.___ **It's a good practice to explicitly define the return type of asynchronous functions forbetter code readability and maintainability. For the functions getOrdersByUser and getOrderById, you can define interfaces or types that represent the structure of the data they return.** [interface/src/lib/helpers/hufi/spot.ts [3-23]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-cf7952a608e6022938e20333e3ac4fa650cee3ec0ee1b9a93b751215f5f15355R3-R23) ```diff -export const getOrdersByUser = async (userId: string) => { +export const getOrdersByUser = async (userId: string): Promise |
Handle exceptions when using localStorage.___ **When setting items inlocalStorage, it's a good practice to handle potential exceptions that might be thrown for various reasons, such as the user's browser being in private mode or the storage being full. Wrap localStorage.setItem calls in a try-catch block to handle these exceptions gracefully.** [interface/src/lib/helpers/mixin.ts [217]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-acf423272bdc878982f8a993563883c5ac16fcef547cb0e7a09972c785a5e839R217-R217) ```diff -localStorage.setItem("mixin-oauth", token) +try { + localStorage.setItem("mixin-oauth", token) +} catch (error) { + console.error("Error saving to localStorage", error); +} ``` | |
Use a DTO to transfer data instead of modifying the entity directly.___ **Directly modifying the entity retrieved from the database can lead to unintended sideeffects. Instead of deleting properties from the entity, consider using a DTO (Data Transfer Object) to safely transfer only the necessary data.** [server/src/modules/mixin/exchange/exchange.repository.ts [57-60]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-4ae06c682ed3768a469dc4da09182b2825f09801283b0d2111ef9863d044ada1R57-R60) ```diff orders = orders.map((order) => { - delete order.apiKeyId; - return order; + const { apiKeyId, ...safeOrder } = order; + return safeOrder; }); ``` | |
Use a method to parse and validate memo content for better code quality.___ **Instead of slicing the decoded memo directly to get the trading type, consider using amore explicit and error-proof method to parse and validate the memo content. This can improve code readability and maintainability.** [server/src/modules/mixin/snapshots/snapshots.service.ts [470-471]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-dd336e520fcec14b837cd72c0852b94e85fb8e130fc2a3431b49fef7e4d94c29R470-R471) ```diff const decodedMemo = Buffer.from(snapshot.memo, 'base64').toString('utf-8'); -const tradingType = decodedMemo.slice(0, 2); +const tradingType = this.parseTradingType(decodedMemo); ``` | |
Use Svelte's onDestroy lifecycle hook to clear intervals and prevent memory leaks.___ **Consider using Svelte's built-in store mechanism for managing intervals and timeouts toensure they are properly cleared when the component is destroyed. This prevents potential memory leaks or unexpected behavior.** [interface/src/lib/components/dialogs/orderConfirm.svelte [76-88]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-76ee31be4b4a8017d8e5a9d48dbfc2f7b107694d79c9d06c93ed6e7670a60451R76-R88) ```diff -var interval = setInterval(() => { - console.log(`${new Date()} called`); - // TODO: TEST IT; - found = getOrderById(trace); - totalTime += FETCH_INTERVAL; +import { onDestroy } from 'svelte'; +let interval; - if (found) { +function startInterval() { + interval = setInterval(() => { + console.log(`${new Date()} called`); + // TODO: TEST IT; + found = getOrderById(trace); + totalTime += FETCH_INTERVAL; + + if (found) { + clearInterval(interval); + goto(`/spot/history/${trace}`); + } else if (totalTime >= TIMEOUT_DURATION) { + clearInterval(interval); + console.log('Timeout reached, stopping execution.'); + } + }, FETCH_INTERVAL); +} + +onDestroy(() => { + if (interval) { clearInterval(interval); - goto(`/spot/history/${trace}`); - } else if (totalTime >= TIMEOUT_DURATION) { - clearInterval(interval); - console.log('Timeout reached, stopping execution.'); } -}, FETCH_INTERVAL); +}); +startInterval(); + ``` | |
| Maintainability |
Remove or utilize the unused variable.___ **The variablelang is declared but its value is never used. Consider removing it if it's not needed, or use it if it was intended for localization or formatting purposes.** [interface/src/lib/helpers/utils.ts [90]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-cca6fb1729226e759cd4dba0b09a9c83f7f109709c77fd68c77feaf0ba512380R90-R90) ```diff -lang; +// If lang was intended for use, implement its functionality. Otherwise, remove this line. ``` |
Refactor symbol splitting and checking for a cleaner approach.___ **Instead of manually splitting the symbol and then checking if it includes 'USDT', you canuse a more robust approach to handle different cases and make the code cleaner.** [server/src/modules/mixin/listeners/exchange.listener.ts [26-29]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-91e3bad7e30c4409762ddb39bfeccd21a4e656b55ff4f2c169fc2959820973aeR26-R29) ```diff -let [baseSymbol, targetSymbol] = event.symbol.split('/'); -if (baseSymbol.includes('USDT')) { - baseSymbol = 'USDT'; -} +const [baseSymbol, targetSymbol] = event.symbol.split('/').map(sym => sym === 'USDT' ? 'USDT' : sym); ``` | |
Define event handler functions outside the template for better code organization.___ **Instead of using an inline async function in the on:click event, define the functionoutside the template to improve readability and maintainability.** [interface/src/lib/components/common/connectWalletBtn.svelte [33-38]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-e4c4aabcf6a4083502cddfbd41e4ba22bff0ecf35ece70283f5c287ad7ed8b78R33-R38) ```diff + + | |
Use a mapping object for cleaner handling of state messages.___ **Use a switch statement or a mapping object for cleaner and more efficient handling ofmultiple conditions.** [interface/src/lib/components/spot/history/orderDetail.svelte [20-32]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-f23b2cd24acc1ec41f0fb4400075be6ed4ca2249c023bd60333e00cc92b420acR20-R32) ```diff -{#if state == 'ORDER_CREATED'} - {$_('order_created')} -{:else if state == 'EXCHANGE_ORDER_PLACED'} - {$_('order_placed')} -{:else if state == 'EXCHANGE_ORDER_PARTIAL_FILLED'} - {$_('order_partially_filled')} -{:else if state == 'EXCHANGE_ORDER_FILLED'} - {$_('order_filled')} -{:else if state == 'ORDER_SUCCESS'} - {$_('order_success')} -{/if} + + + {stateMessages[state] || 'Unknown State'} + + ``` | |
Use more descriptive variable names to improve code readability.___ **Consider using a more descriptive variable name thano for the exported object to improve code readability. A name like orderDetails would make the code more understandable to someone else reading it for the first time.** [interface/src/lib/components/spot/history/singleSpotHistory.svelte [9-24]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-46990d6b58514b9c48cffbfea1af4818ee778f89c86f04c68656b4601132054dR9-R24) ```diff -export let o = { +export let orderDetails = { ``` | |
Extract conditional class logic into a separate function for better maintainability.___ **To improve the maintainability and readability of the conditionals, consider extractingthe logic that determines the class names based on the buy variable into a separate function. This will make the template cleaner and the logic easier to update in the future.** [interface/src/lib/components/spot/history/singleSpotHistory.svelte [52]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-46990d6b58514b9c48cffbfea1af4818ee778f89c86f04c68656b4601132054dR52-R52) ```diff -
+
+
```
| |
| Possible issue |
Ensure
___
**The type of |
| Enhancement |
Specify column options for new database fields.___ **For the new columns added (exchangeName, amount, limitPrice), consider adding more specific column options such as type and nullable. This can help with database optimizations and ensure data integrity.** [server/src/common/entities/spot-order.entity.ts [18-42]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-a61566519c8ab641e647992e682ae1bb77027eda900aec4328fcaa42b6411939R18-R42) ```diff +@Column({ type: 'varchar', nullable: false }) exchangeName: string; // Name of exchange ... +@Column({ type: 'decimal', precision: 15, scale: 2, nullable: false }) amount: string; // The amount of pay asset received ... +@Column({ type: 'decimal', precision: 15, scale: 2, nullable: true }) limitPrice?: string; // Limit price for limit order ``` |
Improve error handling by returning a suitable HTTP response on exceptions.___ **It's recommended to handle the exception more gracefully by returning an appropriate HTTPresponse. In the current implementation, if an error occurs, the function will return undefined which might not be the expected behavior for the client. Consider returning a Response object with a suitable HTTP status code and error message.**
[server/src/modules/mixin/exchange/exchange-client.controller.ts [21-25]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-3a27056e1e0e58b7ecb405918aba8fa940c6a6f56eb179fdef1eb89790685d6eR21-R25)
```diff
try {
return this.exchagneService.readOrdersByUser(userId);
} catch (e) {
this.logger.error(`Get orders by user error: ${e.message}`);
+ throw new HttpException('Internal server error', HttpStatus.INTERNAL_SERVER_ERROR);
}
```
| |
Disable the button during loading to prevent multiple requests.___ **To improve the user experience, consider disabling the button while the connection isloading, preventing multiple requests.** [interface/src/lib/components/home/oauth/connectMixin.svelte [32-34]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-9900fdef6564837a60597b2cc56579c0a4fd4657bbe885bf665f6f0ea6ab9348R32-R34) ```diff | |
Add
___
**For better accessibility, consider adding an | |
| Performance |
Filter orders before mapping to avoid unnecessary promise creation.___ **Instead of usingPromise.all with a map that returns undefined for some conditions, consider filtering out the orders that do not meet the condition before mapping. This approach can avoid unnecessary promise creation and make the code cleaner.** [server/src/modules/mixin/exchange/exchange.service.ts [579-587]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-b4d701c594440771180bbf709c88654f1d8a3806e45b2e035b3b36572ab3cda5R579-R587) ```diff +const ordersWithFetchOrder = orders.filter(o => this.exchangeInstances[o.exchangeName].has['fetchOrder']); await Promise.all( - orders.map(async (o) => { + ordersWithFetchOrder.map(async (o) => { const instance = this.exchangeInstances[o.exchangeName]; - if (!instance.has['fetchOrder']) { - return await this.updateSpotOrderState( - o.orderId, - STATE_TEXT_MAP['EXCHANGE_DOESNT_SUPPORT_FETCH_ORDER'], - ); - } + const order = await instance.fetchOrder(o.orderId); ``` |
Simplify filtering logic for better performance and readability.___ **Simplify the filtering logic by using a direct comparison instead of a function for betterperformance and readability.** [interface/src/lib/components/dialogs/candle/candlePairSelector.svelte [17-18]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-090902fdcadd148833cc97fe9b2404e2b460d51bfb6fcd743ed98e36038518dfR17-R18) ```diff let items = [ - { name: 'all' }, ...SUPPORTED_EXCHANGES.filter((e)=>{return e!='bitfinex'}).map(exchange => ({ name: exchange })) + { name: 'all' }, ...SUPPORTED_EXCHANGES.filter(e => e !== 'bitfinex').map(exchange => ({ name: exchange })) ]; ``` | |
Use a smaller library or native methods for date formatting to reduce bundle size.___ **Since themoment library can be relatively large and you're only using it for formatting dates, consider using a smaller library like date-fns or native JavaScript Date methods for formatting dates. This can help reduce the bundle size and improve the load time of your application.** [interface/src/lib/components/spot/history/singleSpotHistory.svelte [58]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-46990d6b58514b9c48cffbfea1af4818ee778f89c86f04c68656b4601132054dR58-R58) ```diff - {moment(o.createdAt).format('YYYY-MM-DD hh:mm:ss')} + {new Date(o.createdAt).toLocaleString()} ``` | |
| Security |
Add validation or sanitization for dynamic values used in navigation to prevent security risks.___ **It's recommended to validate or sanitize the dynamic values used in thegoto function to prevent potential security risks such as open redirects. Ensure that the orderId parameter passed to the redirect function is validated against a known list of valid order IDs or sanitized to remove any special characters that could be used in a malicious way.** [interface/src/lib/components/spot/history/singleSpotHistory.svelte [26]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-46990d6b58514b9c48cffbfea1af4818ee778f89c86f04c68656b4601132054dR26-R26) ```diff -const redirect = (orderId: string) => { goto(`/spot/history/${orderId}`); } +const redirect = (orderId: string) => { + // Example validation/sanitization logic + if (isValidOrderId(orderId)) { + goto(`/spot/history/${orderId}`); + } else { + console.error("Invalid order ID"); + } +} ``` |
github-actions[bot]
commented
3 months ago PR Description updated to latest commit (https://github.com/Hu-Fi/Mr.Market/commit/4c35f794980357ad51cd2b9322321df3aa319977)
github-actions[bot]
commented
3 months ago Persistent review updated to latest commit https://github.com/Hu-Fi/Mr.Market/commit/4c35f794980357ad51cd2b9322321df3aa319977
github-actions[bot]
commented
3 months ago | Category | Suggestions |
| Enhancement |
Add error handling for JSON parsing of server responses.___ **It's recommended to handle the case where the response from the server is not in theexpected JSON format. This can be done by wrapping the response.json() call in a try-catch block to catch any potential errors that may arise from parsing the response. This ensures that the application can gracefully handle unexpected server responses and provide a better error message or fallback behavior.** [interface/src/lib/helpers/hufi/spot.ts [9-10]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-cf7952a608e6022938e20333e3ac4fa650cee3ec0ee1b9a93b751215f5f15355R9-R10) ```diff -const data = await response.json(); +let data; +try { + data = await response.json(); +} catch (error) { + console.error('Error parsing JSON from response:', error); + throw new Error('Failed to parse JSON from response'); +} return data; ``` |
Add error handling to return a response on exceptions.___ **It's important to handle exceptions gracefully and ensure that the API always returns aresponse to the client, even in case of errors. In the current implementation, if an exception is caught, the method ends without returning a response, which can lead to client timeouts or undefined behavior. Consider returning a proper error response with a status code and message.** [server/src/modules/mixin/exchange/exchange-client.controller.ts [21-25]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-3a27056e1e0e58b7ecb405918aba8fa940c6a6f56eb179fdef1eb89790685d6eR21-R25) ```diff try { return this.exchagneService.readOrdersByUser(userId); } catch (e) { this.logger.error(`Get orders by user error: ${e.message}`); + throw new HttpException('Internal server error', HttpStatus.INTERNAL_SERVER_ERROR); } ``` | |
Improve symbol parsing for robustness.___ **The current implementation splits the symbol string to extract base and target symbols butdoes not account for symbols that might not follow the 'BASE/TARGET' format strictly. Using a more robust method to parse symbols can prevent potential errors.** [server/src/modules/mixin/listeners/exchange.listener.ts [26-29]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-91e3bad7e30c4409762ddb39bfeccd21a4e656b55ff4f2c169fc2959820973aeR26-R29) ```diff -let [baseSymbol, targetSymbol] = event.symbol.split('/'); +const symbols = event.symbol.split('/'); +let baseSymbol = symbols[0]; +let targetSymbol = symbols[1] || ''; // Fallback in case the symbol does not contain '/' if (baseSymbol.includes('USDT')) { baseSymbol = 'USDT'; } ``` | |
Use recursive setTimeout instead of setInterval for fetching order status with a timeout.___ **Instead of usingsetInterval and manually tracking the total time to implement a timeout for fetching the order status, consider using setTimeout recursively. This approach simplifies the logic and makes it easier to manage the timeout condition. Additionally, ensure that getOrderById is awaited since it's likely an asynchronous operation.**
[interface/src/lib/components/dialogs/orderConfirm.svelte [76-88]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-76ee31be4b4a8017d8e5a9d48dbfc2f7b107694d79c9d06c93ed6e7670a60451R76-R88)
```diff
-var interval = setInterval(() => {
+const checkOrderStatus = async () => {
console.log(`${new Date()} called`);
- // TODO: TEST IT;
- found = getOrderById(trace);
- totalTime += FETCH_INTERVAL;
+ found = await getOrderById(trace);
if (found) {
- clearInterval(interval);
goto(`/spot/history/${trace}`);
- } else if (totalTime >= TIMEOUT_DURATION) {
- clearInterval(interval);
+ } else if (totalTime < TIMEOUT_DURATION) {
+ totalTime += FETCH_INTERVAL;
+ setTimeout(checkOrderStatus, FETCH_INTERVAL);
+ } else {
console.log('Timeout reached, stopping execution.');
}
-}, FETCH_INTERVAL);
+};
+checkOrderStatus();
```
| |
Add an aria-label to the connect wallet button for better accessibility.___ **To improve the user experience and accessibility, consider adding anaria-label attribute to the button element. This label should clearly describe the action that the button performs, such as "Connect to Wallet".** [interface/src/lib/components/common/connectWalletBtn.svelte [33-39]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-e4c4aabcf6a4083502cddfbd41e4ba22bff0ecf35ece70283f5c287ad7ed8b78R33-R39) ```diff | |
Move duplicated loading state reset to a finally block for clarity.___ **ThemixinConnectLoading.set(false); is duplicated in both the onError and onSuccess callbacks. Consider moving this statement to a finally block within the auth function to ensure it is executed regardless of the outcome, reducing duplication and improving code clarity.** [interface/src/lib/components/home/oauth/connectMixin.svelte [17-26]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-9900fdef6564837a60597b2cc56579c0a4fd4657bbe885bf665f6f0ea6ab9348R17-R26) ```diff onError: (error: Error) => { console.error(error); - mixinConnectLoading.set(false); return; }, onSuccess: async (token: string) => { await AfterMixinOauth(token) - mixinConnectLoading.set(false); return; }, +finally: () => { + mixinConnectLoading.set(false); +}, ``` | |
Add
___
**To improve the user experience, consider adding an | |
| Best practice |
Use JSON.stringify when storing the token in localStorage.___ **To ensure that thelocalStorage.setItem("mixin-oauth", token) call is consistent with the rest of the codebase, consider using JSON.stringify(token) when storing the token. This ensures that the token is stored in a consistent format, making it easier to parse when retrieved from localStorage.** [interface/src/lib/helpers/mixin.ts [217]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-acf423272bdc878982f8a993563883c5ac16fcef547cb0e7a09972c785a5e839R217-R217) ```diff -localStorage.setItem("mixin-oauth", token) +localStorage.setItem("mixin-oauth", JSON.stringify(token)) ``` |
Use DTOs instead of modifying entities directly.___ **Directly modifying the entity retrieved from the database can lead to unintended sideeffects, especially when using ORM tools that might track entity changes. Instead of deleting properties from the entity objects, consider using a DTO (Data Transfer Object) to selectively return the necessary information to the client.** [server/src/modules/mixin/exchange/exchange.repository.ts [56-61]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-4ae06c682ed3768a469dc4da09182b2825f09801283b0d2111ef9863d044ada1R56-R61) ```diff -let orders = await this.spotOrderRepository.find({ where: { userId } }); -orders = orders.map((order) => { - delete order.apiKeyId; - return order; -}); -return orders; +const orders = await this.spotOrderRepository.find({ where: { userId } }); +return orders.map(({ apiKeyId, ...order }) => order); ``` | |
Rename script for consistency and clarity.___ **The script namepr:check uses a colon, which is commonly used in npm scripts for namespacing. For consistency and clarity, consider using a dash or an underscore instead if this script is not meant to be part of a namespace.** [interface/package.json [16]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-456a1b78239f95ef64c57f4524b153136f934c7eab7c893c201b0580028eb61cR16-R16) ```diff -"pr:check": "bun run lint:fix && bun run test" +"pr-check": "bun run lint:fix && bun run test" ``` | |
Use constants instead of magic strings.___ **For better maintainability and to avoid magic strings, consider defining constants fororder types like 'Limit Buy' and states like 'ORDER_CREATED'.** [interface/src/lib/components/spot/history/singleSpotHistory.svelte [14-15]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-46990d6b58514b9c48cffbfea1af4818ee778f89c86f04c68656b4601132054dR14-R15) ```diff -type: "Limit Buy", -state: "ORDER_CREATED", +type: ORDER_TYPES.LIMIT_BUY, +state: ORDER_STATES.CREATED, ``` | |
Use a constant for date formats.___ **Instead of hardcoding the date format in the moment.js formatting, consider using aconstant. This makes it easier to change the date format application-wide if needed.** [interface/src/lib/components/spot/history/singleSpotHistory.svelte [58]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-46990d6b58514b9c48cffbfea1af4818ee778f89c86f04c68656b4601132054dR58-R58) ```diff - {moment(o.createdAt).format('YYYY-MM-DD hh:mm:ss')} + {moment(o.createdAt).format(DATE_FORMAT)} ``` | |
| Maintainability |
Remove unused variables to improve code readability.___ **The variablelang is declared but never used in the function formatWalletBalanceFull. To adhere to best practices and improve code readability, it's recommended to remove unused variables. If the intention was to use this variable for future localization or formatting, consider adding a comment or implementing the intended functionality.** [interface/src/lib/helpers/utils.ts [90]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-cca6fb1729226e759cd4dba0b09a9c83f7f109709c77fd68c77feaf0ba512380R90-R90) ```diff -lang; +// Removed unused variable lang ``` |
Simplify function names for checking valid trading types, spot order types, and exchange indexes.___ **The function namesisTradingTypeValueValid, isSpotOrderTypeValueValid, and isExchangeIndexValueValid could be simplified for better readability and consistency. Consider renaming these functions to isValidTradingType, isValidSpotOrderType, and isValidExchangeIndex respectively. This naming convention is more direct and aligns with common practices for boolean-returning functions.** [server/src/common/helpers/checks/spotChecks.ts [12-24]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-149a7f6a722ed2b5d682d3cb8c1e228d5664c2540841e5056dc063ae9f3c754aR12-R24) ```diff -export const isTradingTypeValueValid = (tradingType: string): boolean => { ... }; -export const isSpotOrderTypeValueValid = (spotOrderType: string): boolean => { ... }; -export const isExchangeIndexValueValid = (exchangeIndex: string): boolean => { ... }; +export const isValidTradingType = (tradingType: string): boolean => { ... }; +export const isValidSpotOrderType = (spotOrderType: string): boolean => { ... }; +export const isValidExchangeIndex = (exchangeIndex: string): boolean => { ... }; ``` | |
Refactor duplicated filtering logic into a shared function for maintainability.___ **The filtering logic to exclude 'bitfinex' fromSUPPORTED_EXCHANGES is duplicated in both the items initialization and the filteredPairs computation. Consider refactoring this logic into a shared function or computed store to avoid duplication and improve maintainability.** [interface/src/lib/components/dialogs/candle/candlePairSelector.svelte [17-36]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-090902fdcadd148833cc97fe9b2404e2b460d51bfb6fcd743ed98e36038518dfR17-R36) ```diff +const filterExchanges = (exchanges) => exchanges.filter(e => e.toUpperCase() != 'BITFINEX'); let items = [ - { name: 'all' }, ...SUPPORTED_EXCHANGES.filter((e)=>{return e!='bitfinex'}).map(exchange => ({ name: exchange })) + { name: 'all' }, ...filterExchanges(SUPPORTED_EXCHANGES).map(exchange => ({ name: exchange })) ]; ... -}).filter((item) => { - return item.exchange.toUpperCase() != 'BITFINEX' -}) +}).filter((item) => filterExchanges([item.exchange]).length > 0) ``` | |
Refactor duplicated loading state reset into a single operation.___ **Similar to another suggestion, themixinConnectLoading.set(false); is duplicated in both the onError and onSuccess callbacks. Consider refactoring to ensure this operation is only written once, improving code maintainability.** [interface/src/lib/components/spot/bids/inputs.svelte [161-168]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-12445a9b778b968349e9b5aa908c04ef41c20d7fa6f232d972fb3e936a93bfd0R161-R168) ```diff onError: (error: Error) => { console.error(error); - mixinConnectLoading.set(false); return; }, onSuccess: async (token: string) => { await AfterMixinOauth(token); +}, +finally: () => { mixinConnectLoading.set(false); }, ``` | |
Use descriptive variable names.___ **Consider using a more descriptive variable name instead ofo for the export let statement. Descriptive names improve code readability and maintainability.** [interface/src/lib/components/spot/history/singleSpotHistory.svelte [9-24]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-46990d6b58514b9c48cffbfea1af4818ee778f89c86f04c68656b4601132054dR9-R24) ```diff -export let o = { +export let orderDetails = { ``` | |
Encapsulate progress indicator logic into a separate component.___ **Theradial-progress class seems to be used for styling a progress indicator. Consider encapsulating this logic into a separate Svelte component for better reusability and separation of concerns.** [interface/src/lib/components/spot/history/singleSpotHistory.svelte [65]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-46990d6b58514b9c48cffbfea1af4818ee778f89c86f04c68656b4601132054dR65-R65) ```diff -
+ | |
| Possible issue |
Ensure
___
**The type of |
| Performance |
Improve handling of concurrent operations.___ **UsingPromise.all with a map function that performs asynchronous operations can lead to performance issues if the array is large, as it will send all requests at once. Consider using for...of loop with async/await for better control over concurrency, or use libraries like p-limit to limit the number of concurrent operations.**
[server/src/modules/mixin/exchange/exchange.service.ts [579-630]](https://github.com/Hu-Fi/Mr.Market/pull/110/files#diff-b4d701c594440771180bbf709c88654f1d8a3806e45b2e035b3b36572ab3cda5R579-R630)
```diff
-await Promise.all(
- orders.map(async (o) => {
- const instance = this.exchangeInstances[o.exchangeName];
- if (!instance.has['fetchOrder']) {
- return await this.updateSpotOrderState(
- o.orderId,
- STATE_TEXT_MAP['EXCHANGE_DOESNT_SUPPORT_FETCH_ORDER'],
- );
- }
- ...
- }),
-);
+for (const o of orders) {
+ const instance = this.exchangeInstances[o.exchangeName];
+ if (!instance.has['fetchOrder']) {
+ await this.updateSpotOrderState(
+ o.orderId,
+ STATE_TEXT_MAP['EXCHANGE_DOESNT_SUPPORT_FETCH_ORDER'],
+ );
+ continue;
+ }
+ ...
+}
```
|
User description
Add order details and history page Add connect wallet in history page Add get order info handler
Type
enhancement, bug_fix
Description
Changes walkthrough
10 files
spot.ts
Add Functions to Fetch Orders in Hufi Helperinterface/src/lib/helpers/hufi/spot.ts
getOrdersByUserandgetOrderByIdfunctions for fetching ordersby user ID and order ID respectively.
mixin.ts
Integrate Order Fetching in Mixin Helperinterface/src/lib/helpers/mixin.ts
getOrdersByUserfromhufi/spot.getUserOrdersfunction to fetch and set user orders.getUserOrdersinAfterMixinOauthto fetch orders after OAuth.wallet.ts
Add Stores for User Orders Managementinterface/src/lib/stores/wallet.ts
userOrdersanduserOrdersLoadedstores for managing user orders.spot.ts
Define Type for Spot Ordersinterface/src/lib/types/hufi/spot.ts - Defined `SpotOrder` type for representing spot orders.
+page.ts
Implement Order Details Fetching for History Pageinterface/src/routes/(secondary)/(spot)/spot/history/[id]/+page.ts - Added server-side logic to validate and fetch order details by ID.
exchange-client.controller.ts
Add Endpoints for Orders Fetching in Exchange Moduleserver/src/modules/mixin/exchange/exchange-client.controller.ts - Added endpoints for fetching orders by user and order ID.
historys.svelte
Implement History Page UI for Displaying Ordersinterface/src/lib/components/spot/history/historys.svelte
integration.
openOrders.svelte
Update Open Orders Display with Fetched Ordersinterface/src/lib/components/spot/manage/openOrders.svelte - Updated open orders display to integrate with fetched orders.
singleOrder.svelte
Modify Single Order Component for Fetched Datainterface/src/lib/components/spot/manage/singleOrder.svelte - Modified single order component to use fetched order data.
orderDetail.svelte
Create Order Detail Component for History Pageinterface/src/lib/components/spot/history/orderDetail.svelte
information.