Skip to content

chore: make get last transaction backward compatible#1256

Merged
MicBun merged 1 commit into
mainfrom
legacyGetLastTransactionv1
Nov 10, 2025
Merged

chore: make get last transaction backward compatible#1256
MicBun merged 1 commit into
mainfrom
legacyGetLastTransactionv1

Conversation

@MicBun
Copy link
Copy Markdown
Contributor

@MicBun MicBun commented Nov 10, 2025

using return select statement directly will cause error

❌ Error: JSON RPC call error: code: -300, message: query planner error: error in UNKNOWN section of sql statement: function does not exist: "get_last_transactions_v1"

This change the format into return next, so it works.

already migrated and currently working

resolves: https://github.com/trufnetwork/truf-network/issues/1338

Summary by CodeRabbit

  • Chores
    • Updated internal database migration for transaction activity retrieval to improve data handling efficiency. No user-facing changes to functionality or features.

@MicBun MicBun requested a review from outerlook November 10, 2025 17:26
@MicBun MicBun self-assigned this Nov 10, 2025
@holdex
Copy link
Copy Markdown

holdex Bot commented Nov 10, 2025

Time Submission Status

Member Status Time Action Last Update
MicBun ✅ Submitted 2h Update time Nov 11, 2025, 6:33 AM
@outerlook ❌ Missing - ⚠️ Submit time -

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Nov 10, 2025

Walkthrough

This PR refactors a PostgreSQL migration function to change the get_last_transactions wrapper from a single SELECT query to a set-returning function that iterates through get_last_transactions_v1 results, normalizing field mappings by setting specific fields to NULL and fee_distributions to empty strings per row.

Changes

Cohort / File(s) Summary
Migration function refactoring
internal/migrations/010-get-latest-write-activity.sql
Changed get_last_transactions wrapper from direct RETURN SELECT to FOR...IN...RETURN NEXT loop pattern, iterating over v1 results and normalizing field mappings (tx_id, caller, and fee-related fields to NULL; fee_distributions to empty string)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20-30 minutes

  • Requires careful verification of field mappings between v1 and wrapper schemas
  • Need to confirm NULL assignments and empty string defaults are correct
  • Validate that the loop-based iteration maintains expected row cardinality and data integrity
  • Understand impact on data retrieval behavior and dependent queries

Possibly related PRs

Suggested labels

type: chore

Suggested reviewers

  • outerlook

Poem

A function now loops where it once stood still,
Each row flows through with logic and will,
NULL fields planted, distributions clear,
The data flows onward without a fear. 🐰✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR addresses a migration function compatibility issue, but the linked issue #1338 focuses on type changes (int8 to numeric) affecting tests and examples, which is unrelated to the function's return mechanism change. Clarify whether this PR fully addresses issue #1338 or if it's a prerequisite fix; consider if the numeric type issue still needs separate resolution.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title references making 'get last transaction' backward compatible, which matches the PR's objective of fixing the function to restore backward compatibility using 'return next' instead of returning SELECT directly.
Out of Scope Changes check ✅ Passed The changes are narrowly focused on modifying get_last_transactions wrapper function to use 'return next' iteration, which is directly in scope with the PR's backward compatibility objective.
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 legacyGetLastTransactionv1

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9edf3ed and fa5e59a.

📒 Files selected for processing (1)
  • internal/migrations/010-get-latest-write-activity.sql (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: outerlook
Repo: trufnetwork/node PR: 1236
File: internal/migrations/007-composed-query-derivate.sql:0-0
Timestamp: 2025-10-28T14:52:20.414Z
Learning: In `internal/migrations/007-composed-query-derivate.sql` and `internal/migrations/009-truflation-query.sql`, the `get_last_record_composed` function intentionally passes `$before` as the `from` argument to `helper_check_cache(..., $before, NULL, NULL)`. This allows `HasCachedData` to: (a) ensure the request stays within the cached window, (b) fall back to anchor lookup when needed, and (c) hit the index-only path for base_time. Switching to `(NULL, $before, ...)` would skip these safeguards and cause valid cache hits to become misses.
⏰ 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). (1)
  • GitHub Check: acceptance-test
🔇 Additional comments (1)
internal/migrations/010-get-latest-write-activity.sql (1)

192-202: Verify client integration and fee_distributions behavior.

The wrapper function is properly designed and explicitly documented as a temporary bridge from v1 to v2. However, verification is needed for three critical assumptions in the original review:

  1. fee_distributions as empty string: Line 193 sets this to '' rather than NULL, but there is no inline documentation explaining the semantic intent. Confirm whether this distinction matters for downstream clients (e.g., JSON serialization, empty object handling).

  2. RPC error resolution: The stated "RPC error 'function does not exist'" cannot be verified in the codebase. The wrapper still calls get_last_transactions_v1() on line 192, which itself uses RETURN SELECT (not the new FOR...IN...RETURN NEXT pattern). Clarify what error this change is meant to fix and confirm it's actually resolved.

  3. Wrapper test coverage: The wrapper function itself is not exercised in tests—only get_last_transactions_v1 and get_last_transactions_v2 are tested separately. Confirm the wrapper is actually called by production RPC handlers and that NULL values for v2-specific fields (tx_id, caller, fee_amount, fee_recipient, metadata) are handled correctly by clients.


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.

❤️ Share

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

@MicBun MicBun force-pushed the legacyGetLastTransactionv1 branch from 9edf3ed to fa5e59a Compare November 10, 2025 17:29
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
internal/migrations/010-get-latest-write-activity.sql (1)

192-202: Optional: Move variable declaration outside the loop for clarity.

Line 193 declares $fee_distributions inside the loop, which re-initializes it on each iteration. While functionally correct, moving the declaration before the loop (line 192) would improve readability and avoid unnecessary per-iteration allocations.

    IF $limit_val > 100 {
        ERROR('Limit size cannot exceed 100');
    }

+   $fee_distributions TEXT := '';
    FOR $row IN get_last_transactions_v1($normalized_provider, $limit_val) {
-       $fee_distributions TEXT := '';
        RETURN NEXT NULL::TEXT,
            $row.created_at,
            $row.method,
            NULL::TEXT,
            NULL::NUMERIC(78, 0),
            NULL::TEXT,
            NULL::TEXT,
            $fee_distributions;
    }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1117c3d and 9edf3ed.

📒 Files selected for processing (17)
  • internal/migrations/000-extensions.sql (0 hunks)
  • internal/migrations/000-initial-data.sql (0 hunks)
  • internal/migrations/010-get-latest-write-activity.sql (1 hunks)
  • internal/migrations/017-normalize-tables.sql (0 hunks)
  • internal/migrations/0_test_only/bootstrap_erc20.sql (0 hunks)
  • internal/migrations/backfill-digest/.gitignore (0 hunks)
  • internal/migrations/backfill-digest/README.md (0 hunks)
  • internal/migrations/backfill-digest/candidates.sql (0 hunks)
  • internal/migrations/backfill-digest/candidates_no_table.sql (0 hunks)
  • internal/migrations/backfill-digest/insert_batches.sh (0 hunks)
  • internal/migrations/backfill-digest/migrate_actions.sh (0 hunks)
  • internal/migrations/backfill-digest/phase_analysis.md (0 hunks)
  • internal/migrations/backfill-digest/process_pending_day.sh (0 hunks)
  • internal/migrations/backfill-digest/split_into_chunks.sh (0 hunks)
  • internal/migrations/erc20-bridge/000-extension.sql (0 hunks)
  • internal/migrations/erc20-bridge/001-actions.sql (0 hunks)
  • internal/migrations/erc20-bridge/002-public-transfer-actions.sql (0 hunks)
💤 Files with no reviewable changes (16)
  • internal/migrations/0_test_only/bootstrap_erc20.sql
  • internal/migrations/erc20-bridge/002-public-transfer-actions.sql
  • internal/migrations/erc20-bridge/000-extension.sql
  • internal/migrations/backfill-digest/phase_analysis.md
  • internal/migrations/backfill-digest/README.md
  • internal/migrations/backfill-digest/.gitignore
  • internal/migrations/017-normalize-tables.sql
  • internal/migrations/backfill-digest/process_pending_day.sh
  • internal/migrations/backfill-digest/insert_batches.sh
  • internal/migrations/backfill-digest/migrate_actions.sh
  • internal/migrations/backfill-digest/candidates_no_table.sql
  • internal/migrations/backfill-digest/split_into_chunks.sh
  • internal/migrations/000-extensions.sql
  • internal/migrations/erc20-bridge/001-actions.sql
  • internal/migrations/000-initial-data.sql
  • internal/migrations/backfill-digest/candidates.sql
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: outerlook
Repo: trufnetwork/node PR: 1111
File: internal/migrations/017-normalize-tables.sql:68-77
Timestamp: 2025-08-19T14:32:35.051Z
Learning: In the trufnetwork/node project, schema migrations may be applied to match production state without including backfill steps in the migration files, particularly when bootstrapping fresh environments or when the backfill was already performed on production.
Learnt from: outerlook
Repo: trufnetwork/node PR: 1113
File: internal/migrations/020-digest-init.sql:3-11
Timestamp: 2025-08-20T12:22:14.250Z
Learning: In the trufnetwork/node repository, for the digest_config table in internal/migrations/020-digest-init.sql, the maintainer prefers to leave the table without initial seed rows, allowing the extension to handle the empty state case at runtime rather than seeding default configuration values in the migration.
Learnt from: williamrusdyputra
Repo: trufnetwork/node PR: 1228
File: internal/migrations/erc20-bridge/000-extension.sql:16-19
Timestamp: 2025-10-23T07:00:26.796Z
Learning: In internal/migrations/erc20-bridge/000-extension.sql, the active sepolia_bridge configuration is intentionally for test environments only and does not include the distribution_period parameter. The commented sepolia_bridge and ethereum_bridge blocks with distribution_period are production configurations.
Learnt from: MicBun
Repo: trufnetwork/node PR: 1123
File: internal/migrations/020-digest-actions.sql:379-399
Timestamp: 2025-08-25T15:28:08.459Z
Learning: In the batch_digest action in internal/migrations/020-digest-actions.sql, the cleanup step intentionally removes all valid candidates from pending_prune_days using $valid_stream_refs, including days that were skipped due to insufficient records. This prevents repeatedly considering days that don't need OHLC processing in future auto_digest runs, as confirmed by MicBun.
⏰ 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). (1)
  • GitHub Check: acceptance-test
🔇 Additional comments (1)
internal/migrations/010-get-latest-write-activity.sql (1)

192-202: LGTM – set-returning refactor correctly restores backward compatibility.

The change from RETURN SELECT to FOR...IN + RETURN NEXT properly resolves the RPC error by iterating over get_last_transactions_v1 results as a set within the function body rather than in the return statement itself. Column order and type casting (lines 194–201) correctly match the function signature, with NULL/empty defaults for the v2 fields (tx_id, caller, fee_amount, fee_recipient, metadata, fee_distributions) as expected.

Please verify that all callers of get_last_transactions expect NULL values for tx_id, caller, fee_amount, fee_recipient, and metadata fields. If callers previously relied on actual data in these columns (populated from v2), they will now receive NULL and this could break downstream logic.

@MicBun MicBun merged commit 9bbb1c1 into main Nov 10, 2025
4 of 7 checks passed
@MicBun MicBun deleted the legacyGetLastTransactionv1 branch November 10, 2025 17:31
@MicBun MicBun changed the title chore: make get last tranasction backward compatible chore: make get last transaction backward compatible Nov 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants