SHARD-1104 - Postgres DB support by yaseen-oakrev · Pull Request #17 · shardeum/collector

yaseen-oakrev · 2024-09-05T16:13:52Z

No description provided.

… cycle, log

…mary key instead

… too

github-actions · 2024-09-05T16:14:58Z

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 Security concerns SQL Injection Risk: The dynamic SQL generation in `src/storage/transaction.ts` could lead to SQL injection if not handled properly. Ensure all inputs are sanitized and use parameterized queries where possible.
⚡ Key issues to review SQL Injection Risk The SQL queries constructed in functions like `insertTransaction` and `bulkInsertTransactions` are dynamically creating SQL strings which could potentially lead to SQL injection if not properly sanitized. It's crucial to ensure that all inputs are properly sanitized and parameterized to avoid SQL injection vulnerabilities. Code Duplication There is significant code duplication in the transaction handling for PostgreSQL and SQLite. Consider abstracting the database interaction to reduce duplication and improve maintainability. Error Handling The current implementation logs the error but does not handle it in a way that the calling function can react to it. Consider throwing the error or handling it in a way that the caller can make decisions based on the type of error.

src/storage/pgStorage.ts

arhamj · 2024-09-06T04:44:00Z

src/storage/block.ts

  try {
-    const sql = `SELECT * FROM blocks ORDER BY number DESC LIMIT ${count}`
-    const blocks: DbBlock[] = await db.all(sql)
+    const sql = `SELECT *${config.postgresEnabled ? ', readableBlock::TEXT' : ''} FROM blocks ORDER BY number DESC LIMIT ${count}`


Instead of something like this can we abstract the query creation into a method (of a class maybe) and have the conditionals in that method instead of having it in place? This is hard to debug in case of issues and from readability POV it's not pleasant. Same applies for the rest of the queries.

@arhamj, due to the differences in sql syntax of postgres and sqlite, these queries are different.
If I understand this correctly, having a separate method that I call to abstract out select * from ..., select field from ... queries would result in that method becoming a rule-based mess. Please let me know if this isn't what you had in mind. Also, could you give me an example? That'll help me visualise the solution better.

arhamj · 2024-09-06T04:48:38Z

src/storage/block.ts

-    const sql = `SELECT * FROM blocks ORDER BY number DESC LIMIT ${count}`
-    const blocks: DbBlock[] = await db.all(sql)
+    const sql = `SELECT *${config.postgresEnabled ? ', readableBlock::TEXT' : ''} FROM blocks ORDER BY number DESC LIMIT ${count}`
+    const blocks: DbBlock[] = config.postgresEnabled


Can choosing the DB instance also be abstracted into a single method? If I have to update the config in the future or add a new database it will be cumbersome. Unless there is a specific reason for this.

Ideally, I would have liked if we used a simple repo interface defining run, all, etc. which is composed into this file and has multiple implementations one for sqlite and one for postgres.

mgthuramoemyint

The query functions look okay, just wanna make sure in insert functions, all variables should come from trusted source or validate already.

mgthuramoemyint · 2024-09-06T07:39:23Z

src/storage/account.ts

Are these data such as account, token, _accountId coming from trusted source, type-check validation already done? All query functions look okay, just some concern for update function.

All the above come from the distributor. And we use parameterized queries to insert data, so threat of an SQL injection should be minimum.

If the data are properly validated, then it will be fine. Just wanted to bring your attention that there could be SQL injection even using parameterized queries. Example: insertAccount(account: Account), if the Account object has the key name like maliciousdata') , the query will become:
INSERT INTO accounts (maliciousdata')) VALUES ($1) ON CONFLICT(accountId) DO UPDATE SET ... which could change the query.

yes, this data is queried from the distributor

mgthuramoemyint · 2024-09-06T07:59:06Z

src/storage/accountHistoryState.ts

The same goes for AccountHistoryState, is it coming from trusted data?

yes, this data is queried from the distributor

aniketdivekar · 2024-09-06T04:15:29Z

package.json

    "node-cron": "3.0.2",
    "nodemon": "^2.0.20",
+    "pg": "8.12.0",
+    "pg-format": "^1.0.4",


Suggested change

"pg-format": "^1.0.4",

"pg-format": "1.0.4",

Please also commit necessary changes in package-lock.json

aniketdivekar · 2024-09-06T04:15:39Z

package.json

    "@typescript-eslint/eslint-plugin": "5.60.1",
    "@typescript-eslint/parser": "5.60.1",
    "@typescript-eslint/typescript-estree": "5.61.0",
+    "@types/pg-format": "^1.0.5",


Suggested change

"@types/pg-format": "^1.0.5",

"@types/pg-format": "1.0.5",

aniketdivekar · 2024-09-06T09:27:13Z

src/storage/account.ts

+  const balance = BigInt(`0x${account.account.account.balance}`)
+  const nonce = BigInt(`0x${account.account.account.nonce}`)


have we missed using these below?

I think they aren't required, removed them

aniketdivekar · 2024-09-06T09:28:49Z

src/storage/account.ts

-    const sql = 'INSERT OR REPLACE INTO accounts (' + fields + ') VALUES (' + placeholders + ')'
-    await db.run(sql, values)
+
+    if (config.postgresEnabled) {


I think Arham has already mentioned this; it would be great if we can have some kind of factory pattern on datasource or config driven data source (or if I can say DAO layer which abstracts the methods) instead of if/else conditions.

This was a conscious decision (Here's the relevant discussion)

aniketdivekar · 2024-09-06T09:50:41Z

src/storage/cycle.ts

+      await pgDb.run(sql, values)
+
+      // await transformCycle(cycle)
+      await updateCycleAnalytics()


why is this needed here? can this be done separately? I'm sorry I may not have enough context here but we may need to check impact it may have on inserting cycles. Will this run as a separate collector inserting only in PG and will not contribute to network?

@aniketdivekar this is to store things for marketing dashboard. This runs only on one server, and has no response times. I don't think we need to do it seperately as it does not hold up anything which can cause downstream problems.

yaseen-oakrev and others added 30 commits July 23, 2024 16:52

added the pg package

e979002

added PG-related env variables to config

995904f

created pgStorage file for abstracting PG db operations

9323b59

pg package update restricted

7fcac00

pg db usage is disabled by default

b95bdea

corrected DB config options for PG

42ac829

changed to follow js convention

be08d9e

modified db initialisation for PG

a98a952

changes for different query types

49ed72c

changes for different query types

8990155

changed db.run for account, accountEntry, accountHistoryState, block,…

f203a42

… cycle, log

converted db.run for originalTxData and receipt

b577d57

changed db.run to use pg

2e90b01

letting pg automatically determine presence of conflict from the prei…

03ebd52

…mary key instead

db.get modifications for account.ts

2be9d1e

db.get method changes

813b5be

db.all pg-specific changes

3c81cf0

using the default db for any db namespace

12c82af

added key to check if sqlite db is to be used

e38a826

reverted sqlite check change

6338ce9

changes for the queries that were missed

cae8ffd

mentioned the columns used as the primary key in conflict resolution

984b7f7

converting postgres' bigint to js number using the pg library

d3f631c

getting json data as string from pg to maintain parity with sqlite

0f33ddf

creating case-sensitive column names

265e93a

using qoutes for postgresql case sensitive column names

722d6e2

using Number as the typeParser for BigInt since sqlite returns Number…

706ab6b

… too

added transformation for the transaction model for analytics

fafc935

fixed on conflict for analytics

224d298

added transform for cycle

c402e71

yaseen-oakrev added 4 commits August 19, 2024 21:44

changed updateAccount method to use timestamptz too

c3c096b

timestamptz changes to analycticsCycle table corrections elsewhere

2779531

Merge remote-tracking branch 'origin/dev' into pg-collector

6eecc04

added new columns to fix casing

ed105dc

github-actions bot added the Review effort [1-5]: 4 label Sep 5, 2024

trying to fix github reviewer issue #1

e1b1de0

github-advanced-security bot found potential problems Sep 5, 2024

View reviewed changes

src/storage/pgStorage.ts Fixed Show fixed Hide fixed

src/storage/pgStorage.ts Fixed Show fixed Hide fixed

src/storage/pgStorage.ts Fixed Show fixed Hide fixed

src/storage/pgStorage.ts Fixed Show fixed Hide fixed

shobhitic added 12 commits September 5, 2024 21:58

trying to fix github reviewer issue #2

bc2f87a

trying to fix github reviewer issue #3

b60069f

trying to fix github reviewer issue #4

a630996

trying to fix github reviewer issue #5

6f6bafb

installed pg-format

0090f89

github reviewer fix #1

5f5d8b2

github reviewer fix #2

38922e9

github reviewer fix #3

606a5e8

github reviewer fix #4

3b3b726

github reviewer fix #5

63337f0

github reviewer fix #6

a8987b7

testing whether numbers are fine for github reviewer

36ccd2d

arhamj reviewed Sep 6, 2024

View reviewed changes

mgthuramoemyint reviewed Sep 6, 2024

View reviewed changes

aniketdivekar reviewed Sep 6, 2024

View reviewed changes

yaseen-oakrev and others added 5 commits September 6, 2024 16:05

abstracted away db methods through the dbStorage.ts

2643275

default value for optional field

434cdc2

Merge remote-tracking branch 'origin/pg-collector' into pg-collector

73d3dd4

Add nodeVersion to analyticsCycle table

136e99e

fix analyticscycle insert query (#23)

21d1921

justin-shardeum changed the title ~~Postgres DB support~~ SHARD-1104 - Postgres DB support Nov 9, 2024

mhanson-github force-pushed the dev branch from 0c85f33 to d8c9dec Compare April 26, 2025 03:55

		const balance = BigInt(`0x${account.account.account.balance}`)
		const nonce = BigInt(`0x${account.account.account.nonce}`)

Conversation

yaseen-oakrev commented Sep 5, 2024

Uh oh!

github-actions bot commented Sep 5, 2024

PR Reviewer Guide 🔍

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgthuramoemyint left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaseen-oakrev Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

yaseen-oakrev Sep 6, 2024 •

edited

Loading