Closed Faouzijedidi1 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 11, 2024 1:48am |
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/9f7633b1d0dd98e1179629402711ec9d3c102a8e)
github-actions[bot]
commented
2 months ago | ⏱️ Estimated effort to review [1-5] | 2, because the changes are straightforward, involving renaming entities and making a column nullable. The PR touches multiple files but the nature of changes is repetitive and doesn't seem to introduce complex logic. |
| 🧪 Relevant tests | No |
| 🔍 Possible issues | Possible Bug: Making `clientId` nullable in `MMOrder` and `ArbitrageOrder` entities without updating related logic might introduce bugs where `clientId` is expected to be non-null. |
| 🔒 Security concerns | No |
| relevant file | server/src/common/entities/mm-order.entity.ts |
| suggestion | Consider adding a migration script for database changes, especially for making `clientId` nullable. This ensures existing databases are correctly updated without manual intervention. [important] |
| relevant line | @Column({ nullable: true }) |
| relevant file | server/src/modules/strategy/strategy.service.ts |
| suggestion | Ensure that the logic consuming `getUserOrders` method handles the case where `clientId` can be null, to avoid runtime errors. [important] |
| relevant line | async getUserOrders(userId: string): Promise |
| relevant file | server/src/common/entities/arbitrage-order.entity.ts |
| suggestion | Validate the impact of making `clientId` nullable on the business logic, especially in areas where `clientId` was previously assumed to be always present. [medium] |
| relevant line | @Column({ nullable: true }) |
| relevant file | server/src/app.module.ts |
| suggestion | After renaming `Order` to `MMOrder`, verify if any module imports or service injections need to be updated to reflect the new entity name to avoid dependency resolution issues. [medium] |
| relevant line | import { MMOrder } from './common/entities/mm-order.entity'; |
github-actions[bot]
commented
2 months ago | Category | Suggestions |
| Enhancement |
Correct a typo in the route path for better readability.___ **Consider renaming the route from '/maretmaking/:userId' to '/marketmaking/:userId' tocorrect the typo and improve the API's readability.** [server/src/modules/strategy/orders.controller.ts [12]](https://github.com/Hu-Fi/Mr.Market/pull/145/files#diff-0069d0715a0f071e72068a2530df2fa7b1663ab751cc3b8f9a90d343369b516fR12-R12) ```diff -@Get('/maretmaking/:userId') +@Get('/marketmaking/:userId') ``` |
Add error handling to database queries for improved robustness.___ **When fetching user orders ingetUserOrders, consider adding error handling to manage cases where the database query fails, improving the robustness of your application.** [server/src/modules/strategy/strategy.service.ts [710-713]](https://github.com/Hu-Fi/Mr.Market/pull/145/files#diff-413cb1b28e0d47a46768f97d10145a8e14d9e46b0a195768786127305916d944R710-R713) ```diff -return await this.orderRepository.find({ +try { + return await this.orderRepository.find({ + where: { userId }, + }); +} catch (error) { + throw new InternalServerErrorException(error); +} ``` | |
| Best practice |
Specify column type for better database schema consistency.___ **For theclientId column, consider adding an explicit type, such as varchar, to ensure consistency across different databases and to make the schema more readable.** [server/src/common/entities/mm-order.entity.ts [11]](https://github.com/Hu-Fi/Mr.Market/pull/145/files#diff-db3678ceab2b08be6527186ae21cae7c0bdb4eeedb4f035144aced59dbd28729R11-R11) ```diff -@Column({ nullable: true }) +@Column({ type: 'varchar', nullable: true }) ``` |
| Possible issue |
Verify the nullable attribute of
___
**Ensure that making |
| Maintainability |
Organize imports in a consistent manner for better readability.___ **Consider organizing imports in a consistent order, such as alphabetizing them or groupingby source location, to improve code readability and maintainability.** [server/src/app.module.ts [41]](https://github.com/Hu-Fi/Mr.Market/pull/145/files#diff-4e8033eb0f9fd87924c445b7ac0f1c1192d4890fc1589b2fad2679797d4f4ce0R41-R41) ```diff +import { ArbitrageOrder } from './common/entities/arbitrage-order.entity'; import { MMOrder } from './common/entities/mm-order.entity'; ``` |
Type
enhancement, bug_fix
Description
Orderentity toMMOrderacross various modules and services to better reflect its purpose.clientIdcolumn nullable inArbitrageOrderandMMOrderentities to accommodate orders without client IDs.Changes walkthrough
app.module.ts
Rename Order Entity to MMOrder in AppModule Importsserver/src/app.module.ts
Orderentity toMMOrderin module imports and providers.mm-order.entity.ts
Rename and Update MMOrder Entityserver/src/common/entities/mm-order.entity.ts
Orderentity toMMOrder.clientIdcolumn nullable.orders.controller.ts
Update OrdersController to Use MMOrderserver/src/modules/strategy/orders.controller.ts - Updated imports and method return types from `Order` to `MMOrder`.
strategy.module.ts
Update StrategyModule to Use MMOrderserver/src/modules/strategy/strategy.module.ts - Updated module imports and providers from `Order` to `MMOrder`.
strategy.service.spec.ts
Update StrategyService Spec to Use MMOrderserver/src/modules/strategy/strategy.service.spec.ts - Updated mock repository token from `Order` to `MMOrder`.
strategy.service.ts
Refactor StrategyService to Use MMOrderserver/src/modules/strategy/strategy.service.ts
OrdertoMMOrder.arbitrage-order.entity.ts
Make clientId Nullable in ArbitrageOrder Entityserver/src/common/entities/arbitrage-order.entity.ts - Made `clientId` column nullable in `ArbitrageOrder` entity.