Hu-Fi / Mr.Market

Mr. Market is the exchange oracle of HuFi, and a CeFi crypto bot on Mixin Messenger
https://mr-market-one.vercel.app
GNU Affero General Public License v3.0
1 stars 6 forks source link

Add manual strategy execution. #175

Closed Faouzijedidi1 closed 4 months ago

Faouzijedidi1 commented 4 months ago

PR Type

enhancement


Description


Changes walkthrough πŸ“

Relevant files
Enhancement
health.service.spec.ts
Simplify Mock Implementation in Health Service Tests         

server/src/modules/health/health.service.spec.ts
  • Simplified the mock implementation for fetchBalance for the 'mexc'
    case.
  • +1/-3     
    health.service.ts
    Improve Error Handling and Readability in Health Service 

    server/src/modules/health/health.service.ts
  • Added comments and improved readability for handling non-existent
    balance fields in responses.
  • +2/-1     
    strategy.controller.ts
    Add Endpoints for Manual Strategy Execution in Strategy Controller

    server/src/modules/strategy/strategy.controller.ts
  • Added new endpoints for executing arbitrage and pure market making
    strategies.
  • Introduced DTOs for the new endpoints to handle strategy parameters.
  • Enhanced API documentation for the new strategy execution methods.
  • +38/-0   

    πŸ’‘ PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    vercel[bot] commented 4 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 May 9, 2024 0:53am
    railway-app[bot] commented 4 months ago

    πŸš… Previously deployed to Railway in the quiet-desk project. Environment has been deleted.

    github-actions[bot] commented 4 months ago

    PR Description updated to latest commit (https://github.com/Hu-Fi/Mr.Market/commit/105f13390ddf91ef3345d55c602384481e03d5f0)

    github-actions[bot] commented 4 months ago

    PR Review πŸ”

    ⏱️ Estimated effort to review [1-5] 3, because the PR involves multiple files and functionalities including API endpoint additions and internal logic changes. The changes are moderate in size and complexity, requiring a thorough review to ensure functionality and integration.
    πŸ§ͺ Relevant tests No
    ⚑ Possible issues Possible Bug: The error handling in `health.service.ts` might not correctly identify all types of errors since it only checks for the presence of a response. It should also handle cases where the response might be there but is not valid.
    πŸ”’ Security concerns No
    Code feedback:
    relevant fileserver/src/modules/health/health.service.ts
    suggestion       Consider adding more robust error handling to manage different types of invalid responses or errors from the exchange APIs. This could involve checking the structure of the response or the presence of specific fields rather than just the existence of any response. [important]
    relevant line// there is no field like balance in responses[i]

    relevant fileserver/src/modules/strategy/strategy.controller.ts
    suggestion       Implement input validation for the new endpoints `/execute-arbitrage` and `/execute-pure-market-making` to ensure that the incoming requests contain valid and complete data before processing. This can prevent issues related to bad input data leading to runtime errors or incorrect strategy execution. [important]
    relevant lineasync executeArbitrage(@Body() strategyParamsDto: ArbitrageStrategyDto) {

    relevant fileserver/src/modules/strategy/strategy.controller.ts
    suggestion       Add error handling for the new POST endpoints to manage and respond to possible exceptions during the execution of strategies. This could involve try-catch blocks around the strategy execution calls, with appropriate error responses to the client. [important]
    relevant linereturn this.strategyService.startArbitrageStrategyForUser(

    relevant fileserver/src/modules/strategy/strategy.controller.ts
    suggestion       Consider adding logging for the execution start and end of the strategies in both new endpoints. This would help in monitoring and debugging the strategy execution lifecycle. [medium]
    relevant linereturn this.strategyService.executePureMarketMakingStrategy(

    github-actions[bot] commented 4 months ago

    PR Code Suggestions ✨

    CategorySuggestions                                                                                                                                                       
    Possible issue
    Handle potential undefined values to avoid runtime errors. ___ **Consider handling the case when fetchBalance returns undefined to avoid potential runtime
    errors in the application. You can provide a default value or throw an error.** [server/src/modules/health/health.service.spec.ts [41]](https://github.com/Hu-Fi/Mr.Market/pull/175/files#diff-3f39116ae1a88a5debee442b1fae06726ef0423069846db7e75caaf008973d97R41-R41) ```diff -exchangeMock.fetchBalance = jest.fn().mockResolvedValue(undefined); +exchangeMock.fetchBalance = jest.fn().mockResolvedValue(undefined || { total: 0 }); ```
    Enhancement
    Improve the accuracy of health checks by refining the conditions. ___ **Ensure that the response handling logic correctly interprets what constitutes a 'dead'
    exchange. Consider checking specific fields or error codes instead of a nullish response.** [server/src/modules/health/health.service.ts [117]](https://github.com/Hu-Fi/Mr.Market/pull/175/files#diff-7b0c63150c3cc9314b9427fa5c13a4130d8214c39d1186a17dd9fc91baf771cdR117-R117) ```diff -if (!responses[i]) { +if (!responses[i] || !responses[i].balance) { ```
    Add error handling to improve reliability and client communication. ___ **Add error handling for the executeArbitrage method to manage exceptions and provide
    meaningful error responses to the client.** [server/src/modules/strategy/strategy.controller.ts [104-106]](https://github.com/Hu-Fi/Mr.Market/pull/175/files#diff-1e7694e47e509da7fcbde80792f7da820604e376c9521762faf21e6a1264c3beR104-R106) ```diff -return this.strategyService.startArbitrageStrategyForUser(strategyParamsDto); +try { + return await this.strategyService.startArbitrageStrategyForUser(strategyParamsDto); +} catch (error) { + this.logger.error('Failed to execute arbitrage strategy', error); + throw new HttpException('Failed to execute arbitrage strategy', HttpStatus.INTERNAL_SERVER_ERROR); +} ```
    Best practice
    Validate input data to ensure all necessary parameters are present. ___ **Validate the strategyParamsDto in executePureMarketMaking to ensure all required fields
    are provided before proceeding with the strategy execution.** [server/src/modules/strategy/strategy.controller.ts [183]](https://github.com/Hu-Fi/Mr.Market/pull/175/files#diff-1e7694e47e509da7fcbde80792f7da820604e376c9521762faf21e6a1264c3beR183-R183) ```diff +if (!strategyParamsDto.requiredField) { + throw new HttpException('Missing required parameter: requiredField', HttpStatus.BAD_REQUEST); +} return this.strategyService.executePureMarketMakingStrategy(strategyParamsDto); ```
    Maintainability
    Add logging to monitor strategy execution stages. ___ **Consider adding logging to the executePureMarketMaking method to track the initiation and
    completion of the strategy, which can help in debugging and monitoring the system.** [server/src/modules/strategy/strategy.controller.ts [183]](https://github.com/Hu-Fi/Mr.Market/pull/175/files#diff-1e7694e47e509da7fcbde80792f7da820604e376c9521762faf21e6a1264c3beR183-R183) ```diff -return this.strategyService.executePureMarketMakingStrategy(strategyParamsDto); +this.logger.log(`Initiating pure market making strategy for user with params: ${JSON.stringify(strategyParamsDto)}`); +const result = await this.strategyService.executePureMarketMakingStrategy(strategyParamsDto); +this.logger.log('Strategy execution completed successfully'); +return result; ```