Skip to content

Conversation

@bludnic
Copy link
Member

@bludnic bludnic commented Aug 13, 2024

No description provided.

@bludnic
Copy link
Member Author

bludnic commented Aug 13, 2024

Config file:

  1. Create config.sample.json5 and add it to gitignore
  2. Add instructions in README like: copy config.sample.json5 into config.json5, set the passphrase, and DB url, etc.
  3. Define zod schema and validate the config at runtime. See example here https://github.com/adamant-al/etf-xbot/blob/dev/src/shared/config/schema.ts

@MerdyNumber1

@bludnic
Copy link
Member Author

bludnic commented Aug 13, 2024

Regarding prisma:

  1. There is no need passing prisma instance using function arguments. You can define it once: export const prisma = new PrismaClient(), and then import it in several places as import { prisma } from './prisma.js'. This should work fine.
  2. Add prisma generate in postinstall script. Also mention about applying migrations using prisma migrate dev in README.md.

@zasulskii
Copy link
Member

Regarding prisma:

  1. There is no need passing prisma instance using function arguments. You can define it once: export const prisma = new PrismaClient(), and then import it in several places as import { prisma } from './prisma.js'. This should work fine.
  2. Add prisma generate in postinstall script. Also mention about applying migrations using prisma migrate dev in README.md.
  1. Yes, but I use dependency injection functional pattern and passing logger as well too, modules should not be depended on imports like prisma

@bludnic
Copy link
Member Author

bludnic commented Aug 13, 2024

Dependency injection make sense when you want to ensure that same instance is shared across modules. There is only one instance of Prisma. The DI is not needed here.

Anyway, I advice you to not use this pattern. It make sense if you have right tools, where the DI is handled by a special lib (an example are decorators in Nest.js). But passing the instance manually to every function just increases the code complexity.

@MerdyNumber1

@bludnic
Copy link
Member Author

bludnic commented Aug 13, 2024

I suggest reviewing the architecture of retryNotifyJob.ts and transactionsJobs.ts as there is too much responsibility concentrated in these modules.

I recommend moving a significant part of the logic into designated channels:

  • TransactionsChannel: This channel handles any new transactions appearing on the blockchain, such as sending messages from one user to another. We need to monitor direct token transfers (type: 0) and message transactions (type: 8). Use a polling mechanism to retrieve these transactions.

  • MessagesChannel: This channel manages messages sent from the user to the ANS service, like SignalMessage. You can use sockets since we are subscribing to a specific ADM address. You can use polling as a fallback if you wish, but this logic must be encapsulated within that class.

Both classes should extend from EventEmitter. When a new transaction arrives, the class should emit a newTransaction event.

Usage:

const transactionsChannel = new TransactionsChannel();
transactionsChannel.on("newTransaction", (transaction) => {
  // perform a push notification action if needed
});

const messagesChannel = new MessagesChannel();
messagesChannel.on("newTransaction", (transaction) => {
  // save/remove Device token from DB
});

Since the architecture is event-based, you probably don't need a cron job. You may need a queue to batch notifications if FCM allow this.

logger.info(
`Spawned transaction parser job with ${config.app.txCheckInterval} interval`
)
return schedule(config.app.txCheckInterval, async () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using schedule, you can use a regular setTimeout that invokes itself after each iteration. In this case, you can remove the this.isLocked check, as invoking the callback multiple times won't be possible.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally add destroy method to this class that will:

  1. Stop the timer
  2. Stop listening for adamantClient events (adamantClient.socket.off(callback))

Invoke transactionsChannel.destroy() from main.ts on SIGINT/SIGTERM

where: { admAddress: tx.recipientId }
})

devices = devices.filter((device) => device.admAddress === tx.recipientId)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really need to filter devices here if you already used where: { admAddress: tx.recipientId } on line 16.

Comment on lines 29 to 34
await Promise.all(
devices.map((device) => {
processTransactionToNotify(tx, device)
})
)
})
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to use cargoQueue from async lib. I see that FCM supports notification batching. This should improve performance when sending a large number of notifications.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants