Proposal: Arb-subgraph refactoring #585

DiRaiks · 2024-07-26T18:39:02Z

Disclaimer: This is a proposal for the architectural restructuring and refactoring of some alerts-related code. I may lack complete context or understanding of the issues surrounding alert implementation.

Key Changes:

Replaced Express with Fastify
Utilized Plugins Instead of Various Functions and Classes
Updated ENV Variables Configuration: Migrated configuration to the Fastify framework.
Reduced Necessary Handlers: This might be specific to the provided alert example.

Some classes in the plugins (e.g., metrics) have been used as-is. However, any code structure can be used as needed. There is no strict requirement for using classes.

What is Fastify?
Learn more about Fastify here.

Why Choose Fastify Over Express:

Performance: Fastify offers better performance.
Ease of Custom Plugins: Fastify facilitates the use of custom plugins throughout the application, simplifying the interaction of different alert components.
Reusable Plugins: Plugins can be conveniently shared across different alerts, enabling the creation of a collection of reusable plugins.

Structure: arb-subgraph

Plugins Directory: Located in the plugins folder.
Reusable Plugins: Located in plugins/common.
Constants: Set of constants for the alert.
Utils: Set of utilities for the alert.
main.ts: Main file where server initialization.
env.ts: File for typing, describing, and configuring ENV variables.

Transferred from `l2-bridge-arbitrum` As-Is:

Proto Folder/Files: (the reason for choosing this approach is unclear)
Some Handler code sections

Example of Plugin Connection:

    fastify.register(fastifyEnv, ENV_OPTIONS),
    fastify.register(LoggerPlugin),
    fastify.register(ProviderPlugin),
    fastify.register(MetricsPlugin),
    fastify.register(HealthCheckerPlugin),
    fastify.register(BalanceCheckerPlugin),
    fastify.register(AlertsPlugin),
    fastify.register(ChainHandlerPlugin),
    fastify.register(GRPCServerPlugin),

Fastify also allows for convenient registration of server routes with custom handlers through plugins.

Example of Route Registration:

  await fastify.register(async (instance) => {
    instance.get('/metrics', instance.metrics.routeHandler())
    instance.get('/health', instance.healthChecker.routeHandler())
  })

Example Logs:

info: Logger plugin initialized
info: Provider plugin initialized with Arbitrum RPC provider
info: Metrics plugin initialized
info: HealthChecker plugin initialized
info: BalanceChecker plugin initialized
info: Alerts plugin initialized
info: ChainHandler plugin initialized
info: gRPC server plugin initialized
info: arb-subgraph is listening on 50051
{"level":30,"time":1722019537075,"pid":6517,"hostname":"MacBook-Pro-Andrej-2.local","msg":"Server listening at http://[::1]:3000"}
{"level":30,"time":1722019537076,"pid":6517,"hostname":"MacBook-Pro-Andrej-2.local","msg":"Server listening at http://127.0.0.1:3000"}
{"level":30,"time":1722019537076,"pid":6517,"hostname":"MacBook-Pro-Andrej-2.local","msg":"server listening on 3000"}
info: #arbitrum block: 236335474
info: handleBlock started at 21:45:42. Elapsed: 0.346 seconds

info: #arbitrum block: 236335475
info: handleBlock started at 21:45:42. Elapsed: 0.228 seconds

info: #arbitrum block: 236335476
info: handleBlock started at 21:45:42. Elapsed: 0.230 seconds

This pull request aims to improve the architecture and performance of the alerts system by leveraging Fastify's advanced features and plugin system.
Your feedback and suggestions are welcome.

…tier eslint

…ture/si-1316-refactor-subgraph-forta-alert

sergeyWh1te

Firstly, thank you of your job.

This bot - it's really simple bot. I'd say the most simple bot around all bots, and based on this idea I'd suggest some recommendations or improvements.

Recommendations:

Move your business logic (Balance.checker.ts) directly into handleBlock. There aren't different classes with business logic in arb-subgraph project.
- Few months ago I tried to remove any external dependencies like forta for providing pure tech design that would not contain anything extra deps. Only that we needed for bot working. Here I found fastify 'dependency' as http router and DI container. Speaking about performance: what kind of performance we speak about?! - '/metrics, /health' - handlers. - Strange to think about performance here.
- Speaking about DI - -higher, I texted that the bot is so simple, and it's ok to move business logic directly into HandlerBlock. From this perspective, I believe in the idea that any extra DI system is superfluous here. Also, I found that each file in project for supporting fastigy.DI has to have extra code for declaring module and creating instances of classes. They have(creating instances, of the classes) moved from main.ts into classes.

So, in arb-subgraph is added external dependency 'satisfy', but I don't see real benefits. It does not matter on real performance and creating instances. Everything would work without that extra "deps". Of course, if your team has a habit to work with that library, it might be OK. But I personally don't split up this vision. All authors of programming languages advise to have to use only that what we need and nothing more extra. I do agree with them; do not use extra, and simplify as much as possible.

Improvements:
Once I shared a picture, the bot that works under the l2 network might have time lags. So, in reality, forta-softaware has lags with l2 networks. For us, it means that bots have to iterate under l1 network.

Speaking about this bot. Bot can listen up L1 network that fetch latest l2 block and make some checks.

Also, our local infra would not support interating over l2 network. If your team is going to deploy the bot into local infra - bot must iterate over the l1 network.

Approve/Request changes - I could not be a blocker for your team. I shared my opinion about this PR, and it could also be improved. Have a good hack =)

sergeyWh1te · 2024-07-30T09:08:53Z

arb-subgraph/Dockerfile

@@ -1,14 +1,13 @@
 # Build stage: compile Typescript to Javascript
-FROM node:16.17.1-alpine3.16 AS base
+FROM node:20.9.0-alpine3.18 AS base


I would suggest using the latest node.

FROM node:20.15.1-alpine3.20 AS base

This was left as is. The refactoring only affected the code of the bot itself. Thanks - I'll fix it!

sergeyWh1te · 2024-07-30T09:10:29Z

arb-subgraph/Dockerfile

 COPY --from=builder /app/node_modules ./node_modules
 COPY --from=builder /app/dist ./src
 COPY version.json ./

-ENTRYPOINT ["/sbin/tini", "--"]
 CMD ["yarn", "run", "start:prod"]


I'm not sure that it would work. Check pls.
If it does not work - here is a working combination:

CMD ["yarn", "run", "start:docker:prod"]

ill check it

sergeyWh1te · 2024-07-30T09:11:56Z

arb-subgraph/package.json

+    "build": "tsc && yarn run copy-version && mkdir dist/generated/proto && make gen_proto_prod",
+    "copy-version": "cp version.json dist",
+    "start": "ts-node src/main.ts",
+    "start:dev": "nodemon --watch src --watch forta.config.json -e js,ts,json  --exec \"yarn run build && yarn run copy-version && forta-agent run\"",


Useless command. This commit removes forta-agent dependency

sergeyWh1te · 2024-07-30T09:12:37Z

arb-subgraph/package.json

  },
-  "packageManager": "yarn@3.3.0"
+  "packageManager": "yarn@1.22.21"
 }


I would suggest upgrading all deps in package.json

sergeyWh1te · 2024-07-30T09:13:44Z

arb-subgraph/sample.env

@@ -0,0 +1,6 @@
+
+# The URL of the Arbitrum RPC endpoint
+ARBITRUM_RPC_URL=https://arbitrum.drpc.com


Found that https://arbitrum-one.publicnode.com works better. Especially for reading some data in the past.

This is a sample for envs. I doubt that these URLs will be used in any work of the bot

sergeyWh1te · 2024-07-30T09:31:01Z

arb-subgraph/src/plugins/common/alerts.ts

+import { FastifyPluginAsync } from 'fastify'
+import fp from 'fastify-plugin'
+import { sendUnaryData, ServerUnaryCall } from '@grpc/grpc-js'
+


Empty line. If it made local "import-formatter", pls share settings

#585 (comment)
I apparently didn't find your formatting settings. I'll look again.

sergeyWh1te · 2024-07-30T09:35:52Z

arb-subgraph/src/plugins/common/chain-handler.ts

+      const event = call.request.getEvent()
+      const block = event?.getBlock()
+
+      if (!block) {


Redundant check

By types - block could be undefined

Maybe do assert(block) (or any other a bit more appropriate check) before the logic block? To save a level of nesting and localize the error if block happens to be undefined

sergeyWh1te · 2024-07-30T09:36:39Z

arb-subgraph/src/plugins/common/chain-handler.ts

+        l2blockDtoEvent.number,
+      )
+      if (finding) {
+        findings.push(finding)


Forgot to read onAppInitFindings

sergeyWh1te · 2024-07-30T09:37:48Z

arb-subgraph/src/plugins/common/heath-checker.ts

+import { HealthCheckRequest, HealthCheckResponse, ResponseStatus, Error } from '../../generated/proto/agent_pb'
+import { ENVS } from '../../env'
+
+const BORDER_TIME = 15 * 60 * 1000 // 15 minutes


Would suggest naming for const: 'MINUTES_15'

sergeyWh1te · 2024-07-30T09:59:08Z

arb-subgraph/src/plugins/balance-checker.ts

+      return null
+    }
+
+    const subgraphBillingContract = Billing__factory.connect(BILLING_ADDRESS, this.fastify.provider)


Inject Billing__factory, BILLING_ADDRESS like dependencies please

Sorry, I didn't understand what you meant.

TheDZhon · 2024-07-31T19:55:50Z

I guess the main point of the Fastify change proposal is the following (numbered third in the reasoning section):

Reusable Plugins: Plugins can be conveniently shared across different alerts, enabling the creation of a collection of reusable plugins.

@arwer13 please take a look

DiRaiks · 2024-07-31T20:26:27Z

Firstly, thank you of your job.

This bot - it's really simple bot. I'd say the most simple bot around all bots, and based on this idea I'd suggest some recommendations or improvements.

Recommendations:

Move your business logic (Balance.checker.ts) directly into handleBlock. There aren't different classes with business logic in arb-subgraph project.

Few months ago I tried to remove any external dependencies like forta for providing pure tech design that would not contain anything extra deps. Only that we needed for bot working. Here I found fastify 'dependency' as http router and DI container. Speaking about performance: what kind of performance we speak about?! - '/metrics, /health' - handlers. - Strange to think about performance here.

Speaking about DI - -higher, I texted that the bot is so simple, and it's ok to move business logic directly into HandlerBlock. From this perspective, I believe in the idea that any extra DI system is superfluous here. Also, I found that each file in project for supporting fastigy.DI has to have extra code for declaring module and creating instances of classes. They have(creating instances, of the classes) moved from main.ts into classes.

So, in arb-subgraph is added external dependency 'satisfy', but I don't see real benefits. It does not matter on real performance and creating instances. Everything would work without that extra "deps". Of course, if your team has a habit to work with that library, it might be OK. But I personally don't split up this vision. All authors of programming languages advise to have to use only that what we need and nothing more extra. I do agree with them; do not use extra, and simplify as much as possible.

Improvements: Once I shared a picture, the bot that works under the l2 network might have time lags. So, in reality, forta-softaware has lags with l2 networks. For us, it means that bots have to iterate under l1 network.

Speaking about this bot. Bot can listen up L1 network that fetch latest l2 block and make some checks.

Also, our local infra would not support interating over l2 network. If your team is going to deploy the bot into local infra - bot must iterate over the l1 network.

Approve/Request changes - I could not be a blocker for your team. I shared my opinion about this PR, and it could also be improved. Have a good hack =)

@sergeyWh1te
Thank you for your detailed answer!
Here, I probably presented my proposal incorrectly and prioritized the changes incorrectly.
The main idea is to have a universal approach to writing bots by reusing common code in plugins. Which will give more flexible, simple and fast creation/writing of bots.
In the end, I would like to get a pack of pre-written logic for any, which can be used in any bot through configuration or partial use.
The Fastify framework is rather used here because of its convenient ability to implement what I wanted. Sorry for putting too much emphasis on it, instead of the main idea. =)
On the other comments:

Why is Balance.checker.ts a separate file with its own logic - I specifically removed this code from handleBlock so that handleBlock would become an independent plugin (it was moved to the "common" folder). The idea here is that there is logic that handles blocks, and this logic can call any other external service for specific processing.
Additional code under Fastify - I think you mean the code that initializes the plugin in the Fastify interface. I didn't get what the problem is. Even this piece of code can be hidden under a reusable function that can be called in any of the bots and you don't have to think about how to write it.
Regarding the habit of Fastify - I also disagree. We don't use this library anywhere =) At least not yet. I remembered about its presence only when I started thinking about how to organize the bot code architecturally.
Regarding working on different networks, I didn't quite understand the comment. Apparently there is still not enough context here. I simply copied this logic from the bot code that was given to us as a reference. It would be very useful to get additional information from you on the problem, if there is one. =)

DiRaiks · 2024-07-31T23:25:58Z

If there are concerns about the vendor lock on fastify, then you can implement similar logic on pure ts. And can leave Express.
Here's an example - something like this:

type ServiceFactory<T, R> = (services: T) => R
type Factories<T> = {
  [K in keyof T]: ServiceFactory<T, T[K]>
}

export class ServiceProvider<T> {
  public services: T

  constructor(services: Factories<T>) {
    this.services = {} as T

    const serviceNames = Object.keys(services) as (keyof T)[]

    serviceNames.forEach((key: keyof T) => {
      this.services[key] = services[key](this.services)
    })
  }
}

export type ServicesInstance = {
  logger: ReturnType<typeof LoggerInit>
  metrics: ReturnType<typeof MetricsInit>
  alerts: ReturnType<typeof AlertsInit>
  provider: ReturnType<typeof ProviderInit>
  BalanceChecker: BalanceChecker
}
const services = {
  logger: LoggerInit,
  metrics: MetricsInit,
  BalanceChecker: BalanceCheckerInit,
  alerts: AlertsInit,
  provider: ProviderInit,
}

const serviceProvider = new ServiceProvider(services)
// The metric's service is available here
provider.services.metrics.buildInfo.set({ commitHash: Version.commitHash }, 1)

export class BalanceChecker {
  private services: Services

  constructor(services: ServicesInstance) {
    this.services = services
  }

  public async checkSubgraphBalance() {

    // The provider's service is available here
    console.log(this.services.provider)
    
    // The alert's service is available here
    return this.services.alerts.lowBalance()
  }
}

export const BalanceCheckerInit = (services: ServicesInstance) => {
  // The logger's service is available here
  const { logger } = services

  logger.info('BalanceChecker plugin initialized')
  return new BalanceChecker(services)
}

Well, or, as a last resort, completely abandon the concept of a single instance that unites all services under one namespace and leave the project structure, which will allow you to reuse most of the bot code =)

arwer13 · 2024-08-01T17:59:56Z

From the point of (L2) bots unification, I don't see much difference if to use fastify or not.

Anyway, duplication is removed at the level of imported files by extracting common code, persistent parameterization, placing in common files, and linking together in the main of a specific bot. There doesn't seem to be anything fundamentally different in fastify approach in comparison with the current unification approach of mine: #558

I think it is a bit better with fastify: IMO, through the pre-prepared framework structure, the structure becomes clearer and more understandable for people with a web backend background.

Tarens2 and others added 6 commits July 5, 2024 14:00

feat: refactor arb-subgraph forta alert, updated packages, added pret…

3054e72

…tier eslint

feat: fixed warnings, removed old yarn version

fa7d8a6

Merge branch 'main' into feature/si-1316-refactor-subgraph-forta-alert

feac5c6

added eslint config

ec01887

Merge branch 'main' of github.com:lidofinance/alerting-forta into fea…

c20cbc4

…ture/si-1316-refactor-subgraph-forta-alert

feat: refactoring subgraph alert to fastify package

83902d1

DiRaiks requested review from TheDZhon, arwer13 and sergeyWh1te July 26, 2024 18:39

DiRaiks requested a review from a team as a code owner July 26, 2024 18:39

fix: remove unnecessary files

2b9b02c

sergeyWh1te reviewed Jul 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal: Arb-subgraph refactoring #585

Proposal: Arb-subgraph refactoring #585

DiRaiks commented Jul 26, 2024 •

edited

Loading

sergeyWh1te left a comment •

edited

Loading

sergeyWh1te Jul 30, 2024

DiRaiks Jul 31, 2024

sergeyWh1te Jul 30, 2024

DiRaiks Jul 31, 2024

sergeyWh1te Jul 30, 2024

sergeyWh1te Jul 30, 2024 •

edited

Loading

sergeyWh1te Jul 30, 2024

DiRaiks Jul 31, 2024

sergeyWh1te Jul 30, 2024

DiRaiks Jul 31, 2024

sergeyWh1te Jul 30, 2024

DiRaiks Jul 31, 2024

arwer13 Aug 1, 2024 •

edited

Loading

sergeyWh1te Jul 30, 2024

sergeyWh1te Jul 30, 2024

sergeyWh1te Jul 30, 2024 •

edited

Loading

DiRaiks Jul 31, 2024

TheDZhon commented Jul 31, 2024

DiRaiks commented Jul 31, 2024 •

edited

Loading

DiRaiks commented Jul 31, 2024 •

edited

Loading

arwer13 commented Aug 1, 2024

		@@ -0,0 +1,6 @@

		# The URL of the Arbitrum RPC endpoint
		ARBITRUM_RPC_URL=https://arbitrum.drpc.com

Proposal: Arb-subgraph refactoring #585

Are you sure you want to change the base?

Proposal: Arb-subgraph refactoring #585

Conversation

DiRaiks commented Jul 26, 2024 • edited Loading

Key Changes:

Transferred from l2-bridge-arbitrum As-Is:

sergeyWh1te left a comment • edited Loading

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

sergeyWh1te Jul 30, 2024 • edited Loading

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

arwer13 Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergeyWh1te Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheDZhon commented Jul 31, 2024

DiRaiks commented Jul 31, 2024 • edited Loading

DiRaiks commented Jul 31, 2024 • edited Loading

arwer13 commented Aug 1, 2024

DiRaiks commented Jul 26, 2024 •

edited

Loading

Transferred from `l2-bridge-arbitrum` As-Is:

sergeyWh1te left a comment •

edited

Loading

sergeyWh1te Jul 30, 2024 •

edited

Loading

arwer13 Aug 1, 2024 •

edited

Loading

sergeyWh1te Jul 30, 2024 •

edited

Loading

DiRaiks commented Jul 31, 2024 •

edited

Loading

DiRaiks commented Jul 31, 2024 •

edited

Loading