search

sotatek-dev / multichain-bridge-backend

0 stars 0 forks source link

Miscellaneous Feedback #15

Closed ejMina226 closed 1 month ago

ejMina226 commented 5 months ago

Hi, here is some feedback from @harrysolovay and myself.

We liked seeing the runtime validations, attention to swagger / introspectability, and use of type-orm for type-safe bindings to the DB. While we cannot comment on some of the specifics of NestJS usage (such as its method of composition via the Injectable decorator), some things that caught our eyes as potential issues / JS antipatterns:

The TypeScript configuration is very loose. There are several flags that would dramatically enhance the quality of checking. https://www.typescriptlang.org/tsconfig/#noUncheckedIndexedAccess https://www.typescriptlang.org/tsconfig/#strict https://www.typescriptlang.org/tsconfig/#alwaysStrict

Many successful libraries (such as class-validator and class-transformer) use legacy decorators. These dependencies are not necessarily good choices for new projects. There is a new decorators spec, which has been implemented in JS runtimes. It was introduced in TypeScript 5 (see announcement here). Good blog post about it here.

Why do we have these ungraceful exits. The process should automatically terminate when there are no longer any running jobs.

We strongly recommend avoiding augmentation of the global array type here

michal0mina commented 5 months ago

Further feedback and questions regarding implementation:

  1. Can you give quick rationale why it's NestJS instead of node.js or another well known framework?

  2. There are empty files, not used statements, hello world entries -- is it possible to clean it up?

  3. What's the protection of Admin user? It has power to move all tokens

  4. Bridge transactions are represented by Mina transactions -- this has then constraint on Mina TPS. How external entities faster than Mina will co-operate with the bridge?

  5. There is no ZK proof representing and following correct balance of Mina tokens and minted tokens

  6. What is final branch? main or new-token?

  7. The tests are very sparse and not representing functionality of the bridge

  8. Liveness of the bridge depends on the Typeorm availability -- this is a concern

  9. Lack of validation of input data in controllers (e.g. email). Is it validated in the db?

  10. Can you clean up unused methods, imports and not-relevant comments?

Sotatek-TanHoang commented 5 months ago

Further feedback and questions regarding implementation:

  1. Can you give quick rationale why it's NestJS instead of node.js or another well known framework?
  2. There are empty files, not used statements, hello world entries -- is it possible to clean it up?
  3. What's the protection of Admin user? It has power to move all tokens
  4. Bridge transactions are represented by Mina transactions -- this has then constraint on Mina TPS. How external entities faster than Mina will co-operate with the bridge?
  5. There is no ZK proof representing and following correct balance of Mina tokens and minted tokens
  6. What is final branch? main or new-token?
  7. The tests are very sparse and not representing functionality of the bridge
  8. Liveness of the bridge depends on the Typeorm availability -- this is a concern
  9. Lack of validation of input data in controllers (e.g. email). Is it validated in the db?
  10. Can you clean up unused methods, imports and not-relevant comments?

Hi @michal0mina i'm here to answer these questions:

  1. We chose NestJS because the framework matches with the project tech stack and we have experience with it.
  2. yes, it's crucial to refactor those unused files, statements. 3.the admin account is secured by the private key, which should be secured by keys owner.
  3. do you mean our system can be bottle-necked by Mina TPS?
  4. Can you share more about this?
  5. The final branch is main, the merging process is main <- staging <- testing.
  6. we added tests for crucial functions like mina bridge, evm bridge... other tests will be added gradually like authentication, user management...
  7. do you mean the risk of maintaining the project if typeorm is deprecated?
  8. we added logic to validate (i.e: email, wallet address...) for input data.
  9. yes, we are refactoring the source code. Here are the answer for most of the questions above. Can you give more detail about questions number 4, 5, 8. If any answer is not clear, please let me know. Thanks.
Sotatek-TanHoang commented 5 months ago

Hi, here is some feedback from @harrysolovay and myself.

We liked seeing the runtime validations, attention to swagger / introspectability, and use of type-orm for type-safe bindings to the DB. While we cannot comment on some of the specifics of NestJS usage (such as its method of composition via the Injectable decorator), some things that caught our eyes as potential issues / JS antipatterns:

The TypeScript configuration is very loose. There are several flags that would dramatically enhance the quality of checking. https://www.typescriptlang.org/tsconfig/#noUncheckedIndexedAccess https://www.typescriptlang.org/tsconfig/#strict https://www.typescriptlang.org/tsconfig/#alwaysStrict

Many successful libraries (such as class-validator and class-transformer) use legacy decorators. These dependencies are not necessarily good choices for new projects. There is a new decorators spec, which has been implemented in JS runtimes. It was introduced in TypeScript 5 (see announcement here). Good blog post about it here.

Why do we have these ungraceful exits. The process should automatically terminate when there are no longer any running jobs.

We strongly recommend avoiding augmentation of the global array type here

We are reviewing the source code and will take this information as reference. Thanks.