Closed marc-aurele-besner closed 3 days ago
Name | Link |
---|---|
Latest commit | d8480a8a57b841471488d7eebb057c7344f7d27d |
Latest deploy log | https://app.netlify.com/sites/dev-astral/deploys/6731d03fe4e35f000870077a |
Here are some key observations to aid the review process:
β±οΈ Estimated effort to review: 4 π΅π΅π΅π΅βͺ |
π§ͺ No relevant tests |
π No security concerns identified |
β‘ Recommended focus areas for review Database Schema Changes The PR includes extensive changes to the database schema and initialization scripts. It's crucial to ensure that these changes do not disrupt existing data and functionalities, especially in production environments. Thorough testing and validation against current database states and expected new states are required to prevent data loss or corruption. Service Dependency Management Modifications in service dependency conditions in the docker-compose file need careful review to ensure that services start in the correct order without causing startup failures or delays in environments where this configuration is deployed. GraphQL Schema Simplification The removal of several entities from the GraphQL schema needs to be validated to ensure that it does not affect the existing queries and operations that depend on these entities. It's important to check if any frontend or other dependent services require updates due to these changes. |
Explore these optional code suggestions:
Category | Suggestion | Score |
Typo |
Correct the port mapping for HTTPS to match the intended configuration___ **Correct the port mapping for HTTPS from "443:9944" to "443:9945" to align with theintended configuration as per the comment.** [docker-compose.yml [20]](https://github.com/autonomys/astral/pull/932/files#diff-e45e45baeda1c1e73482975a664062aa56f20c03dd9d64a827aba57775bed0d3R20-R20) ```diff -- "443:9944" # Map external 443 to Caddy's 9945 for HTTPS +- "443:9945" # Map external 443 to Caddy's 9945 for HTTPS ``` Suggestion importance[1-10]: 10Why: This suggestion corrects a critical typo in port mapping that could lead to a misconfiguration of the HTTPS service, impacting the application's security and functionality. | 10 |
Best practice |
Enhance reliability and rollback capabilities by using transactions for schema operations___ **Use explicit transactions or savepoints around schema creation and modification toensure atomicity and rollback capabilities in case of errors.** [indexers/db/docker-entrypoint-initdb.d/init-db.sql [15-16]](https://github.com/autonomys/astral/pull/932/files#diff-1f627d6006ab49e938644df52a983ebe6f7b43ce87c6d27ff4437719260d43e2R15-R16) ```diff -CREATE SCHEMA consensus; +BEGIN; +CREATE SCHEMA IF NOT EXISTS consensus; ALTER SCHEMA consensus OWNER TO postgres; +COMMIT; ``` Suggestion importance[1-10]: 8Why: Using transactions for schema creation and modification ensures atomicity and provides rollback capabilities, which is crucial for maintaining database integrity in case of errors during schema setup. | 8 |
Prevent errors by ensuring schemas are only created if they do not already exist___ **AddIF NOT EXISTS to the CREATE SCHEMA statements to prevent errors if the schema already exists.** [indexers/db/docker-entrypoint-initdb.d/init-db.sql [15]](https://github.com/autonomys/astral/pull/932/files#diff-1f627d6006ab49e938644df52a983ebe6f7b43ce87c6d27ff4437719260d43e2R15-R15) ```diff -CREATE SCHEMA consensus; +CREATE SCHEMA IF NOT EXISTS consensus; ``` Suggestion importance[1-10]: 7Why: Adding `IF NOT EXISTS` to schema creation is a best practice to avoid errors during script execution if the schema already exists, enhancing the robustness of the database setup process. | 7 | |
Ensure the
___
**Ensure that the | 5 |
User description
Seed dictionary and consensus schema at postgres init
This should reduce a bit the complexity of the database setup, removing the need to apply database migration. (Since the database migration needed to be applied after all indexer had seed the db but before the taskboard start using the db, now the db launch all seeded (with schema only) and so we only need to apply metadata for the frontend to work, but the taskboard does not need metadata)
PR Type
enhancement, configuration changes
Description
consensus
anddictionary
, reducing the need for migrations.docker-compose.yml
by adding health conditions.Changes walkthrough π
1 files
docker-compose.yml
Enhance service dependencies and port configurations
docker-compose.yml
depends_on
for several services.consensus_subquery_node
.3 files
init-db.sql
Seed schemas and tables for consensus and dictionary
indexers/db/docker-entrypoint-initdb.d/init-db.sql
consensus
anddictionary
.consensus
anddictionary
.schema.graphql
Simplify GraphQL schema by removing unused entities
indexers/mainnet/consensus/schema.graphql
Section
,ExtrinsicModule
, andAccountProfile
.schema.graphql
Simplify GraphQL schema by removing unused entities
indexers/taurus/consensus/schema.graphql
Section
,ExtrinsicModule
, andAccountProfile
.14 files
down.sql
Remove migration for sections_id_key constraint
indexers/db/migrations/default/1730310914662_alter_table_consensus_sections_alter_column_id/down.sql
sections_id_key
.up.sql
Remove migration for sections_id_key constraint
indexers/db/migrations/default/1730310914662_alter_table_consensus_sections_alter_column_id/up.sql
sections_id_key
.down.sql
Remove migration for event_modules_id_key constraint
indexers/db/migrations/default/1730310946777_alter_table_consensus_event_modules_alter_column_id/down.sql
event_modules_id_key
.up.sql
Remove migration for event_modules_id_key constraint
indexers/db/migrations/default/1730310946777_alter_table_consensus_event_modules_alter_column_id/up.sql
event_modules_id_key
.down.sql
Remove migration for extrinsic_modules_id_key constraint
indexers/db/migrations/default/1730310960555_alter_table_consensus_extrinsic_modules_alter_column_id/down.sql
extrinsic_modules_id_key
.up.sql
Remove migration for extrinsic_modules_id_key constraint
indexers/db/migrations/default/1730310960555_alter_table_consensus_extrinsic_modules_alter_column_id/up.sql
extrinsic_modules_id_key
.down.sql
Remove migration for accounts_id_key constraint
indexers/db/migrations/default/1730310972982_alter_table_consensus_accounts_alter_column_id/down.sql
accounts_id_key
.up.sql
Remove migration for accounts_id_key constraint
indexers/db/migrations/default/1730310972982_alter_table_consensus_accounts_alter_column_id/up.sql
accounts_id_key
.down.sql
Remove migration for account_profiles_id_key constraint
indexers/db/migrations/default/1730310983740_alter_table_consensus_account_profiles_alter_column_id/down.sql
account_profiles_id_key
.up.sql
Remove migration for account_profiles_id_key constraint
indexers/db/migrations/default/1730310983740_alter_table_consensus_account_profiles_alter_column_id/up.sql
account_profiles_id_key
.down.sql
Remove migration for account_rewards_id_key constraint
indexers/db/migrations/default/1730310997319_alter_table_consensus_account_rewards_alter_column_id/down.sql
account_rewards_id_key
.up.sql
Remove migration for account_rewards_id_key constraint
indexers/db/migrations/default/1730310997319_alter_table_consensus_account_rewards_alter_column_id/up.sql
account_rewards_id_key
.down.sql
Remove migration for log_kinds_id_key constraint
indexers/db/migrations/default/1730311021220_alter_table_consensus_log_kinds_alter_column_id/down.sql
log_kinds_id_key
.up.sql
Remove migration for log_kinds_id_key constraint
indexers/db/migrations/default/1730311021220_alter_table_consensus_log_kinds_alter_column_id/up.sql
log_kinds_id_key
.