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 1:04pm |
railway-app[bot]
commented
3 months ago This PR is being deployed to Railway 🚅
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/8f272281d99e8e3129de5381e953b4b930aa5144)
github-actions[bot]
commented
3 months ago | ⏱️ Estimated effort to review [1-5] | 4, due to the extensive changes across multiple files, including both frontend and backend modifications. The PR introduces new functionalities, constants, and UI components, which requires a thorough review to ensure code quality, functionality, and integration with existing systems. |
| 🧪 Relevant tests | No |
| 🔍 Possible issues | Possible Bug: The `GenerateArbitrageMemo` and `GenerateMarketMakingMemo` functions in `interface/src/lib/helpers/memo.ts` are incomplete and do not return any value. This could lead to runtime errors when these functions are called. |
| Code Duplication: There is a noticeable amount of code duplication, especially in the Svelte components for dialogs and navigation bars. This could be refactored to improve maintainability. | |
| Hardcoded Values: The use of hardcoded exchange names and symbols in `SUPPORTED_ARBITRAGE_PAIRS` and `SUPPORTED_MARKET_MAKING_PAIRS` within `interface/src/lib/helpers/constants.ts` might limit flexibility and scalability. Consider fetching these from a server or configuration file. | |
| Incomplete Implementation: The payment functions `ArbitragePay` and `MarketMakingPay` in `interface/src/lib/helpers/mixin.ts` only log the parameters to the console and do not perform any actual payment logic. This seems like an incomplete implementation. | |
| 🔒 Security concerns | No |
| relevant file | interface/src/lib/helpers/memo.ts |
| suggestion | Implement the logic for `GenerateArbitrageMemo` and `GenerateMarketMakingMemo` functions or remove them if they are not needed. Currently, these functions are placeholders without any return statement, which could lead to unexpected behavior. [important] |
| relevant line | export const GenerateArbitrageMemo = () => { |
| relevant file | interface/src/lib/helpers/mixin.ts |
| suggestion | Implement the actual payment logic in `ArbitragePay` and `MarketMakingPay` functions or integrate with an existing payment service. Currently, these functions only log the input parameters, which indicates incomplete functionality. [important] |
| relevant line | export const ArbitragePay = ({ exchange0, exchange1, symbol, amount }: { exchange0: string, exchange1: string, symbol: string, amount: string }) => { |
| relevant file | interface/src/lib/helpers/constants.ts |
| suggestion | Consider dynamically fetching `SUPPORTED_ARBITRAGE_PAIRS` and `SUPPORTED_MARKET_MAKING_PAIRS` from a backend service or configuration file instead of hardcoding them. This approach enhances flexibility and eases updates. [medium] |
| relevant line | export const SUPPORTED_ARBITRAGE_PAIRS = [ |
| relevant file | interface/src/lib/components/dialogs/grow/arbitrage/confirmPayment.svelte |
| suggestion | Refactor the `ConfirmPayment` component to handle payment confirmation dynamically based on the transaction status. Currently, it seems to only simulate the payment process without actual transaction handling. [medium] |
| relevant line | ``` |
Abstract SVG icons into separate components for better reusability and maintainability.___ **To improve code readability and maintainability, consider abstracting the SVG icons intoseparate Svelte components. This will make it easier to reuse icons across your application.** [interface/src/lib/components/grow/arbitrage/new/easy/exchanges.svelte [59-68]](https://github.com/Hu-Fi/Mr.Market/pull/109/files#diff-76a6a96298f7905c2da1e23483669b1951f5de3e0b25f37df050d78eb0878233R59-R68) ```diff + | |
Improve class name for better readability.___ **Consider using a more descriptive class name thanno-animation for the button element to improve code readability and maintainability. The current class name does not convey the purpose or effect of the class.** [interface/src/lib/components/grow/elements/entrance.svelte [19]](https://github.com/Hu-Fi/Mr.Market/pull/109/files#diff-9dd438341b4932b318db300bf202ffda6e31919d222daae0f0c96e69117e6621R19-R19) ```diff -"items-center justify-between no-animation hover:bg-slate-50 focus:bg-slate-50 bg-slate-50 focus:border-none border-none flex flex-col space-y-2.5 rounded-2xl py-5", +"items-center justify-between hover:bg-slate-50 focus:bg-slate-50 bg-slate-50 focus:border-none border-none flex flex-col space-y-2.5 rounded-2xl py-5", ``` | |
Dynamically generate SVG elements to reduce redundancy.___ **Use a loop to generate SVG elements dynamically based on theitems array to reduce redundancy and improve code maintainability.** [interface/src/lib/components/grow/elements/entrance.svelte [26-36]](https://github.com/Hu-Fi/Mr.Market/pull/109/files#diff-9dd438341b4932b318db300bf202ffda6e31919d222daae0f0c96e69117e6621R26-R36) ```diff -{#if i === 0} - - | |
| Best practice |
Use a reactive statement for conditional class names to simplify the template.___ **Avoid using the ternary operator directly inside the class attribute for conditionalstyling. Instead, use a reactive statement or a computed property to clean up the template.** [interface/src/lib/components/grow/arbitrage/new/easy/exchanges.svelte [47-50]](https://github.com/Hu-Fi/Mr.Market/pull/109/files#diff-76a6a96298f7905c2da1e23483669b1951f5de3e0b25f37df050d78eb0878233R47-R50) ```diff -class={clsx( - "capitalize font-medium", - $createArbExchange1 ? "" : "opacity-40", -)} +class={exchange1Class} + + ``` |
Add meaningful alternative text to images for accessibility and SEO.___ **To ensure accessibility and improve SEO, provide meaningful alternative text for imagesinstead of leaving the alt attribute empty.**
[interface/src/lib/components/grow/arbitrage/new/easy/exchanges.svelte [39-41]](https://github.com/Hu-Fi/Mr.Market/pull/109/files#diff-76a6a96298f7905c2da1e23483669b1951f5de3e0b25f37df050d78eb0878233R39-R41)
```diff
| |
| Performance |
Memoize repeated operations within loops for better performance.___ **For better performance and to avoid unnecessary re-renders, consider memoizing the resultof $createArbPair.split("/") since it's used multiple times within an #each block.**
[interface/src/lib/components/grow/arbitrage/new/easy/exchanges.svelte [211-213]](https://github.com/Hu-Fi/Mr.Market/pull/109/files#diff-76a6a96298f7905c2da1e23483669b1951f5de3e0b25f37df050d78eb0878233R211-R213)
```diff
- |
| Accessibility |
Add
___
**Ensure that the SVG elements have an |
github-actions[bot]
commented
3 months ago PR Description updated to latest commit (https://github.com/Hu-Fi/Mr.Market/commit/5f35582a7f3b4f5c74927431d1e585f9f3b4475c)
github-actions[bot]
commented
3 months ago Persistent review updated to latest commit https://github.com/Hu-Fi/Mr.Market/commit/5f35582a7f3b4f5c74927431d1e585f9f3b4475c
github-actions[bot]
commented
3 months ago | Category | Suggestions |
| Enhancement |
Add type definitions for trading pair objects.___ **Consider using a type definition for the objects withinSUPPORTED_ARBITRAGE_PAIRS and SUPPORTED_MARKET_MAKING_PAIRS arrays to ensure consistency and facilitate maintenance. This can be particularly useful if the structure of these objects changes in the future or if more properties are added.** [interface/src/lib/helpers/constants.ts [255-265]](https://github.com/Hu-Fi/Mr.Market/pull/109/files#diff-1d9500711f0f58654b9e0e95aa0e7fdc798a0b74f1c2310b09e52123e52d6bf2R255-R265) ```diff -export const SUPPORTED_ARBITRAGE_PAIRS = [ +type TradingPair = { symbol: string; exchange: string; }; +export const SUPPORTED_ARBITRAGE_PAIRS: TradingPair[] = [ {symbol: "BTC/USDT", exchange: "okx",}, ... ]; ``` |
Implement memo generation functions.___ **Implement the logic insideGenerateArbitrageMemo and GenerateMarketMakingMemo functions to ensure they return a properly formatted memo string. Currently, these functions are placeholders without implementation.** [interface/src/lib/helpers/memo.ts [64-76]](https://github.com/Hu-Fi/Mr.Market/pull/109/files#diff-dccebfba812d3919c4dcd66ee3972b852d0cbf22df17f745e83e08a5adb34767R64-R76) ```diff -export const GenerateArbitrageMemo = () => { - // 1. AB (Memo Type) - ... +export const GenerateArbitrageMemo = (): string => { + // Example implementation + return 'AB:CR:01:02:Z7GC'; }; ``` | |
Implement payment handling in arbitrage and market making functions.___ **Replace the placeholderconsole.log statements in ArbitragePay and MarketMakingPay functions with actual logic to handle payments for arbitrage and market making respectively. This will ensure that the functions perform their intended actions rather than just logging to the console.** [interface/src/lib/helpers/mixin.ts [248-253]](https://github.com/Hu-Fi/Mr.Market/pull/109/files#diff-acf423272bdc878982f8a993563883c5ac16fcef547cb0e7a09972c785a5e839R248-R253) ```diff export const ArbitragePay = ({ exchange0, exchange1, symbol, amount }: { exchange0: string, exchange1: string, symbol: string, amount: string }) => { - console.log(exchange0, exchange1, symbol, amount) + // Example implementation + // Perform payment logic here }; ``` | |
Implement memo decoding functions for arbitrage and market making.___ **Complete the implementation ofdecodeArbitrageMemo and decodeMarketMakingMemo functions to properly decode the memo strings for arbitrage and market making. This is crucial for processing and validating payment memos correctly.** [server/src/common/helpers/mixin/memo.ts [43-59]](https://github.com/Hu-Fi/Mr.Market/pull/109/files#diff-996ebc0a0ef85b44a5c76b6f52e33e03b879f9bc3b8ef861205bb803d309b983R43-R59) ```diff -export const decodeArbitrageMemo = (encodedMemo: string) => { - // Decode base64 - ... +export const decodeArbitrageMemo = (encodedMemo: string): ArbitrageMemoDetails | null => { + // Example implementation + const decodedMemo = Buffer.from(encodedMemo, 'base64').toString('utf-8'); + const parts = decodedMemo.split(':'); + if (parts.length !== expectedPartsLength) return null; + return { + tradingType: parts[0], + action: parts[1], + exchangeAIndex: parts[2], + exchangeBIndex: parts[3], + symbol: parts[4], + }; }; ``` | |
Use a single function to handle modal close actions based on the
___
**To enhance the user experience and reduce code duplication, consider using a single | |
Disable payment buttons while a payment is in progress to prevent multiple submissions.___ **To ensure a consistent user experience, consider disabling the payment buttons while apayment is in progress. This prevents multiple submissions and makes the UI more intuitive.** [interface/src/lib/components/dialogs/grow/market_making/confirmPayment.svelte [65]](https://github.com/Hu-Fi/Mr.Market/pull/109/files#diff-ca3c328d7b2523ef171dbff76e6761a8c2e02f1c9f7e3f88341002f1a716531bR65-R65) ```diff | |
Add meaningful alt attributes to images for better accessibility.___ **For better accessibility and user experience, add meaningfulalt attributes to all This is especially important for users who rely on screen readers.** [interface/src/lib/components/grow/arbitrage/new/easy/exchanges.svelte [39-41]](https://github.com/Hu-Fi/Mr.Market/pull/109/files#diff-76a6a96298f7905c2da1e23483669b1951f5de3e0b25f37df050d78eb0878233R39-R41) ```diff | |
Improve class name for clarity.___ **Consider using a more descriptive class name instead ofno-animation to clearly indicate the purpose or behavior of the elements it's applied to. If no-animation is intended to disable animations, consider a name like disable-animation for better clarity.**
[interface/src/lib/components/grow/elements/entrance.svelte [19]](https://github.com/Hu-Fi/Mr.Market/pull/109/files#diff-9dd438341b4932b318db300bf202ffda6e31919d222daae0f0c96e69117e6621R19-R19)
```diff
-"items-center justify-between no-animation hover:bg-slate-50 focus:bg-slate-50 bg-slate-50 focus:border-none border-none flex flex-col space-y-2.5 rounded-2xl py-5",
+"items-center justify-between disable-animation hover:bg-slate-50 focus:bg-slate-50 bg-slate-50 focus:border-none border-none flex flex-col space-y-2.5 rounded-2xl py-5",
```
| |
Add transition effects for better UX.___ **To enhance the user experience, consider adding a transition effect for the hover andfocus states of the buttons. This can make the interaction smoother and more visually appealing.** [interface/src/lib/components/grow/elements/entrance.svelte [19]](https://github.com/Hu-Fi/Mr.Market/pull/109/files#diff-9dd438341b4932b318db300bf202ffda6e31919d222daae0f0c96e69117e6621R19-R19) ```diff -"items-center justify-between no-animation hover:bg-slate-50 focus:bg-slate-50 bg-slate-50 focus:border-none border-none flex flex-col space-y-2.5 rounded-2xl py-5", +"items-center justify-between transition-colors duration-200 ease-in-out hover:bg-slate-50 focus:bg-slate-50 bg-slate-50 focus:border-none border-none flex flex-col space-y-2.5 rounded-2xl py-5", ``` | |
| Bug |
Correct function call for decoding market making memos.___ **Correct the typo in callingdecodeArbitrageMemo for decoding market making memos inside the SnapshotsService class. Use a dedicated function for decoding market making memos or ensure the correct function is called for each case.** [server/src/modules/mixin/snapshots/snapshots.service.ts [494-495]](https://github.com/Hu-Fi/Mr.Market/pull/109/files#diff-dd336e520fcec14b837cd72c0852b94e85fb8e130fc2a3431b49fef7e4d94c29R494-R495) ```diff case 'MM': - const mmDetails = decodeArbitrageMemo(snapshot.memo); + const mmDetails = decodeMarketMakingMemo(snapshot.memo); ``` |
| Maintainability |
Refactor the payment function to use an object for managing button states.___ **To improve code maintainability and avoid potential bugs, consider using a more robustapproach for handling the payment function. Instead of using separate variables for each button's state, you could use an object to manage the states. This approach simplifies the function and makes it easier to extend in the future.** [interface/src/lib/components/dialogs/grow/arbitrage/confirmPayment.svelte [17-22]](https://github.com/Hu-Fi/Mr.Market/pull/109/files#diff-94efc6491a5ac4ba49c323d40db203e28413357471538d2f387436823a658dfeR17-R22) ```diff +const paymentStates = { + '1': { loading: false, paid: false }, + '2': { loading: false, paid: false }, +}; + const payment = (type: string) => { - if (type === '1') btn1Loading = true; - if (type === '2') btn2Loading = true; - btn1Paid = false; - btn2Paid = false; + paymentStates[type].loading = true; + paymentStates['1'].paid = false; + paymentStates['2'].paid = false; } ``` |
Extract button click logic into a separate function for better readability.___ **To improve code readability and maintainability, consider extracting the button clicklogic into a separate function. This makes the template cleaner and the logic easier to understand and modify.** [interface/src/lib/components/dialogs/grow/arbitrage/selectPair.svelte [44]](https://github.com/Hu-Fi/Mr.Market/pull/109/files#diff-57866b5503464f8fc6fba691189e9728f79f7e49092e9b44926480439c406cddR44-R44) ```diff -on:click={()=>{ createArbPair.set(pair); selectArbPairDialog.set(false); createArbAmount.set([]); }} +const selectPair = (pair) => { + createArbPair.set(pair); + selectArbPairDialog.set(false); + createArbAmount.set([]); +} +on:click={() => selectPair(pair)} + ``` | |
Use a separate function for toggling dialog visibility to improve code readability.___ **Consider using a reactive statement to handle the toggling ofselectArbExchange1Dialog and selectArbExchange2Dialog instead of doing it inline within the on:click event. This will make your code more readable and maintainable.** [interface/src/lib/components/grow/arbitrage/new/easy/exchanges.svelte [31-33]](https://github.com/Hu-Fi/Mr.Market/pull/109/files#diff-76a6a96298f7905c2da1e23483669b1951f5de3e0b25f37df050d78eb0878233R31-R33) ```diff -on:click={() => { +on:click={toggleSelectArbExchange1Dialog} + + ``` | |
Create a reusable component for the caret icons to reduce code duplication.___ **To avoid repetition and improve maintainability, create a reusable component for the"Caret down Icon" and "Caret up Icon" since they are used multiple times throughout the file.** [interface/src/lib/components/grow/arbitrage/new/easy/exchanges.svelte [124-133]](https://github.com/Hu-Fi/Mr.Market/pull/109/files#diff-76a6a96298f7905c2da1e23483669b1951f5de3e0b25f37df050d78eb0878233R124-R133) ```diff - - + | |
| Performance |
Merge repeated split operations into a single operation for efficiency.___ **To improve code efficiency and readability, consider merging the repeated calls tocreateArbPair.split("/") into a single operation that assigns both baseAssetSymbol and targetAssetSymbol in one go.**
[interface/src/lib/components/dialogs/grow/arbitrage/confirmPayment.svelte [7-9]](https://github.com/Hu-Fi/Mr.Market/pull/109/files#diff-94efc6491a5ac4ba49c323d40db203e28413357471538d2f387436823a658dfeR7-R9)
```diff
-$: baseAssetSymbol = $createArbPair.split("/")[0] || ''
-$: targetAssetSymbol = $createArbPair.split("/")[1] || ''
+$: [baseAssetSymbol, targetAssetSymbol] = $createArbPair.split("/");
+$: baseAssetSymbol = baseAssetSymbol || '';
+$: targetAssetSymbol = targetAssetSymbol || '';
```
|
| Best practice |
Use reactive statements for conditional styling to clean up the template.___ **Avoid using the ternary operator directly inside the class attribute for conditionalstyling. Instead, use a reactive statement or a computed property to clean up the template code.** [interface/src/lib/components/grow/arbitrage/new/easy/exchanges.svelte [47-50]](https://github.com/Hu-Fi/Mr.Market/pull/109/files#diff-76a6a96298f7905c2da1e23483669b1951f5de3e0b25f37df050d78eb0878233R47-R50) ```diff -class={clsx( - "capitalize font-medium", - $createArbExchange1 ? "" : "opacity-40", -)} +class={exchange1Class} + + ``` |
Use a derived store or reactive statement for splitting pair tokens to enhance code clarity.___ **Instead of using theArray(2) directly in the #each block, consider defining a derived store or a reactive statement that represents the split parts of $createArbPair. This will make the code more descriptive and easier to understand.** [interface/src/lib/components/grow/arbitrage/new/easy/exchanges.svelte [208]](https://github.com/Hu-Fi/Mr.Market/pull/109/files#diff-76a6a96298f7905c2da1e23483669b1951f5de3e0b25f37df050d78eb0878233R208-R208) ```diff -{#each Array(2) as _, i} +{#each pairTokens as token, i} + + ``` | |
Use a component for SVG icons for better maintainability.___ **To ensure consistency and maintainability, consider using a Svelte component for the SVGicons instead of inline SVG. This will make it easier to reuse, scale, and manage the icons across your application.** [interface/src/lib/components/grow/elements/entrance.svelte [28]](https://github.com/Hu-Fi/Mr.Market/pull/109/files#diff-9dd438341b4932b318db300bf202ffda6e31919d222daae0f0c96e69117e6621R28-R28) ```diff - + | |
Declare props explicitly for clarity.___ **It's a good practice to explicitly declare all props used in the component at the top ofthe script tag. This enhances readability and maintainability by making it clear which props are expected by the component.** [interface/src/lib/components/grow/elements/entrance.svelte [11]](https://github.com/Hu-Fi/Mr.Market/pull/109/files#diff-9dd438341b4932b318db300bf202ffda6e31919d222daae0f0c96e69117e6621R11-R11) ```diff ``` | |
| Accessibility |
Add
___
**For better accessibility, consider adding |
User description
Create arbitrage and market making UI The memo decoder of arbitrage and market making
Type
enhancement, bug_fix
Description
Changes walkthrough
21 files
constants.ts
Add Constants for Arbitrage and Market Making Pairsinterface/src/lib/helpers/constants.ts
SUPPORTED_ARBITRAGE_PAIRSandSUPPORTED_MARKET_MAKING_PAIRSconstants.
SUPPORTED_UNIQUE_ARBITRAGE_PAIRSconstant.memo.ts
Implement Memo Generation for Arbitrage and Market Makinginterface/src/lib/helpers/memo.ts
GenerateArbitrageMemoandGenerateMarketMakingMemofunctions.mixin.ts
Add Payment Functions for Arbitrage and Market Makinginterface/src/lib/helpers/mixin.ts - Added `ArbitragePay` and `MarketMakingPay` functions.
grow.ts
Add Svelte Stores for Arbitrage and Market Making UI Statesinterface/src/lib/stores/grow.ts
states.
spot.ts
Cleanup and Adjustments in Spot Storeinterface/src/lib/stores/spot.ts - Minor adjustments and cleanup in spot store.
memo.ts
Define Constants for Arbitrage and Market Making Memo Actionsserver/src/common/constants/memo.ts - Added constants for arbitrage and market making memo actions.
memo.ts
Implement Memo Decoding for Arbitrage and Market Makingserver/src/common/helpers/mixin/memo.ts
decodeArbitrageMemoanddecodeMarketMakingMemofunctions.memo.ts
Add Interfaces for Arbitrage and Market Making Memo Detailsserver/src/common/types/memo/memo.ts - Added interfaces for arbitrage and market making memo details.
snapshots.service.ts
Extend Snapshot Service for Arbitrage and Market Making Memosserver/src/modules/mixin/snapshots/snapshots.service.ts
en.json
Add i18n Entries for Arbitrage and Market Making UIinterface/src/i18n/en.json - Added new entries for arbitrage and market making UI texts.
growNewArbNav.svelte
Create Bottom Navigation for New Arbitrageinterface/src/lib/components/bottomNav/growNewArbNav.svelte - Created a new bottom navigation component for arbitrage creation.
growNewMMNav.svelte
Create Bottom Navigation for New Market Makinginterface/src/lib/components/bottomNav/growNewMMNav.svelte
bottomTrade.svelte
Adjust Bottom Trade Dialog Componentinterface/src/lib/components/dialogs/bottomTrade.svelte - Minor adjustments in the bottom trade dialog component.
confirmPayment.svelte
Add Confirm Payment Dialog for Arbitrageinterface/src/lib/components/dialogs/grow/arbitrage/confirmPayment.svelte - Added a new dialog component for confirming arbitrage payments.
selectExchange.svelte
Add Select Exchange Dialog for Arbitrageinterface/src/lib/components/dialogs/grow/arbitrage/selectExchange.svelte - Added a new dialog component for selecting exchanges in arbitrage.
selectPair.svelte
Add Select Trading Pair Dialog for Arbitrageinterface/src/lib/components/dialogs/grow/arbitrage/selectPair.svelte
confirmPayment.svelte
Add Confirm Payment Dialog for Market Makinginterface/src/lib/components/dialogs/grow/market_making/confirmPayment.svelte - Added a new dialog component for confirming market making payments.
exchanges.svelte
Create Component for Exchange and Pair Selection in Arbitrage Creationinterface/src/lib/components/grow/arbitrage/new/easy/exchanges.svelte
creation.
amount.svelte
Create Component for Entering Amounts in Market Making Creationinterface/src/lib/components/grow/marketMaking/new/easy/amount.svelte
creation.
exchange.svelte
Create Component for Displaying Exchange and Pair in Market MakingCreationinterface/src/lib/components/grow/marketMaking/new/easy/exchange.svelte
making creation.
pair.svelte
Create Component for Selecting Trading Pair in Market Making Creationinterface/src/lib/components/grow/marketMaking/new/easy/pair.svelte
creation.