SHARD-1104 - Postgres DB support #17
Conversation
PR Reviewer Guide 🔍
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -214,8 +263,10 @@ export async function queryBlockCount(): Promise<number> { | |||
|
|
|||
| export async function queryLatestBlocks(count: number): Promise<DbBlock[]> { | |||
| 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}` | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
src/storage/block.ts
Outdated
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
All the above come from the distributor. And we use parameterized queries to insert data, so threat of an SQL injection should be minimum.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
yes, this data is queried from the distributor
src/storage/accountHistoryState.ts
Outdated
There was a problem hiding this comment.
The same goes for AccountHistoryState, is it coming from trusted data?
There was a problem hiding this comment.
yes, this data is queried from the distributor
| @@ -55,6 +55,8 @@ | |||
| "morgan": "1.9.1", | |||
| "node-cron": "3.0.2", | |||
| "nodemon": "^2.0.20", | |||
| "pg": "8.12.0", | |||
| "pg-format": "^1.0.4", | |||
There was a problem hiding this comment.
| "pg-format": "^1.0.4", | |
| "pg-format": "1.0.4", |
There was a problem hiding this comment.
Please also commit necessary changes in package-lock.json
| @@ -76,6 +78,7 @@ | |||
| "@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", | |||
There was a problem hiding this comment.
| "@types/pg-format": "^1.0.5", | |
| "@types/pg-format": "1.0.5", |
| const balance = BigInt(`0x${account.account.account.balance}`) | ||
| const nonce = BigInt(`0x${account.account.account.nonce}`) |
There was a problem hiding this comment.
have we missed using these below?
There was a problem hiding this comment.
I think they aren't required, removed them
| const sql = 'INSERT OR REPLACE INTO accounts (' + fields + ') VALUES (' + placeholders + ')' | ||
| await db.run(sql, values) | ||
|
|
||
| if (config.postgresEnabled) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This was a conscious decision (Here's the relevant discussion)
| await pgDb.run(sql, values) | ||
|
|
||
| // await transformCycle(cycle) | ||
| await updateCycleAnalytics() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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.
No description provided.