vercel[bot]
commented
5 months ago The latest updates on your projects. Learn more about Vercel for Git ↗︎
| Name | Status | Preview | Updated (UTC) |
|---|---|---|---|
| mr-market | ✅ Ready (Inspect) | Visit Preview | Apr 11, 2024 10:05am |
Closed zed-wong closed 5 months ago
vercel[bot]
commented
5 months ago The latest updates on your projects. Learn more about Vercel for Git ↗︎
| Name | Status | Preview | Updated (UTC) |
|---|---|---|---|
| mr-market | ✅ Ready (Inspect) | Visit Preview | Apr 11, 2024 10:05am |
railway-app[bot]
commented
5 months ago This PR is being deployed to Railway 🚅
railway-app[bot]
commented
5 months ago This PR is being deployed to Railway 🚅
Mr.Market: ◻️ REMOVED
github-actions[bot]
commented
5 months ago PR Description updated to latest commit (https://github.com/Hu-Fi/Mr.Market/commit/59b28d26ef6587f293163d4fb76a7929e4e4758c)
github-actions[bot]
commented
5 months ago | ⏱️ Estimated effort to review [1-5] | 3, because the PR introduces a significant amount of changes across multiple files, including both frontend and backend modifications. The changes involve adding new features, fixing bugs, and refactoring code, which requires a thorough review to ensure compatibility and adherence to best practices. |
| 🧪 Relevant tests | No |
| 🔍 Possible issues | Possible Bug: The use of `replaceAll` for removing '-ERC20' from the pair string in `interface/src/routes/(secondary)/(grow)/grow/arbitrage/[id]/+page.svelte` and `interface/src/routes/(secondary)/(grow)/grow/market_making/[id]/+page.svelte` might not be the most reliable method for handling token symbols. If the intention is to display a more user-friendly token symbol, consider implementing a more robust solution that maps ERC20 tokens to their symbols without relying on string manipulation. |
| Performance Concern: The `setInterval` function used in `interface/src/lib/components/dialogs/grow/arbitrage/confirmPayment.svelte` and `interface/src/lib/components/dialogs/grow/market_making/confirmPayment.svelte` for polling payment state might lead to unnecessary network requests and performance degradation, especially if the interval is too short or if many instances are running simultaneously. Consider using a more efficient method for state updates, such as WebSocket or Server-Sent Events (SSE), to reduce the number of requests and improve user experience. | |
| 🔒 Security concerns | No |
| relevant file | interface/src/lib/components/dialogs/grow/arbitrage/confirmPayment.svelte |
| suggestion | Consider implementing a more efficient method for updating payment state than polling with `setInterval`. Using WebSocket or Server-Sent Events (SSE) can reduce the number of network requests and improve the user experience by providing real-time updates without the need for frequent polling. [important] |
| relevant line | var interval = setInterval(async () => { |
| relevant file | interface/src/routes/(secondary)/(grow)/grow/arbitrage/[id]/+page.svelte |
| suggestion | Implement a more robust solution for displaying user-friendly token symbols instead of using `replaceAll` to remove '-ERC20'. Consider creating a mapping of ERC20 tokens to their symbols or using a utility function that can handle various token formats more reliably. This approach can prevent potential bugs and improve code maintainability. [important] |
| relevant line | const pairTarget = $page.data.data.pair.split('/')[1].replaceAll('-ERC-20', '') |
| relevant file | interface/src/lib/components/grow/marketMaking/new/easy/exchange.svelte |
| suggestion | Ensure that cleanup actions in the `onDestroy` lifecycle method do not inadvertently reset user progress or data that might still be needed. If the intention is to reset form fields or state when the component is destroyed, consider adding checks to ensure that this behavior is clear to the user and does not lead to data loss or confusion. [medium] |
| relevant line | onDestroy(() => { |
github-actions[bot]
commented
5 months ago | Category | Suggestions |
| Enhancement |
Improve error handling instead of just logging to the console.___ **Instead of logging the error to the console, it's better to handle errors gracefully.Consider using a more robust error handling strategy, such as displaying a user-friendly message or retrying the request.** [interface/src/lib/helpers/hufi/coin.ts [24-25]](https://github.com/Hu-Fi/Mr.Market/pull/147/files#diff-256a64b9ed50e82ac22104895399dbc403c4aae9a4b3901816b45b49d91e499cR24-R25) ```diff -console.error('Server error') +// Assuming there's a function to handle errors more gracefully +handleError('Server error') return [] ``` |
Add error handling for failed fetch requests in
___
**The function | |
Ensure
___
**The check for | |
Provide specific error handling when
___
**The check for | |
Initialize the
___
**Consider initializing the | |
Refactor inline conditional class application to a function for scalability and maintainability.___ **Consider using a more scalable approach for applying classes conditionally. Instead ofinline ternary operations for each condition, you could use a function that returns the appropriate class names based on the value of i. This will make the code cleaner and easier to maintain, especially if more conditions are added in the future.** [interface/src/lib/components/grow/elements/entrance.svelte [25]](https://github.com/Hu-Fi/Mr.Market/pull/147/files#diff-9dd438341b4932b318db300bf202ffda6e31919d222daae0f0c96e69117e6621R25-R25) ```diff -
+
+
```
| |
| Best practice |
Define a specific interface for the return type of
___
**The method |
Simplify entity creation and persistence with direct save method usage.___ **Instead of manually creating and saving entities, consider using TypeORM'ssave method directly with the object literal to leverage its upsert capability and reduce boilerplate code.** [server/src/modules/strategy/strategy.service.ts [665-678]](https://github.com/Hu-Fi/Mr.Market/pull/147/files#diff-413cb1b28e0d47a46768f97d10145a8e14d9e46b0a195768786127305916d944R665-R678) ```diff -const arbitrageOrder = this.arbitrageHistoryRepository.create({ +await this.arbitrageHistoryRepository.save({ userId, clientId, pair: symbol, }); -await this.arbitrageHistoryRepository.save(arbitrageOrder); ``` | |
Use a custom class for specific width to enhance style consistency.___ **Use a more specific class name thanmin-w-36 to ensure consistent styling across different components and avoid potential conflicts with utility classes.** [interface/src/lib/components/bottomNav/growNewArbNav.svelte [17]](https://github.com/Hu-Fi/Mr.Market/pull/147/files#diff-47e79012c29c3dbeb1cc2c42e1b21463e473626a3c6e67071c1aec9c8ecf16faR17-R17) ```diff -class={clsx("btn btn-md min-w-36 !h-[2.5rem] border-none bg-base-content hover:bg-base-content rounded-full text-base-100 no-animation", !inputValid && "btn-disabled")} +class={clsx("btn btn-md custom-min-width !h-[2.5rem] border-none bg-base-content hover:bg-base-content rounded-full text-base-100 no-animation", !inputValid && "btn-disabled")} ``` | |
Define a TypeScript interface for the
___
**To improve type safety and code readability, consider defining a TypeScript interface for | |
Review the necessity of cleanup logic in
___
**Ensure that the cleanup logic in | |
| Maintainability |
Rename method to more accurately describe its functionality.___ **Consider using a more descriptive method name thangetUserOrders for fetching market making history, to avoid confusion with other order fetching methods.** [server/src/modules/strategy/strategy.controller.ts [136-148]](https://github.com/Hu-Fi/Mr.Market/pull/147/files#diff-1e7694e47e509da7fcbde80792f7da820604e376c9521762faf21e6a1264c3beR136-R148) ```diff -async getUserOrders(userId: string): Promise |
Refactor interval logic into a separate function for clarity.___ **Extract the interval logic into a separate function to improve readability andmaintainability.** [interface/src/lib/components/dialogs/grow/arbitrage/confirmPayment.svelte [59-78]](https://github.com/Hu-Fi/Mr.Market/pull/147/files#diff-94efc6491a5ac4ba49c323d40db203e28413357471538d2f387436823a658dfeR59-R78) ```diff -var interval = setInterval(async () => { - const state = await getPaymentState(orderId); - totalTime += ORDER_STATE_FETCH_INTERVAL; - if (!state) { - return; - } - if (state.data.firstSnapshotId) { - btn1Loading = false; - btn1Paid = true; - } - if (state.data.secondSnapshotId) { - clearInterval(interval); - goto(`/grow/arbitrage/${orderId}`); - } else if (totalTime >= ORDER_STATE_TIMEOUT_DURATION) { - clearInterval(interval); - console.log('Timeout reached, stopping execution.'); - } -}, ORDER_STATE_FETCH_INTERVAL); +checkPaymentState(); +function checkPaymentState() { + var interval = setInterval(async () => { + const state = await getPaymentState(orderId); + totalTime += ORDER_STATE_FETCH_INTERVAL; + if (!state) { + return; + } + if (state.data.firstSnapshotId) { + btn1Loading = false; + btn1Paid = true; + } + if (state.data.secondSnapshotId) { + clearInterval(interval); + goto(`/grow/arbitrage/${orderId}`); + } else if (totalTime >= ORDER_STATE_TIMEOUT_DURATION) { + clearInterval(interval); + console.log('Timeout reached, stopping execution.'); + } + }, ORDER_STATE_FETCH_INTERVAL); +} + ``` | |
Remove or properly handle commented-out code to avoid confusion.___ **Instead of commenting out code for future use, consider removing it or implementing afeature toggle. Commented-out code can lead to confusion and clutter in the codebase.** [interface/src/lib/components/grow/arbitrage/details/infos.svelte [7-8]](https://github.com/Hu-Fi/Mr.Market/pull/147/files#diff-fdc4a6d412562e0c5527e1c40fac00778a0420658220c4e02239fa5e44fe5bd3R7-R8) ```diff -// export let amount0Start = 0; -// export let amount1Start = 0; +// Consider removing these lines if they are not needed. ``` | |
| Performance |
Remove unnecessary
___
**Avoid using |
| Accessibility |
Improve image accessibility by providing descriptive alt text.___ **Use a more descriptive alt text for images to improve accessibility. For example, insteadof leaving the alt attribute empty, you could describe the image or the action it represents.** [interface/src/lib/components/grow/arbitrage/card.svelte [30-35]](https://github.com/Hu-Fi/Mr.Market/pull/147/files#diff-c5c5ebe8bae11dae01ebd79e77396a6018c24e12ef077d425fbe7f6ea12a62e4R30-R35) ```diff - |
github-actions[bot]
commented
5 months ago PR Description updated to latest commit (https://github.com/Hu-Fi/Mr.Market/commit/59b28d26ef6587f293163d4fb76a7929e4e4758c)
github-actions[bot]
commented
5 months ago Persistent review updated to latest commit https://github.com/Hu-Fi/Mr.Market/commit/59b28d26ef6587f293163d4fb76a7929e4e4758c
github-actions[bot]
commented
5 months ago | Category | Suggestions |
| Enhancement |
Replace console.error with throwing an Error for better error handling.___ **Instead of logging the error to the console, consider handling the error more gracefully.You could potentially return an error object or throw a custom error to be caught by the calling function, allowing for more robust error handling and debugging.** [interface/src/lib/helpers/hufi/coin.ts [24-25]](https://github.com/Hu-Fi/Mr.Market/pull/147/files#diff-256a64b9ed50e82ac22104895399dbc403c4aae9a4b3901816b45b49d91e499cR24-R25) ```diff -console.error('Server error') -return [] +throw new Error('Server error') ``` |
Improve error handling by returning an error object in catch blocks.___ **It's recommended to handle the catch block by returning a specific error object or nullinstead of just logging to the console. This way, the calling function can handle the error based on the returned value.** [interface/src/lib/helpers/hufi/strategy.ts [15]](https://github.com/Hu-Fi/Mr.Market/pull/147/files#diff-44e02f14022d0b410cbac941d9268b0e209909bca68e3dbce6cbf50f1e68788bR15-R15) ```diff -console.error('Error fetching payment state:', error); +return { error: 'Error fetching payment state', details: error }; ``` | |
Add a more explicit check for the data array in the sortCoins function.___ **To ensure the functionsortCoins is more robust, consider explicitly checking for an array and its length before attempting to sort. This prevents potential runtime errors when data is undefined or empty.** [interface/src/lib/helpers/sortTable.ts [42-44]](https://github.com/Hu-Fi/Mr.Market/pull/147/files#diff-53e861ddae0f6bf506b78171ab5a55abdd6c4e49940df22de7b047c3cd276223R42-R44) ```diff -if (!data) { - return data; +if (!Array.isArray(data) || data.length === 0) { + return []; } ``` | |
Enhance data validation in marketData derived store.___ **Instead of checking ifparams is an array, it might be more accurate to check if it's not undefined and contains the expected data structure before proceeding. This ensures that the data manipulation logic only runs when valid data is present.** [interface/src/lib/stores/market.ts [59-61]](https://github.com/Hu-Fi/Mr.Market/pull/147/files#diff-72258a776e444a641d4fa06eba2b515e923ec009db3017b8a22c453061b9375dR59-R61) ```diff -if (!Array.isArray(params)) { +if (!params || !Array.isArray(params) || params.length === 0) { marketDataState.set('error'); return; } ``` | |
Improve error handling in the
___
**Consider using a more specific exception filter or handling mechanism for different types | |
Replace hardcoded data object with a prop or store.___ **Instead of hardcoding thedata object within the script tag, consider fetching this data from a prop or a store. This approach enhances the component's reusability and maintainability by decoupling it from specific data, allowing it to be used in various contexts with different data sources.** [interface/src/lib/components/grow/arbitrage/card.svelte [7-18]](https://github.com/Hu-Fi/Mr.Market/pull/147/files#diff-c5c5ebe8bae11dae01ebd79e77396a6018c24e12ef077d425fbe7f6ea12a62e4R7-R18) ```diff -export let data = { - "orderId": "c825457f-c25d-4ada-ad9e-4b9847b4eb27", - ... -} +export let data; ``` | |
Implement conditional rendering for balance information.___ **Use a conditional rendering approach to display the balance information instead of leavinga TODO comment. This ensures that the component is fully functional and does not leave placeholders in the UI.** [interface/src/lib/components/grow/arbitrage/card.svelte [76]](https://github.com/Hu-Fi/Mr.Market/pull/147/files#diff-c5c5ebe8bae11dae01ebd79e77396a6018c24e12ef077d425fbe7f6ea12a62e4R76-R76) ```diff - +{#if data.balanceA && data.balanceB} + + Balance A: {data.balanceA} | Balance B: {data.balanceB} + +{/if} ``` | |
Enable and use start amount variables for percentage change calculation.___ **Uncomment and utilize theamount0Start and amount1Start variables to display the percentage change in the token amounts. This provides more insightful information to the user regarding the performance of their arbitrage.** [interface/src/lib/components/grow/arbitrage/details/infos.svelte [7-8]](https://github.com/Hu-Fi/Mr.Market/pull/147/files#diff-fdc4a6d412562e0c5527e1c40fac00778a0420658220c4e02239fa5e44fe5bd3R7-R8) ```diff -// export let amount0Start = 0; -// export let amount1Start = 0; +export let amount0Start = 0; +export let amount1Start = 0; ``` | |
Refactor conditional class application for readability and maintainability.___ **Consider refactoring the conditional class application to use a more concise and readableapproach. Instead of using multiple inline ternary operators, you can define a function that returns the appropriate class names based on the value of i. This will make the code easier to read and maintain.** [interface/src/lib/components/grow/elements/entrance.svelte [25]](https://github.com/Hu-Fi/Mr.Market/pull/147/files#diff-9dd438341b4932b318db300bf202ffda6e31919d222daae0f0c96e69117e6621R25-R25) ```diff -
+
+
```
| |
| Maintainability |
Implement the initialization for the minimum balance table.___ **Implement the initialization logic for the minimum balance table as indicated by the TODOcomment. This could involve setting up default values or loading initial data from a configuration file or database.** [server/src/modules/mixin/rebalance/rebalance.service.ts [28]](https://github.com/Hu-Fi/Mr.Market/pull/147/files#diff-b4be66a193644b9e3594919648875ca3f2bfe709433b125a55c4d405b9f33dedR28-R28) ```diff -// TODO: Initalize minium balance table +// Example initialization logic +this.initializeMinimumBalanceTable(); +// Further implementation needed ``` |
| Best practice |
Use transactions for creating and saving arbitrage orders.___ **To ensure the consistency of the database state, consider wrapping the operations ofcreating and saving the arbitrageOrder in a transaction.**
[server/src/modules/strategy/strategy.service.ts [665-678]](https://github.com/Hu-Fi/Mr.Market/pull/147/files#diff-413cb1b28e0d47a46768f97d10145a8e14d9e46b0a195768786127305916d944R665-R678)
```diff
-const arbitrageOrder = this.arbitrageHistoryRepository.create({
-...
-await this.arbitrageHistoryRepository.save(arbitrageOrder);
+await this.arbitrageHistoryRepository.manager.transaction(async transactionalEntityManager => {
+ const arbitrageOrder = transactionalEntityManager.create(ArbitrageHistory, {...});
+ await transactionalEntityManager.save(ArbitrageHistory, arbitrageOrder);
+});
```
|
Properly reset stores on component destruction.___ **Ensure to properly clean up or reset the stores in theonDestroy lifecycle hook to prevent potential memory leaks or unintended behavior in single-page applications (SPAs). This is particularly important for stores that might hold sensitive data or are used across different parts of the application.** [interface/src/lib/components/grow/arbitrage/new/easy/exchanges.svelte [23-30]](https://github.com/Hu-Fi/Mr.Market/pull/147/files#diff-76a6a96298f7905c2da1e23483669b1951f5de3e0b25f37df050d78eb0878233R23-R30) ```diff onDestroy(()=>{ - createArbAmount.set([]); - createArbExchange1.set(''); - createArbExchange2.set(''); - createArbPair.set(''); + createArbAmount.set(undefined); + createArbExchange1.set(undefined); + createArbExchange2.set(undefined); + createArbPair.set(undefined); createArbConfirmDialog.set(false); editArbitrageDialog.set(false); }) ``` | |
Specify a more detailed type for the histories prop.___ **Define a more specific type for thehistories prop to improve type safety and code readability. Using Array is too generic and does not provide enough information about the expected structure of the histories.** [interface/src/lib/components/grow/arbitrage/details/history.svelte [4]](https://github.com/Hu-Fi/Mr.Market/pull/147/files#diff-ba3f51ae862329837ed9687f752b20ce3615feda616ea65de6572adef2a6ad64R4-R4) ```diff -export let histories: Array | |
| Performance |
Add pagination to
___
**For better performance and scalability, consider using a query builder or repository |
| User experience |
Improve user feedback on payment state check timeout.___ **To improve user experience, provide feedback or a timeout message when the payment statecheck exceeds the expected time limit, instead of just logging to the console.** [interface/src/lib/components/dialogs/grow/arbitrage/confirmPayment.svelte [76]](https://github.com/Hu-Fi/Mr.Market/pull/147/files#diff-94efc6491a5ac4ba49c323d40db203e28413357471538d2f387436823a658dfeR76-R76) ```diff -console.log('Timeout reached, stopping execution.'); +alert('Timeout reached, please check your order status in the history page.'); ``` |
| Robustness |
Add error handling for the
___
**Implement error handling for the |
User description
This pr is mainly for fixing the db bug
Type
enhancement, bug_fix
Description
Changes walkthrough
10 files
constants.ts
Added Constants for Order State Handlinginterface/src/lib/helpers/constants.ts
strategy.ts
Added Payment State Fetch Functioninterface/src/lib/helpers/hufi/strategy.ts - Added a new function to fetch payment state by order ID.
mixin.ts
Store Enhancements and Function Exportinterface/src/lib/helpers/mixin.ts
wallet.ts
Added New Stores for Arbitrage and Market Making Ordersinterface/src/lib/stores/wallet.ts
store.
+page.ts
Async Loading of Arbitrage Details by IDinterface/src/routes/(secondary)/(grow)/grow/arbitrage/[id]/+page.ts - Changed to async function for loading arbitrage details by ID.
+page.ts
Async Loading of Market Making Details by IDinterface/src/routes/(secondary)/(grow)/grow/market_making/[id]/+page.ts - Changed to async function for loading market making details by ID.
arbitrage-order.entity.ts
Renamed ArbitrageOrder Entity to ArbitrageHistoryserver/src/common/entities/arbitrage-order.entity.ts - Renamed ArbitrageOrder entity to ArbitrageHistory.
mm-order.entity.ts
Renamed MMOrder Entity to MarketMakingHistoryserver/src/common/entities/mm-order.entity.ts - Renamed MMOrder entity to MarketMakingHistory.
strategy-user.service.ts
Added New Function and Updated Return Structureserver/src/modules/strategy/strategy-user.service.ts
strategy.controller.ts
Added New Endpoints for Strategy Controllerserver/src/modules/strategy/strategy.controller.ts
making history.
2 files
coin.ts
Improved Error Handling in Coin Helperinterface/src/lib/helpers/hufi/coin.ts
an empty array.
sortTable.ts
Improved Error Handling in Sorting Functionsinterface/src/lib/helpers/sortTable.ts
3 files
market.ts
Fixed Error Handling in Market Storeinterface/src/lib/stores/market.ts - Fixed error handling by checking for array type in marketData.
arbitrage.listener.ts
Updated Function Call in Arbitrage Listenerserver/src/modules/mixin/listeners/arbitrage.listener.ts - Updated function call to findPaymentStateByIdRaw.
market_making.listener.ts
Updated Function Call in Market Making Listenerserver/src/modules/mixin/listeners/market_making.listener.ts - Updated function call to findPaymentStateByIdRaw.
1 files
app.module.ts
Cleanup of Unused Entity Imports in App Moduleserver/src/app.module.ts - Removed MMOrder and ArbitrageOrder entities from module imports.
2 files
arbitrage.listener.spec.ts
Updated Function Name in Arbitrage Listener Testsserver/src/modules/mixin/listeners/arbitrage.listener.spec.ts - Updated function name to findPaymentStateByIdRaw in tests.
market_making.listener.spec.ts
Updated Function Name in Market Making Listener Testsserver/src/modules/mixin/listeners/market_making.listener.spec.ts - Updated function name to findPaymentStateByIdRaw in tests.