fix: auto enabled features on first ledger created on a bucket (EN-217)

[gfyrag]

No description provided.

[coderabbitai]

Walkthrough

The migration refactors trigger creation logic by replacing move-based triggers with transaction and account-based triggers. Trigger wiring is redistributed from a single batch mapping into multiple conditional loops based on ledger features for effective volumes, metadata history, and hash logging.

Changes

Cohort / File(s)

Change Summary

Database Migration: Trigger Restructuring internal/storage/bucket/migrations/11-make-stateless/up.sql

Rewired trigger creation from moves, transactions, and logs tables. Replaced single-batch trigger logic with multiple feature-conditioned loops targeting: (1) transaction address handlers replacing effective volumes on moves, (2) account metadata history triggers when ACCOUNT_METADATA_HISTORY is SYNC, (3) transaction metadata history triggers when TRANSACTION_METADATA_HISTORY is SYNC, (4) effective volumes triggers on moves when MOVES_HISTORY_POST_COMMIT_EFFECTIVE_VOLUMES is SYNC, and (5) log hash triggers when HASH_LOGS is SYNC.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

• Trigger rewiring logic: Validate that all trigger mappings from moves/transactions/logs to their new targets are correct and maintain functional equivalence.
• Feature-conditional loop correctness: Ensure each conditional loop (ACCOUNT_METADATA_HISTORY, TRANSACTION_METADATA_HISTORY, MOVES_HISTORY_POST_COMMIT_EFFECTIVE_VOLUMES, HASH_LOGS) properly constructs and executes the intended triggers.
• Stateless design impact: Verify that distributed trigger creation achieves intended stateless behavior without introducing race conditions or incomplete state.
• Data consistency: Confirm the refactored trigger order and nesting don't break dependency chains or leave data in intermediate states during migration execution.

Poem

🐰 From tangled moves to clearer tracks,
Triggers hop through conditional stacks,
Stateless and spry, they find their way,
Ledger features guide them day by day!
✨ thump thump ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	No pull request description was provided by the author.	Add a pull request description explaining the purpose of the trigger restructuring and how it addresses auto-enabling features on first ledger creation.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'fix: auto enabled features on first ledger created on a bucket' directly relates to the main change: restructuring trigger creation logic to conditionally wire ledger-specific features based on configurations.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/auto-enabled-features

---

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 0870634 and ee0a41f.

📒 Files selected for processing (1)

• internal/storage/bucket/migrations/11-make-stateless/up.sql (1 hunks)

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-05-20T13:07:54.504Z

Learnt from: gfyrag
Repo: formancehq/ledger PR: 935
File: internal/controller/system/state_tracker.go:50-55
Timestamp: 2025-05-20T13:07:54.504Z
Learning: In the ledger codebase's `handleState` method, when updating ledger state from `StateInitializing` to `StateInUse`, it's intentional to proceed silently when `rowsAffected == 0`. This indicates another parallel transaction has already updated the ledger state and configured the sequences, so no error needs to be returned and no sequence updating is required.

Applied to files:

• internal/storage/bucket/migrations/11-make-stateless/up.sql

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: Tests
• GitHub Check: Dirty

🔇 Additional comments (4)

internal/storage/bucket/migrations/11-make-stateless/up.sql (4)

498-506: Ensure trigger creation is safe against re-runs or add defensive checks.

The DO block creates triggers without checking if they already exist. If this migration is executed multiple times (e.g., in a recovery scenario or schema re-initialization), the CREATE TRIGGER statements will fail on duplicate trigger names.

For robustness, consider wrapping trigger creation in exception handling or using CREATE TRIGGER IF NOT EXISTS (available in PostgreSQL 13+). Alternatively, if migrations are guaranteed to run only once, add a comment documenting this assumption.

Verify:

1. What PostgreSQL version is the target? (IF NOT EXISTS requires 13+)
2. Are migrations ever re-run or recovered? Or is this a one-time operation?
3. Should defensive error handling be added?

If re-runs are possible, apply this pattern:

DO $$
BEGIN
  BEGIN
    EXECUTE vsql;
  EXCEPTION WHEN duplicate_object THEN
    NULL; -- trigger already exists, skip
  END;
END $$;

---

508-535: Verify that conditional loops properly filter ledgers and respect feature flags.

The four conditional loops correctly filter ledgers by feature flags:

• Line 508: features->>'ACCOUNT_METADATA_HISTORY' = 'SYNC'
• Line 516: features->>'TRANSACTION_METADATA_HISTORY' = 'SYNC'
• Line 524: features->>'MOVES_HISTORY_POST_COMMIT_EFFECTIVE_VOLUMES' = 'SYNC'
• Line 532: features->>'HASH_LOGS' = 'SYNC'

However, the first unconditional loop (lines 498-506) creates three triggers for ALL ledgers without feature flag checks. Ensure this aligns with the PR intent to "auto enable" these features. If some ledgers should opt-out of transaction_set_addresses, accounts_set_address_array, or transaction_set_addresses_segments, this unconditional creation may cause issues.

Confirm:

1. Is it intentional that transaction_set_addresses, accounts_set_address_array, and transaction_set_addresses_segments are created for ALL ledgers?
2. Should these three triggers also be conditioned on feature flags (e.g., a new TRANSACTION_ADDRESSES feature)?
3. Are there any ledgers that should NOT have these triggers?

Based on the PR title ("auto enabled features"), this appears intentional, but please confirm there are no ledgers that need to opt-out.

---

40-79: Column renames are correctly propagated through dependent functions.

The rename of account_address to accounts_address and account_address_array to accounts_address_array is properly reflected in the four updated functions. All uses of create or replace function ensure safe re-execution.

---

32-44: Schema changes look intentional for stateless migration, but verify NULL handling downstream.

The migration relaxes NOT NULL constraints on several columns (post_commit_volumes, post_commit_effective_volumes, account_address_array, hash, address_array), making them nullable. While this aligns with the stateless pattern, dependent code must handle NULL values correctly. Ensure application code checks for nulls before dereferencing these columns.

Verify that code consuming these columns handles NULL values:

1. Functions using post_commit_effective_volumes or post_commit_volumes on moves
2. Functions accessing address_array on accounts
3. Functions accessing hash on logs

No changes needed in the migration itself, but confirm that higher-layer code is defensive.

Also applies to: 202-237

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.17%. Comparing base (fba221d) to head (ee0a41f).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1149      +/-   ##
==========================================
+ Coverage   78.38%   82.17%   +3.79%     
==========================================
  Files         190      190              
  Lines        9408     9319      -89     
==========================================
+ Hits         7374     7658     +284     
+ Misses       1305     1211      -94     
+ Partials      729      450     -279     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.