Skip to content

chore: fix max_fee type mismatch in request_attestation action#1249

Merged
williamrusdyputra merged 1 commit intomainfrom
maxFee
Nov 4, 2025
Merged

chore: fix max_fee type mismatch in request_attestation action#1249
williamrusdyputra merged 1 commit intomainfrom
maxFee

Conversation

@MicBun
Copy link
Member

@MicBun MicBun commented Nov 4, 2025

The request_attestation action previously used INT8 for the $max_fee parameter, limiting fee caps to ~9.2 TRUF (9,223,372,036,854,775,807 wei). Since the attestation fee is 40 TRUF (40,000,000,000,000,000,000 wei), users could not set meaningful max_fee limits. This change updates the parameter type to NUMERIC(78, 0) for consistency with the token amount system and implements proper validation logic.

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

Summary by CodeRabbit

  • New Features

    • Enhanced attestation requests with high-precision fee validation; attestation fees are now validated against specified maximum fee limits, with clear error messaging when limits are exceeded.
  • Tests

    • Added comprehensive test coverage for fee validation scenarios, including edge cases for maximum fee constraints.

The `request_attestation` action previously used `INT8` for the `$max_fee` parameter, limiting fee caps to ~9.2 TRUF (9,223,372,036,854,775,807 wei). Since the attestation fee is 40 TRUF (40,000,000,000,000,000,000 wei), users could not set meaningful max_fee limits. This change updates the parameter type to `NUMERIC(78, 0)` for consistency with the token amount system and implements proper validation logic.

resolves: trufnetwork/truf-network#1323
@MicBun MicBun self-assigned this Nov 4, 2025
@holdex
Copy link

holdex bot commented Nov 4, 2025

Time Submission Status

Member Status Time Action Last Update
MicBun ✅ Submitted 4h Update time Nov 4, 2025, 11:25 AM
williamrusdyputra ✅ Submitted 10min Update time Nov 4, 2025, 11:25 AM

@coderabbitai
Copy link

coderabbitai bot commented Nov 4, 2025

Walkthrough

The pull request changes the max_fee parameter type for the request_attestation action from INT8 to NUMERIC(78, 0) to support larger fee cap values, adds validation to ensure the 40 TRUF attestation fee does not exceed the caller's specified max fee limit, and updates test files to use NULL instead of zero for unlimited fee caps.

Changes

Cohort / File(s) Summary
SQL Migration
internal/migrations/024-attestation-actions.sql
Updated max_fee parameter type from INT8 to NUMERIC(78, 0) and added validation logic to enforce that the 40 TRUF attestation fee does not exceed the caller's max_fee when provided and greater than 0.
New Attestation Fee Test
tests/streams/attestation/attestation_max_fee_test.go
Added new test file with TestMaxFeeValidation function covering multiple scenarios: NULL max_fee, zero max_fee, exactly 40 TRUF, above 40 TRUF, below 40 TRUF (expects failure), very large max_fee, and negative max_fee.
Existing Test Updates
tests/streams/attestation/attestation_request_test.go, tests/streams/attestation/request_attestation_fee_test.go, tests/streams/attestation/test_helpers.go
Replaced int64(0) with nil in request_attestation action calls to represent NULL (no limit) max_fee values instead of explicit zero.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Specific areas requiring attention:
    • Verify the NUMERIC(78, 0) type correctly handles the expected range of fee values and precision requirements
    • Confirm the validation logic properly handles NULL, zero, and negative max_fee values as "no limit" cases
    • Ensure the new test TestMaxFeeValidation covers all edge cases and the error message format for the "exceeds caller max_fee limit" scenario

Possibly related PRs

  • #1247: Modifies the same request_attestation action to add the 40 TRUF fee collection and transfer flow, which this PR now validates against.
  • #1207: Introduces the request_attestation action that is being enhanced here with the new max_fee parameter type and validation.
  • #1226: Also modifies the same request_attestation action to add authorization checks, parallel to these fee-related modifications.

Suggested labels

type: chore

Suggested reviewers

  • outerlook
  • williamrusdyputra

Poem

🐰 The fee cap grows from tiny INT8 shores,
NUMERIC(78,0) opens broader doors!
Forty TRUF attestations now have room,
No more limits trapped in a narrow tomb!

Pre-merge checks and finishing touches

✅ 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 clearly and concisely describes the main change: fixing a type mismatch for the max_fee parameter from INT8 to NUMERIC(78, 0) in the request_attestation action.
Linked Issues check ✅ Passed The PR successfully addresses issue #1323 by changing max_fee from INT8 to NUMERIC(78, 0), adding validation logic, and updating all test cases to reflect the new behavior.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing the max_fee type mismatch: schema migration, new validation tests, and updates to existing tests to use NULL instead of 0 for unlimited fees.
✨ 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 maxFee

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.

Copy link

@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/024-attestation-actions.sql (1)

55-61: Verify negative max_fee behavior is intentional.

The validation logic treats negative max_fee values as "no limit" (since it checks $max_fee > 0), allowing them to bypass the fee cap check. While this works, it may be confusing for callers. Consider whether negative values should:

  1. Be explicitly rejected with a clear error message, or
  2. Be documented as equivalent to NULL/zero for "no limit"

The test suite includes testMaxFeeNegative which expects this behavior, but it's worth confirming this is the intended design rather than an edge case oversight.

Additionally, the empty line at line 55 could be removed for consistency with the surrounding code style.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7fbfc7f and 8c7ba04.

📒 Files selected for processing (5)
  • internal/migrations/024-attestation-actions.sql (2 hunks)
  • tests/streams/attestation/attestation_max_fee_test.go (1 hunks)
  • tests/streams/attestation/attestation_request_test.go (2 hunks)
  • tests/streams/attestation/request_attestation_fee_test.go (1 hunks)
  • tests/streams/attestation/test_helpers.go (2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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
📚 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
🧬 Code graph analysis (1)
tests/streams/attestation/attestation_max_fee_test.go (5)
tests/streams/attestation/test_helpers.go (6)
  • NewTestAddresses (65-74)
  • NewAttestationTestHelper (85-115)
  • TestActionIDRequest (27-27)
  • AttestationTestHelper (77-82)
  • TestDataProviderHex (31-31)
  • TestStreamID (30-30)
tests/streams/utils/runner.go (1)
  • RunSchemaTest (37-116)
internal/migrations/migration.go (1)
  • GetSeedScriptStatements (60-109)
tests/streams/utils/utils.go (1)
  • GetTestOptionsWithCache (75-88)
extensions/tn_utils/serialization.go (1)
  • EncodeActionArgs (45-76)
⏰ 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: lint
  • GitHub Check: acceptance-test
🔇 Additional comments (10)
tests/streams/attestation/request_attestation_fee_test.go (1)

414-415: LGTM! Consistent max_fee handling.

The change from int64(0) to nil correctly signals unlimited fee caps, aligning with the new NUMERIC(78, 0) type and validation logic in the migration.

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

71-71: LGTM! Clear intent with NULL max_fee.

The change to nil with the explanatory comment correctly indicates unlimited fee caps for this test scenario.


165-165: LGTM! Consistent with other test updates.

The unauthorized user test correctly uses nil for max_fee, maintaining consistency across the test suite.

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

201-201: LGTM! Helper method updated consistently.

The RequestAttestation helper correctly uses nil for unlimited fee caps, ensuring consistency across all test invocations.


271-271: LGTM! Consistent helper update.

The CreateAttestationForRequester helper correctly uses nil for max_fee, maintaining consistency with the updated fee handling semantics.

internal/migrations/024-attestation-actions.sql (1)

20-20: LGTM! Type change resolves the core issue.

The change from INT8 (max ~9.2 TRUF) to NUMERIC(78, 0) correctly resolves the limitation preventing users from setting meaningful fee caps above the 40 TRUF attestation fee.

tests/streams/attestation/attestation_max_fee_test.go (4)

19-67: LGTM! Comprehensive test coverage for max_fee validation.

The test suite thoroughly covers all critical scenarios for the new max_fee parameter, including NULL, zero, exact fee match, above/below fee, very large values, and negative values. The test structure is clean and well-organized.


175-199: LGTM! Critical validation test.

This test correctly verifies that when max_fee (30 TRUF) is below the attestation fee (40 TRUF), the request fails with the expected error message. This is the core validation behavior introduced by this PR.


201-228: LGTM! Good validation of NUMERIC(78,0) capacity.

The test with 1 billion TRUF confirms that the new NUMERIC(78,0) type can handle extremely large values that would overflow the previous INT8 type (which maxed at ~9.2 TRUF). This validates the core motivation for the type change.


230-255: Verify negative max_fee design intent.

This test expects negative max_fee to be treated as "no limit" and succeed. While this aligns with the SQL validation logic (line 57 in 024-attestation-actions.sql checks $max_fee > 0), this behavior may be unintuitive for API consumers.

Consider whether negative values should be explicitly rejected or clearly documented as equivalent to NULL for "no limit" semantics.

@williamrusdyputra williamrusdyputra merged commit 0caee36 into main Nov 4, 2025
7 checks passed
@williamrusdyputra williamrusdyputra deleted the maxFee branch November 4, 2025 11:25
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