Closed marc-aurele-besner closed 16 hours ago
Here are some key observations to aid the review process:
โฑ๏ธ Estimated effort to review: 4 ๐ต๐ต๐ต๐ตโช |
๐งช No relevant tests |
๐ Security concerns SQL Injection: The use of dynamic SQL in the `updateLeaderboardRanking` function could be vulnerable to SQL injection if not properly handled. Ensure that inputs are sanitized or use parameterized queries to prevent potential security risks. |
โก Recommended focus areas for review Data Integrity The transition from non-historical to historical data models in the database might lead to data integrity issues if not handled correctly. Ensure that data migration strategies are in place to handle existing data and maintain consistency. SQL Injection The dynamic SQL generation in `updateLeaderboardRanking` function could potentially lead to SQL injection if not properly sanitized. Ensure that all inputs are validated or use parameterized queries. Error Handling The `leaderboardSortAndRank` function lacks comprehensive error handling which might lead to unhandled exceptions during the leaderboard ranking process. |
Explore these optional code suggestions:
Category | Suggestion | Score |
Possible bug |
Correct the assignment of timestamps to use actual date values derived from block numbers___ **Ensure that thecreatedAt and updatedAt fields are correctly set to represent the timestamp of the block, not the block number.** [indexers/mainnet/leaderboard/src/mappings/db.ts [38-39]](https://github.com/autonomys/astral/pull/947/files#diff-7a7163f599d7374df6dc09344f2650dd3f4f3f0df166044f6b581c7178cac3eaR38-R39) ```diff -createdAt: blockNumber, -updatedAt: blockNumber, +createdAt: new Date(blockNumber * 1000), +updatedAt: new Date(blockNumber * 1000), ``` Suggestion importance[1-10]: 8Why: The suggestion correctly identifies a potential bug where block numbers are used directly as timestamps, which is incorrect. Converting them to actual date values is crucial for accurate timestamping. | 8 |
Correct the data type for date fields to prevent type mismatches___ **Ensure that thecreatedAt and updatedAt fields are assigned the correct date type values instead of the block number which is an integer. This might cause type mismatches or logical errors in the application.** [indexers/taurus/leaderboard/src/mappings/db.ts [129-130]](https://github.com/autonomys/astral/pull/947/files#diff-7243797099278ff5c0cdb18d90c15e6f58e3fc6ddbcfcb5c79c9825d3eaa5354R129-R130) ```diff -createdAt: blockNumber, -updatedAt: blockNumber, +createdAt: new Date(blockNumber), +updatedAt: new Date(blockNumber), ``` Suggestion importance[1-10]: 8Why: The suggestion correctly identifies a potential type mismatch in the assignment of `createdAt` and `updatedAt` fields using `blockNumber`, which is an integer. Using `new Date(blockNumber)` ensures the correct date type is used, preventing logical errors. | 8 | |
Add checks for table name derivation to prevent execution with invalid table names___ **Validate the existence ofsourceTable and targetTable before executing the ranking query to prevent runtime errors.** [indexers/taskboard/src/tasks/leaderboardSortAndRank.ts [40-43]](https://github.com/autonomys/astral/pull/947/files#diff-cfb78d8265dfaf2334e637e58f9787f2212625481f61ae3650ac4c7d00cd47f6R40-R43) ```diff const sourceTable = entryTypeToTable( LEADERBOARD_ENTRY_TYPE[key] + "Historie" ); const targetTable = entryTypeToTable(LEADERBOARD_ENTRY_TYPE[key]); +if (!sourceTable || !targetTable) { + throw new Error('Invalid table names derived from entry types'); +} ``` Suggestion importance[1-10]: 7Why: This suggestion is valid as it prevents runtime errors by ensuring that the derived table names are valid before executing database operations, which is crucial for avoiding crashes. | 7 | |
Best practice |
Standardize the data types for 'created_at' and 'updated_at' to use timestamps___ **Use consistent data types forcreated_at and updated_at across all leaderboard tables, changing them from integer to timestamp without time zone or timestamp with time zone as appropriate.**
[indexers/db/docker-entrypoint-initdb.d/init-db.sql [326-332]](https://github.com/autonomys/astral/pull/947/files#diff-1f627d6006ab49e938644df52a983ebe6f7b43ce87c6d27ff4437719260d43e2R326-R332)
```diff
CREATE TABLE leaderboard.account_extrinsic_failed_total_counts (
id text NOT NULL,
rank integer NOT NULL,
value numeric NOT NULL,
last_contribution_at timestamp without time zone NOT NULL,
- created_at integer NOT NULL,
- updated_at integer NOT NULL
+ created_at timestamp without time zone NOT NULL,
+ updated_at timestamp without time zone NOT NULL
);
```
Suggestion importance[1-10]: 8Why: Standardizing the data types for 'created_at' and 'updated_at' to use timestamps instead of integers is a significant improvement. It ensures consistency in how time-related data is stored and handled across the database, which is crucial for accurate time-based operations and comparisons. | 8 |
Implement error handling for database operations to enhance reliability___ **Add error handling for thesave operation to manage potential database write failures.** [indexers/mainnet/leaderboard/src/mappings/db.ts [60]](https://github.com/autonomys/astral/pull/947/files#diff-7a7163f599d7374df6dc09344f2650dd3f4f3f0df166044f6b581c7178cac3eaR60-R60) ```diff -await account.save(); +try { + await account.save(); +} catch (error) { + console.error('Failed to save account:', error); + throw error; +} ``` Suggestion importance[1-10]: 7Why: Adding error handling for database operations is a best practice that enhances the robustness of the code by managing potential failures during the save operation. | 7 | |
Enhancement |
Implement error handling for database operations to enhance reliability___ **Add error handling for the database operations to manage exceptions or failuresduring the save operation, ensuring the application's stability and reliability.** [indexers/taurus/leaderboard/src/mappings/db.ts [132]](https://github.com/autonomys/astral/pull/947/files#diff-7243797099278ff5c0cdb18d90c15e6f58e3fc6ddbcfcb5c79c9825d3eaa5354R132-R132) ```diff -await account.save(); +try { + await account.save(); +} catch (error) { + console.error('Failed to save account:', error); + throw error; +} ``` Suggestion importance[1-10]: 7Why: Adding error handling around the `account.save()` operation is a good practice to enhance the reliability and stability of the application by managing exceptions effectively. | 7 |
Assess configuration changes for their impact on data consistency and performance___ **Review the implications of enabling--unfinalized-blocks=true and --disable-historical=false to ensure they align with the application's data consistency and performance requirements.** [docker-compose.yml [255-256]](https://github.com/autonomys/astral/pull/947/files#diff-e45e45baeda1c1e73482975a664062aa56f20c03dd9d64a827aba57775bed0d3R255-R256) ```diff -- --unfinalized-blocks=true -- --disable-historical=false +- --unfinalized-blocks=true # Confirm the impact on data consistency +- --disable-historical=false # Verify performance implications ``` Suggestion importance[1-10]: 3Why: Reviewing the implications of configuration flags like `--unfinalized-blocks=true` and `--disable-historical=false` is important for understanding their effects on data consistency and performance. However, the suggestion is more about operational review rather than a direct code improvement. | 3 | |
Performance |
Improve performance for queries filtering by the 'last_contribution_at' field___ **Consider adding indexes on thelast_contribution_at fields in the leaderboard tables to improve query performance on time-based queries.** [indexers/db/docker-entrypoint-initdb.d/init-db.sql [326-332]](https://github.com/autonomys/astral/pull/947/files#diff-1f627d6006ab49e938644df52a983ebe6f7b43ce87c6d27ff4437719260d43e2R326-R332) ```diff CREATE TABLE leaderboard.account_extrinsic_failed_total_counts ( id text NOT NULL, rank integer NOT NULL, value numeric NOT NULL, last_contribution_at timestamp without time zone NOT NULL, created_at integer NOT NULL, updated_at integer NOT NULL ); +CREATE INDEX idx_last_contribution_at ON leaderboard.account_extrinsic_failed_total_counts(last_contribution_at); ``` Suggestion importance[1-10]: 6Why: Adding an index on the 'last_contribution_at' field could indeed enhance performance for time-based queries, which is beneficial for large datasets. This suggestion is practical and could improve query efficiency. | 6 |
Optimize SQL queries for performance improvements___ **Optimize the SQL query by usingDISTINCT ON instead of GROUP BY for better performance when aggregating unique entries.** [indexers/taskboard/src/utils/db.ts [108]](https://github.com/autonomys/astral/pull/947/files#diff-db0d86c9ea233b79948460c2d92408ed132bcba4840668ef6bf32e9a87a0c606R108-R108) ```diff -GROUP BY id +DISTINCT ON (id) ``` Suggestion importance[1-10]: 5Why: Using 'DISTINCT ON' instead of 'GROUP BY' can offer performance benefits in SQL queries by reducing the computational overhead, especially on large datasets. However, the impact might vary based on specific database configurations and data characteristics. | 5 | |
Possible issue |
Confirm the server configuration for the new network port to avoid connectivity issues___ **Verify that the new port3020 is correctly configured and open on the server to ensure the application can communicate without network issues.** [indexers/taurus/leaderboard/src/mappings/helper.ts [19]](https://github.com/autonomys/astral/pull/947/files#diff-081704d39444c86dae09e7fa29daa67b1cf3a6f46cb9b0f72ad52e3b96f7b2bbR19-R19) ```diff -port: 3020, +port: 3020, // Ensure this port is configured and open ``` Suggestion importance[1-10]: 2Why: The suggestion to verify server configuration for the new port is valid but it's more of a deployment or operational concern rather than a code change. It's a low-impact suggestion as it doesn't directly improve code quality or functionality. | 2 |
Name | Link |
---|---|
Latest commit | 5895477beefe3b1c52ad7bcf7901d21da6653839 |
Latest deploy log | https://app.netlify.com/sites/dev-astral/deploys/6735d5e719841d00085d1a65 |
User description
Modify leaderboard indexer to work with unfinalized block
PR Type
enhancement, configuration changes
Description
Changes walkthrough ๐
7 files
helper.ts
Update taskboard service port number
indexers/mainnet/consensus/src/mappings/helper.ts - Updated the port number for the taskboard service.
helper.ts
Update taskboard service port number
indexers/mainnet/leaderboard/src/mappings/helper.ts - Updated the port number for the taskboard service.
helper.ts
Update taskboard service port number
indexers/taurus/consensus/src/mappings/helper.ts - Updated the port number for the taskboard service.
helper.ts
Update taskboard service port number
indexers/taurus/leaderboard/src/mappings/helper.ts - Updated the port number for the taskboard service.
docker-compose.yml
Enable unfinalized blocks and historical data in leaderboard
docker-compose.yml
actions.yaml
Update handler URLs in actions configuration
indexers/db/metadata/actions.yaml - Updated handler URLs to use single quotes.
package.json
Add script for applying seeds in mainnet
indexers/package.json - Added new script for applying seeds in mainnet.
7 files
db.ts
Switch to historical entity types for leaderboard
indexers/mainnet/leaderboard/src/mappings/db.ts
leaderboardSortAndRank.ts
Use historical data for leaderboard ranking
indexers/taskboard/src/tasks/leaderboardSortAndRank.ts
db.ts
Enhance leaderboard ranking with aggregation and conflict handling
indexers/taskboard/src/utils/db.ts
db.ts
Switch to historical entity types for leaderboard
indexers/taurus/leaderboard/src/mappings/db.ts
init-db.sql
Create schemas and tables for leaderboard historical data
indexers/db/docker-entrypoint-initdb.d/init-db.sql
leaderboard_account_extrinsic_failed_total_count_histories.yaml
Add metadata for leaderboard historical tables
indexers/db/metadata/databases/default/tables/leaderboard_account_extrinsic_failed_total_count_histories.yaml - Added metadata for leaderboard historical tables.
schema.graphql
Update entity types to historical in GraphQL schema
indexers/mainnet/leaderboard/schema.graphql - Changed entity types to historical types.
1 files
actions.graphql
Reformat GraphQL mutation definitions
indexers/db/metadata/actions.graphql - Reformatted mutation definitions for better readability.