feat: cleanup database

[gfyrag]

No description provided.

[codecov]

Codecov Report

Attention: Patch coverage is 93.87755% with 3 lines in your changes missing coverage. Please review.

Project coverage is 83.13%. Comparing base (b6271da) to head (a8b5140).

Files with missing lines	Patch %	Lines
internal/storage/ledger/adapters.go	87.50%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@                      Coverage Diff                       @@
##           feat/ledger-stateless-next     #518      +/-   ##
==============================================================
+ Coverage                       82.87%   83.13%   +0.25%     
==============================================================
  Files                             118      110       -8     
  Lines                            6927     6088     -839     
==============================================================
- Hits                             5741     5061     -680     
+ Misses                            902      779     -123     
+ Partials                          284      248      -36     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[coderabbitai]

Walkthrough

This pull request introduces several significant changes to the ledger storage system, focusing on database schema modifications, query optimizations, and code cleanup. The changes include updating configuration metadata handling, modifying database migrations, simplifying transaction and account querying, and removing legacy storage components. The modifications aim to streamline the ledger system's data management and improve database interaction efficiency.

Changes

File	Change Summary
internal/README.md, internal/ledger.go	Updated Metadata field struct tag to include nullzero option
internal/storage/bucket/default_bucket.go	Updated MinimalSchemaVersion from 12 to 27 and modified struct field order
internal/storage/bucket/migrations/26-accounts-recreate-unique-index/up.sql	Created new unique index accounts_ledger2 on ledger and address columns
internal/storage/bucket/migrations/27-clean-database/up.sql	Comprehensive database cleanup, dropping multiple functions, triggers, and columns
internal/storage/driver/adapters.go	Removed error handling for store bucket version check
internal/storage/driver/driver.go	Removed automatic initialization of Metadata field
internal/storage/ledger/resource_aggregated_balances.go	Updated column expression to use public schema prefix
internal/storage/system/migrations.go	Added migration to set default metadata on ledgers
Removed Files	Deleted multiple legacy storage-related files in internal/storage/ledger/legacy/

Poem

🐰 Hop, hop, through code's domain,
Legacy files swept clean again,
Migrations dance, schemas align,
Nullzero magic starts to shine,
A leaner ledger takes the stage! 🚀

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (5)

internal/storage/ledger/balances.go (2)

52-54: Use clear naming or inline comments for CTE.
The variable name "ins" might be confusing when reading the query. Consider using a more descriptive alias for clarity, e.g. "insert_zero_balances".

- .With("ins",
+ .With("insert_zero_balances",

---

55-57: Grammar nitpick in comments.
Replace “It the complete sql transaction fail” with “If the complete SQL transaction fails” for clarity.

- // It the complete sql transaction fail, the account volumes will not be inserted.
+ // If the complete SQL transaction fails, the account volumes will not be inserted.

internal/storage/ledger/adapters.go (2)

14-16: Properly encapsulate store pointer.
Embedding *Store in DefaultStoreAdapter is fine, but if you need to swap the underlying store or guard access, consider a more explicit field instead.

---

18-20: Method name clarity.
IsUpToDate describes an internal check on minimal schema. The name is acceptable, though “HasMinimalSchemaVersion” might be more transparent.

internal/storage/bucket/migrations/27-clean-database/up.sql (1)

83-121: Scrutinize potential performance impact for set_log_hash function
This function hashes the new log entry with the previous log’s hash. Ensure that the ordering on logs by id desc is supported by an index for quick lookups.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b1cb3cf and 87fcd13.

⛔ Files ignored due to path filters (3)

• go.mod is excluded by !**/*.mod
• internal/storage/bucket/migrations/26-accounts-recreate-unique-index/notes.yaml is excluded by !**/*.yaml
• internal/storage/bucket/migrations/27-clean-database/notes.yaml is excluded by !**/*.yaml

📒 Files selected for processing (29)

• internal/README.md (1 hunks)
• internal/ledger.go (1 hunks)
• internal/storage/bucket/default_bucket.go (2 hunks)
• internal/storage/bucket/migrations/26-accounts-recreate-unique-index/up.sql (1 hunks)
• internal/storage/bucket/migrations/27-clean-database/up.sql (1 hunks)
• internal/storage/driver/adapters.go (2 hunks)
• internal/storage/driver/driver.go (0 hunks)
• internal/storage/ledger/adapters.go (1 hunks)
• internal/storage/ledger/balances.go (1 hunks)
• internal/storage/ledger/balances_test.go (1 hunks)
• internal/storage/ledger/legacy/accounts.go (0 hunks)
• internal/storage/ledger/legacy/accounts_test.go (0 hunks)
• internal/storage/ledger/legacy/adapters.go (0 hunks)
• internal/storage/ledger/legacy/balances.go (0 hunks)
• internal/storage/ledger/legacy/balances_test.go (0 hunks)
• internal/storage/ledger/legacy/debug.go (0 hunks)
• internal/storage/ledger/legacy/errors.go (0 hunks)
• internal/storage/ledger/legacy/logs.go (0 hunks)
• internal/storage/ledger/legacy/logs_test.go (0 hunks)
• internal/storage/ledger/legacy/main_test.go (0 hunks)
• internal/storage/ledger/legacy/queries.go (0 hunks)
• internal/storage/ledger/legacy/store.go (0 hunks)
• internal/storage/ledger/legacy/transactions.go (0 hunks)
• internal/storage/ledger/legacy/transactions_test.go (0 hunks)
• internal/storage/ledger/legacy/utils.go (0 hunks)
• internal/storage/ledger/legacy/volumes.go (0 hunks)
• internal/storage/ledger/legacy/volumes_test.go (0 hunks)
• internal/storage/ledger/resource_aggregated_balances.go (1 hunks)
• internal/storage/system/migrations.go (1 hunks)

💤 Files with no reviewable changes (18)

• internal/storage/driver/driver.go
• internal/storage/ledger/legacy/logs_test.go
• internal/storage/ledger/legacy/errors.go
• internal/storage/ledger/legacy/balances_test.go
• internal/storage/ledger/legacy/volumes_test.go
• internal/storage/ledger/legacy/debug.go
• internal/storage/ledger/legacy/balances.go
• internal/storage/ledger/legacy/transactions_test.go
• internal/storage/ledger/legacy/main_test.go
• internal/storage/ledger/legacy/accounts_test.go
• internal/storage/ledger/legacy/logs.go
• internal/storage/ledger/legacy/store.go
• internal/storage/ledger/legacy/volumes.go
• internal/storage/ledger/legacy/transactions.go
• internal/storage/ledger/legacy/utils.go
• internal/storage/ledger/legacy/accounts.go
• internal/storage/ledger/legacy/queries.go
• internal/storage/ledger/legacy/adapters.go

✅ Files skipped from review due to trivial changes (1)

• internal/storage/ledger/resource_aggregated_balances.go

🔇 Additional comments (21)

internal/storage/ledger/balances.go (1)

58-70: Validate concurrency and locking strategy.
Locking the volumes rows with FOR UPDATE can help avoid race conditions when multiple transactions concurrently insert or update balances. Ensure that this approach aligns with your broader concurrency model, especially if there's a chance of deadlocks.

Consider running an analysis to see if other code paths also lock rows in a possibly inconsistent ordering:

✅ Verification successful

Let me search for more context around locking patterns to ensure a comprehensive verification.

---

Let me search for more specific context around the locking implementation.

---

Locking strategy is properly implemented with consistent ordering

The verification confirms that the locking strategy is well-implemented:

1. The FOR UPDATE lock is only used in one place in the codebase (balances.go)
2. The code explicitly maintains a consistent locking order through the Order("accounts_address", "asset") clause, which is documented with the comment "Keep order, it ensures consistent locking order and limit deadlocks"
3. The codebase includes proper deadlock detection and handling, as evidenced by the presence of postgres.ErrDeadlockDetected error handling and deadlock metrics tracking
4. The ordering is consistently applied whenever the rows are locked, preventing circular wait conditions that could cause deadlocks

The implementation follows best practices for preventing deadlocks by ensuring a consistent order of locking operations across the application.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Searching for other statements containing "FOR UPDATE" to verify consistent ordering
rg "For\(\"update\"\)"

Length of output: 77

---

Script:

#!/bin/bash
# Search for any ordering patterns in SQL queries that might be related to locking
rg -B 3 -A 3 "Order\("

# Also search for potential deadlock-related comments
rg -i "deadlock"

Length of output: 53955

---

Script:

#!/bin/bash
# Search for the implementation of the volumes-related functions to understand the locking strategy
ast-grep --pattern 'func ($_, $_) Volumes() $_'

# Also search for any other FOR UPDATE clauses in SQL queries
rg -B 5 -A 5 'For\("update"\)'

Length of output: 934

internal/storage/driver/adapters.go (2)

5-5: Import usage acknowledgment.
Importing ledgerstore from internal/storage/ledger is appropriate for returning a new DefaultStoreAdapter. No issues found.

---

22-22: Confirm removal of bucket 'up-to-date' checks.
Previously, calls to store.GetBucket().IsUpToDate(ctx) might have ensured correct schema version. Removing these checks can lead to runtime errors if the schema is out-of-date. Confirm that these checks are no longer needed.

✅ Verification successful

Let me gather more context about the schema version checks.

---

Let me check the context around the store adapter creation.

---

Let me check how the schema version is handled in the OpenLedger function.

---

Let me try a different approach to check the OpenLedger implementation.

---

Let me check the driver's Initialize method to see how schema version is handled during initialization.

---

Let me check one final thing about how migrations are handled in the driver.

---

Schema version checks are still properly enforced

Based on the code analysis, the schema version checks are still maintained through a robust system:

1. The driver's Initialize method performs schema migrations and rollback detection
2. Schema version is checked in multiple places:
   • System store checks via systemStore.IsUpToDate
   • Bucket-level checks via bucket.IsUpToDate
   • Migrations are handled during initialization with systemStore.Migrate
3. The detectRollbacks function specifically prevents schema downgrades

The removal of direct IsUpToDate calls in the store adapter creation is safe because schema version consistency is enforced at a higher level in the driver initialization and operation flow.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for IsUpToDate calls to confirm that they're handled elsewhere or truly removed
rg "IsUpToDate"

Length of output: 3904

---

Script:

#!/bin/bash
# Check how schema version is handled in the driver
rg -A 5 "systemStore.IsUpToDate" internal/storage/driver/

Length of output: 447

---

Script:

#!/bin/bash
# Check the context around NewDefaultStoreAdapter
rg -B 5 -A 5 "NewDefaultStoreAdapter" internal/storage/

Length of output: 1457

---

Script:

#!/bin/bash
# Check the OpenLedger implementation to see how schema version is handled
ast-grep --pattern 'func $_($_ context.Context, $_ string) ($_, $_, error) {
  $$$
}'  internal/storage/driver/driver.go

Length of output: 121

---

Script:

#!/bin/bash
# Check the OpenLedger implementation
rg -B 5 -A 15 "func.*OpenLedger" internal/storage/driver/driver.go

Length of output: 698

---

Script:

#!/bin/bash
# Check the Initialize implementation and schema version handling
rg -B 2 -A 20 "func.*Initialize.*Context.*error" internal/storage/driver/driver.go

Length of output: 948

---

Script:

#!/bin/bash
# Check where migrations are handled in the driver
rg -B 5 -A 10 "func.*detectRollbacks|func.*Migrate" internal/storage/driver/

Length of output: 5865

internal/storage/ledger/adapters.go (8)

1-9: Package separation looks good.
Creating a separate package for ledger adapters clarifies responsibilities. No immediate issues noted.

---

10-12: Embed store pointer carefully.
Embedding *Store in TX is convenient but watch for potential nil dereferences if TX is used after a rollback or commit. Ensure that usage patterns avoid referencing a closed transaction.

---

22-31: Transactional begin success.
Starting a transaction and returning a new DefaultStoreAdapter is consistent. Ensure that callers handle rollback logic carefully to avoid leaks.

---

33-35: Transaction commit trust.
Committing the underlying store transaction works as expected. Validate that any references to d.Store after commit are safe.

---

37-39: Transaction rollback safety.
Same caution as Commit: ensure that references to d.Store after rollback handle the closed transaction scenario.

---

41-43: Method naming alignment.
AggregatedBalances() returning d.AggregatedVolumes() is consistent, but if this method is commonly used, confirm that the naming is self-explanatory in the broader context.

---

45-49: Constructor for adapter is straightforward.
The constructor is succinct and adequate. No issues found.

---

51-51: Interface conformance.
Confirming DefaultStoreAdapter implements ledgercontroller.Store is good.

internal/ledger.go (1)

87-87: Validate null metadata usage.
Changing Metadata to nullzero is beneficial for optional metadata. Confirm any queries or JSON marshalling logic handle nil fields gracefully.

internal/storage/bucket/default_bucket.go (3)

18-18: Schema version jump.
Raising MinimalSchemaVersion from 12 to 27 is significant. Verify that your migrations support this jump to avoid blocking users on older versions.

---

21-22: Field reordering.
Reordering struct fields typically doesn’t cause issues. Just be mindful if reflection-based logic or field tags rely on a specific order.

---

84-85: Constructor ordering.
Adjusting the initialization order (db first, name second) matches the struct fields. This is consistent and avoids potential overshadowing.

internal/storage/system/migrations.go (1)

218-229: Ensure consistency with existing metadata handling
By setting the default metadata to '{}'::jsonb, newly inserted rows will always have a JSON object, potentially streamlining queries that assume non-null metadata. However, verify that no part of the application expects a NULL value in metadata.

internal/storage/ledger/balances_test.go (1)

46-70: Great test coverage for non-existing accounts
This test scenario correctly ensures that a not-yet-existing account is automatically created with zero volumes, improving confidence in system consistency.

internal/storage/bucket/migrations/26-accounts-recreate-unique-index/up.sql (1)

1-9: Concurrent index creation is advisable and well-implemented
Creating the index concurrently avoids lengthy table locks. Ensure that client code checks for potential partial index availability in case of concurrency issues or failures.

internal/storage/bucket/migrations/27-clean-database/up.sql (2)

1-82: Validate that all dropped objects are truly unused
This script removes numerous functions, aggregates, and columns. Confirm that every reference to them has been removed from the codebase and no external integrations still rely on them.

---

123-123: Confirm no references to seq remain in the logs table
Dropping the seq column finalizes the cleanup. Verify that no leftover code or queries still rely on logs.seq.

internal/README.md (1)

248-248: LGTM! Verify handling of null metadata values.

The addition of nullzero to the Metadata field's bun tag aligns with the database cleanup objective. This change allows empty metadata to be stored as NULL in the database instead of an empty object.

Let's verify the handling of null metadata values in the codebase:

✅ Verification successful

LGTM! The nullzero tag addition is safe and well-handled

The addition of nullzero to the Configuration's Metadata field is safe based on the codebase analysis:

1. The Configuration struct is primarily used for ledger configuration and initialization, with default values ensuring metadata is never nil:

   • Default initialization sets empty metadata: Metadata: metadata.Metadata{}
   • No direct metadata field access without proper initialization was found

2. The database layer already handles null metadata values:

   • Existing migrations ensure metadata is not null: set metadata = '{}'::jsonb where metadata is null
   • Query operations use coalesce when retrieving metadata: coalesce(accounts_metadata.metadata, '{}'::jsonb)

3. The nullzero tag aligns with the codebase's handling of empty metadata as NULL in the database while maintaining the empty object representation in the application layer.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for direct metadata access that might assume non-null values
ast-grep --pattern 'Configuration {
  $$$
  $x.Metadata$$$
}'

# Check for potential SQL queries that might need NULL handling
rg -A 2 "metadata.*FROM.*configurations"

Length of output: 106

---

Script:

#!/bin/bash
# Let's try a different approach to find metadata usage

# Find files containing "Configuration" struct references
rg -l "Configuration.*struct"

# Search for metadata field access patterns
rg -i "metadata" --type go

# Look for SQL operations on configurations table
rg -i "configurations.*metadata|metadata.*configurations" --type go

Length of output: 77321

---

Script:

#!/bin/bash
# Let's check for Configuration struct and its metadata field usage
ast-grep --pattern 'type Configuration struct {
  $$$
  Metadata $$$
  $$$
}'

# Look for metadata field access in code
rg -A 2 'Configuration.*Metadata' --type go

# Check for database migrations or schema related to configurations table
rg -A 5 'CREATE TABLE.*configurations|ALTER TABLE.*configurations' --type sql

Length of output: 579

---

Script:

#!/bin/bash
# Let's check how the Configuration struct is used
rg -A 3 'Configuration\{' --type go

# Look for database operations involving Configuration
rg -A 3 'Configuration.*metadata' --type go

# Check for any existing null metadata handling
rg -A 3 'metadata.*null|null.*metadata' --type go

Length of output: 13615