Feature/fiat payments #251

ygrishajev · 2024-06-27T10:04:33Z

Proposal for Billing Feature Structure

This PR proposes a structure for the upcoming billing feature. It includes some entities to showcase the structure, so please overlook any weak typing or lack of implementations for now.

Main Concepts

Dedicated Module

The billing module should ideally include all entities necessary to implement its business requirements within its domain.
It would be beneficial for the module to contain database accessors, services, controllers, and routers.
The module should aim to hide the complexity of business logic under the hood.
It should expose only HTTP APIs and control what can be imported by other modules.

Flat Structure

To maintain clarity, avoid deeply nested submodules. Since the application domain is already clear, a flat structure is suggested.
Recommended directory structure:

Correct:
-- billing
-- -- services
-- -- -- wallet-manager
-- -- -- -- WalletManager.ts
-- -- -- -- WalletManager.spec.ts
-- -- -- wallet-refill-service
-- -- -- -- WalletRefillService.ts
-- -- -- -- WalletRefillService.spec.ts

Wrong:
-- billing
-- -- services
-- -- -- wallet-manager
-- -- -- -- WalletManager.ts
-- -- -- -- WalletManager.spec.ts
-- -- -- -- WalletRefillService.ts
-- -- -- -- WalletRefillService.spec.ts

Naming

Aim to use consistent naming conventions:
- kebab-case for directories.
- CamelCase for files.
Names should ideally reflect the main export. While names like utils and helpers are common, they can be misleading. Instead, consider using names that clearly describe their purpose, such as StringService for something that works with strings.

Exports

It's generally better to avoid files containing code for multiple purposes. Splitting them can help ensure each file serves a single purpose, which aids in clarity and maintenance.

Database

Direct access to Sequelize DB models should be limited. Abstracting Sequelize using a repository pattern can help ensure the app doesn’t depend heavily on the storage mechanism.
This abstraction can be beneficial, especially if ORM or DB changes are needed in the future.

Workers

Workers should operate independently of the main HTTP app to prevent potential performance issues under load.
Consider using a CLI as a dedicated entry point for workers. Building a container for the job and running it with the infrastructure (e.g., this example) can provide a dedicated environment while maintaining a unified codebase.

Dependency Injection (DI)

Utilizing dependency injection can produce more testable and independent code that relies on interfaces.
While classes are a natural fit for DI, a functional approach can also be effective. DI can be beneficial even in apps without a specific framework.

Migrations

We need to discuss our approach to migrations. Using Sequelize for this might increase our dependency on it and impose structural obligations on our app.
It may be worth considering other tools or methods for handling migrations to maintain flexibility.

Note: This is a proposal. I welcome counter-arguments and suggestions. The goal is to have a constructive discussion and agree on an approach we can follow consistently.

baktun14 · 2024-06-27T14:27:54Z

How do you DI being implemented? I know there's some libraries to automate the instantiation based on the type.

baktun14

I think the general direction makes sense. I would tend to go with a simpler approach and not implement everything all at once (migrations + di + repositories).

As for the migrations, I'm not opposed to using another library to handle them. I haven't taken the time to do proper research for the possible solutions.

baktun14 · 2024-06-27T14:22:21Z

apps/api/src/billing/models/user-wallet/UserWalletModel.ts

I agree that the modules should be in their separate domain specific folders, however I think these should be in the database package.

Why? If we have a service that is responsible for working with this table. What's the reason for it to be in a package. Shouldn't other apps use api exposed by a service?

I understand your point, but I don't think it's a good idea to spread the same db schema into multiple projects, especially that there's relation between the user_setting table and the user_wallet.

There is no relation. The relation is on user isn't it? And user is an external service anyway (auth0). This is a totally normal thing in micro services world

This might also be a good point. Every app should have own db after all. It's no issue having the same, but the point is that there is still clear domain based separation of concerns.
Schemas shouldn't be shared indeed. Services should use apis to interact.

ygrishajev · 2024-06-27T14:53:44Z

@baktun14 DI is an approach. Instead of importing a dep and using directly it should be provided as an arg to a function/class. Like I did in the example.

Although I agree some automation would be nuts.

ygrishajev · 2024-06-27T16:24:54Z

@baktun14 @Redm4x BTW decided to investigate DI question just a bit. Some options:

both things should be easy to use from the start.

Basically the flow would be:

use classes as usual
register them
create context and put it into hono route context

Say, we have these dependency flow:
WalletController -> WalletService -> UserWalletModel

These classes are the same as if there's no automated DI, so no extra work. What needs to be done:

install new lib to manage that (no effort)
create a simple service defining all the deps. In case with the lib that would be creating a container and a lib, in case with nest it would be a module + context. (we have 2-4 classes to start with - 30min?)
add it to hono with a middleware (30min to try and test)

I don't see too much effort in this. If you guys think this is beneficial it's better be done imo.

ygrishajev · 2024-06-27T16:47:22Z

oh, this one seems even better https://www.npmjs.com/package/typedi

baktun14 · 2024-06-27T17:00:58Z

oh, this one seems even better https://www.npmjs.com/package/typedi

Yea this is the one I saw in an article.

ygrishajev · 2024-06-28T08:17:01Z

Ok, added TypeDI - this is really not a big deal in terms of effort, the impact though is changing imo.

UPDATE:

ugh... I don't like this typestack/typedi#1235

will try some of these

UPDATE:

ok, TSyringe from Microsoft looks real nice and must be reliable b2cf5ac

ygrishajev · 2024-06-28T10:22:10Z

added drizzle 837319c

this is a fully functional example. I personally see no reason not to start migrating slowly.

apps/api/src/billing/models/user-wallet/UserWalletModel.ts

Redm4x · 2024-07-03T15:22:06Z

apps/api/src/billing/model-schemas/userWalletSchema.ts

+import { integer, pgTable, serial, varchar } from "drizzle-orm/pg-core";
+
+export const userWalletSchema = pgTable("user_wallets", {
+  id: serial("serial").primaryKey(),


Why use serial instead of uuid here? Is it to use it as the wallet index?

Also the column name (serial) doesn't match the property name (id)

Yes, it's going to be used as address index

Redm4x · 2024-07-03T15:24:41Z

apps/api/src/billing/models/user-wallet/UserWalletModel.ts

+@Table({
+  modelName: "user_wallet"
+})
+export class UserWalletModel extends Model<UserWallet> implements Omit<UserWalletModel, "createdAt" | "updatedAt"> {


Is the goal to have a drizzle AND a sequelize model during the transition period to drizzle?

I don't see any application for this atm, so I'd drop it. But yea, we can have both. Wdyt?

baktun14 · 2024-07-04T14:42:48Z

apps/api/src/billing/services/wallet-manager/WalletManager.ts

+  private async estimateFee(messages: readonly EncodeObject[], denom: string) {
+    const client = await this.getClient();
+    const address = await this.getMasterWalletAddress();
+    const gasEstimation = await client.simulate(address, messages, "allowance grant");


We can remove the memo

I think the address should be based on the current tx and not hardcoded to be the master wallet

I thought it's a transaction on the master wallet, so the estimation of the fee should be also based on master wallet. Could you please elaborate on this comment a bit more because I don't not follow.

We can remove the memo

Memo is a required parameter. Can not omit it due to type

baktun14

What's the naming convention for services? camelCase or PascalCase?

baktun14 · 2024-07-04T14:43:50Z

apps/api/src/billing/services/wallet-manager/WalletManager.ts

+  private async estimateFee(messages: readonly EncodeObject[], denom: string) {
+    const client = await this.getClient();
+    const address = await this.getMasterWalletAddress();
+    const gasEstimation = await client.simulate(address, messages, "allowance grant");


I think the address should be based on the current tx and not hardcoded to be the master wallet

ygrishajev · 2024-07-04T15:14:34Z

What's the naming convention for services? camelCase or PascalCase?

I tried to describe is the convention in the description of this PR. I personally like it in a different way. However, I tried to match something that we already have in the project. I am open to change this to something we can commonly agree to.

I personally like nest JS convention when directories are named in snake case and files are named in snake case with an extension containing the type of the file. Like some-class.service.ts. The file should contain a class or a function that that is named the same as the file by camel case. To me, this approach reduces cognitive load when going through the code radically.

baktun14 · 2024-07-04T15:22:55Z

What's the naming convention for services? camelCase or PascalCase?

I tried to describe is the convention in the description of this PR. I personally like it in a different way. However, I tried to match something that we already have in the project. I am open to change this to something we can commonly agree to.

I personally like nest JS convention when directories are named in snake case and files are named in snake case with an extension containing the type of the file. Like some-class.service.ts. The file should contain a class or a function that that is named the same as the file by camel case. To me, this approach reduces cognitive load when going through the code radically.

I was referring to logHttpRequests.ts and postgres.ts that are camelCase.

ygrishajev · 2024-07-04T15:24:10Z

What's the naming convention for services? camelCase or PascalCase?

I tried to describe is the convention in the description of this PR. I personally like it in a different way. However, I tried to match something that we already have in the project. I am open to change this to something we can commonly agree to.

I personally like nest JS convention when directories are named in snake case and files are named in snake case with an extension containing the type of the file. Like some-class.service.ts. The file should contain a class or a function that that is named the same as the file by camel case. To me, this approach reduces cognitive load when going through the code radically.

I was referring to logHttpRequests.ts and postgres.ts that are camelCase.

Right, those are files and match the main exported function

baktun14 · 2024-07-08T16:33:26Z

apps/api/test/functional/create-wallet.spec.ts

+import { ApiPgDatabase, POSTGRES_DB } from "@src/core";
+import { closeConnections } from "@src/db/dbConnection";
+
+jest.setTimeout(30000);


was this for testing?

Request was taking more then default 5sec for me quite a bit

As a part of this add: - DI container - Drizzle ORM - logging refs #247

ygrishajev force-pushed the feature/fiat-payments branch from a6bb1b8 to 8ab3e8a Compare June 27, 2024 10:05

baktun14 reviewed Jun 27, 2024

View reviewed changes

ygrishajev force-pushed the feature/fiat-payments branch from 0b52d37 to b2cf5ac Compare June 28, 2024 11:33

Redm4x reviewed Jul 3, 2024

View reviewed changes

ygrishajev force-pushed the feature/fiat-payments branch from e4088c1 to bacc119 Compare July 4, 2024 10:34

baktun14 reviewed Jul 4, 2024

View reviewed changes

baktun14 approved these changes Jul 8, 2024

View reviewed changes

ygrishajev force-pushed the feature/fiat-payments branch 4 times, most recently from 1afe31e to bd61dcf Compare July 9, 2024 11:54

feat(billing): add billing module with trial wallet creation

3dfaeff

As a part of this add: - DI container - Drizzle ORM - logging refs #247

ygrishajev force-pushed the feature/fiat-payments branch from bd61dcf to 3dfaeff Compare July 9, 2024 11:56

ygrishajev merged commit d1ca550 into main Jul 9, 2024
5 checks passed

ygrishajev deleted the feature/fiat-payments branch July 9, 2024 11:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/fiat payments #251

Feature/fiat payments #251

ygrishajev commented Jun 27, 2024 •

edited

Loading

baktun14 commented Jun 27, 2024

baktun14 left a comment

baktun14 Jun 27, 2024

ygrishajev Jun 27, 2024

baktun14 Jun 27, 2024

ygrishajev Jun 27, 2024

ygrishajev Jun 27, 2024

ygrishajev commented Jun 27, 2024

ygrishajev commented Jun 27, 2024 •

edited

Loading

ygrishajev commented Jun 27, 2024

baktun14 commented Jun 27, 2024

ygrishajev commented Jun 28, 2024 •

edited

Loading

ygrishajev commented Jun 28, 2024

Redm4x Jul 3, 2024

Redm4x Jul 3, 2024

ygrishajev Jul 3, 2024

Redm4x Jul 3, 2024

ygrishajev Jul 3, 2024

baktun14 Jul 4, 2024

baktun14 Jul 4, 2024

ygrishajev Jul 4, 2024

ygrishajev Jul 4, 2024

baktun14 left a comment

baktun14 Jul 4, 2024

ygrishajev commented Jul 4, 2024 •

edited

Loading

baktun14 commented Jul 4, 2024

ygrishajev commented Jul 4, 2024

baktun14 Jul 8, 2024

ygrishajev Jul 8, 2024

Feature/fiat payments #251

Feature/fiat payments #251

Conversation

ygrishajev commented Jun 27, 2024 • edited Loading

Proposal for Billing Feature Structure

Main Concepts

Dedicated Module

Flat Structure

Naming

Exports

Database

Workers

Dependency Injection (DI)

Migrations

baktun14 commented Jun 27, 2024

baktun14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ygrishajev commented Jun 27, 2024

ygrishajev commented Jun 27, 2024 • edited Loading

ygrishajev commented Jun 27, 2024

baktun14 commented Jun 27, 2024

ygrishajev commented Jun 28, 2024 • edited Loading

ygrishajev commented Jun 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baktun14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ygrishajev commented Jul 4, 2024 • edited Loading

baktun14 commented Jul 4, 2024

ygrishajev commented Jul 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ygrishajev commented Jun 27, 2024 •

edited

Loading

ygrishajev commented Jun 27, 2024 •

edited

Loading

ygrishajev commented Jun 28, 2024 •

edited

Loading

ygrishajev commented Jul 4, 2024 •

edited

Loading