chore: request attestation bounded#1347
Merged
Merged
Conversation
Time Submission Status
You can submit time with the command. Example: See available commands to help comply with our Guidelines. |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds a new VIEW precompile Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Engine
participant Precompile as tn_utils.validate_attestation_date_range
participant DB as Postgres
Client->>Engine: CALL request_attestation(action_id, args_bytes, ...)
Engine->>Precompile: tn_utils.validate_attestation_date_range(action_id, args_bytes)
Precompile->>Precompile: decode action_id and args_bytes
alt action_id in [1..3]
Precompile->>Precompile: extract from/to, validate both-or-none, from<=to, range<=90d
Precompile-->>Engine: return error (if invalid) or success
else other action_id
Precompile-->>Engine: return success (no validation)
end
Engine->>DB: dispatch attestation/query (if precompile returned success)
DB-->>Engine: query result
Engine-->>Client: result or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
MicBun
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
extensions/tn_utils/precompiles.go (1)
807-810: Consider validating thatfromis not greater thanto.The validation checks if
dateRange > MaxAttestationDateRangeSeconds, but iffrom > to,dateRangebecomes negative and passes the check. While downstream queries may handle this gracefully (returning empty results), an explicit validation would provide a clearer error message.Proposed fix
dateRange := toTS - fromTS + if dateRange < 0 { + return fmt.Errorf("attestation 'from' timestamp (%d) must not be greater than 'to' timestamp (%d)", fromTS, toTS) + } if dateRange > MaxAttestationDateRangeSeconds { return fmt.Errorf("attestation date range of %d seconds exceeds maximum of %d seconds (90 days)", dateRange, MaxAttestationDateRangeSeconds) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/tn_utils/precompiles.go` around lines 807 - 810, Add an explicit check that fromTS is not greater than toTS before computing dateRange in the attestation validation logic (around the dateRange := toTS - fromTS block); if fromTS > toTS return a clear error (e.g., "invalid attestation date range: from is after to") instead of allowing a negative dateRange to bypass MaxAttestationDateRangeSeconds; reference the variables fromTS, toTS, dateRange and the constant MaxAttestationDateRangeSeconds when implementing this validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/streams/attestation/attestation_date_range_test.go`:
- Around line 144-156: Update the inline signature comment for get_last_record
to include the missing $use_cache parameter: change the comment currently above
the test (referencing get_last_record and the test variables lastRecordArgs,
argsBytes, tn_utils.EncodeActionArgs, and requestAttestationWithArgsBytes) from
four parameters to five by appending ", $use_cache BOOL" so it matches the
actual arguments passed in the test.
In `@tests/streams/attestation/request_attestation_fee_test.go`:
- Around line 460-472: The new helper calling platform.Engine.Call for
"request_attestation" passes dataProvider with different casing than the
existing callRequestAttestationActionWithTimeRange; normalize dataProvider by
lowercasing it (e.g., use strings.ToLower(dataProvider)) before passing to
platform.Engine.Call so it matches the other helper and avoids hash/lookup
mismatches; update the call in this helper to use the lowercased dataProvider
variable (and add the strings import if missing).
---
Nitpick comments:
In `@extensions/tn_utils/precompiles.go`:
- Around line 807-810: Add an explicit check that fromTS is not greater than
toTS before computing dateRange in the attestation validation logic (around the
dateRange := toTS - fromTS block); if fromTS > toTS return a clear error (e.g.,
"invalid attestation date range: from is after to") instead of allowing a
negative dateRange to bypass MaxAttestationDateRangeSeconds; reference the
variables fromTS, toTS, dateRange and the constant
MaxAttestationDateRangeSeconds when implementing this validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c272be1f-7bcc-4301-b1d8-5d344cc652d7
📒 Files selected for processing (8)
extensions/tn_utils/precompiles.gointernal/migrations/005-primitive-query.sqlinternal/migrations/006-composed-query.sqlinternal/migrations/007-composed-query-derivate.sqlinternal/migrations/009-truflation-query.sqlinternal/migrations/024-attestation-actions.sqltests/streams/attestation/attestation_date_range_test.gotests/streams/attestation/request_attestation_fee_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/streams/attestation/request_attestation_fee_test.go (1)
439-482: Extract a sharedrequest_attestationcall helper.This duplicates the
TxContext+platform.Engine.Call(...)setup fromcallRequestAttestationActionWithTimeRangealmost verbatim. A single helper that takes pre-encodedargsByteswould keep normalization/auth/proposer changes from drifting between the two paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/streams/attestation/request_attestation_fee_test.go` around lines 439 - 482, The test contains duplicated setup for TxContext and calling the engine between requestAttestationWithArgsBytes and callRequestAttestationActionWithTimeRange; extract a shared helper (e.g., makeRequestAttestationCall or reuse requestAttestationWithArgsBytes) that builds the common TxContext/EngineContext (using TxContext, BlockContext, Signer, Caller, TxID, Authenticator) and invokes platform.Engine.Call with the pre-encoded argsBytes, then have both callRequestAttestationActionWithTimeRange and requestAttestationWithArgsBytes delegate to that helper to avoid drift in normalization/auth/proposer logic.tests/streams/attestation/attestation_date_range_test.go (1)
137-155: Add aget_change_over_timecase here too.This is the only range-validated action family member that still is not exercised end-to-end. Since the precompile branches on action IDs
1..3, one analogous success/failure case forget_change_over_timewould pin that remaining mapping down as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/streams/attestation/attestation_date_range_test.go` around lines 137 - 155, Add an end-to-end test exercising the range-validated action "get_change_over_time" alongside the existing get_index/get_last_record tests: construct appropriate args (matching the signature used by get_change_over_time), encode them with tn_utils.EncodeActionArgs, and call requestAttestationWithTimeRange (for the failure case with from180/to180) and requestAttestationWithArgsBytes (for the success/skip case if applicable) using the same ctx, platform, &systemAdmin, systemAdmin.Address(), and streamID variables; ensure you assert the expected failure contains "exceeds maximum" for the 180-day range and a successful NoError where validation should be skipped, mirroring the pattern used for "get_index" and "get_last_record".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extensions/tn_utils/precompiles.go`:
- Around line 785-805: fromVal/toVal parsing currently only unwraps *int/*int64
and then calls toInt64, which misses integer shapes DecodeActionArgs can produce
(int8,int16,int32,uint,uint8,uint16,uint32,uint64 and pointer variants); update
the parsing to normalize every supported integer shape into a single int64
value: either extend toInt64 to accept all integer kinds (and pointer forms) via
a comprehensive type switch or update derefIntPtr to return the raw value and
perform a type switch that converts int8/int16/int32/uint*/uint64/etc. to int64
before using it; ensure you update both the shown range-checking block
(variables fromVal/toVal) and the other similar parsing site handling ranges so
all integer widths and pointer variants from DecodeActionArgs are handled
consistently.
---
Nitpick comments:
In `@tests/streams/attestation/attestation_date_range_test.go`:
- Around line 137-155: Add an end-to-end test exercising the range-validated
action "get_change_over_time" alongside the existing get_index/get_last_record
tests: construct appropriate args (matching the signature used by
get_change_over_time), encode them with tn_utils.EncodeActionArgs, and call
requestAttestationWithTimeRange (for the failure case with from180/to180) and
requestAttestationWithArgsBytes (for the success/skip case if applicable) using
the same ctx, platform, &systemAdmin, systemAdmin.Address(), and streamID
variables; ensure you assert the expected failure contains "exceeds maximum" for
the 180-day range and a successful NoError where validation should be skipped,
mirroring the pattern used for "get_index" and "get_last_record".
In `@tests/streams/attestation/request_attestation_fee_test.go`:
- Around line 439-482: The test contains duplicated setup for TxContext and
calling the engine between requestAttestationWithArgsBytes and
callRequestAttestationActionWithTimeRange; extract a shared helper (e.g.,
makeRequestAttestationCall or reuse requestAttestationWithArgsBytes) that builds
the common TxContext/EngineContext (using TxContext, BlockContext, Signer,
Caller, TxID, Authenticator) and invokes platform.Engine.Call with the
pre-encoded argsBytes, then have both callRequestAttestationActionWithTimeRange
and requestAttestationWithArgsBytes delegate to that helper to avoid drift in
normalization/auth/proposer logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd6022b1-3be1-425b-9ffb-60ca301adfcb
📒 Files selected for processing (3)
extensions/tn_utils/precompiles.gotests/streams/attestation/attestation_date_range_test.gotests/streams/attestation/request_attestation_fee_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
extensions/tn_utils/precompiles.go (1)
797-803:⚠️ Potential issue | 🟠 MajorInteger-shape normalization is still incomplete for
from/toconversion.
derefIntPtrnow unwraps*int16,*int8, and*uint, buttoInt64still rejectsint16,int8, anduint. Those valid decoded shapes will still fail during validation.🔧 Proposed fix
func toInt64(value any) (int64, error) { switch v := value.(type) { case int64: return v, nil case int32: return int64(v), nil + case int16: + return int64(v), nil + case int8: + return int64(v), nil case int: return int64(v), nil + case uint: + if uint64(v) > math.MaxInt64 { + return 0, fmt.Errorf("value %d exceeds int64 max", v) + } + return int64(v), nil case uint64: if v > math.MaxInt64 { return 0, fmt.Errorf("value %d exceeds int64 max", v) } return int64(v), nil case uint32: return int64(v), nil case uint16: return int64(v), nil case uint8: return int64(v), nil default: return 0, fmt.Errorf("expected integer type, got %T", value) } }Also applies to: 821-889
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/tn_utils/precompiles.go` around lines 797 - 803, The validation fails because derefIntPtr now returns pointer types like *int16, *int8 and *uint but toInt64 still rejects those shapes; update the toInt64 function (used when parsing fromVal/toVal into fromTS/toTS) to accept and correctly convert int8, int16, and uint (and their pointer-unwrapped equivalents) to int64 (handling unsigned-to-signed conversion safely) so the decoded shapes accepted by derefIntPtr no longer cause errors; also apply the same broadened type handling to the other toInt64 usages in the adjacent block (the region corresponding to the 821–889 diff).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@extensions/tn_utils/precompiles.go`:
- Around line 797-803: The validation fails because derefIntPtr now returns
pointer types like *int16, *int8 and *uint but toInt64 still rejects those
shapes; update the toInt64 function (used when parsing fromVal/toVal into
fromTS/toTS) to accept and correctly convert int8, int16, and uint (and their
pointer-unwrapped equivalents) to int64 (handling unsigned-to-signed conversion
safely) so the decoded shapes accepted by derefIntPtr no longer cause errors;
also apply the same broadened type handling to the other toInt64 usages in the
adjacent block (the region corresponding to the 821–889 diff).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2cfbdfb0-4f77-4279-8939-5f5695a875ec
📒 Files selected for processing (2)
extensions/tn_utils/precompiles.gotests/streams/attestation/attestation_date_range_test.go
✅ Files skipped from review due to trivial changes (1)
- tests/streams/attestation/attestation_date_range_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
resolves: https://github.com/truflation/website/issues/3565
Summary by CodeRabbit
New Features
Changes
Tests