Reviewing files that changed from the base of the PR and between dd37d3e and 1a48f02.

• internal/storage/bucket/migrations/30-transaction-updated-at-trigger/notes.yaml is excluded by !**/*.yaml
• internal/storage/bucket/migrations/31-fix-transaction-updated-at/notes.yaml is excluded by !**/*.yaml
• internal/storage/bucket/migrations/32-accounts-recreate-unique-index/notes.yaml is excluded by !**/*.yaml
• internal/storage/bucket/migrations/33-clean-database/notes.yaml is excluded by !**/*.yaml
• internal/storage/bucket/migrations/34-logs-async-hash-procedure/notes.yaml is excluded by !**/*.yaml

• internal/controller/ledger/controller_default.go (4 hunks)
• internal/controller/ledger/controller_default_test.go (2 hunks)
• internal/controller/ledger/store.go (1 hunks)
• internal/controller/ledger/store_generated_test.go (2 hunks)
• internal/storage/bucket/migrations/29-fix-invalid-metadata-on-reverts/up.sql (2 hunks)
• internal/storage/bucket/migrations/29-fix-invalid-metadata-on-reverts/up_tests_before.sql (1 hunks)
• internal/storage/bucket/migrations/30-transaction-updated-at-trigger/up.sql (1 hunks)
• internal/storage/bucket/migrations/31-fix-transaction-updated-at/up.sql (1 hunks)
• internal/storage/bucket/migrations/31-fix-transaction-updated-at/up_tests_after.sql (1 hunks)
• internal/storage/bucket/migrations_test.go (2 hunks)
• internal/storage/ledger/transactions.go (2 hunks)
• internal/storage/ledger/transactions_test.go (7 hunks)

• GitHub Check: Tests
• GitHub Check: Dirty

internal/storage/bucket/migrations/31-fix-transaction-updated-at/up_tests_after.sql (1)

1-6: Effective post-migration validation test

This SQL test script correctly verifies that after the migration runs, there are no rows with null updated_at values in the transactions table. It's a good practice to include such validation to ensure the migration successfully fixed all affected records.

internal/storage/bucket/migrations_test.go (2)

55-55: Good use of error wrapping with %w

This change improves error handling by using Go's error wrapping feature with %w instead of string formatting with %s. This approach preserves the original error's type and context, enabling proper error inspection with errors.Is and errors.As up the call stack.

---

75-75: Good use of error wrapping with %w

Same as above, this change correctly updates error handling to use Go's error wrapping instead of string formatting, preserving the error chain for better error inspection and debugging.

internal/storage/bucket/migrations/29-fix-invalid-metadata-on-reverts/up_tests_before.sql (1)

6-20: LGTM: Appropriate test data setup

This addition properly sets up the test data required for the assertion on line 21. Inserting a transaction before checking metadata properties ensures the test environment has the necessary data for meaningful validation.

internal/controller/ledger/controller_default_test.go (2)

152-152: Correctly updated test to match new method signature

This change adds the required time.Time{} parameter to the mock expectation, matching the updated method signature that now accepts a timestamp parameter for setting the updated_at field.

---

193-193: Correctly updated test to match new method signature

Similar to line 152, this change properly adds the time.Time{} parameter to the DeleteTransactionMetadata mock expectation, ensuring the test remains compatible with the updated method signature.

internal/storage/bucket/migrations/29-fix-invalid-metadata-on-reverts/up.sql (3)

14-16: Improved data extraction from memento field

The change from using the data JSON field to extracting information from the memento field (converted from UTF-8 to JSONB) provides a more reliable way to access transaction data. This is a good approach as the memento field likely contains a more complete snapshot of the transaction state.

---

20-21: Enhanced selection with revert timestamp

Adding reversed.revertedAt to the selected fields ensures the revert timestamp is available for subsequent operations, aligning with the PR objective to properly set the updated_at field.

---

45-47: Fixed missing updated_at timestamp for reverted transactions

This change addresses the core issue described in the PR title. By setting updated_at = data.revertedAt, reverted transactions now correctly track when they were modified. This ensures consistency in the transaction history and fixes the issue where the updated_at field was not being set properly.

internal/controller/ledger/controller_default.go (4)

224-224: Passing log timestamp for transaction metadata updates

Updating the method call to include log.Date as the timestamp parameter ensures that when transaction metadata is updated during log import, the operation will be timestamped with the actual event time rather than the current time. This maintains historical accuracy.

---

239-239: Passing log timestamp for transaction metadata deletions

Similar to the update operation, this change ensures that metadata deletions during log import are timestamped with the log's event time. This maintains consistency with the actual sequence of events.

---

432-432: Using zero time for direct metadata updates

When metadata is updated directly (not as part of log import), a zero time value is passed. This indicates that the storage layer should use a different mechanism (likely current time) for setting the updated_at field. This approach maintains a clear distinction between importing historical events and creating new ones.

---

470-470: Using zero time for direct metadata deletions

Consistent with the pattern for updates, metadata deletions that aren't part of log imports use a zero time value. This maintains consistency in how timestamps are handled across the codebase.

internal/controller/ledger/store.go (1)

45-46:

✅ Verification successful

Updated interface to support timestamped metadata operations

The Store interface has been updated to include a time.Time parameter in both metadata methods, allowing implementations to use the provided timestamp when updating the updated_at field. This change is essential for propagating the correct timestamp through the system layers.

You should verify that all implementations of this interface have been updated to accept this new parameter. Run this script to check for any missed implementations:

---

🏁 Script executed:

#!/bin/bash
# Find all implementations of UpdateTransactionMetadata and DeleteTransactionMetadata
# to ensure they've been updated with the new parameter

echo "Checking for UpdateTransactionMetadata implementations:"
rg "func \(.+\) UpdateTransactionMetadata\(" --type go

echo -e "\nChecking for DeleteTransactionMetadata implementations:"
rg "func \(.+\) DeleteTransactionMetadata\(" --type go

Length of output: 4590

---

All Store interface implementations now include the time.Time parameter

I’ve verified that every implementation of both UpdateTransactionMetadata and DeleteTransactionMetadata has been updated to accept the new at time.Time argument:

• internal/storage/ledger/transactions.go
• internal/controller/ledger/store_generated_test.go (mocks and mock recorder)

No remaining action required here.

internal/storage/bucket/migrations/31-fix-transaction-updated-at/up.sql (1)

1-46:

❓ Verification inconclusive

Data migration to backfill missing updated_at values

This migration script addresses existing transactions with null updated_at values by setting them to match their inserted_at values. The script:

1. Uses batched updates for performance (1000 rows per batch)
2. Reports progress through notifications
3. Includes proper error handling and early exit when no rows need updating
4. Cleans up temporary tables

This is a well-structured migration that ensures data consistency without locking the entire table at once.

Run this verification script to check if there are any transactions with null updated_at values after the migration:

---

🏁 Script executed:

#!/bin/bash
# Verify the effectiveness of the migration by checking if any transactions 
# still have null updated_at values in test databases

fd --type file "up_tests_after.sql" --exec cat {} \; | grep -i "updated_at is null"

Length of output: 194

---

Verify that no updated_at values remain null

Please connect to your test database (using the same schema) and run:

SELECT count(*) AS null_updated_ats
FROM transactions
WHERE updated_at IS NULL;

Ensure this returns 0 before approving the migration.

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

146-147: Method calls updated to include new timestamp parameter.

The UpdateTransactionMetadata, DeleteTransactionMetadata, and RevertTransaction method calls have been updated to include a new zero timestamp parameter (time.Time{}). This change correctly adapts the test to the updated interface methods.

Also applies to: 150-150, 169-169, 174-174, 204-204, 215-215, 220-220, 564-564, 576-576, 581-581, 719-719

---

646-647: Model struct updated to include UpdatedAt field.

The UpdatedAt field has been added to the test model structure to correctly verify that the transaction's updated_at timestamp is being set. The test also properly verifies that this field matches the current time (now).

Also applies to: 669-670

internal/storage/bucket/migrations/30-transaction-updated-at-trigger/up.sql (1)

1-24:

❓ Verification inconclusive

Well-designed migration for handling transaction updated_at values.

This migration addresses the issue mentioned in the PR title by:

1. Creating a trigger function that automatically sets updated_at to match inserted_at for new transactions
2. Adding a NOT NULL constraint (marked as not valid to avoid validation on existing rows)

The approach is solid, handling both new insertions and providing a path to fixing existing data.

Verify that there's a subsequent migration that validates the constraint after fixing existing data:

---

🏁 Script executed:

#!/bin/bash
# Look for a migration that validates the constraint or updates existing rows
fd -e sql -e SQL -p "transactions.*updated_at" internal/storage/bucket/migrations

Length of output: 83

---

Please add or confirm a follow‑up migration to backfill and validate the new constraint.

I didn’t find any migration that:

• Updates existing transactions rows to populate updated_at
• Runs
  ALTER TABLE transactions VALIDATE CONSTRAINT transactions_updated_at_not_null;

Without that step, the NOT NULL constraint can never be marked valid.
Please verify (or add) a migration under internal/storage/bucket/migrations/ to backfill old data and validate the constraint.

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

123-123: Updated return columns to include updated_at.

The InsertTransaction method now correctly returns the updated_at column along with other fields, ensuring this new field is properly populated in the returned transaction.

---

220-248: Added timestamp parameter to UpdateTransactionMetadata.

The method now accepts an at time.Time parameter and conditionally sets the updated_at field:

• If at is zero, it uses the database's transaction date function
• Otherwise, it uses the provided timestamp

This change ensures proper tracking of when transaction metadata was last updated.

internal/controller/ledger/store_generated_test.go (1)

135-148: Updated mock implementation with new timestamp parameter.

The mock implementations for DeleteTransactionMetadata and UpdateTransactionMetadata have been correctly updated to include the new at time.Time parameter, ensuring test coverage for the interface changes.

Also applies to: 325-338