SolomonDefi / solomon-monorepo

Monorepo containing core Solomon apps, services, libraries, and deploy config.
6 stars 4 forks source link

Create api NestJS app with MikroORM #224

Closed solomondefi-dev closed 2 years ago

solomondefi-dev commented 2 years ago

As part of the process of merging api-evidence and api-dispute, create a NestJS app called api, with MikroORM as the database, so libraries can be easily shared with the blockchain-watcher app.

solomondefi-dev commented 2 years ago

This one can be split further, most of the development/initialization can be done outside skaffold, but we should also create the Dockerfile and k8s config.

solomondefi-dev commented 2 years ago

@apolkingg8 I have a couple of comments and requests after working on the build system:

apolkingg8 commented 2 years ago

Let's try to keep controllers very small, for example just decorators and a single function call into a service. I'd like to avoid unit testing controllers, and rely on functional/integration tests for that (we can still unit test services). I think unit testing controllers isn't worth the effort now, functional tests are more useful.

No matter how small the controller is, it should still be covered by tests because it not "equal" to service even if they are only one line (and usually, it's not one line only). I'll bring the tests back.

solomondefi-dev commented 2 years ago

Yes, but if the controllers are clean, it's more effective to test with functional/integration tests as it is with unit tests. With a well written controller, unit tests are somewhat redundant.

apolkingg8 commented 2 years ago

Let's try to keep controllers very small, for example just decorators and a single function call into a service. I'd like to avoid unit testing controllers, and rely on functional/integration tests for that (we can still unit test services). I think unit testing controllers isn't worth the effort now, functional tests are more useful.

When we choose the DB, the adapter is not the first priority. If the node-sqlite passed all the tests in MikroORM, that means it's "good enough" for use. I will consider the DB because sqlite is a temp choice for better developer experience, but I don't think the adapter should be the most important reason.

solomondefi-dev commented 2 years ago

MikroORM is not using node-sqlite, it's using a temporary fork due to an unpatched vulnerability. They will switch back to node-sqlite if it's fixed, or maybe change to better-sqlite. That's not my point though, I'm more concerned about the lack of arm64 binaries, and keeping the database choice consistent for maintenance purposes. It shouldn't matter from the developer perspective, since regardless of sqlite/pg, skaffold should handle everything.

apolkingg8 commented 2 years ago

The so-called "consistency" does not constitute a reason at all, because we are operating based on ORM. When making a choice, we should be concerned with whether the choice makes sense in the context of the "current project" rather than the "other project". Engineers should be able to use different libs for different projects, and it is not costly to switch between libs, unlike programming languages. It's great if you can use the same architecture, but the rationality of the project itself should always be the top priority, not consistency.

apolkingg8 commented 2 years ago

n this case, priority should be given to "which database is better for the project" rather than "which one is used by other projects".

solomondefi-dev commented 2 years ago

Yes, my main point is that we should stick to one database on this project. I'm not talking about another project. So we'll use postgres.

apolkingg8 commented 2 years ago

Split db migrate into #238

apolkingg8 commented 2 years ago

Close from #237