Skip to content

chore: filter attestation list by transaction id#1257

Merged
MicBun merged 2 commits into
mainfrom
chore/list-attestation-filter-by-tx-id
Nov 18, 2025
Merged

chore: filter attestation list by transaction id#1257
MicBun merged 2 commits into
mainfrom
chore/list-attestation-filter-by-tx-id

Conversation

@williamrusdyputra
Copy link
Copy Markdown
Contributor

@williamrusdyputra williamrusdyputra commented Nov 18, 2025

resolves: https://github.com/trufnetwork/trufscan/issues/139

Summary by CodeRabbit

  • New Features

    • Added ability to filter attestations by specific request transaction ID. When specified, this filter provides exact matching and takes precedence over other filtering options.
  • Tests

    • Added comprehensive test coverage for the new request transaction ID filtering functionality, including edge cases and multiple transaction scenarios.

@williamrusdyputra williamrusdyputra self-assigned this Nov 18, 2025
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Nov 18, 2025

Walkthrough

This change adds filtering capability to the list_attestations action by introducing a new $request_tx_id parameter, enabling retrieval of attestation metadata by specific transaction ID. When provided, this parameter bypasses other filters for direct transaction lookup; otherwise, existing filtering behavior is preserved.

Changes

Cohort / File(s) Summary
Migration Update
internal/migrations/024-attestation-actions.sql
Updated list_attestations signature to accept $request_tx_id TEXT parameter. Added early-return logic: when $request_tx_id IS NOT NULL, fetches attestation for exact transaction ID in single-row result. When NULL, preserves previous filtering behavior with explicit handling for NULL requester cases.
Test Coverage
tests/streams/attestation/attestation_retrieval_test.go
Added new "FilterByRequestTxID" test subcase with testListFilterByRequestTxID helper to verify filtering by request transaction ID. Updated all existing test calls (testListEmpty, testListNoFilter, testListFilterByRequester, testListPagination, testListMaxLimit) to accommodate the new fifth parameter.

Sequence Diagram

sequenceDiagram
    actor Client
    participant ListAttest as list_attestations
    participant DB as Database
    
    Client->>ListAttest: request_tx_id, requester, limit, offset, hash_filter
    
    alt request_tx_id IS NOT NULL
        ListAttest->>DB: SELECT by exact request_tx_id
        DB-->>ListAttest: Single attestation (or none)
        ListAttest-->>Client: Result [0..1 row]
    else request_tx_id IS NULL
        alt requester IS NOT NULL
            ListAttest->>DB: SELECT filtered by requester<br/>with ordering & pagination
            DB-->>ListAttest: Attestations matching requester
            ListAttest-->>Client: Results with offset/limit
        else requester IS NULL
            ListAttest->>DB: SELECT all attestations<br/>with ordering & pagination
            DB-->>ListAttest: All attestations
            ListAttest-->>Client: Results with offset/limit
        end
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Database logic: Verify the control flow for the new $request_tx_id parameter correctly prioritizes direct transaction lookup and properly handles NULL cases
  • Test updates: Confirm all five test cases correctly pass the new parameter and that the new FilterByRequestTxID test validates the expected behavior (correct count, tx_id, hash matching, and non-existent ID handling)
  • Backward compatibility: Ensure existing filtering logic remains unchanged when $request_tx_id IS NULL

Possibly related PRs

  • chore: user can request attestations #1207: Adds request_attestation action and attestation schema modifications—contextually related as foundational work for the attestation system that this PR extends with metadata retrieval functionality.

Suggested reviewers

  • MicBun: Primary reviewer for attestation action logic and test coverage

Poem

🐰 A filter by transaction, hopping through with glee,
Request IDs now findable with just a single key,
Tests verify each attestation stays put just right,
The rabbit's SQL magic shines ever so bright! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ 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 accurately describes the main change: adding transaction ID filtering to the attestation list function.
Linked Issues check ✅ Passed The PR implements the core objective from issue #139: enabling retrieval of attestation metadata by transaction ID through the new $request_tx_id parameter.
Out of Scope Changes check ✅ Passed All changes are directly related to the stated objective: SQL migration updates the list_attestations function and tests verify the new filtering capability.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore/list-attestation-filter-by-tx-id

📜 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 9bbb1c1 and 838ae41.

📒 Files selected for processing (2)
  • internal/migrations/024-attestation-actions.sql (2 hunks)
  • tests/streams/attestation/attestation_retrieval_test.go (5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-10T13:00:13.731Z
Learnt from: outerlook
Repo: trufnetwork/node PR: 1207
File: internal/migrations/023-attestation-schema.sql:20-21
Timestamp: 2025-10-10T13:00:13.731Z
Learning: In the attestations table (internal/migrations/023-attestation-schema.sql), the primary key is (requester, created_height, attestation_hash) because attestation_hash is computed deterministically from user input only (version|algo|data_provider|stream_id|action_id|args) and does not include created_height. This allows the same user to request the same attestation at different block heights.

Applied to files:

  • internal/migrations/024-attestation-actions.sql
📚 Learning: 2025-10-10T13:00:14.189Z
Learnt from: outerlook
Repo: trufnetwork/node PR: 1207
File: internal/migrations/024-attestation-actions.sql:58-76
Timestamp: 2025-10-10T13:00:14.189Z
Learning: In the attestation system for internal/migrations/024-attestation-actions.sql, the attestation_hash is computed from (version|algo|data_provider|stream_id|action_id|args) and intentionally excludes created_height. This design ensures the hash is deterministic based only on user input, not network state like block height.

Applied to files:

  • internal/migrations/024-attestation-actions.sql
🧬 Code graph analysis (1)
tests/streams/attestation/attestation_retrieval_test.go (1)
tests/streams/attestation/test_helpers.go (3)
  • AttestationTestHelper (77-82)
  • TestAddresses (58-62)
  • InvalidTxID (35-35)
⏰ 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: acceptance-test
  • GitHub Check: lint
🔇 Additional comments (4)
internal/migrations/024-attestation-actions.sql (2)

274-279: LGTM! Parameter addition is well-integrated.

The new $request_tx_id parameter enables the requested filtering capability. The documentation is updated appropriately, and while this is a breaking change to the function signature, all callers in the test suite have been correctly updated.


310-318: The original review comment is incorrect and should be dismissed.

The learnings note contained inaccurate information about the primary key structure. The actual schema shows that request_tx_id is the PRIMARY KEY (line 11 of internal/migrations/023-attestation-schema.sql), which automatically provides both indexing and uniqueness enforcement. The separate UNIQUE constraint on (requester, created_height, attestation_hash) is a secondary constraint, not the primary key.

The filtering logic at lines 310-318 is appropriate and efficient. Querying by request_tx_id will use the primary key index and will always return at most one row due to the primary key constraint. No changes are needed.

Likely an incorrect or invalid review comment.

tests/streams/attestation/attestation_retrieval_test.go (2)

132-134: LGTM! Comprehensive test coverage for the new filtering feature.

The new test case thoroughly validates the request_tx_id filtering:

  • Exact match returns correct single result
  • Different tx_ids return their respective attestations
  • Non-existent tx_id returns zero results

The test assertions verify both the result count and the actual returned values, ensuring correctness of the filtering logic.

Also applies to: 180-214


151-151: LGTM! All existing test calls correctly updated.

All invocations of list_attestations have been properly updated to include the new request_tx_id parameter (set to nil to preserve existing test behavior). The parameter ordering is consistent across all call sites.

Also applies to: 161-161, 171-171, 222-222, 226-226, 231-231


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.

@holdex
Copy link
Copy Markdown

holdex Bot commented Nov 18, 2025

Time Submission Status

Member Status Time Action Last Update
williamrusdyputra ✅ Submitted 1h Update time Nov 18, 2025, 6:59 PM
MicBun ✅ Submitted 10min Update time Nov 19, 2025, 1:48 AM

@MicBun
Copy link
Copy Markdown
Contributor

MicBun commented Nov 18, 2025

The CI is failing @williamrusdyputra

@williamrusdyputra
Copy link
Copy Markdown
Contributor Author

The CI is failing @williamrusdyputra

isn't it because of the whitelist attestation fees, I saw previous PRs also failed CI

Copy link
Copy Markdown
Contributor

@MicBun MicBun left a comment

Choose a reason for hiding this comment

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

Lgtm

@MicBun MicBun merged commit 04fa68d into main Nov 18, 2025
6 checks passed
@MicBun MicBun deleted the chore/list-attestation-filter-by-tx-id branch November 18, 2025 18:59
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