chore: fix max_fee type mismatch in request_attestation action#1249
chore: fix max_fee type mismatch in request_attestation action#1249williamrusdyputra merged 1 commit intomainfrom
Conversation
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
Time Submission Status
|
WalkthroughThe pull request changes the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
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_feevalues 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:
- Be explicitly rejected with a clear error message, or
- Be documented as equivalent to NULL/zero for "no limit"
The test suite includes
testMaxFeeNegativewhich 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
📒 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)tonilcorrectly 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
nilwith 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
nilfor 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
nilfor unlimited fee caps, ensuring consistency across all test invocations.
271-271: LGTM! Consistent helper update.The CreateAttestationForRequester helper correctly uses
nilfor 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) toNUMERIC(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_feeto 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.
The
request_attestationaction previously usedINT8for the$max_feeparameter, 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 toNUMERIC(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
Tests