Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: update service #16

Closed
wants to merge 16 commits into from
Closed

Feat: update service #16

wants to merge 16 commits into from

Conversation

dTechLife
Copy link

@dTechLife dTechLife commented Aug 10, 2023

No description provided.

@@ -26,6 +27,7 @@
"license": "ISC",
"dependencies": {
"@erc725/erc725.js": "^0.17.2",
"@liaoliaots/nestjs-redis": "^9.0.5",
Copy link
Member

Choose a reason for hiding this comment

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

Change to ioredis library instead, always take the most downloaded and maintained libraries (check in leequid-back how it's being used, it s easy)

Copy link
Author

Choose a reason for hiding this comment

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

Easy refactor, sounds good.

@@ -45,7 +47,9 @@
"ethers": "^6.6.2",
"graphql": "^16.6.0",
"graphql-tools": "^8.3.19",
"ioredis": "^5.3.2",
Copy link
Member

Choose a reason for hiding this comment

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

Ah ioredis is here, why did you need the other library then ?

On leequid-back, REDIS service is as simple as that:

import { Injectable, OnModuleDestroy, OnModuleInit } from '@nestjs/common';
import { LoggerService } from '@libs/logger/logger.service';
import winston from 'winston';
import Redis from 'ioredis';

import { REDIS_KEY } from './redis-keys';
import { REDIS_URI } from '../globals';

@Injectable()
export class RedisService implements OnModuleInit, OnModuleDestroy {
  protected client: Redis;
  protected logger: winston.Logger;

  constructor(protected readonly loggerService: LoggerService) {
    this.logger = loggerService.getChildLogger('RedisService');
  }

  onModuleInit(): void {
    this.client = new Redis(REDIS_URI);
  }

  onModuleDestroy(): void {
    this.client.quit();
  }

  async get(key: REDIS_KEY): Promise<string | null> {
    return this.client.get(key);
  }

  async set(key: REDIS_KEY, value: string): Promise<void> {
    await this.client.set(key, value);
  }

  async setTotalWithdrawals(totalWithdrawals: bigint, checkpoint: number): Promise<void> {
    await this.set(REDIS_KEY.TOTAL_WITHDRAWALS, totalWithdrawals.toString());
    await this.set(REDIS_KEY.WITHDRAWALS_CHECKPOINT, checkpoint.toString());
  }
}

package.json Outdated
"knex": "^2.4.2",
"nestjs-redis": "^1.3.3",
Copy link
Member

Choose a reason for hiding this comment

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

Yeah again, not necessary I guess, only ioredis is enough

"@typescript-eslint/eslint-plugin": "^5.56.0",
"@typescript-eslint/parser": "^5.56.0",
"eslint": "^8.36.0",
"eslint-config-prettier": "^8.7.0",
"eslint-plugin-import": "^2.27.5",
"eslint-plugin-prettier": "^4.2.1",
"husky": "^8.0.3",
Copy link
Member

Choose a reason for hiding this comment

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

What is this used for ? I don t see any usage

Copy link
Author

Choose a reason for hiding this comment

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

Husky is used for pre-commit.

@@ -1,4 +1,5 @@
{
"extends": "./tsconfig.json",
"exclude": ["node_modules", "test", "dist", "**/*spec.ts"]
"exclude": ["node_modules", "test", "dist", "**/*spec.ts"],

Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Author

Choose a reason for hiding this comment

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

looks like just an extra comma, probably just automated from formatting/saving the file.


UNION

-- Fetching wrapped transactions with at least one blank parameter value
Copy link
Member

Choose a reason for hiding this comment

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

Again no need to check parameters if not decoded. If you did all of that yourself, this query would be the same as the fetchNonDecodedTransactionsWithInput, they both do the exact same thing, with almost same tables, but 2 totally different and wrong methods

RETURNING "id";
`;

try {
Copy link
Member

Choose a reason for hiding this comment

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

Try catch and logging should not be done in the DB service but outside

Copy link
Member

Choose a reason for hiding this comment

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

Just look at my existing functions, you should be consistent with what s existing

Copy link
Author

Choose a reason for hiding this comment

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

Got it, okay. Looking through and I find functions like this attached code. We should clean these up as well as they are inconsistent as well perhaps, or is there a differentiating factor?

  public async insertTokenHolder(
    tokenHolder: TokenHolderTable, // 'balanceInEth' is not supplied, hence we omit it
    onConflict: 'throw' | 'update' | 'do nothing' = 'throw',
  ): Promise<void> {
    let query = `
          INSERT INTO ${DB_DATA_TABLE.TOKEN_HOLDER} ("holderAddress", "contractAddress", "tokenId", "balanceInWei", "balanceInEth", "holderSinceBlock")
          VALUES ($1, $2, $3, $4, $5, $6)
        `;

    try {
      await this.executeQuery(query, [
        tokenHolder.holderAddress,
        tokenHolder.contractAddress,
        tokenHolder.tokenId,
        tokenHolder.balanceInWei,
        tokenHolder.balanceInEth,
        tokenHolder.holderSinceBlock,
      ]);
    } catch (error) {
      if (onConflict === 'do nothing' && JSON.stringify(error.message).includes('duplicate')) {
        // Do nothing
      } else if (onConflict === 'update' && JSON.stringify(error.message).includes('duplicate')) {
        query = `
          UPDATE ${DB_DATA_TABLE.TOKEN_HOLDER} 
          SET "balanceInWei" = $4, "balanceInEth" = $5
          WHERE "holderAddress" = $1 AND "contractAddress" = $2 
          AND (("tokenId" = $3 AND $3 IS NOT NULL) OR ("tokenId" IS NULL AND $3 IS NULL));
        `;

        await this.executeQuery(query, [
          tokenHolder.holderAddress,
          tokenHolder.contractAddress,
          tokenHolder.tokenId,
          tokenHolder.balanceInWei,
          tokenHolder.balanceInEth,
        ]);
      } else {
        // If some other error occurred, rethrow it
        throw error;
      }
    }
  }

-- Fetching events with blank eventName and with non-null parameter value
SELECT e1.*, ep1.value
FROM ${DB_DATA_TABLE.EVENT} e1
JOIN ${DB_DATA_TABLE.EVENT_PARAMETER} ep1 ON e1.id = ep1."eventId"
Copy link
Member

Choose a reason for hiding this comment

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

Again no need to check for the event_parameter, only the event without event name

Copy link
Author

@dTechLife dTechLife Aug 22, 2023

Choose a reason for hiding this comment

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

If the event params errored last update we should iterate over them again no? The purpose of this is error handling.

@@ -6,4 +6,5 @@ export interface ContractInterfaceTable {
name: string;
version: string;
type: CONTRACT_TYPE | null;
createdAt?: Date;
Copy link
Member

Choose a reason for hiding this comment

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

Why ?

Copy link
Author

Choose a reason for hiding this comment

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

to fix a type error I believe? Does it not make sense logically to you that this is needed here?

@@ -9,6 +10,13 @@ import { createLogger, format, transports, Logger as WinstonLogger } from 'winst
@Injectable()
export class LoggerService {
private readonly logger: WinstonLogger;
private readonly dummyLogger: WinstonLogger = createLogger({
Copy link
Member

Choose a reason for hiding this comment

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

What is that ? Why is it used ?

Copy link
Author

Choose a reason for hiding this comment

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

This is the dummy logger, like /dev/null in linux it's a black hole to allow throwing out the data. This is required for the ONLY_LOG_SERVICE= env variable for all the undefined services.

@samuel-videau samuel-videau deleted the feat/update_service branch September 25, 2023 11:12
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.

2 participants