search

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 more exchange support #204

Closed Faouzijedidi1 closed 1 week ago

Faouzijedidi1 commented 1 week ago

User description

Description

Add exchange init service Add Bigone, BitMart, and Lbank to supported exchanges.

Summary of changes

How to test the changes

PR Type

Enhancement, Tests

Description

Changes walkthrough 📝

Relevant files
Enhancement
app.module.ts
Add ExchangeInitService to AppModule providers                     
server/src/app.module.ts - Added `ExchangeInitService` to the providers array.
+2/-1     
exchangeInit.service.ts
Implement ExchangeInitService for managing exchanges         
server/src/modules/exchangeInit/exchangeInit.service.ts
  • Created ExchangeInitService to initialize and manage exchange
    instances.
  • Added methods to get exchange instances and supported exchanges.
    		•  +79/-0   
    strategy.service.ts
    Refactor StrategyService to use ExchangeInitService           
    server/src/modules/strategy/strategy.service.ts
  • Refactored to use ExchangeInitService for exchange management.
  • Removed local exchange initialization logic.
    		•  +9/-58   
    Tests
    exchangeInit.service.spec.ts
    Add unit tests for ExchangeInitService                                     
    server/src/modules/exchangeInit/exchangeInit.service.spec.ts
  • Added unit tests for ExchangeInitService.
  • Verified service definition.
    		•  +18/-0   
    Configuration changes
    .env.example
    Update .env.example with new exchange credentials               
    server/.env.example
  • Added API keys and secrets for Lbank, Bigone, and BitMart exchanges.
    		•  +9/-0     

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

    railway-app[bot] commented 1 week ago

    🚅 Previously deployed to Railway in the quiet-desk project. Environment has been deleted.

    vercel[bot] commented 1 week 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 Jun 26, 2024 9:52pm
    github-actions[bot] commented 1 week ago

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 3
    🧪 Relevant tests Yes
    🔒 Security concerns No
    ⚡ Key issues to review Possible Bug:
    The ExchangeInitService initializes exchanges in its constructor, which might not handle asynchronous initialization properly if any of the exchange APIs require async operations. This could lead to incomplete or failed initializations without proper error handling or retries.
    Hardcoded API Keys:
    The initialization of exchanges directly uses environment variables within the class. It's generally a better practice to inject these dependencies to make the service easier to test and configure.
    Error Handling:
    The method getExchange in ExchangeInitService throws a generic InternalServerErrorException which might not be informative enough for the caller to understand the actual issue. Consider throwing more specific exceptions or returning error codes.
    github-actions[bot] commented 1 week ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add error handling to the exchange initialization process to manage potential exceptions ___ **Add error handling for the initializeExchanges method to manage exceptions that may occur
    during the initialization of exchanges, such as network issues or invalid configuration.** [server/src/modules/exchangeInit/exchangeInit.service.ts [14-20]](https://github.com/Hu-Fi/Mr.Market/pull/204/files#diff-7568abcdca44efd6412caab9f5b788bcb5158285fe49e402618f72683ae16784R14-R20) ```diff private initializeExchanges() { - // Initialize exchanges - this.exchanges.set( - 'okx', - new ccxt.pro.okx({ - apiKey: process.env.OKX_API_KEY, - secret: process.env.OKX_SECRET, - }), - ); + try { + // Initialize exchanges + this.exchanges.set( + 'okx', + new ccxt.pro.okx({ + apiKey: this.configService.get('OKX_API_KEY'), + secret: this.configService.get('OKX_SECRET'), + }), + ); + // Add other exchanges similarly + } catch (error) { + this.logger.error('Failed to initialize exchanges', error); + throw new InternalServerErrorException('Failed to initialize exchanges'); + } } ```
    Suggestion importance[1-10]: 10 Why: Adding error handling is crucial for robustness, ensuring that the system can gracefully handle initialization failures.
    		10
    Security
    Improve security by using a configuration service to manage environment variables ___ **Consider using environment variables more securely by not directly accessing process.env
    within the ExchangeInitService. Instead, use a configuration service that can provide
    these values. This approach improves security by centralizing the management of
    environment variables and potentially integrating with secret management tools.** [server/src/modules/exchangeInit/exchangeInit.service.ts [19-20]](https://github.com/Hu-Fi/Mr.Market/pull/204/files#diff-7568abcdca44efd6412caab9f5b788bcb5158285fe49e402618f72683ae16784R19-R20) ```diff -apiKey: process.env.OKX_API_KEY, -secret: process.env.OKX_SECRET, +apiKey: this.configService.get('OKX_API_KEY'), +secret: this.configService.get('OKX_SECRET'), ```
    Suggestion importance[1-10]: 9 Why: This suggestion enhances security by centralizing environment variable management, reducing the risk of exposing sensitive information.
    		9
    Maintainability
    Refactor repetitive exchange setup code into a loop for better maintainability ___ **Refactor the repetitive code for setting up each exchange into a loop that iterates over a
    configuration object. This will reduce the amount of code and make it easier to add or
    remove exchanges in the future.** [server/src/modules/exchangeInit/exchangeInit.service.ts [16-20]](https://github.com/Hu-Fi/Mr.Market/pull/204/files#diff-7568abcdca44efd6412caab9f5b788bcb5158285fe49e402618f72683ae16784R16-R20) ```diff -this.exchanges.set( - 'okx', - new ccxt.pro.okx({ - apiKey: process.env.OKX_API_KEY, - secret: process.env.OKX_SECRET, - }), -); +const exchangeConfigs = [ + { name: 'okx', type: ccxt.pro.okx }, + { name: 'bitfinex', type: ccxt.pro.bitfinex }, + // Add other exchanges here +]; +exchangeConfigs.forEach(config => { + this.exchanges.set( + config.name, + new config.type({ + apiKey: this.configService.get(`${config.name.toUpperCase()}_API_KEY`), + secret: this.configService.get(`${config.name.toUpperCase()}_SECRET`), + }) + ); +}); ```
    Suggestion importance[1-10]: 8 Why: This refactoring improves code maintainability and readability by reducing redundancy and making it easier to add or remove exchanges.
    		8
    Best practice
    Adjust the ExchangeInitService variable name to follow TypeScript naming conventions ___ **Ensure that the ExchangeInitService variable follows the TypeScript naming conventions,
    which recommend camelCase for instance variables. The current implementation uses
    PascalCase, which is typically reserved for classes or interfaces.** [server/src/modules/strategy/strategy.service.ts [114]](https://github.com/Hu-Fi/Mr.Market/pull/204/files#diff-413cb1b28e0d47a46768f97d10145a8e14d9e46b0a195768786127305916d944R114-R114) ```diff -private ExchangeInitService: ExchangeInitService, +private exchangeInitService: ExchangeInitService, ```
    Suggestion importance[1-10]: 7 Why: Following TypeScript naming conventions improves code readability and consistency, though it is a minor change.
    		7