Closed zed-wong closed 2 months ago
vercel[bot]
commented
2 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 10, 2024 9:26am |
railway-app[bot]
commented
2 months ago This PR is being deployed to Railway 🚅
railway-app[bot]
commented
2 months ago This PR is being deployed to Railway 🚅
Mr.Market: ◻️ REMOVED
github-actions[bot]
commented
2 months ago PR Description updated to latest commit (https://github.com/Hu-Fi/Mr.Market/commit/8ae753e5e5c81db97d4013a66a87f5268ec1a3b6)
github-actions[bot]
commented
2 months ago | ⏱️ Estimated effort to review [1-5] | 5, due to the extensive changes across multiple files, including backend and frontend, involving new features, tests, and refactoring. The PR introduces new functionalities related to arbitrage and market making strategies, adds corresponding tests, and modifies existing code to accommodate these changes. Reviewing this PR requires a deep understanding of the project's architecture, the newly introduced business logic, and ensuring that the changes do not introduce regressions or security vulnerabilities. |
| 🧪 Relevant tests | Yes |
| 🔍 Possible issues | Possible Bug: The `handleResponse` function in `interface/src/lib/helpers/hufi/strategy.ts` does not handle the case where the response body is empty or not in the expected format. This could lead to runtime errors when attempting to access properties on undefined. |
| Performance Concern: The `findAllStrategyByUser` method in `server/src/modules/strategy/strategy-user.service.ts` retrieves all strategies related to a user and then combines them into a single array. If a user has a large number of strategies, this could lead to performance issues due to the large dataset being processed and returned. | |
| 🔒 Security concerns | No |
| relevant file | server/src/modules/strategy/strategy-user.service.ts |
| suggestion | Consider implementing pagination for the `findAllStrategyByUser` method to improve performance and reduce memory usage when dealing with large datasets. [medium] |
| relevant line | return [...arbitrages, ...market_makings]; |
| relevant file | interface/src/lib/helpers/hufi/strategy.ts |
| suggestion | Add error handling for the case where `response.json()` fails to parse the response body in the `handleResponse` function. This will prevent unhandled promise rejections. [important] |
| relevant line | return await response.json(); |
| relevant file | server/src/modules/mixin/listeners/market_making.listener.ts |
| suggestion | Validate the `details` object in the `handleMarketMakingCreate` method to ensure all required properties are present before proceeding with the logic. [important] |
| relevant line | if (!details || !snapshot) { |
| relevant file | server/src/common/helpers/mixin/memo.ts |
| suggestion | In `decodeArbitrageMemo` and `decodeMarketMakingMemo`, ensure that the decoded memo parts match expected values to avoid potential issues with unexpected memo formats. [medium] |
| relevant line | if (parts.length !== 6) { |
github-actions[bot]
commented
2 months ago | Category | Suggestions |
| Enhancement |
Handle cases where fetch operations do not return a valid response.___ **It's recommended to explicitly handle the case when the fetch operations in the asyncfunctions getAllStrategyByUser, getAllArbitrageByUser, getArbitrageDetailsById, stopArbitrage, getAllMarketMakingByUser, getMarketMakingDetailsById, and stopPureMarketMaking do not throw an error but also do not return a valid response. This can be achieved by returning a default value or an error object to ensure that the calling function can handle this scenario gracefully.** [interface/src/lib/helpers/hufi/strategy.ts [12-70]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-44e02f14022d0b410cbac941d9268b0e209909bca68e3dbce6cbf50f1e68788bR12-R70) ```diff -return await handleResponse(response); +const result = await handleResponse(response); +if (!result) { + // Handle the case where result is undefined or null + return { error: 'No data returned from the server' }; +} +return result; ``` |
Improve error handling in memo generation functions.___ **For the functionsGenerateArbitrageMemo and GenerateMarketMakingMemo, consider adding a more robust error handling mechanism. Instead of just logging the error and returning undefined, you could throw a custom error or return an error object. This would allow the calling code to handle the error more gracefully and take appropriate actions based on the error type.** [interface/src/lib/helpers/memo.ts [82-124]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-dccebfba812d3919c4dcd66ee3972b852d0cbf22df17f745e83e08a5adb34767R82-R124) ```diff -console.error(`GenerateArbitrageMemo failed to get exchange indices for: ${exchangeA}, ${exchangeB}`); -return; +throw new Error(`GenerateArbitrageMemo failed to get exchange indices for: ${exchangeA}, ${exchangeB}`); ``` | |
Improve input validation by throwing errors in payment functions.___ **In theArbitragePay and MarketMakingPay functions, it's important to validate the input parameters before proceeding with the logic. However, the current implementation logs an error and returns undefined in case of invalid input. Consider throwing an error instead, which can be caught and handled by the caller, providing a clearer indication that something went wrong due to invalid inputs.** [interface/src/lib/helpers/mixin.ts [285-338]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-acf423272bdc878982f8a993563883c5ac16fcef547cb0e7a09972c785a5e839R285-R338) ```diff -console.error('Invalid input parameters for ArbitragePay'); -return; +throw new Error('Invalid input parameters for ArbitragePay'); ``` | |
Return error information from sorting functions on failure.___ **The sorting functionssortByString and sortByNumber currently catch and log errors but do not provide feedback to the caller about the failure. To improve this, consider returning a specific error or a status object from these functions when an error occurs. This approach allows the caller to react accordingly, possibly by displaying an error message to the user or attempting a fallback operation.** [interface/src/lib/helpers/sortTable.ts [22-35]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-53e861ddae0f6bf506b78171ab5a55abdd6c4e49940df22de7b047c3cd276223R22-R35) ```diff -console.error(e); +return { error: e.message }; ``` | |
Add null checks for method parameters to prevent potential runtime errors.___ **Consider checking fornull or undefined values for details and snapshot parameters in handleArbitrageCreate method to ensure robustness. This can prevent potential runtime errors when these objects are accessed if they happen to be null or undefined.**
[server/src/modules/mixin/listeners/arbitrage.listener.ts [22-24]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-fda7c44f82e49e1aa3d5543399e4e153e9a02965846ddbbd5f51961f988c1cc0R22-R24)
```diff
async handleArbitrageCreate(
details: ArbitrageMemoDetails,
snapshot: SafeSnapshot,
) {
+ if (!details || !snapshot) {
+ throw new Error('Invalid arguments passed to handleArbitrageCreate');
+ }
```
| |
Use custom error classes for specific error cases.___ **To improve error handling, consider defining a custom error class for not found entitiesinstead of throwing a generic Error. This allows for more precise error handling where these methods are used.** [server/src/modules/strategy/strategy-user.repository.ts [57]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-7d84d7d354a364c8792e28aec213de8898ac0e819b24dafe5c9264880a0dde11R57-R57) ```diff -throw new Error('ArbitrageOrder not found'); +throw new ArbitrageOrderNotFoundError('ArbitrageOrder not found'); ``` | |
Implement transactional operations for multi-step updates.___ **To ensure the integrity of the database, consider implementing transactional operationsespecially for methods that involve multiple steps or updates like updateArbitrageOrderState and updateMarketMakingOrderState.**
[server/src/modules/strategy/strategy-user.repository.ts [60]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-7d84d7d354a364c8792e28aec213de8898ac0e819b24dafe5c9264880a0dde11R60-R60)
```diff
-await this.arbitrageRepository.save(arbitrageOrder);
+await this.arbitrageRepository.manager.transaction(async transactionalEntityManager => {
+ await transactionalEntityManager.save(arbitrageOrder);
+});
```
| |
| Bug |
Implement error handling for undefined or null data in marketData store.___ **In the derived storemarketData, when handling success in handleSuccess, it's good practice to check for the validity of params. However, the current implementation does not define the handleError function used in the check. Ensure that you define and implement handleError to properly handle cases where params is undefined or null, such as logging the error or setting an error state.** [interface/src/lib/stores/market.ts [59-62]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-72258a776e444a641d4fa06eba2b515e923ec009db3017b8a22c453061b9375dR59-R62) ```diff if (!params) { - handleError(); + console.error('No data received'); + marketDataState.set('error'); return; } ``` |
| Best practice |
Define a specific type for
___
**It's recommended to define a type for |
Use numeric types for numeric fields in
___
**For | |
Use numeric types for fields representing numeric values in
___
**For numeric fields such as | |
Use
___
**For the | |
Validate memo parts against known values in memo decoding functions.___ **When decoding memos indecodeArbitrageMemo and decodeMarketMakingMemo, it's a good practice to validate the tradingType, action, and exchangeName against known values to ensure they match expected formats. This can prevent potential issues with unexpected or malformed memo strings.** [server/src/common/helpers/mixin/memo.ts [54]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-996ebc0a0ef85b44a5c76b6f52e33e03b879f9bc3b8ef861205bb803d309b983R54-R54) ```diff -const [tradingType, action, exchangeAIndex, exchangeBIndex, destId, traceId] = parts; -const [tradingType, action, exchangeIndex, destId, traceId] = parts; +// Example validation for decodeArbitrageMemo +if (!TARDING_TYPE_MAP[tradingType] || !ARBITRAGE_MEMO_ACTION_MAP[action] || !SPOT_EXCHANGE_MAP[exchangeAIndex] || !SPOT_EXCHANGE_MAP[exchangeBIndex]) { + return null; +} +// Similar validation can be applied to decodeMarketMakingMemo ``` | |
Use structured logging for better log analysis.___ **It's recommended to use structured logging instead of simple log statements. This allowsfor better searchability and analysis of logs. For instance, instead of using this.logger.log(arbDetails);, consider logging with a structured format that includes both the action type and the details.** [server/src/modules/mixin/snapshots/snapshots.service.ts [486]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-dd336e520fcec14b837cd72c0852b94e85fb8e130fc2a3431b49fef7e4d94c29R486-R486) ```diff -this.logger.log(arbDetails); +this.logger.log({ action: 'arbitrage.create', details: arbDetails }); ``` | |
Use strict inequality for comparison to avoid type coercion issues.___ **Instead of using direct comparison with!= for checking snapshot.asset_id against baseAssetID and targetAssetID, consider using the strict inequality operator !== to ensure both type and value are compared. This is a best practice to avoid potential type coercion issues.** [server/src/modules/mixin/listeners/arbitrage.listener.ts [31-32]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-fda7c44f82e49e1aa3d5543399e4e153e9a02965846ddbbd5f51961f988c1cc0R31-R32) ```diff if ( - snapshot.asset_id != baseAssetID && - snapshot.asset_id != targetAssetID + snapshot.asset_id !== baseAssetID && + snapshot.asset_id !== targetAssetID ) { ``` | |
Use more specific types for ID parameters.___ **Consider using a more specific type thanstring for orderId and userId parameters in methods like findArbitrageByOrderId, findArbitrageByUserId, etc. If these IDs have a specific format or limited set of possible values, using a more specific type can help catch errors at compile time.** [server/src/modules/strategy/strategy-user.repository.ts [31-33]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-7d84d7d354a364c8792e28aec213de8898ac0e819b24dafe5c9264880a0dde11R31-R33) ```diff -findArbitrageByOrderId(orderId: string): Promise | |
Return a boolean for update methods to indicate success.___ **For methods likeupdateArbitrageOrderState and updateMarketMakingOrderState, consider returning a boolean indicating success or failure instead of void or the entity itself. This makes the API more consistent, especially since updatePaymentStateById returns null on failure.** [server/src/modules/strategy/strategy-user.repository.ts [52]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-7d84d7d354a364c8792e28aec213de8898ac0e819b24dafe5c9264880a0dde11R52-R52) ```diff -): Promise | |
Use enums instead of magic strings for state values.___ **Instead of using magic strings for states like 'created' and 'paused' in methodsfindRunningArbitrageOrders and findPausedArbitrageOrders, consider using an enum. This improves code readability and maintainability.** [server/src/modules/strategy/strategy-user.repository.ts [42]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-7d84d7d354a364c8792e28aec213de8898ac0e819b24dafe5c9264880a0dde11R42-R42) ```diff -return this.arbitrageRepository.findBy({ state: 'created' }); +return this.arbitrageRepository.findBy({ state: ArbitrageOrderState.Created }); ``` | |
| Maintainability |
Extract complex logic into separate methods for better readability and maintainability.___ **To improve code readability and maintainability, consider extracting the logic forcreating or updating payment states into separate private methods within the MarketMakingListener class. This can help reduce the complexity of the handleMarketMakingCreate method and make the code easier to understand and modify.**
[server/src/modules/mixin/listeners/market_making.listener.ts [40-61]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-af2b696faff84be74534526cbd7bca07e7a0630c04ce6f07be1b846fc8424ebbR40-R61)
```diff
if (!paymentState) {
+ return await this.createInitialPaymentState(details, snapshot);
+}
+if (paymentState && !paymentState.secondAssetId) {
+ await this.updatePaymentStateWithSecondAsset(details, snapshot);
+}
+// Below the handleMarketMakingCreate method, add the new private methods
+private async createInitialPaymentState(details: MarketMakingMemoDetails, snapshot: SafeSnapshot): Promise |
Organize imports and providers alphabetically for better readability.___ **To enhance the modularity and maintainability of theEventListenersModule, consider organizing the imports and providers in alphabetical order. This small change can make the module more readable and easier to navigate, especially as it grows in complexity.** [server/src/modules/mixin/listeners/events.module.ts [14-27]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-d3d7b0ea9525929bef99b2c4eea5ca29dcb90e5e984f0ea3f2538ecaff74a625R14-R27) ```diff imports: [ + CustomConfigModule, ExchangeModule, SnapshotsModule, - CustomConfigModule, StrategyModule, ], providers: [ + ArbitrageListener, + ConfigService, ExchangeListener, + MarketMakingListener, MixinListener, SpotOrderListener, - ArbitrageListener, - MarketMakingListener, - ConfigService, ], ``` |
zed-wong
commented
2 months ago /improve
github-actions[bot]
commented
2 months ago | Category | Suggestions |
| Best practice |
Add explicit return types to asynchronous functions.___ **It's a good practice to explicitly define the return type for asynchronous functions toimprove code readability and maintainability. For all the exported async functions in this file, consider adding explicit return types.** [interface/src/lib/helpers/hufi/strategy.ts [10-44]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-44e02f14022d0b410cbac941d9268b0e209909bca68e3dbce6cbf50f1e68788bR10-R44) ```diff -export const getAllStrategyByUser = async (userId: string) => { +export const getAllStrategyByUser = async (userId: string): Promise |
Validate input parameters and throw errors if validation fails in payment functions.___ **For functions likeArbitragePay and MarketMakingPay, consider validating the input parameters at the beginning of the function and throwing an error if the validation fails. This approach follows the fail-fast principle and makes the code more robust.** [interface/src/lib/helpers/mixin.ts [285-368]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-acf423272bdc878982f8a993563883c5ac16fcef547cb0e7a09972c785a5e839R285-R368) ```diff if (!exchangeA || !exchangeB || !symbol || !amount || !orderId || !assetId) { - console.error('Invalid input parameters for ArbitragePay'); - return; + throw new Error('Invalid input parameters for ArbitragePay'); } ``` | |
Use a more robust comparison method for asset IDs.___ **Instead of directly comparingsnapshot.asset_id with baseAssetID and targetAssetID using inequality operators, consider using a more robust method that accounts for potential undefined or null values. This can prevent unintended behavior when one of the IDs is not properly retrieved or defined.** [server/src/modules/mixin/listeners/arbitrage.listener.ts [34-39]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-fda7c44f82e49e1aa3d5543399e4e153e9a02965846ddbbd5f51961f988c1cc0R34-R39) ```diff if ( - snapshot.asset_id != baseAssetID && - snapshot.asset_id != targetAssetID + ![baseAssetID, targetAssetID].includes(snapshot.asset_id) ) { // Transfer asset doesn't match any of symbol, refund return await this.snapshotService.refund(snapshot); } ``` | |
Use a type or interface for the
___
**For the | |
| Enhancement |
Improve test case descriptions for clarity.___ **To ensure the test descriptions are clear and consistent, consider using a moredescriptive naming convention for test cases. For example, instead of "Generates correct Spot Memo", use "should generate a correct Spot Memo when provided with valid parameters".** [interface/src/lib/helpers/memo.test.ts [17-29]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-09b4ae4795c7de02dc37164fade993380e28421b8cd800a66c6b0e1b98e47d66R17-R29) ```diff -it('Generates correct Spot Memo', () => { +it('should generate a correct Spot Memo when provided with valid parameters', () => { ``` |
Handle errors by throwing exceptions or returning default values in memo generation functions.___ **Instead of returningundefined when an error occurs in GenerateArbitrageMemo and GenerateMarketMakingMemo, consider throwing a custom error or returning a default error memo. This approach can help in handling errors more gracefully in the calling code.** [interface/src/lib/helpers/memo.ts [82-124]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-dccebfba812d3919c4dcd66ee3972b852d0cbf22df17f745e83e08a5adb34767R82-R124) ```diff -console.error(`GenerateArbitrageMemo failed to get exchange indices for: ${exchangeA}, ${exchangeB}`); -return; +throw new Error(`GenerateArbitrageMemo failed to get exchange indices for: ${exchangeA}, ${exchangeB}`); ``` | |
Improve type safety by specifying a concrete type for
___
**Consider initializing | |
Use numeric types for
___
**For | |
Use numeric types for monetary and time-related fields in
___
**For fields representing amounts, spreads, and times in | |
Use
___
**For the | |
Add error handling and logging to memo decoding functions for better debugging.___ **For thedecodeArbitrageMemo and decodeMarketMakingMemo functions, consider adding error handling or logging to provide feedback when the input does not meet the expected format or when a symbol cannot be found. This improvement can help with debugging and ensure that issues with memo decoding are easier to identify and resolve.** [server/src/common/helpers/mixin/memo.ts [47-50]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-996ebc0a0ef85b44a5c76b6f52e33e03b879f9bc3b8ef861205bb803d309b983R47-R50) ```diff if (!decodedMemo) { + console.error("Empty memo string."); return null; } +// Add similar checks and logging for other error conditions within these functions. ``` | |
Add error handling for asynchronous operation.___ **Consider adding error handling for the asynchronous operation withinfindFirstAPIKeyByExchange. This can be achieved by wrapping the operation in a try-catch block to gracefully handle potential errors that might occur during the execution of readAllAPIKeysByExchange.**
[server/src/modules/mixin/exchange/exchange.service.ts [473-475]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-b4d701c594440771180bbf709c88654f1d8a3806e45b2e035b3b36572ab3cda5R473-R475)
```diff
-const apiKeys = await this.exchangeRepository.readAllAPIKeysByExchange(
- exchange,
-);
+let apiKeys;
+try {
+ apiKeys = await this.exchangeRepository.readAllAPIKeysByExchange(exchange);
+} catch (error) {
+ console.error('Error reading API keys:', error);
+ throw new Error('Failed to read API keys');
+}
```
| |
Add more thorough validation for method arguments.___ **To ensure the robustness of thehandleMarketMakingCreate method, it's recommended to validate the details and snapshot objects more thoroughly. Instead of just checking for their presence, validate that they contain the necessary properties for the operation to proceed.** [server/src/modules/mixin/listeners/market_making.listener.ts [26-28]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-af2b696faff84be74534526cbd7bca07e7a0630c04ce6f07be1b846fc8424ebbR26-R28) ```diff -if (!details || !snapshot) { - console.error('Invalid arguments passed to handleMarketMakingCreate'); +if (!details || !snapshot || !details.traceId || !snapshot.asset_id) { + console.error('Invalid or incomplete arguments passed to handleMarketMakingCreate'); return; } ``` | |
| Performance |
Optimize sorting functions by avoiding unnecessary reverse calls.___ **To improve the efficiency of sorting functions, consider using a singlesort call with a conditional check for ascendingOrder inside the comparison function. This approach reduces the need to call reverse, which can be more performant for large datasets.**
[interface/src/lib/helpers/sortTable.ts [3-24]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-53e861ddae0f6bf506b78171ab5a55abdd6c4e49940df22de7b047c3cd276223R3-R24)
```diff
data = data.sort((obj1, obj2) => {
const value1 = obj1[colHeader].toLowerCase();
const value2 = obj2[colHeader].toLowerCase();
if (value1 < value2) {
- return 1;
+ return ascendingOrder ? -1 : 1;
} else if (value1 > value2) {
- return -1;
+ return ascendingOrder ? 1 : -1;
}
return 0;
});
-if (!ascendingOrder) {
- data = data.reverse();
-}
-
```
|
| Maintainability |
Alphabetically sort module imports and providers for better readability.___ **To improve the maintainability and readability of the module imports and providers,consider alphabetically sorting the imported modules and providers. This can help in quickly identifying if a module or provider is already included and makes the code cleaner.** [server/src/modules/mixin/listeners/events.module.ts [14-27]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-d3d7b0ea9525929bef99b2c4eea5ca29dcb90e5e984f0ea3f2538ecaff74a625R14-R27) ```diff imports: [ + CustomConfigModule, ExchangeModule, SnapshotsModule, - CustomConfigModule, StrategyModule, ], providers: [ + ArbitrageListener, + ConfigService, ExchangeListener, + MarketMakingListener, MixinListener, SpotOrderListener, - ArbitrageListener, - MarketMakingListener, - ConfigService, ], ``` |
Extract logic into separate methods for clarity.___ **To enhance the readability and maintainability of thehandleArbitrageCreate method, consider extracting the logic for creating and updating payment states into separate private methods within the class. This can make the main method more concise and easier to understand.** [server/src/modules/mixin/listeners/arbitrage.listener.ts [45-56]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-fda7c44f82e49e1aa3d5543399e4e153e9a02965846ddbbd5f51961f988c1cc0R45-R56) ```diff if (!paymentState) { + return this.createInitialPaymentState(details, snapshot); +} +// Elsewhere in the class +private async createInitialPaymentState(details: ArbitrageMemoDetails, snapshot: SafeSnapshot): Promise | |
Improve variable naming for better readability.___ **Consider using a more descriptive variable name thankey_id for better code readability. For instance, apiKeyId would make it immediately clear that this ID is related to an API key.** [server/src/modules/mixin/rebalance/rebalance.service.spec.ts [66]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-036b1e1d0fa810a13c6f8d36d083e1e296e3c855fb9c2a0a274369ea0446dd22R66-R66) ```diff -.mockResolvedValue({ key_id: 'key1' }), +.mockResolvedValue({ apiKeyId: 'key1' }), ``` | |
Reduce duplication by extracting mock implementations into a separate function or variable.___ **Consider extracting the mock implementations foraggregateBalancesByExchange into a separate function or variable to reduce duplication and improve maintainability.** [server/src/modules/mixin/rebalance/rebalance.service.spec.ts [41-60]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-036b1e1d0fa810a13c6f8d36d083e1e296e3c855fb9c2a0a274369ea0446dd22R41-R60) ```diff -aggregateBalancesByExchange: jest.fn().mockReturnValue({ - exchange1: { - total: { BTC: '1' }, - apiKeyBalances: [ - { - balances: { - BTC: '10', - }, +const mockAggregateBalances = { + exchange1: { + total: { BTC: '1' }, + apiKeyBalances: [ + { + balances: { + BTC: '10', }, - ], - }, - exchange2: { - total: { BTC: '1' }, - apiKeyBalances: [ - { - balances: { - BTC: '2', - }, + }, + ], + }, + exchange2: { + total: { BTC: '1' }, + apiKeyBalances: [ + { + balances: { + BTC: '2', }, - ], - }, - }), + }, + ], + }, +}; +aggregateBalancesByExchange: jest.fn().mockReturnValue(mockAggregateBalances), ``` | |
| Possible issue |
Ensure consistent return types for method calls.___ **Ensure consistent return types forfindFirstAPIKeyByExchange. The method returns an object in one instance and an array of objects in another. Decide on one format to maintain consistency and prevent potential errors.** [server/src/modules/mixin/rebalance/rebalance.service.spec.ts [66-154]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-036b1e1d0fa810a13c6f8d36d083e1e296e3c855fb9c2a0a274369ea0446dd22R66-R154) ```diff -.mockResolvedValue({ key_id: 'key1' }), -.mockResolvedValue([{ key_id: 'key1' }]), +.mockResolvedValue([{ apiKeyId: 'key1' }]), +.mockResolvedValue([{ apiKeyId: 'key1' }]), ``` |
github-actions[bot]
commented
2 months ago PR Description updated to latest commit (https://github.com/Hu-Fi/Mr.Market/commit/edf8fa3b99add38a8ce0bcbfeb26187acd564f4f)
github-actions[bot]
commented
2 months ago Persistent review updated to latest commit https://github.com/Hu-Fi/Mr.Market/commit/edf8fa3b99add38a8ce0bcbfeb26187acd564f4f
github-actions[bot]
commented
2 months ago | Category | Suggestions |
| Enhancement |
Add error handling for fetch operations.___ **It's recommended to handle the case where the fetch operations in the async functionsmight fail due to network issues or server errors. You can wrap the fetch call in a try-catch block to catch any potential errors and handle them accordingly. This ensures that your application can gracefully handle network or server failures.** [interface/src/lib/helpers/hufi/strategy.ts [12]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-44e02f14022d0b410cbac941d9268b0e209909bca68e3dbce6cbf50f1e68788bR12-R12) ```diff -const response = await fetch(`${HUFI_BACKEND_URL}/strategy/all?userId=${userId}`); +let response; +try { + response = await fetch(`${HUFI_BACKEND_URL}/strategy/all?userId=${userId}`); +} catch (error) { + console.error('Network error:', error); + return; +} ``` |
Implement a fallback mechanism for missing data.___ **To ensure that the application can handle cases where no data is received from the API,it's recommended to implement a fallback mechanism or a default state. This could involve setting a default state for the UI or displaying a message to the user indicating that no data is available.** [interface/src/lib/stores/market.ts [59-62]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-72258a776e444a641d4fa06eba2b515e923ec009db3017b8a22c453061b9375dR59-R62) ```diff if (!params) { console.error('No data received'); marketDataState.set('error'); + // Implement a fallback UI state or notify the user return; } ``` | |
Use
___
**For numeric properties in | |
Add error logging for invalid memo decoding cases.___ **When decoding memos indecodeArbitrageMemo and decodeMarketMakingMemo, consider adding error logging for cases where the memo format is invalid or when the destId does not map to a symbol. This can aid in debugging and provide clearer insights into why a memo decoding might fail.** [server/src/common/helpers/mixin/memo.ts [51-60]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-996ebc0a0ef85b44a5c76b6f52e33e03b879f9bc3b8ef861205bb803d309b983R51-R60) ```diff if (parts.length !== 6) { + console.error(`Invalid memo format: ${decodedMemo}`); return null; } if (!symbol) { + console.error(`destId does not map to a symbol: ${destId}`); return null; } ``` | |
Add error handling for the repository method call.___ **Consider implementing error handling for thereadAllAPIKeysByExchange method call to handle possible failures gracefully.** [server/src/modules/mixin/exchange/exchange.service.ts [473-475]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-b4d701c594440771180bbf709c88654f1d8a3806e45b2e035b3b36572ab3cda5R473-R475) ```diff -const apiKeys = await this.exchangeRepository.readAllAPIKeysByExchange( - exchange, -); +let apiKeys; +try { + apiKeys = await this.exchangeRepository.readAllAPIKeysByExchange(exchange); +} catch (error) { + console.error(`Failed to read API keys for exchange ${exchange}:`, error); + throw new Error('Failed to read API keys from the repository'); +} ``` | |
Add assertions for mock function calls.___ **For better test reliability, consider adding assertions to verify that the mock functions(e.g., findFirstAPIKeyByExchange, getDepositAddress) are called with the expected arguments.** [server/src/modules/mixin/rebalance/rebalance.service.spec.ts [64-66]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-036b1e1d0fa810a13c6f8d36d083e1e296e3c855fb9c2a0a274369ea0446dd22R64-R66) ```diff -findFirstAPIKeyByExchange: jest - .fn() - .mockResolvedValue({ key_id: 'key1' }), +expect(findFirstAPIKeyByExchange).toHaveBeenCalledWith(expectedExchangeArgument); ``` | |
| Best practice |
Use custom errors instead of returning undefined on failure.___ **Instead of returningundefined when an error occurs in GenerateArbitrageMemo and GenerateMarketMakingMemo, consider throwing a custom error with a meaningful message. This approach makes it easier to debug issues and handle errors upstream where these functions are called.** [interface/src/lib/helpers/memo.ts [82-83]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-dccebfba812d3919c4dcd66ee3972b852d0cbf22df17f745e83e08a5adb34767R82-R83) ```diff -console.error(`GenerateArbitrageMemo failed to get exchange indices for: ${exchangeA}, ${exchangeB}`); -return; +throw new Error(`GenerateArbitrageMemo failed to get exchange indices for: ${exchangeA}, ${exchangeB}`); ``` |
Add try-catch block for network requests.___ **When performing operations that might fail, such as network requests, it's a good practiceto wrap these operations in try-catch blocks. This suggestion applies to the mixinPay function, where the axios call could fail due to network issues.** [interface/src/lib/helpers/mixin.ts [51-53]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-acf423272bdc878982f8a993563883c5ac16fcef547cb0e7a09972c785a5e839R51-R53) ```diff -const { data } = await axios.post(`${MIXIN_API_BASE_URL}/payments`, JSON.stringify({ - asset_id: asset_id, - opponent_multisig: { - receivers: receivers, - threshold: 2, - }, - amount: amount, - memo: memo, - trace: trace_id, -})) +let data; +try { + const response = await axios.post(`${MIXIN_API_BASE_URL}/payments`, JSON.stringify({ + asset_id: asset_id, + opponent_multisig: { + receivers: receivers, + threshold: 2, + }, + amount: amount, + memo: memo, + trace: trace_id, + })); + data = response.data; +} catch (error) { + console.error('Failed to make payment:', error); + return; +} ``` | |
Use specific types instead of
___
**Consider initializing | |
Use numeric types for properties representing numeric values.___ **For theArbitrageOrder interface, consider using more precise types for properties like amountToTrade and minProfitability. If these properties represent numeric values, using number or bigint instead of string can provide better type safety and avoid the need for type conversions later in the code.** [interface/src/lib/types/hufi/arbitrage.ts [10-11]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-e4560fce9166c6b2b00e99e070c02fbabb4a55ad8539b5259568149af90a6f5bR10-R11) ```diff -amountToTrade: string; -minProfitability: string; +amountToTrade: number; +minProfitability: number; ``` | |
Use a logging framework or error handling strategy instead of console.error.___ **Replace direct console.error logging with a more robust logging framework or errorhandling strategy to improve error tracking and handling.** [server/src/modules/mixin/listeners/arbitrage.listener.ts [26]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-fda7c44f82e49e1aa3d5543399e4e153e9a02965846ddbbd5f51961f988c1cc0R26-R26) ```diff -console.error('Invalid arguments passed to handleArbitrageCreate'); +// Assuming a logging framework like Winston or a custom error handling strategy +logger.error('Invalid arguments passed to handleArbitrageCreate'); ``` | |
Use strict inequality for comparisons to avoid type coercion issues.___ **Ensure consistent comparison operators by using strict inequality (!==) instead of loose inequality ( !=) to avoid potential type coercion bugs.**
[server/src/modules/mixin/listeners/arbitrage.listener.ts [35-36]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-fda7c44f82e49e1aa3d5543399e4e153e9a02965846ddbbd5f51961f988c1cc0R35-R36)
```diff
-snapshot.asset_id != baseAssetID &&
-snapshot.asset_id != targetAssetID
+snapshot.asset_id !== baseAssetID &&
+snapshot.asset_id !== targetAssetID
```
| |
Replace magic numbers with named constants.___ **It's recommended to avoid using magic numbers directly in the code. Consider defining aconstant for the BTC values ('1', '10', '2', '0.1') used in the mockReturnValue to enhance code readability and maintainability.** [server/src/modules/mixin/rebalance/rebalance.service.spec.ts [43]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-036b1e1d0fa810a13c6f8d36d083e1e296e3c855fb9c2a0a274369ea0446dd22R43-R43) ```diff -total: { BTC: '1' }, +const BTC_BALANCE_EXCHANGE1 = '1'; +total: { BTC: BTC_BALANCE_EXCHANGE1 }, ``` | |
| Maintainability |
Abstract comparison logic into separate functions.___ **To improve the readability and maintainability of the sorting functions, considerabstracting the comparison logic into separate functions. This approach reduces code duplication and makes the code easier to understand.** [interface/src/lib/helpers/sortTable.ts [5-15]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-53e861ddae0f6bf506b78171ab5a55abdd6c4e49940df22de7b047c3cd276223R5-R15) ```diff -data = data.sort((obj1, obj2) => { - const value1 = obj1[colHeader].toLowerCase(); - const value2 = obj2[colHeader].toLowerCase(); - if (value1 < value2) { - return 1; - } else if (value1 > value2) { - return -1; - } +const compareStrings = (value1, value2) => { + if (value1 < value2) return 1; + if (value1 > value2) return -1; return 0; -}); +}; +data = data.sort((obj1, obj2) => compareStrings(obj1[colHeader].toLowerCase(), obj2[colHeader].toLowerCase())); ``` |
Group related module imports and declarations for better readability.___ **Consider organizing imports and module declarations by grouping related entities togetherto improve readability and maintainability of the module configuration.** [server/src/modules/mixin/listeners/events.module.ts [14-19]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-d3d7b0ea9525929bef99b2c4eea5ca29dcb90e5e984f0ea3f2538ecaff74a625R14-R19) ```diff imports: [ + // Mixin related modules ExchangeModule, SnapshotsModule, + // Strategy related modules + StrategyModule, + // Configuration modules CustomConfigModule, - StrategyModule, ], ``` | |
Extract logic into separate methods for better maintainability.___ **To improve code maintainability, consider extracting the logic for creating or updatingpayment states and market making into separate private methods within the class.** [server/src/modules/mixin/listeners/market_making.listener.ts [44-55]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-af2b696faff84be74534526cbd7bca07e7a0630c04ce6f07be1b846fc8424ebbR44-R55) ```diff if (!paymentState) { + return await this.createInitialPaymentState(details, snapshot); +} +... +private async createInitialPaymentState(details: MarketMakingMemoDetails, snapshot: SafeSnapshot): Promise | |
Improve variable naming for better readability.___ **Consider using a more descriptive variable name instead ofkey_id for better code readability. For instance, apiKeyId would make it immediately clear that it refers to the ID of an API key.** [server/src/modules/mixin/rebalance/rebalance.service.spec.ts [66]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-036b1e1d0fa810a13c6f8d36d083e1e296e3c855fb9c2a0a274369ea0446dd22R66-R66) ```diff -.mockResolvedValue({ key_id: 'key1' }), +.mockResolvedValue({ apiKeyId: 'key1' }), ``` | |
Refactor repeated mock structures into a helper function.___ **To improve the test's clarity and maintainability, consider refactoring the repeated mockstructure for aggregateBalancesByExchange into a helper function. This function can then be reused across different tests.** [server/src/modules/mixin/rebalance/rebalance.service.spec.ts [41-51]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-036b1e1d0fa810a13c6f8d36d083e1e296e3c855fb9c2a0a274369ea0446dd22R41-R51) ```diff -aggregateBalancesByExchange: jest.fn().mockReturnValue({ - exchange1: { - total: { BTC: '1' }, - apiKeyBalances: [ - { - balances: { - BTC: '10', +function mockAggregateBalancesByExchange() { + return { + exchange1: { + total: { BTC: '1' }, + apiKeyBalances: [ + { + balances: { + BTC: '10', + }, }, - }, - ], - }, + ], + }, + }; +} +aggregateBalancesByExchange: jest.fn().mockReturnValue(mockAggregateBalancesByExchange()), ``` | |
| Possible issue |
Ensure consistent mock data structures.___ **Ensure consistent data structure for mockResolvedValue. If the function is expected toreturn an array of objects, make sure this is consistently mocked across all tests to avoid potential issues during testing.** [server/src/modules/mixin/rebalance/rebalance.service.spec.ts [66]](https://github.com/Hu-Fi/Mr.Market/pull/139/files#diff-036b1e1d0fa810a13c6f8d36d083e1e296e3c855fb9c2a0a274369ea0446dd22R66-R66) ```diff -.mockResolvedValue({ key_id: 'key1' }), +.mockResolvedValue([{ apiKeyId: 'key1' }]), ``` |
Type
enhancement, tests
Description
Changes walkthrough
7 files
strategy-user.service.spec.ts
Add tests for StrategyUserServiceserver/src/modules/strategy/strategy-user.service.spec.ts
StrategyUserServicecovering methods likefindAllStrategyByUser,createArbitrage,createMarketMaking, etc.market_making.listener.spec.ts
Add tests for MarketMakingListenerserver/src/modules/mixin/listeners/market_making.listener.spec.ts
MarketMakingListenerto ensure correct handling ofmarket making related events.
arbitrage.listener.spec.ts
Add tests for ArbitrageListenerserver/src/modules/mixin/listeners/arbitrage.listener.spec.ts
ArbitrageListenerto ensure correct handling ofarbitrage related events.
memo.spec.ts
Add tests for decoding arbitrage and market making memosserver/src/common/helpers/mixin/memo.spec.ts - Adds tests for decoding arbitrage and market making memos.
memo.test.ts
Add tests for memo generation functionsinterface/src/lib/helpers/memo.test.ts
GenerateSpotMemo,GenerateArbitrageMemo, andGenerateMarketMakingMemo.strategy.controller.spec.ts
Add mock for StrategyUserService in StrategyController testsserver/src/modules/strategy/strategy.controller.spec.ts
StrategyUserServicein theStrategyControllertestsetup.
strategy.service.spec.ts
Update strategy key format in StrategyService testsserver/src/modules/strategy/strategy.service.spec.ts
StrategyService.23 files
strategy-user.service.ts
Implement StrategyUserService for managing strategy ordersserver/src/modules/strategy/strategy-user.service.ts
StrategyUserServicewith methods to manage strategy orders(arbitrage and market making).
payment states.
mixin.ts
Add payment functions for arbitrage and market makinginterface/src/lib/helpers/mixin.ts
ArbitragePayandMarketMakingPayfor handling paymentsin arbitrage and market making strategies.
getUserOrderstogetUserSpotOrdersand addsgetUserStrategyOrders.strategy.controller.ts
Add StrategyController endpoints for arbitrage and market makingserver/src/modules/strategy/strategy.controller.ts
including fetching all strategies, arbitrage details, and stopping
strategies.
strategy.service.ts
Enhance StrategyService with strategy execution control methodsserver/src/modules/strategy/strategy.service.ts
startArbitrageIfNotStarted,pauseStrategyIfNotPaused, andstartMarketMakingIfNotStartedto manage strategy execution.strategy-user.repository.ts
Implement StrategyUserRepository for strategy order managementserver/src/modules/strategy/strategy-user.repository.ts
StrategyUserRepositoryfor database operations related tostrategy orders.
memo.ts
Implement memo generation for arbitrage and market making paymentsinterface/src/lib/helpers/memo.ts
GenerateArbitrageMemoandGenerateMarketMakingMemoforgenerating memos for strategy payments.
memo.ts
Implement decoding for arbitrage and market making memosserver/src/common/helpers/mixin/memo.ts
market_making.listener.ts
Implement MarketMakingListener for market making event handlingserver/src/modules/mixin/listeners/market_making.listener.ts
MarketMakingListenerto handle market making relatedevents.
arbitrage.listener.ts
Implement ArbitrageListener for arbitrage event handlingserver/src/modules/mixin/listeners/arbitrage.listener.ts - Implements `ArbitrageListener` to handle arbitrage related events.
strategy.ts
Add frontend helper functions for strategy managementinterface/src/lib/helpers/hufi/strategy.ts
arbitrage and market making strategies.
memo.ts
Add types for arbitrage and market making memo detailsserver/src/common/types/memo/memo.ts
details.
snapshots.service.ts
Add snapshot handling for arbitrage and market makingserver/src/modules/mixin/snapshots/snapshots.service.ts
refund logic.
market_making.ts
Define types for market making orders and statesinterface/src/lib/types/hufi/market_making.ts - Defines types and interfaces for market making orders and states.
arbitrage.ts
Define types for arbitrage orders and statesinterface/src/lib/types/hufi/arbitrage.ts - Defines types and interfaces for arbitrage orders and states.
wallet.ts
Refactor wallet store for strategy order handlinginterface/src/lib/stores/wallet.ts
strategy orders.
market_making.event.ts
Define MarketMakingCreateEvent classserver/src/modules/mixin/events/market_making.event.ts
MarketMakingCreateEventclass for market making event data.+page.ts
Implement server-side loading for arbitrage details pageinterface/src/routes/(secondary)/(grow)/grow/arbitrage/[id]/+page.ts
getArbitrageDetailsById.grow.ts
Update store variables for creating market making ordersinterface/src/lib/stores/grow.ts - Updates store variables for creating market making orders.
states.ts
Add types for arbitrage and market making order statesserver/src/common/types/orders/states.ts - Adds types for arbitrage and market making order states.
+page.ts
Implement server-side loading for market making details pageinterface/src/routes/(secondary)/(grow)/grow/market_making/[id]/+page.ts
getMarketMakingDetailsById.strategyKey.ts
Implement createStrategyKey functionserver/src/common/helpers/strategyKey.ts
createStrategyKeyfunction for generating strategy keys.arbitrage.event.ts
Define ArbitrageCreateEvent classserver/src/modules/mixin/events/arbitrage.event.ts - Defines `ArbitrageCreateEvent` class for arbitrage event data.
connectWallet.svelte
Refactor connect wallet button to use componentinterface/src/lib/components/wallet/connect/connectWallet.svelte
ConnectWalletBtncomponent.2 files
sortTable.ts
Improve error handling in table sorting functionsinterface/src/lib/helpers/sortTable.ts
market.ts
Add error handling for market data storeinterface/src/lib/stores/market.ts - Adds error handling for missing data in market data store.
2 files
events.module.ts
Register new listeners in EventListenersModuleserver/src/modules/mixin/listeners/events.module.ts
ArbitrageListenerandMarketMakingListenerin theEventListenersModule.strategy.module.ts
Register StrategyUserService and StrategyUserRepository inStrategyModuleserver/src/modules/strategy/strategy.module.ts
StrategyUserServiceandStrategyUserRepositoryin theStrategyModule.1 files
transactions.service.ts
Fix import paths in TransactionsServiceserver/src/modules/transactions/transactions.service.ts - Fixes import paths for `Transaction` and `UserBalance` entities.
3 files
rebalance.service.ts
Format arguments in rebalance service method callserver/src/modules/mixin/rebalance/rebalance.service.ts
rebalanceFromMixinToExchangemethod call forreadability.
exchange.service.ts
Format method signature in ExchangeServiceserver/src/modules/mixin/exchange/exchange.service.ts
findFirstAPIKeyByExchangeforreadability.
ongoing.svelte
Clean up and adjust margins in ongoing strategies displayinterface/src/lib/components/grow/elements/ongoing.svelte
display.