Check schema consistency test#92
Conversation
WalkthroughA new database schema file was added, defining several tables and enums for plugin policies, triggers, transaction history, and transaction indexing. The Makefile gained a Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant CI
participant PostgreSQL
participant Repo
Developer->>Repo: Push code/schema changes
CI->>PostgreSQL: Dump current schema (pg_dump)
CI->>Repo: Read committed schema.sql
CI->>CI: Compare dumped schema with schema.sql
alt Schemas match
CI->>Developer: Schema is up to date
else Schemas differ
CI->>Developer: Fail with update instructions
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
neavra
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
Makefile (3)
22-28: Mark dump-schema as .PHONY and validate prerequisites
The newdump-schematarget writes to a tracked schema file and depends onyq.
- Add
.PHONY: dump-schemato prevent collisions with the output file.- Insert a check at the top to ensure
yqis installed (e.g.,command -v yq >/dev/null || { echo "yq not found"; exit 1; }).
20-21: Remove trailing whitespace
Line 20 contains trailing spaces, triggering lint errors. Please trim the line to passmake lint.
1-40: Document external dependencies
Sincedump-schemarelies onyq, add a brief note in the Makefile header or project README specifying the requiredyqversion (e.g.,yq >= 4.x).🧰 Tools
🪛 checkmake (0.2.2)
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
.github/workflows/migration-test.yml (4)
131-132: Remove trailing whitespace
Lines 131–132 end with spaces, causing YAML lint errors. Please trim them.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 132-132: trailing spaces
(trailing-spaces)
133-155: DRY: centralize schema dump logic
TheCheck schema updatesstep reimplements thepg_dump | sed | awkpipeline already in the Makefile. Consider invoking thedump-schematarget (possibly extended to accept an output file) to avoid duplication and ensure consistency.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 148-148: trailing spaces
(trailing-spaces)
[error] 155-155: trailing spaces
(trailing-spaces)
148-148: Remove trailing whitespace
Line 148 has trailing spaces. Please trim it to satisfy YAMLlint.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 148-148: trailing spaces
(trailing-spaces)
155-156: Ensure newline at end-of-file and no trailing spaces
YAMLlint reports no newline at EOF and trailing whitespace on line 155. Add a final newline and remove any spaces.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 155-155: trailing spaces
(trailing-spaces)
[error] 156-156: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/migration-test.yml(1 hunks)Makefile(1 hunks)internal/storage/postgres/schema/schema.sql(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/migration-test.yml
[error] 132-132: trailing spaces
(trailing-spaces)
[error] 148-148: trailing spaces
(trailing-spaces)
[error] 155-155: trailing spaces
(trailing-spaces)
[error] 156-156: no new line character at the end of file
(new-line-at-end-of-file)
| CREATE TABLE "tx_indexer" ( | ||
| "id" "uuid" DEFAULT "gen_random_uuid"() NOT NULL, | ||
| "plugin_id" character varying(255) NOT NULL, | ||
| "tx_hash" character varying(255), | ||
| "chain_id" integer NOT NULL, | ||
| "policy_id" "uuid" NOT NULL, | ||
| "from_public_key" character varying(255) NOT NULL, | ||
| "proposed_tx_hex" "text" NOT NULL, | ||
| "status" "tx_indexer_status" DEFAULT 'PROPOSED'::"public"."tx_indexer_status" NOT NULL, | ||
| "status_onchain" "tx_indexer_status_onchain", | ||
| "lost" boolean DEFAULT false NOT NULL, | ||
| "broadcasted_at" timestamp without time zone, | ||
| "created_at" timestamp without time zone DEFAULT CURRENT_TIMESTAMP NOT NULL, | ||
| "updated_at" timestamp without time zone DEFAULT CURRENT_TIMESTAMP NOT NULL | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent plugin_id column type in tx_indexer
The tx_indexer table defines plugin_id as character varying(255), whereas plugin_policies.plugin_id uses the plugin_id enum. To enforce consistency and prevent invalid values, consider changing tx_indexer.plugin_id to use the same plugin_id enum type.
🤖 Prompt for AI Agents
In internal/storage/postgres/schema/schema.sql around lines 82 to 96, the
tx_indexer table defines plugin_id as character varying(255), but
plugin_policies.plugin_id uses the plugin_id enum type. To ensure consistency
and data integrity, change the plugin_id column in tx_indexer to use the same
plugin_id enum type instead of character varying(255).
| ALTER TABLE ONLY "transaction_history" | ||
| ADD CONSTRAINT "fk_policy" FOREIGN KEY ("policy_id") REFERENCES "plugin_policies"("id"); | ||
|
|
||
| ALTER TABLE ONLY "time_triggers" | ||
| ADD CONSTRAINT "time_triggers_policy_id_fkey" FOREIGN KEY ("policy_id") REFERENCES "plugin_policies"("id"); | ||
|
|
||
| ALTER TABLE ONLY "transaction_history" | ||
| ADD CONSTRAINT "transaction_history_policy_id_fkey" FOREIGN KEY ("policy_id") REFERENCES "plugin_policies"("id"); | ||
|
|
There was a problem hiding this comment.
Duplicate foreign key constraint on transaction_history.policy_id
There are two FKs referencing transaction_history.policy_id (fk_policy and transaction_history_policy_id_fkey). Remove one to avoid redundancy and naming conflicts.
🤖 Prompt for AI Agents
In internal/storage/postgres/schema/schema.sql around lines 133 to 141, there
are two foreign key constraints on transaction_history.policy_id named fk_policy
and transaction_history_policy_id_fkey. Remove one of these duplicate foreign
key constraints to avoid redundancy and naming conflicts, keeping only a single
foreign key constraint referencing plugin_policies(id).
| CREATE TYPE "plugin_id" AS ENUM ( | ||
| 'vultisig-dca-0000', | ||
| 'vultisig-payroll-0000', | ||
| 'vultisig-fees-feee' | ||
| ); |
There was a problem hiding this comment.
Possible typo in plugin_id enum value
The entry 'vultisig-fees-feee' has three es in feee. Please confirm that this is intentional; otherwise correct it to the intended plugin identifier.
🤖 Prompt for AI Agents
In internal/storage/postgres/schema/schema.sql lines 2 to 6, the enum value
'vultisig-fees-feee' likely contains a typo with an extra 'e' in 'feee'. Verify
the correct plugin identifier and update the enum value to the intended
spelling, such as 'vultisig-fees-fee', if the triple 'e' is unintentional.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
.github/workflows/migration-test.yml (2)
132-132: Fix trailing spaces and add newline at EOF.
YAMLlint reports trailing whitespace on lines 132, 148, and 155, and the file is missing a final newline. Please remove the extra spaces and ensure a newline at the end of the file.Also applies to: 148-148, 155-156
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 132-132: trailing spaces
(trailing-spaces)
133-147: DRY: Leverage the existingdump-schemaMakefile target.
This inlinepg_dump | sed | awkpipeline duplicates logic captured inmake dump-schema. Consider replacing it with:run: | make dump-schema CONFIG=config-plugin.yaml if ! diff -u storage/postgres/schema/schema.sql current_schema.sql; then echo "Schema has changed but schema.sql was not updated!" exit 1 fiThis reduces duplication and centralizes schema dumping.
storage/postgres/schema/schema.sql (1)
43-45: InconsistentNOT NULLon timestamp columns.
Some tables enforceNOT NULLoncreated_at/updated_atwhile others allow NULLs. For consistency and data integrity, consider marking all timestamp fields asNOT NULL.Also applies to: 76-78, 94-96
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/migration-test.yml(1 hunks)Makefile(1 hunks)storage/postgres/schema/schema.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/migration-test.yml
[error] 132-132: trailing spaces
(trailing-spaces)
[error] 148-148: trailing spaces
(trailing-spaces)
[error] 155-155: trailing spaces
(trailing-spaces)
[error] 156-156: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (1)
.github/workflows/migration-test.yml (1)
149-154: Schema comparison logic looks solid.
Thediff -ucheck will fail the job when discrepancies exist and provides clear instructions for updating the file.
| ALTER TABLE ONLY "transaction_history" | ||
| ADD CONSTRAINT "fk_policy" FOREIGN KEY ("policy_id") REFERENCES "plugin_policies"("id"); | ||
|
|
||
| ALTER TABLE ONLY "time_triggers" | ||
| ADD CONSTRAINT "time_triggers_policy_id_fkey" FOREIGN KEY ("policy_id") REFERENCES "plugin_policies"("id"); |
There was a problem hiding this comment.
Duplicate foreign key for transaction_history.policy_id.
The policy_id FK on transaction_history is applied twice (fk_policy and transaction_history_policy_id_fkey). Remove one to avoid conflicts during schema application.
🤖 Prompt for AI Agents
In storage/postgres/schema/schema.sql around lines 133 to 137, there are
duplicate foreign key constraints on transaction_history.policy_id named
fk_policy and transaction_history_policy_id_fkey. Remove one of these foreign
key constraints to avoid conflicts during schema application, ensuring only a
single foreign key constraint exists for transaction_history.policy_id
referencing plugin_policies(id).
| ALTER TABLE ONLY "transaction_history" | ||
| ADD CONSTRAINT "unique_tx_hash" UNIQUE ("tx_hash"); | ||
|
|
There was a problem hiding this comment.
Redundant index on transaction_history.tx_hash.
You already add a unique constraint (unique_tx_hash) which creates an index. The separate idx_transaction_history_tx_hash is unnecessary and should be removed.
Also applies to: 129-129
🤖 Prompt for AI Agents
In storage/postgres/schema/schema.sql around lines 112 to 114 and line 129,
there is a redundant index on transaction_history.tx_hash because the unique
constraint unique_tx_hash already creates an index. Remove the separate index
named idx_transaction_history_tx_hash to avoid duplication and unnecessary
overhead.
| CREATE TYPE "plugin_id" AS ENUM ( | ||
| 'vultisig-dca-0000', | ||
| 'vultisig-payroll-0000', | ||
| 'vultisig-fees-feee' | ||
| ); |
There was a problem hiding this comment.
Typo in enum value ‘vultisig-fees-feee’.
The third entry in the plugin_id enum has an extra ‘e’. Please confirm the intended plugin identifier and correct this value.
🤖 Prompt for AI Agents
In storage/postgres/schema/schema.sql around lines 2 to 6, there is a typo in
the third enum value of the "plugin_id" type: 'vultisig-fees-feee' has an extra
'e'. Confirm the correct plugin identifier and update this enum value to the
intended spelling, removing the extra 'e' if it is a mistake.
3 participants
Replicated vultisig/verifier#143 for plugins. Added in a Check schema updates test in migration-test. This generates a schema and checks it against the committed schema to see if there are any inconsistencies
Summary by CodeRabbit
New Features
Chores